From f9796d59587e985d5535749a204610a98932f87f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 08:01:57 +0000 Subject: [PATCH 1/5] Initial plan From 966a05a4287b09a3668d12518a608c6b9b8e77bd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 08:36:26 +0000 Subject: [PATCH 2/5] refactor: extract helpers in pkg/linters to reduce large-function findings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../appendbytestring/appendbytestring.go | 110 ++++----- .../appendoneelement/appendoneelement.go | 156 +++++++------ .../bytesbufferstring/bytesbufferstring.go | 139 +++++------ .../bytescomparestring/bytescomparestring.go | 146 +++++------- pkg/linters/errorfwrapv/errorfwrapv.go | 180 ++++++++------- .../execcommandwithoutcontext.go | 100 ++++---- .../hardcodedfilepath/hardcodedfilepath.go | 94 ++++---- pkg/linters/httpnoctx/httpnoctx.go | 84 +++---- .../ioutildeprecated/ioutildeprecated.go | 26 +-- pkg/linters/largefunc/largefunc.go | 86 +++---- pkg/linters/lenstringzero/lenstringzero.go | 98 ++++---- pkg/linters/mapclearloop/mapclearloop.go | 212 +++++++++-------- pkg/linters/mapdeletecheck/mapdeletecheck.go | 168 +++++++------- pkg/linters/nilctxpassed/nilctxpassed.go | 96 ++++---- pkg/linters/seenmapbool/seenmapbool.go | 67 +++--- pkg/linters/sprintfbool/sprintfbool.go | 95 ++++---- pkg/linters/sprintferrdot/sprintferrdot.go | 86 +++---- pkg/linters/sprintfint/sprintfint.go | 103 ++++----- .../strconvparseignorederror.go | 92 ++++---- .../stringscountcontains.go | 86 ++++--- .../stringsindexcontains.go | 95 ++++---- pkg/linters/timeafterleak/timeafterleak.go | 43 ++-- pkg/linters/timenowsub/timenowsub.go | 109 ++++----- pkg/linters/trimleftright/trimleftright.go | 101 ++++---- .../writebytestring/writebytestring.go | 218 +++++++++--------- 25 files changed, 1385 insertions(+), 1405 deletions(-) diff --git a/pkg/linters/appendbytestring/appendbytestring.go b/pkg/linters/appendbytestring/appendbytestring.go index bb960e2d93b..525f6f9a29a 100644 --- a/pkg/linters/appendbytestring/appendbytestring.go +++ b/pkg/linters/appendbytestring/appendbytestring.go @@ -38,68 +38,70 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } + analyzeAppendByteString(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - // Match append(b, x...) with exactly 2 arguments and an ellipsis. - ident, ok := call.Fun.(*ast.Ident) - if !ok || ident.Name != "append" { - return - } - if len(call.Args) != 2 || !call.Ellipsis.IsValid() { - return - } +// analyzeAppendByteString checks whether a call is an append(b, []byte(s)...) +// that can be simplified to append(b, s...) and reports a diagnostic if so. +func analyzeAppendByteString(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendbytestring") { - return - } + // Match append(b, x...) with exactly 2 arguments and an ellipsis. + ident, ok := call.Fun.(*ast.Ident) + if !ok || ident.Name != "append" { + return + } + if len(call.Args) != 2 || !call.Ellipsis.IsValid() { + return + } - // The first argument must be []byte. - if !astutil.IsByteSlice(pass, call.Args[0]) { - return - } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendbytestring") { + return + } - // The second argument must be a []byte(s) conversion where s is a string. - conv, ok := call.Args[1].(*ast.CallExpr) - if !ok { - return - } - if !astutil.IsByteSliceConversion(pass, conv) { - return - } - if len(conv.Args) != 1 { - return - } - strArg := conv.Args[0] - if !astutil.IsStringType(pass, strArg) { - return - } + // The first argument must be []byte. + if !astutil.IsByteSlice(pass, call.Args[0]) { + return + } - sText := astutil.NodeText(pass.Fset, strArg) - if sText == "" { - return - } + // The second argument must be a []byte(s) conversion where s is a string. + conv, ok := call.Args[1].(*ast.CallExpr) + if !ok { + return + } + if !astutil.IsByteSliceConversion(pass, conv) { + return + } + if len(conv.Args) != 1 { + return + } + strArg := conv.Args[0] + if !astutil.IsStringType(pass, strArg) { + return + } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: fmt.Sprintf("append(b, []byte(%s)...) can be simplified to append(b, %s...); the []byte conversion is unnecessary", sText, sText), - SuggestedFixes: buildFix(pass, conv, strArg), - }) - }) + sText := astutil.NodeText(pass.Fset, strArg) + if sText == "" { + return + } - return nil, nil + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("append(b, []byte(%s)...) can be simplified to append(b, %s...); the []byte conversion is unnecessary", sText, sText), + SuggestedFixes: buildFix(pass, conv, strArg), + }) } // buildFix returns a SuggestedFix rewriting append(b, []byte(s)...) to append(b, s...). diff --git a/pkg/linters/appendoneelement/appendoneelement.go b/pkg/linters/appendoneelement/appendoneelement.go index 82afe70717e..4efbc437168 100644 --- a/pkg/linters/appendoneelement/appendoneelement.go +++ b/pkg/linters/appendoneelement/appendoneelement.go @@ -39,88 +39,94 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } - - // Must be append(x, y...) with exactly 2 arguments and ellipsis. - ident, ok := call.Fun.(*ast.Ident) - if !ok || ident.Name != "append" { - return - } - if pass.TypesInfo.ObjectOf(ident) != types.Universe.Lookup("append") { - return - } - if len(call.Args) != 2 || !call.Ellipsis.IsValid() { - return - } + analyzeAppendOneElement(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendoneelement") { - return - } +// analyzeAppendOneElement checks whether a call is an append(s, []T{x}...) that +// can be simplified to append(s, x) and reports a diagnostic if so. +func analyzeAppendOneElement(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } - // The second argument must be a composite literal with exactly one element. - lit, ok := call.Args[1].(*ast.CompositeLit) - if !ok { - return - } - // Must be a slice type ([]T). *ast.ArrayType covers both slices and arrays; - // only slices have Len == nil. - arrayType, ok := lit.Type.(*ast.ArrayType) - if !ok || arrayType.Len != nil { - return - } - if len(lit.Elts) != 1 { - return - } + ident, ok := call.Fun.(*ast.Ident) + if !ok || ident.Name != "append" { + return + } + if pass.TypesInfo.ObjectOf(ident) != types.Universe.Lookup("append") { + return + } + if len(call.Args) != 2 || !call.Ellipsis.IsValid() { + return + } - elem := lit.Elts[0] - if _, ok := elem.(*ast.KeyValueExpr); ok { - return - } - if nestedLit, ok := elem.(*ast.CompositeLit); ok && nestedLit.Type == nil { - return - } - elemText := astutil.NodeText(pass.Fset, elem) - if elemText == "" { - return - } - litText := astutil.NodeText(pass.Fset, lit) - if litText == "" { - return - } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "appendoneelement") { + return + } - sliceText := astutil.NodeText(pass.Fset, call.Args[0]) - if sliceText == "" { - return - } + sliceText, elemText, litText, ok := matchSingleElementSpread(pass, call) + if !ok { + return + } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: fmt.Sprintf("append(s, %s...) can be simplified to append(s, %s)", litText, elemText), - SuggestedFixes: []analysis.SuggestedFix{{ - Message: fmt.Sprintf("Replace %s... with %s", litText, elemText), - TextEdits: []analysis.TextEdit{ - { - Pos: call.Pos(), - End: call.End(), - NewText: fmt.Appendf(nil, "append(%s, %s)", sliceText, elemText), - }, - }, + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("append(s, %s...) can be simplified to append(s, %s)", litText, elemText), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: fmt.Sprintf("Replace %s... with %s", litText, elemText), + TextEdits: []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: fmt.Appendf(nil, "append(%s, %s)", sliceText, elemText), }}, - }) + }}, }) +} - return nil, nil +// matchSingleElementSpread validates that call.Args[1] is a single-element slice +// literal spread and returns the text representations of slice, element, and literal. +func matchSingleElementSpread(pass *analysis.Pass, call *ast.CallExpr) (sliceText, elemText, litText string, ok bool) { + lit, litOK := call.Args[1].(*ast.CompositeLit) + if !litOK { + return "", "", "", false + } + arrayType, atOK := lit.Type.(*ast.ArrayType) + if !atOK || arrayType.Len != nil { + return "", "", "", false + } + if len(lit.Elts) != 1 { + return "", "", "", false + } + + elem := lit.Elts[0] + if _, ok := elem.(*ast.KeyValueExpr); ok { + return "", "", "", false + } + if nestedLit, ok := elem.(*ast.CompositeLit); ok && nestedLit.Type == nil { + return "", "", "", false + } + + elemText = astutil.NodeText(pass.Fset, elem) + if elemText == "" { + return "", "", "", false + } + litText = astutil.NodeText(pass.Fset, lit) + if litText == "" { + return "", "", "", false + } + sliceText = astutil.NodeText(pass.Fset, call.Args[0]) + if sliceText == "" { + return "", "", "", false + } + return sliceText, elemText, litText, true } diff --git a/pkg/linters/bytesbufferstring/bytesbufferstring.go b/pkg/linters/bytesbufferstring/bytesbufferstring.go index f59288ef115..132ed898c95 100644 --- a/pkg/linters/bytesbufferstring/bytesbufferstring.go +++ b/pkg/linters/bytesbufferstring/bytesbufferstring.go @@ -39,83 +39,88 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } + analyzeStringBytesCall(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - // Match string(...) type conversion. - typeInfo, ok := pass.TypesInfo.Types[call.Fun] - if !ok || !typeInfo.IsType() { - return - } - basic, ok := typeInfo.Type.(*types.Basic) - if !ok || basic.Kind() != types.String { - return - } +// analyzeStringBytesCall checks whether a call is a string(buf.Bytes()) that +// can be simplified to buf.String() and reports a diagnostic if so. +func analyzeStringBytesCall(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } - if len(call.Args) != 1 { - return - } + // Match string(...) type conversion. + typeInfo, ok := pass.TypesInfo.Types[call.Fun] + if !ok || !typeInfo.IsType() { + return + } + basic, ok := typeInfo.Type.(*types.Basic) + if !ok || basic.Kind() != types.String { + return + } + if len(call.Args) != 1 { + return + } - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "bytesbufferstring") { - return - } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "bytesbufferstring") { + return + } - // The argument must be buf.Bytes() where buf is a bytes.Buffer value. - inner, ok := call.Args[0].(*ast.CallExpr) - if !ok { - return - } - sel, ok := inner.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Bytes" { - return - } - if len(inner.Args) != 0 { - return - } - receiverType := pass.TypesInfo.TypeOf(sel.X) - if receiverType == nil { - return - } - // Only flag value receivers (bytes.Buffer), not pointer receivers (*bytes.Buffer). - // The rewrite string(buf.Bytes()) → buf.String() is not semantics-preserving when - // buf is a nil *bytes.Buffer: string(buf.Bytes()) panics, while buf.String() returns - // "". Restricting to value receivers avoids this semantic difference entirely. - if !isBytesBufferValue(receiverType) { - return - } + inner, sel, ok := matchBufBytesArg(pass, call) + if !ok { + return + } + _ = inner - receiverText := astutil.NodeText(pass.Fset, sel.X) - if receiverText == "" { - return - } + receiverText := astutil.NodeText(pass.Fset, sel.X) + if receiverText == "" { + return + } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: fmt.Sprintf("string(%s.Bytes()) can be simplified to %s.String()", receiverText, receiverText), - SuggestedFixes: []analysis.SuggestedFix{{ - Message: fmt.Sprintf("Replace string(%s.Bytes()) with %s.String()", receiverText, receiverText), - TextEdits: []analysis.TextEdit{{ - Pos: call.Pos(), - End: call.End(), - NewText: []byte(receiverText + ".String()"), - }}, + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("string(%s.Bytes()) can be simplified to %s.String()", receiverText, receiverText), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: fmt.Sprintf("Replace string(%s.Bytes()) with %s.String()", receiverText, receiverText), + TextEdits: []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: []byte(receiverText + ".String()"), }}, - }) + }}, }) +} - return nil, nil +// matchBufBytesArg checks whether call.Args[0] is a buf.Bytes() call where +// buf is a bytes.Buffer value (not pointer). Returns the inner call, selector, +// and ok=true when matched. +func matchBufBytesArg(pass *analysis.Pass, call *ast.CallExpr) (*ast.CallExpr, *ast.SelectorExpr, bool) { + inner, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return nil, nil, false + } + sel, ok := inner.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Bytes" { + return nil, nil, false + } + if len(inner.Args) != 0 { + return nil, nil, false + } + receiverType := pass.TypesInfo.TypeOf(sel.X) + if receiverType == nil || !isBytesBufferValue(receiverType) { + return nil, nil, false + } + return inner, sel, true } // isBytesBufferValue reports whether t is exactly bytes.Buffer (value receiver, not pointer). diff --git a/pkg/linters/bytescomparestring/bytescomparestring.go b/pkg/linters/bytescomparestring/bytescomparestring.go index e34af17c94a..a08b3b99eb6 100644 --- a/pkg/linters/bytescomparestring/bytescomparestring.go +++ b/pkg/linters/bytescomparestring/bytescomparestring.go @@ -43,79 +43,68 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - // seenImportFiles tracks files that have already received a bytes import - // TextEdit in this pass, preventing duplicate overlapping edits when a - // single file contains multiple flagged comparisons. seenImportFiles := make(map[token.Pos]bool) - - nodeFilter := []ast.Node{ - (*ast.BinaryExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - bin, ok := n.(*ast.BinaryExpr) - if !ok { - return - } - - // Only flag == and != operators. - if bin.Op != token.EQL && bin.Op != token.NEQ { - return - } - - pos := pass.Fset.PositionFor(bin.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "bytescomparestring") { - return - } - - // Both sides must be string(x) conversions where x is []byte. - lhsArg, ok := extractByteSliceStringConv(pass, bin.X) - if !ok { - return - } - rhsArg, ok := extractByteSliceStringConv(pass, bin.Y) - if !ok { - return - } + analyzeBinaryExpr(pass, n, generatedFiles, noLintIndex, seenImportFiles) + }) + return nil, nil +} - lText := astutil.NodeText(pass.Fset, lhsArg) - rText := astutil.NodeText(pass.Fset, rhsArg) - if lText == "" || rText == "" { - return +// analyzeBinaryExpr checks whether a binary expression is a string(a) == string(b) +// or string(a) != string(b) comparison that should use bytes.Equal. +func analyzeBinaryExpr(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex, seenImportFiles map[token.Pos]bool) { + bin, ok := n.(*ast.BinaryExpr) + if !ok { + return + } + if bin.Op != token.EQL && bin.Op != token.NEQ { + return + } + pos := pass.Fset.PositionFor(bin.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "bytescomparestring") { + return + } + lhsArg, ok := extractByteSliceStringConv(pass, bin.X) + if !ok { + return + } + rhsArg, ok := extractByteSliceStringConv(pass, bin.Y) + if !ok { + return + } + lText := astutil.NodeText(pass.Fset, lhsArg) + rText := astutil.NodeText(pass.Fset, rhsArg) + if lText == "" || rText == "" { + return + } + qualifier, skipFix := bytesQualifier(pass, bin.Pos()) + if bin.Op == token.EQL { + var fixes []analysis.SuggestedFix + if !skipFix { + fixes = buildFix(pass, bin, fmt.Sprintf("%s.Equal(%s, %s)", qualifier, lText, rText), seenImportFiles) } - - // Determine the local qualifier for "bytes" and whether the fix is safe. - qualifier, skipFix := bytesQualifier(pass, bin.Pos()) - - if bin.Op == token.EQL { - var fixes []analysis.SuggestedFix - if !skipFix { - fixes = buildFix(pass, bin, fmt.Sprintf("%s.Equal(%s, %s)", qualifier, lText, rText), seenImportFiles) - } - pass.Report(analysis.Diagnostic{ - Pos: bin.Pos(), - End: bin.End(), - Message: fmt.Sprintf("string(%s) == string(%s) is a []byte comparison written the long way; use bytes.Equal(%s, %s) for clearer intent", lText, rText, lText, rText), - SuggestedFixes: fixes, - }) - } else { - var fixes []analysis.SuggestedFix - if !skipFix { - fixes = buildFix(pass, bin, fmt.Sprintf("!%s.Equal(%s, %s)", qualifier, lText, rText), seenImportFiles) - } - pass.Report(analysis.Diagnostic{ - Pos: bin.Pos(), - End: bin.End(), - Message: fmt.Sprintf("string(%s) != string(%s) is a []byte comparison written the long way; use !bytes.Equal(%s, %s) for clearer intent", lText, rText, lText, rText), - SuggestedFixes: fixes, - }) + pass.Report(analysis.Diagnostic{ + Pos: bin.Pos(), + End: bin.End(), + Message: fmt.Sprintf("string(%s) == string(%s) is a []byte comparison written the long way; use bytes.Equal(%s, %s) for clearer intent", lText, rText, lText, rText), + SuggestedFixes: fixes, + }) + } else { + var fixes []analysis.SuggestedFix + if !skipFix { + fixes = buildFix(pass, bin, fmt.Sprintf("!%s.Equal(%s, %s)", qualifier, lText, rText), seenImportFiles) } - }) - - return nil, nil + pass.Report(analysis.Diagnostic{ + Pos: bin.Pos(), + End: bin.End(), + Message: fmt.Sprintf("string(%s) != string(%s) is a []byte comparison written the long way; use !bytes.Equal(%s, %s) for clearer intent", lText, rText, lText, rText), + SuggestedFixes: fixes, + }) + } } // bytesQualifier returns the local binding name for the "bytes" package in the @@ -187,29 +176,25 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos, seenImportFiles map[ if file == nil { return analysis.TextEdit{}, false } - - // Skip if an import edit for this file was already emitted in this pass. if seenImportFiles[file.Pos()] { return analysis.TextEdit{}, false } - - // Check if "bytes" is already imported in this file. for _, imp := range file.Imports { if imp.Path.Value == `"`+bytesPkg+`"` { return analysis.TextEdit{}, false } } + return buildBytesImportTextEdit(pass, file, seenImportFiles) +} - // Compute the edit to add and mark the file so subsequent violations in the - // same pass do not emit a duplicate overlapping TextEdit. - - // Find an existing grouped import declaration to add into. +// buildBytesImportTextEdit constructs the TextEdit that adds a "bytes" import +// to file. It marks the file in seenImportFiles and returns the edit with true. +func buildBytesImportTextEdit(pass *analysis.Pass, file *ast.File, seenImportFiles map[token.Pos]bool) (analysis.TextEdit, bool) { for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { continue } - // Insert "bytes" before the closing paren of the import block. seenImportFiles[file.Pos()] = true return analysis.TextEdit{ Pos: genDecl.Rparen, @@ -217,21 +202,16 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos, seenImportFiles map[ NewText: []byte("\t\"" + bytesPkg + "\"\n"), }, true } - - // If the file has exactly one import (non-grouped), convert it to a grouped - // import block while adding "bytes" before it (alphabetical order). if len(file.Imports) == 1 { for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() || len(genDecl.Specs) != 1 { continue } - specText := astutil.NodeText(pass.Fset, genDecl.Specs[0]) if specText == "" { continue } - seenImportFiles[file.Pos()] = true return analysis.TextEdit{ Pos: genDecl.Pos(), @@ -240,8 +220,6 @@ func addBytesImportEdit(pass *analysis.Pass, pos token.Pos, seenImportFiles map[ }, true } } - - // No grouped import block; insert a standalone import after the package name. seenImportFiles[file.Pos()] = true return analysis.TextEdit{ Pos: file.Name.End(), diff --git a/pkg/linters/errorfwrapv/errorfwrapv.go b/pkg/linters/errorfwrapv/errorfwrapv.go index c77ea26b0b1..14df6b0e4a5 100644 --- a/pkg/linters/errorfwrapv/errorfwrapv.go +++ b/pkg/linters/errorfwrapv/errorfwrapv.go @@ -42,6 +42,10 @@ type formatVerb struct { verb rune } +// formatArgOffset is the index of the first format argument in fmt.Errorf calls. +// call.Args[0] is the format string; real arguments start at index 1. +const formatArgOffset = 1 + // Analyzer is the errorfwrapv analysis pass. var Analyzer = &analysis.Analyzer{ Name: "errorfwrapv", @@ -52,8 +56,6 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - const formatArgOffset = 1 // call.Args[0] is the format string. - if errorIface == nil { return nil, errors.New("failed to resolve built-in error interface from types.Universe") } @@ -71,104 +73,116 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} + insp.Preorder(nodeFilter, func(n ast.Node) { + analyzeFmtErrorfCall(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} + +// analyzeFmtErrorfCall checks whether a call expression is a fmt.Errorf that +// misuses error arguments (via %v or without %w) and reports a diagnostic. +func analyzeFmtErrorfCall(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return } - insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } + position := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { + return + } + if !astutil.IsFmtErrorf(pass, call) { + return + } + if len(call.Args) == 0 { + return + } + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return + } - position := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { - return - } + suppressed := nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") + verbs := parseFormatVerbs(lit.Value) + errorArgVerbs, wrappedErrorArgs, hasVerbV := classifyErrorArgs(pass, call, verbs) - if !astutil.IsFmtErrorf(pass, call) { + if hasVerbV { + if suppressed { return } + pass.ReportRangef(call, "fmt.Errorf formats an error argument with %%v; use %%w to preserve the error chain") + return + } - if len(call.Args) == 0 { - return - } + if len(call.Args) <= formatArgOffset { + return + } - lit, ok := call.Args[0].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - return + for i := formatArgOffset; i < len(call.Args); i++ { + tv, ok := pass.TypesInfo.Types[call.Args[i]] + if !ok || tv.Type == nil { + continue } - suppressed := nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") - - verbs := parseFormatVerbs(lit.Value) - errorArgVerbs := make(map[int][]rune) - wrappedErrorArgs := make(map[int]bool) - for _, fv := range verbs { - callArgIdx := fv.argIdx + formatArgOffset - if callArgIdx >= len(call.Args) { - continue - } - tv, ok := pass.TypesInfo.Types[call.Args[callArgIdx]] - if !ok || tv.Type == nil { - continue - } - if !types.Implements(tv.Type, errorIface) { - continue - } - errorArgVerbs[callArgIdx] = append(errorArgVerbs[callArgIdx], fv.verb) - if fv.verb == 'w' { - wrappedErrorArgs[callArgIdx] = true - } - if fv.verb != 'v' { + if !types.Implements(tv.Type, errorIface) { + continue + } + if wrappedErrorArgs[i] { + continue + } + if verbsForArg, ok := errorArgVerbs[i]; ok { + if !needsWrapping(verbsForArg) { continue } - if suppressed { - return - } - pass.ReportRangef(call, "fmt.Errorf formats an error argument with %%v; use %%w to preserve the error chain") - // Keep diagnostics to one per call to avoid noisy duplicate reports. - return } - - if len(call.Args) <= formatArgOffset { + if suppressed { return } + pass.ReportRangef(call, "fmt.Errorf passes an error argument without %%w; use %%w to preserve the error chain") + // Keep diagnostics to one per call to avoid noisy duplicate reports. + return + } +} - for i := formatArgOffset; i < len(call.Args); i++ { - tv, ok := pass.TypesInfo.Types[call.Args[i]] - if !ok || tv.Type == nil { - continue - } - if !types.Implements(tv.Type, errorIface) { - continue - } - if wrappedErrorArgs[i] { - continue - } - verbsForArg, ok := errorArgVerbs[i] - if ok { - needsWrap := false - for _, verb := range verbsForArg { - if verb == 'T' || verb == 'p' { - continue - } - needsWrap = true - break - } - if !needsWrap { - continue - } - } - if suppressed { - return - } - pass.ReportRangef(call, "fmt.Errorf passes an error argument without %%w; use %%w to preserve the error chain") - // Keep diagnostics to one per call to avoid noisy duplicate reports. - return +// classifyErrorArgs iterates over format verbs and classifies error arguments. +// It returns errorArgVerbs (verb list per arg index), wrappedErrorArgs (args +// that already use %w), and hasVerbV (whether any error arg uses %v). +func classifyErrorArgs(pass *analysis.Pass, call *ast.CallExpr, verbs []formatVerb) (errorArgVerbs map[int][]rune, wrappedErrorArgs map[int]bool, hasVerbV bool) { + errorArgVerbs = make(map[int][]rune) + wrappedErrorArgs = make(map[int]bool) + for _, fv := range verbs { + callArgIdx := fv.argIdx + formatArgOffset + if callArgIdx >= len(call.Args) { + continue } - }) + tv, ok := pass.TypesInfo.Types[call.Args[callArgIdx]] + if !ok || tv.Type == nil { + continue + } + if !types.Implements(tv.Type, errorIface) { + continue + } + errorArgVerbs[callArgIdx] = append(errorArgVerbs[callArgIdx], fv.verb) + if fv.verb == 'w' { + wrappedErrorArgs[callArgIdx] = true + } + if fv.verb == 'v' { + hasVerbV = true + } + } + return +} - return nil, nil +// needsWrapping reports whether a slice of format verbs for an error argument +// requires a %w replacement. Verbs %T and %p are considered display-only and +// do not require wrapping. +func needsWrapping(verbs []rune) bool { + for _, verb := range verbs { + if verb != 'T' && verb != 'p' { + return true + } + } + return false } func parseFormatVerbs(s string) []formatVerb { diff --git a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go index 990011dd9eb..8bada2f8490 100644 --- a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go +++ b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go @@ -11,6 +11,7 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" "github.com/github/gh-aw/pkg/linters/internal/astutil" "github.com/github/gh-aw/pkg/linters/internal/filecheck" @@ -41,63 +42,66 @@ func run(pass *analysis.Pass) (any, error) { } for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { - call, ok := cur.Node().(*ast.CallExpr) - if !ok { - continue - } - sel, ok := execCommandSelector(pass, call) - if !ok { - continue - } + checkExecCommandCall(pass, cur, generatedFiles, noLintIndex) + } + return nil, nil +} - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { +// checkExecCommandCall inspects a single call expression and reports a diagnostic +// when exec.Command is used inside a context-receiving function. +func checkExecCommandCall(pass *analysis.Pass, cur inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := cur.Node().(*ast.CallExpr) + if !ok { + return + } + sel, ok := execCommandSelector(pass, call) + if !ok { + return + } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "execcommandwithoutcontext") { + return + } + for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { + funcNode := encl.Node() + funcType := astutil.EnclosingFuncType(funcNode) + if funcType == nil { continue } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "execcommandwithoutcontext") { + ctxParamName, hasCtx := astutil.ContextParamName(pass, funcType) + if !hasCtx { + if _, isFuncLit := funcNode.(*ast.FuncLit); isFuncLit && !astutil.IsGoOrDeferClosure(encl) { + break + } continue } - - for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { - funcNode := encl.Node() - funcType := astutil.EnclosingFuncType(funcNode) - if funcType == nil { - continue - } - ctxParamName, hasCtx := astutil.ContextParamName(pass, funcType) - if !hasCtx { - if _, isFuncLit := funcNode.(*ast.FuncLit); isFuncLit && !astutil.IsGoOrDeferClosure(encl) { - break - } - continue - } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: fmt.Sprintf("use exec.CommandContext(%s, ...) instead of exec.Command to propagate context cancellation", ctxParamName), - SuggestedFixes: []analysis.SuggestedFix{ - { - Message: fmt.Sprintf("Replace exec.Command with exec.CommandContext(%s, ...)", ctxParamName), - TextEdits: []analysis.TextEdit{ - { - Pos: sel.Sel.Pos(), - End: sel.Sel.End(), - NewText: []byte("CommandContext"), - }, - { - Pos: call.Lparen + 1, - End: call.Lparen + 1, - NewText: []byte(ctxParamName + ", "), - }, + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("use exec.CommandContext(%s, ...) instead of exec.Command to propagate context cancellation", ctxParamName), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("Replace exec.Command with exec.CommandContext(%s, ...)", ctxParamName), + TextEdits: []analysis.TextEdit{ + { + Pos: sel.Sel.Pos(), + End: sel.Sel.End(), + NewText: []byte("CommandContext"), + }, + { + Pos: call.Lparen + 1, + End: call.Lparen + 1, + NewText: []byte(ctxParamName + ", "), }, }, }, - }) - break - } + }, + }) + break } - - return nil, nil } // execCommandSelector reports the selector expression for calls to diff --git a/pkg/linters/hardcodedfilepath/hardcodedfilepath.go b/pkg/linters/hardcodedfilepath/hardcodedfilepath.go index cd9add4e6c8..1d2db6b395e 100644 --- a/pkg/linters/hardcodedfilepath/hardcodedfilepath.go +++ b/pkg/linters/hardcodedfilepath/hardcodedfilepath.go @@ -237,7 +237,6 @@ func run(pass *analysis.Pass) (any, error) { if err != nil { return nil, err } - noLintIndex, err := nolint.Index(pass) if err != nil { return nil, err @@ -249,58 +248,55 @@ func run(pass *analysis.Pass) (any, error) { knownConsts := collectKnownPathConsts(pass) for cur := range insp.Root().Preorder((*ast.BasicLit)(nil)) { - lit, ok := cur.Node().(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - continue - } - - pos := pass.Fset.PositionFor(lit.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - continue - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "hardcodedfilepath") { - continue - } - - raw := unquoteStringLit(lit.Value) - if !isPathLike(raw) { - continue - } - if hasFormatVerb(raw) { - continue - } + checkHardcodedFilePath(pass, cur, generatedFiles, noLintIndex, knownConsts) + } + return nil, nil +} - // Skip literals that are the value of a const declaration — those are - // the canonical definitions, not inline usages. - if isConstDeclValue(cur) { - continue +// checkHardcodedFilePath inspects a single string literal and reports a +// diagnostic when it is a hard-coded file path that should be a named constant. +func checkHardcodedFilePath(pass *analysis.Pass, cur inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex, knownConsts map[string]constRef) { + lit, ok := cur.Node().(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return + } + pos := pass.Fset.PositionFor(lit.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "hardcodedfilepath") { + return + } + raw := unquoteStringLit(lit.Value) + if !isPathLike(raw) { + return + } + if hasFormatVerb(raw) { + return + } + if isConstDeclValue(cur) { + return + } + inLog := enclosingCallIsLogPrint(pass, cur) + if ref, found := knownConsts[raw]; found { + msg := fmt.Sprintf( + "hard-coded file path %q: use constant %s instead of inline string literal", + raw, ref, + ) + if inLog { + msg += " (path appears in log/print call — keeping consistent via constant is especially important)" } - - // Detect whether the literal is a direct argument of a log/print call. - inLog := enclosingCallIsLogPrint(pass, cur) - - if ref, found := knownConsts[raw]; found { - msg := fmt.Sprintf( - "hard-coded file path %q: use constant %s instead of inline string literal", - raw, ref, - ) - if inLog { - msg += " (path appears in log/print call — keeping consistent via constant is especially important)" - } - pass.ReportRangef(lit, "%s", msg) - } else { - msg := fmt.Sprintf( - "hard-coded file path %q: consider extracting as a named constant", - raw, - ) - if inLog { - msg += " (path appears in log/print call)" - } - pass.ReportRangef(lit, "%s", msg) + pass.ReportRangef(lit, "%s", msg) + } else { + msg := fmt.Sprintf( + "hard-coded file path %q: consider extracting as a named constant", + raw, + ) + if inLog { + msg += " (path appears in log/print call)" } + pass.ReportRangef(lit, "%s", msg) } - - return nil, nil } // isConstDeclValue reports whether the cursor's node is the value expression diff --git a/pkg/linters/httpnoctx/httpnoctx.go b/pkg/linters/httpnoctx/httpnoctx.go index dcd087f7f47..b7f0830cac8 100644 --- a/pkg/linters/httpnoctx/httpnoctx.go +++ b/pkg/linters/httpnoctx/httpnoctx.go @@ -53,56 +53,56 @@ func run(pass *analysis.Pass) (any, error) { } for cursor := range insp.Root().Preorder((*ast.CallExpr)(nil)) { - call, ok := cursor.Node().(*ast.CallExpr) - if !ok { - continue - } - - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - continue - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "httpnoctx") { - continue - } - - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - continue - } - if contextFreeMethods[sel.Sel.Name] { - if isHTTPClientReceiver(pass, sel.X) { - pass.ReportRangef(call, - "(*http.Client).%s does not accept a context; use http.NewRequestWithContext + client.Do to propagate cancellation", - sel.Sel.Name, - ) - continue - } - - if isHTTPPackage(pass, sel.X) { - pass.ReportRangef(call, - "http.%s does not accept a context; use http.NewRequestWithContext + http.DefaultClient.Do to propagate cancellation", - sel.Sel.Name, - ) - continue - } - } + checkHTTPCall(pass, cursor, generatedFiles, noLintIndex) + } + return nil, nil +} - if sel.Sel.Name == "NewRequest" && isHTTPPackage(pass, sel.X) && hasContextInEnclosingFunc(pass, cursor) { +// checkHTTPCall inspects a single call expression and reports a diagnostic +// when it is a context-free HTTP request path. +func checkHTTPCall(pass *analysis.Pass, cursor inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := cursor.Node().(*ast.CallExpr) + if !ok { + return + } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "httpnoctx") { + return + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return + } + if contextFreeMethods[sel.Sel.Name] { + if isHTTPClientReceiver(pass, sel.X) { pass.ReportRangef(call, - "http.NewRequest does not propagate context; use http.NewRequestWithContext when context.Context is in scope", + "(*http.Client).%s does not accept a context; use http.NewRequestWithContext + client.Do to propagate cancellation", + sel.Sel.Name, ) - continue + return } - - if sel.Sel.Name == "Do" && isHTTPDefaultClient(pass, sel.X) { + if isHTTPPackage(pass, sel.X) { pass.ReportRangef(call, - "http.DefaultClient.Do uses a timeout-less client; use a dedicated *http.Client with Timeout set", + "http.%s does not accept a context; use http.NewRequestWithContext + http.DefaultClient.Do to propagate cancellation", + sel.Sel.Name, ) + return } } - - return nil, nil + if sel.Sel.Name == "NewRequest" && isHTTPPackage(pass, sel.X) && hasContextInEnclosingFunc(pass, cursor) { + pass.ReportRangef(call, + "http.NewRequest does not propagate context; use http.NewRequestWithContext when context.Context is in scope", + ) + return + } + if sel.Sel.Name == "Do" && isHTTPDefaultClient(pass, sel.X) { + pass.ReportRangef(call, + "http.DefaultClient.Do uses a timeout-less client; use a dedicated *http.Client with Timeout set", + ) + } } // isHTTPClientReceiver reports whether expr has type *http.Client. diff --git a/pkg/linters/ioutildeprecated/ioutildeprecated.go b/pkg/linters/ioutildeprecated/ioutildeprecated.go index d9a48ab7d75..fbfd47934b2 100644 --- a/pkg/linters/ioutildeprecated/ioutildeprecated.go +++ b/pkg/linters/ioutildeprecated/ioutildeprecated.go @@ -9,6 +9,7 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" "github.com/github/gh-aw/pkg/linters/internal/astutil" "github.com/github/gh-aw/pkg/linters/internal/filecheck" @@ -54,13 +55,18 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } - // Handle regular qualified imports: ioutil.ReadAll(...), ioutil.Discard, etc. + checkQualifiedIoutilUsage(pass, root, generatedFiles, noLintIndex) + checkDotImportIoutilUsage(pass, root, generatedFiles, noLintIndex) + return nil, nil +} + +// checkQualifiedIoutilUsage reports ioutil.X usages via qualified selector expressions. +func checkQualifiedIoutilUsage(pass *analysis.Pass, root inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { for cur := range root.Preorder((*ast.SelectorExpr)(nil)) { sel, ok := cur.Node().(*ast.SelectorExpr) if !ok { continue } - pos := pass.Fset.PositionFor(sel.Pos(), false) if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { continue @@ -68,7 +74,6 @@ func run(pass *analysis.Pass) (any, error) { if nolint.HasDirectiveForLinter(pos, noLintIndex, "ioutildeprecated") { continue } - pkgIdent, ok := sel.X.(*ast.Ident) if !ok { continue @@ -81,28 +86,23 @@ func run(pass *analysis.Pass) (any, error) { if !ok || pkgName.Imported().Path() != "io/ioutil" { continue } - funcName := sel.Sel.Name if replacement, found := replacements[funcName]; found { pass.ReportRangef(sel, "ioutil.%s is deprecated; use %s instead", funcName, replacement) } } +} - // Handle dot imports: import . "io/ioutil" followed by bare ReadAll(r) or Discard. - // In this case the identifier is an *ast.Ident (not a SelectorExpr), and - // TypesInfo.Uses resolves it to an object whose package path is "io/ioutil". +// checkDotImportIoutilUsage reports bare ioutil function names used via dot-imports. +func checkDotImportIoutilUsage(pass *analysis.Pass, root inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { for cur := range root.Preorder((*ast.Ident)(nil)) { ident, ok := cur.Node().(*ast.Ident) if !ok { continue } - - // Skip identifiers that are the Sel field of a SelectorExpr; those are - // already handled by the qualified-import loop above. if _, ok := cur.Parent().Node().(*ast.SelectorExpr); ok { continue } - pos := pass.Fset.PositionFor(ident.Pos(), false) if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { continue @@ -110,7 +110,6 @@ func run(pass *analysis.Pass) (any, error) { if nolint.HasDirectiveForLinter(pos, noLintIndex, "ioutildeprecated") { continue } - obj := pass.TypesInfo.Uses[ident] if obj == nil { continue @@ -119,12 +118,9 @@ func run(pass *analysis.Pass) (any, error) { if pkg == nil || pkg.Path() != "io/ioutil" { continue } - name := obj.Name() if replacement, found := replacements[name]; found { pass.ReportRangef(ident, "ioutil.%s is deprecated; use %s instead", name, replacement) } } - - return nil, nil } diff --git a/pkg/linters/largefunc/largefunc.go b/pkg/linters/largefunc/largefunc.go index bff3f3c7ba7..bd4720acea9 100644 --- a/pkg/linters/largefunc/largefunc.go +++ b/pkg/linters/largefunc/largefunc.go @@ -52,53 +52,53 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.FuncDecl)(nil), - (*ast.FuncLit)(nil), + nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)} + insp.Preorder(nodeFilter, func(n ast.Node) { + checkFuncBodyLength(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} + +// checkFuncBodyLength reports a diagnostic when the body of a function +// declaration or literal exceeds maxLines. +func checkFuncBodyLength(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + var body *ast.BlockStmt + var name string + var reportNode ast.Node + + switch fn := n.(type) { + case *ast.FuncDecl: + body = fn.Body + name = fn.Name.Name + reportNode = fn.Name + case *ast.FuncLit: + body = fn.Body + name = "func literal" + reportNode = body } - insp.Preorder(nodeFilter, func(n ast.Node) { - var body *ast.BlockStmt - var name string - var reportNode ast.Node - - switch fn := n.(type) { - case *ast.FuncDecl: - body = fn.Body - name = fn.Name.Name - reportNode = fn.Name - case *ast.FuncLit: - body = fn.Body - name = "func literal" - reportNode = body - } + if body == nil { + return + } - if body == nil { - return - } + position := pass.Fset.PositionFor(reportNode.Pos(), false) + if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { + return + } - position := pass.Fset.PositionFor(reportNode.Pos(), false) - if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { - return - } + start := pass.Fset.Position(body.Lbrace) + end := pass.Fset.Position(body.Rbrace) + lines := end.Line - start.Line - 1 - start := pass.Fset.Position(body.Lbrace) - end := pass.Fset.Position(body.Rbrace) - // Subtract 1 to exclude the closing brace line itself, counting only body lines. - lines := end.Line - start.Line - 1 - - if lines > maxLines { - if nolint.HasDirectiveForLinter(position, noLintIndex, "largefunc") { - return - } - pkgLog.Printf("flagging %s: %d lines exceeds limit %d", name, lines, maxLines) - pass.ReportRangef( - reportNode, - "%s is %d lines long (limit: %d); consider breaking it up", - name, lines, maxLines, - ) + if lines > maxLines { + if nolint.HasDirectiveForLinter(position, noLintIndex, "largefunc") { + return } - }) - - return nil, nil + pkgLog.Printf("flagging %s: %d lines exceeds limit %d", name, lines, maxLines) + pass.ReportRangef( + reportNode, + "%s is %d lines long (limit: %d); consider breaking it up", + name, lines, maxLines, + ) + } } diff --git a/pkg/linters/lenstringzero/lenstringzero.go b/pkg/linters/lenstringzero/lenstringzero.go index 16ff2e04c75..34f220a3639 100644 --- a/pkg/linters/lenstringzero/lenstringzero.go +++ b/pkg/linters/lenstringzero/lenstringzero.go @@ -41,62 +41,60 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } lenStringAliases := collectLenStringAliases(pass) - nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)} - insp.Preorder(nodeFilter, func(n ast.Node) { - expr, ok := n.(*ast.BinaryExpr) - if !ok { - return - } - switch expr.Op { - case token.EQL, token.NEQ, token.GTR, token.GEQ, token.LSS, token.LEQ: - default: - return - } - - pos := pass.Fset.PositionFor(expr.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "lenstringzero") { - return - } - - lenArg, isDirect, normalOp, lit, matched := matchLenLiteralExpr(pass, expr, lenStringAliases) - if !matched { - return - } - - fixOp, cmpVerb, valid := resolveFixOp(normalOp, lit) - if !valid { - return - } - - t := pass.TypesInfo.TypeOf(lenArg) - if t == nil { - return - } - basic, ok := t.Underlying().(*types.Basic) - if !ok || basic.Kind() != types.String { - return - } - - var fixes []analysis.SuggestedFix - if isDirect { - fixes = buildLenStringFix(pass, expr, lenArg, fixOp) - } - pass.Report(analysis.Diagnostic{ - Pos: expr.Pos(), - End: expr.End(), - Message: fmt.Sprintf(`use s %s "" to check for %s string instead of len(s) %s %d`, fixOp, cmpVerb, normalOp, lit), - SuggestedFixes: fixes, - }) + analyzeLenStringExpr(pass, n, generatedFiles, noLintIndex, lenStringAliases) }) - return nil, nil } +// analyzeLenStringExpr checks whether a binary expression is a len(s) comparison +// with 0 or 1 that should use == "" or != "" instead. +func analyzeLenStringExpr(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex, lenStringAliases map[types.Object]ast.Expr) { + expr, ok := n.(*ast.BinaryExpr) + if !ok { + return + } + switch expr.Op { + case token.EQL, token.NEQ, token.GTR, token.GEQ, token.LSS, token.LEQ: + default: + return + } + pos := pass.Fset.PositionFor(expr.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "lenstringzero") { + return + } + lenArg, isDirect, normalOp, lit, matched := matchLenLiteralExpr(pass, expr, lenStringAliases) + if !matched { + return + } + fixOp, cmpVerb, valid := resolveFixOp(normalOp, lit) + if !valid { + return + } + t := pass.TypesInfo.TypeOf(lenArg) + if t == nil { + return + } + basic, ok := t.Underlying().(*types.Basic) + if !ok || basic.Kind() != types.String { + return + } + var fixes []analysis.SuggestedFix + if isDirect { + fixes = buildLenStringFix(pass, expr, lenArg, fixOp) + } + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: fmt.Sprintf(`use s %s "" to check for %s string instead of len(s) %s %d`, fixOp, cmpVerb, normalOp, lit), + SuggestedFixes: fixes, + }) +} + // matchLenLiteralExpr tries to match len(s)/alias OP literal or literal OP len(s)/alias. // Returns (lenArg, isDirect, normalOp, lit, matched) where: // - lenArg is the string expression passed to len() diff --git a/pkg/linters/mapclearloop/mapclearloop.go b/pkg/linters/mapclearloop/mapclearloop.go index ea597a8f6c8..76f18dbe75c 100644 --- a/pkg/linters/mapclearloop/mapclearloop.go +++ b/pkg/linters/mapclearloop/mapclearloop.go @@ -40,117 +40,129 @@ func run(pass *analysis.Pass) (any, error) { } nodeFilter := []ast.Node{(*ast.RangeStmt)(nil)} - insp.Preorder(nodeFilter, func(n ast.Node) { - rangeStmt, ok := n.(*ast.RangeStmt) - if !ok { - return - } + analyzeRangeStmt(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - pos := pass.Fset.PositionFor(rangeStmt.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "mapclearloop") { - return - } +// analyzeRangeStmt checks whether a range statement is a clearable range-delete +// loop and reports a diagnostic if so. +func analyzeRangeStmt(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + rangeStmt, ok := n.(*ast.RangeStmt) + if !ok { + return + } - // The range expression must be a map type. - mapType := pass.TypesInfo.TypeOf(rangeStmt.X) - if mapType == nil { - return - } - if _, ok := mapType.Underlying().(*types.Map); !ok { - return - } + pos := pass.Fset.PositionFor(rangeStmt.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "mapclearloop") { + return + } - // The key variable must be present (not blank or absent). - keyIdent, ok := rangeStmt.Key.(*ast.Ident) - if !ok || keyIdent.Name == "_" { - return - } - keyObj := pass.TypesInfo.Defs[keyIdent] - if keyObj == nil { - keyObj = pass.TypesInfo.Uses[keyIdent] - } - if keyObj == nil { - return - } + mapType := pass.TypesInfo.TypeOf(rangeStmt.X) + if mapType == nil { + return + } + if _, ok := mapType.Underlying().(*types.Map); !ok { + return + } - // The value variable must be absent or blank. - if rangeStmt.Value != nil { - valueIdent, ok := rangeStmt.Value.(*ast.Ident) - if !ok || valueIdent.Name != "_" { - return - } - } + keyObj, ok := extractRangeKeyObj(pass, rangeStmt) + if !ok { + return + } - // The body must contain exactly one statement: delete(m, k). - if len(rangeStmt.Body.List) != 1 { - return - } - exprStmt, ok := rangeStmt.Body.List[0].(*ast.ExprStmt) - if !ok { - return - } - callExpr, ok := exprStmt.X.(*ast.CallExpr) - if !ok { - return - } - delIdent, ok := callExpr.Fun.(*ast.Ident) - if !ok || delIdent.Name != "delete" { - return - } - delBuiltin, ok := pass.TypesInfo.Uses[delIdent].(*types.Builtin) - if !ok || delBuiltin.Name() != "delete" { - return - } - if len(callExpr.Args) != 2 { - return - } + if !matchDeleteBody(pass, rangeStmt, keyObj) { + return + } - // First arg to delete must be the same map as the range expression. - if !sameObject(pass, callExpr.Args[0], rangeStmt.X) { - return - } + mText := astutil.NodeText(pass.Fset, rangeStmt.X) + if mText == "" { + return + } + if !builtinVisibleAtPos(pass.Pkg, rangeStmt.Pos(), "clear") { + return + } - // Second arg to delete must be the key variable from the range. - delKeyIdent, ok := callExpr.Args[1].(*ast.Ident) - if !ok { - return - } - delKeyObj := pass.TypesInfo.Uses[delKeyIdent] - if delKeyObj == nil || delKeyObj != keyObj { - return - } + diag := analysis.Diagnostic{ + Pos: rangeStmt.Pos(), + End: rangeStmt.End(), + Message: "range-delete loop over map can be replaced with clear(" + mText + ")", + } + if !astutil.HasOverlappingComment(pass.Files, rangeStmt.Pos(), rangeStmt.End()) { + diag.SuggestedFixes = []analysis.SuggestedFix{{ + Message: "Replace range-delete loop with clear", + TextEdits: []analysis.TextEdit{{ + Pos: rangeStmt.Pos(), + End: rangeStmt.End(), + NewText: []byte("clear(" + mText + ")"), + }}, + }} + } + pass.Report(diag) +} - mText := astutil.NodeText(pass.Fset, rangeStmt.X) - if mText == "" { - return - } - if !builtinVisibleAtPos(pass.Pkg, rangeStmt.Pos(), "clear") { - return - } +// matchDeleteBody reports whether rangeStmt.Body contains exactly one +// delete(m, k) call where k is the range key object and m is the range map. +func matchDeleteBody(pass *analysis.Pass, rangeStmt *ast.RangeStmt, keyObj types.Object) bool { + if len(rangeStmt.Body.List) != 1 { + return false + } + exprStmt, ok := rangeStmt.Body.List[0].(*ast.ExprStmt) + if !ok { + return false + } + callExpr, ok := exprStmt.X.(*ast.CallExpr) + if !ok { + return false + } + delIdent, ok := callExpr.Fun.(*ast.Ident) + if !ok || delIdent.Name != "delete" { + return false + } + delBuiltin, ok := pass.TypesInfo.Uses[delIdent].(*types.Builtin) + if !ok || delBuiltin.Name() != "delete" { + return false + } + if len(callExpr.Args) != 2 { + return false + } + if !sameObject(pass, callExpr.Args[0], rangeStmt.X) { + return false + } + delKeyIdent, ok := callExpr.Args[1].(*ast.Ident) + if !ok { + return false + } + delKeyObj := pass.TypesInfo.Uses[delKeyIdent] + return delKeyObj != nil && delKeyObj == keyObj +} - diag := analysis.Diagnostic{ - Pos: rangeStmt.Pos(), - End: rangeStmt.End(), - Message: "range-delete loop over map can be replaced with clear(" + mText + ")", - } - if !astutil.HasOverlappingComment(pass.Files, rangeStmt.Pos(), rangeStmt.End()) { - diag.SuggestedFixes = []analysis.SuggestedFix{{ - Message: "Replace range-delete loop with clear", - TextEdits: []analysis.TextEdit{{ - Pos: rangeStmt.Pos(), - End: rangeStmt.End(), - NewText: []byte("clear(" + mText + ")"), - }}, - }} +// extractRangeKeyObj validates the key and value variables of a range statement +// and returns the key object. The key must be non-blank; the value must be absent +// or blank. Returns (nil, false) when the conditions are not met. +func extractRangeKeyObj(pass *analysis.Pass, rangeStmt *ast.RangeStmt) (types.Object, bool) { + keyIdent, ok := rangeStmt.Key.(*ast.Ident) + if !ok || keyIdent.Name == "_" { + return nil, false + } + keyObj := pass.TypesInfo.Defs[keyIdent] + if keyObj == nil { + keyObj = pass.TypesInfo.Uses[keyIdent] + } + if keyObj == nil { + return nil, false + } + if rangeStmt.Value != nil { + valueIdent, ok := rangeStmt.Value.(*ast.Ident) + if !ok || valueIdent.Name != "_" { + return nil, false } - pass.Report(diag) - }) - - return nil, nil + } + return keyObj, true } // builtinVisibleAtPos reports whether name resolves to a builtin object at pos. diff --git a/pkg/linters/mapdeletecheck/mapdeletecheck.go b/pkg/linters/mapdeletecheck/mapdeletecheck.go index 8f1992fe696..13da6a4f4a9 100644 --- a/pkg/linters/mapdeletecheck/mapdeletecheck.go +++ b/pkg/linters/mapdeletecheck/mapdeletecheck.go @@ -40,101 +40,97 @@ func run(pass *analysis.Pass) (any, error) { } nodeFilter := []ast.Node{(*ast.IfStmt)(nil)} - insp.Preorder(nodeFilter, func(n ast.Node) { - ifStmt, ok := n.(*ast.IfStmt) - if !ok { - return - } - - pos := pass.Fset.PositionFor(ifStmt.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "mapdeletecheck") { - return - } - - // Must have an init statement: _, ok := m[k] - // Must have a simple condition: ok (or the bool variable from the assignment) - // Must have a single-statement body: delete(m, k) - // Must have no else clause. + analyzeIfStmt(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - if ifStmt.Else != nil { - return - } +// analyzeIfStmt checks whether an if statement is a redundant membership check +// before a delete call and reports a diagnostic if so. +func analyzeIfStmt(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + ifStmt, ok := n.(*ast.IfStmt) + if !ok { + return + } - mapExpr, keyExpr, okIdent := matchMapIndexAssign(pass, ifStmt.Init) - if okIdent == nil { - return - } + pos := pass.Fset.PositionFor(ifStmt.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "mapdeletecheck") { + return + } + if ifStmt.Else != nil { + return + } - // The condition must be the ok identifier from the init. - condIdent, ok := ifStmt.Cond.(*ast.Ident) - if !ok { - return - } - // Make sure the condition refers to the same object as the init. - condObj := pass.TypesInfo.Uses[condIdent] - okObj := pass.TypesInfo.Defs[okIdent] - if condObj == nil || okObj == nil || condObj != okObj { - return - } + mapExpr, keyExpr, okIdent := matchMapIndexAssign(pass, ifStmt.Init) + if okIdent == nil { + return + } - // The body must be exactly one statement: delete(m, k) - if len(ifStmt.Body.List) != 1 { - return - } - exprStmt, ok := ifStmt.Body.List[0].(*ast.ExprStmt) - if !ok { - return - } - delCall, ok := exprStmt.X.(*ast.CallExpr) - if !ok { - return - } - delIdent, ok := delCall.Fun.(*ast.Ident) - if !ok || delIdent.Name != "delete" { - return - } - delBuiltin, ok := pass.TypesInfo.Uses[delIdent].(*types.Builtin) - if !ok || delBuiltin.Name() != "delete" { - return - } - if len(delCall.Args) != 2 { - return - } + condIdent, ok := ifStmt.Cond.(*ast.Ident) + if !ok { + return + } + condObj := pass.TypesInfo.Uses[condIdent] + okObj := pass.TypesInfo.Defs[okIdent] + if condObj == nil || okObj == nil || condObj != okObj { + return + } - // delete(m, k) must use the same map and key as the index expression. - if !sameExpr(pass, delCall.Args[0], mapExpr) { - return - } - if !sameExpr(pass, delCall.Args[1], keyExpr) { - return - } + if !matchDeleteCallStmt(pass, ifStmt.Body, mapExpr, keyExpr) { + return + } - mText := astutil.NodeText(pass.Fset, mapExpr) - kText := astutil.NodeText(pass.Fset, keyExpr) + mText := astutil.NodeText(pass.Fset, mapExpr) + kText := astutil.NodeText(pass.Fset, keyExpr) - diag := analysis.Diagnostic{ - Pos: ifStmt.Pos(), - End: ifStmt.End(), - Message: "redundant existence check before delete: delete(" + mText + ", " + kText + ") is already a no-op when the key is absent; remove the if statement", - } - if !astutil.HasOverlappingComment(pass.Files, ifStmt.Pos(), ifStmt.End()) { - diag.SuggestedFixes = []analysis.SuggestedFix{{ - Message: "Replace if-check with plain delete", - TextEdits: []analysis.TextEdit{{ - Pos: ifStmt.Pos(), - End: ifStmt.End(), - NewText: []byte("delete(" + mText + ", " + kText + ")"), - }}, - }} - } - pass.Report(diag) - }) + diag := analysis.Diagnostic{ + Pos: ifStmt.Pos(), + End: ifStmt.End(), + Message: "redundant existence check before delete: delete(" + mText + ", " + kText + ") is already a no-op when the key is absent; remove the if statement", + } + if !astutil.HasOverlappingComment(pass.Files, ifStmt.Pos(), ifStmt.End()) { + diag.SuggestedFixes = []analysis.SuggestedFix{{ + Message: "Replace if-check with plain delete", + TextEdits: []analysis.TextEdit{{ + Pos: ifStmt.Pos(), + End: ifStmt.End(), + NewText: []byte("delete(" + mText + ", " + kText + ")"), + }}, + }} + } + pass.Report(diag) +} - return nil, nil +// matchDeleteCallStmt reports whether body contains exactly one statement of +// the form delete(mapExpr, keyExpr) using the same map and key expressions. +func matchDeleteCallStmt(pass *analysis.Pass, body *ast.BlockStmt, mapExpr, keyExpr ast.Expr) bool { + if len(body.List) != 1 { + return false + } + exprStmt, ok := body.List[0].(*ast.ExprStmt) + if !ok { + return false + } + delCall, ok := exprStmt.X.(*ast.CallExpr) + if !ok { + return false + } + delIdent, ok := delCall.Fun.(*ast.Ident) + if !ok || delIdent.Name != "delete" { + return false + } + delBuiltin, ok := pass.TypesInfo.Uses[delIdent].(*types.Builtin) + if !ok || delBuiltin.Name() != "delete" { + return false + } + if len(delCall.Args) != 2 { + return false + } + return sameExpr(pass, delCall.Args[0], mapExpr) && sameExpr(pass, delCall.Args[1], keyExpr) } // matchMapIndexAssign returns map expression, key expression, and ok identifier diff --git a/pkg/linters/nilctxpassed/nilctxpassed.go b/pkg/linters/nilctxpassed/nilctxpassed.go index 2f15a6330f5..4b42c38f6eb 100644 --- a/pkg/linters/nilctxpassed/nilctxpassed.go +++ b/pkg/linters/nilctxpassed/nilctxpassed.go @@ -8,6 +8,7 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" "github.com/github/gh-aw/pkg/linters/internal/astutil" "github.com/github/gh-aw/pkg/linters/internal/filecheck" @@ -38,64 +39,59 @@ func run(pass *analysis.Pass) (any, error) { } for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { - call, ok := cur.Node().(*ast.CallExpr) - if !ok { + checkCallForNilContext(pass, cur, generatedFiles, noLintIndex) + } + return nil, nil +} + +// checkCallForNilContext inspects a single call expression and reports a +// diagnostic for each argument that passes nil as a context.Context. +func checkCallForNilContext(pass *analysis.Pass, cur inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := cur.Node().(*ast.CallExpr) + if !ok { + return + } + callPos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(callPos.Filename, generatedFiles) { + return + } + sig := calleeSignature(pass, call) + if sig == nil { + return + } + params := sig.Params() + for i, arg := range call.Args { + var paramType types.Type + if sig.Variadic() && params.Len() > 0 && i >= params.Len()-1 { + if call.Ellipsis.IsValid() && i == len(call.Args)-1 { + continue + } + sliceType, ok := params.At(params.Len() - 1).Type().(*types.Slice) + if !ok { + continue + } + paramType = sliceType.Elem() + } else if i < params.Len() { + paramType = params.At(i).Type() + } else { continue } - - callPos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(callPos.Filename, generatedFiles) { + if !isContextContext(paramType) { continue } - - sig := calleeSignature(pass, call) - if sig == nil { + if !isBuiltinNil(pass, arg) { continue } - - params := sig.Params() - for i, arg := range call.Args { - var paramType types.Type - if sig.Variadic() && params.Len() > 0 && i >= params.Len()-1 { - if call.Ellipsis.IsValid() && i == len(call.Args)-1 { - // Spread call passes the whole variadic slice (e.g. f(nil...)), - // not an individual variadic element. - continue - } - // Variadic: the last param is a slice; check its element type. - sliceType, ok := params.At(params.Len() - 1).Type().(*types.Slice) - if !ok { - continue - } - paramType = sliceType.Elem() - } else if i < params.Len() { - paramType = params.At(i).Type() - } else { - continue - } - - if !isContextContext(paramType) { - continue - } - - if !isBuiltinNil(pass, arg) { - continue - } - - argPos := pass.Fset.PositionFor(arg.Pos(), false) - if nolint.HasDirectiveForLinter(argPos, noLintIndex, "nilctxpassed") { - continue - } - - pass.Report(analysis.Diagnostic{ - Pos: arg.Pos(), - End: arg.End(), - Message: "nil passed as context.Context; use context.Background() or context.TODO() instead", - }) + argPos := pass.Fset.PositionFor(arg.Pos(), false) + if nolint.HasDirectiveForLinter(argPos, noLintIndex, "nilctxpassed") { + continue } + pass.Report(analysis.Diagnostic{ + Pos: arg.Pos(), + End: arg.End(), + Message: "nil passed as context.Context; use context.Background() or context.TODO() instead", + }) } - - return nil, nil } // isContextContext reports whether t is the context.Context interface type, diff --git a/pkg/linters/seenmapbool/seenmapbool.go b/pkg/linters/seenmapbool/seenmapbool.go index a2e41e6413a..d75c262328b 100644 --- a/pkg/linters/seenmapbool/seenmapbool.go +++ b/pkg/linters/seenmapbool/seenmapbool.go @@ -70,12 +70,32 @@ func run(pass *analysis.Pass) (any, error) { // inspectBody walks a function body and reports map[string]bool variables // that are only ever assigned the literal true (i.e., used as a set). func inspectBody(pass *analysis.Pass, body *ast.BlockStmt, noLintIndex nolint.DirectiveIndex) { - // Collect map[string]bool local variables defined in this scope. - candidates := make(map[types.Object]ast.Node) // object -> declaration node for reporting + candidates := collectSeenMapCandidates(pass, body) + if len(candidates) == 0 { + return + } + + nonSetMaps := findNonSetMaps(pass, body, candidates) + + for obj, declNode := range candidates { + if nonSetMaps[obj] { + continue + } + if nolint.HasDirectiveForLinter(pass.Fset.PositionFor(declNode.Pos(), false), noLintIndex, "seenmapbool") { + continue + } + pass.ReportRangef( + declNode, + "map[string]bool %q used as a set; use map[string]struct{} to avoid allocating a bool per entry", + obj.Name(), + ) + } +} - // First pass: collect declarations of map[string]bool locals. - // Stop at nested FuncLit boundaries so each closure is handled by its own - // Preorder visit — preventing duplicate diagnostics. +// collectSeenMapCandidates returns a map of local map[string]bool variables +// declared in body (via := or var), stopping at nested function literals. +func collectSeenMapCandidates(pass *analysis.Pass, body *ast.BlockStmt) map[types.Object]ast.Node { + candidates := make(map[types.Object]ast.Node) ast.Inspect(body, func(n ast.Node) bool { if n == nil { return false @@ -85,7 +105,6 @@ func inspectBody(pass *analysis.Pass, body *ast.BlockStmt, noLintIndex nolint.Di } switch stmt := n.(type) { case *ast.AssignStmt: - // seen := make(map[string]bool) or seen := map[string]bool{} if stmt.Tok.String() != ":=" { return true } @@ -106,7 +125,6 @@ func inspectBody(pass *analysis.Pass, body *ast.BlockStmt, noLintIndex nolint.Di } } case *ast.DeclStmt: - // var seen map[string]bool genDecl, ok := stmt.Decl.(*ast.GenDecl) if !ok { return true @@ -132,15 +150,13 @@ func inspectBody(pass *analysis.Pass, body *ast.BlockStmt, noLintIndex nolint.Di } return true }) + return candidates +} - if len(candidates) == 0 { - return - } - - // Second pass: check that every write to these maps only assigns true. - // If any non-true assignment is found, remove the map from candidates. +// findNonSetMaps returns the subset of candidates that are assigned a value +// other than the literal true (and therefore cannot be treated as sets). +func findNonSetMaps(pass *analysis.Pass, body *ast.BlockStmt, candidates map[types.Object]ast.Node) map[types.Object]bool { nonSetMaps := make(map[types.Object]bool) - ast.Inspect(body, func(n ast.Node) bool { assign, ok := n.(*ast.AssignStmt) if !ok { @@ -162,30 +178,13 @@ func inspectBody(pass *analysis.Pass, body *ast.BlockStmt, noLintIndex nolint.Di if _, isCandidate := candidates[obj]; !isCandidate { continue } - // Check the value being assigned. - if i < len(assign.Rhs) { - if !isBoolTrue(assign.Rhs[i]) { - nonSetMaps[obj] = true - } + if i < len(assign.Rhs) && !isBoolTrue(assign.Rhs[i]) { + nonSetMaps[obj] = true } } return true }) - - // Report remaining candidates that are pure sets. - for obj, declNode := range candidates { - if nonSetMaps[obj] { - continue - } - if nolint.HasDirectiveForLinter(pass.Fset.PositionFor(declNode.Pos(), false), noLintIndex, "seenmapbool") { - continue - } - pass.ReportRangef( - declNode, - "map[string]bool %q used as a set; use map[string]struct{} to avoid allocating a bool per entry", - obj.Name(), - ) - } + return nonSetMaps } // isMapStringBool returns true if t is map[string]bool. diff --git a/pkg/linters/sprintfbool/sprintfbool.go b/pkg/linters/sprintfbool/sprintfbool.go index bd6a6a9a68c..8cba92eb422 100644 --- a/pkg/linters/sprintfbool/sprintfbool.go +++ b/pkg/linters/sprintfbool/sprintfbool.go @@ -11,6 +11,7 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" "github.com/github/gh-aw/pkg/linters/internal/astutil" "github.com/github/gh-aw/pkg/linters/internal/filecheck" @@ -28,6 +29,12 @@ type replacement struct { canFix bool } +type candidate struct { + call *ast.CallExpr + arg ast.Expr + file *ast.File +} + // Analyzer is the sprintfbool analysis pass. var Analyzer = &analysis.Analyzer{ Name: "sprintfbool", @@ -51,32 +58,34 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - // seenImportFiles tracks files that have already received an import edit in - // this pass, preventing duplicate overlapping edits when a single file - // contains multiple flagged calls. seenImportFiles := make(map[token.Pos]bool) - orphanFmtByFile := make(map[token.Pos]bool) + candidates, targetCallsByFile, filesByPos := collectSprintfBoolCandidates(pass, insp, generatedFiles, noLintIndex) - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), + replacements := make([]replacement, len(candidates)) + fixableCallsByFile := make(map[token.Pos]int) + for i, c := range candidates { + repl := replacementForCall(pass, c.call, c.arg, c.file) + replacements[i] = repl + if repl.canFix && c.file != nil { + fixableCallsByFile[c.file.Pos()]++ + } } - type candidate struct { - call *ast.CallExpr - arg ast.Expr - file *ast.File - } - candidates := make([]candidate, 0) - targetCallsByFile := make(map[token.Pos]int) - fixableCallsByFile := make(map[token.Pos]int) - filesByPos := make(map[token.Pos]*ast.File) + orphanFmtByFile := computeOrphanFmtStatus(pass, filesByPos, targetCallsByFile, fixableCallsByFile) + reportSprintfBoolDiagnostics(pass, candidates, replacements, seenImportFiles, orphanFmtByFile) + + return nil, nil +} - insp.Preorder(nodeFilter, func(n ast.Node) { +// collectSprintfBoolCandidates traverses the AST and returns all fmt.Sprintf("%t", b) calls. +func collectSprintfBoolCandidates(pass *analysis.Pass, insp *inspector.Inspector, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) ([]candidate, map[token.Pos]int, map[token.Pos]*ast.File) { + candidates := make([]candidate, 0) + targetCallsByFile, filesByPos := make(map[token.Pos]int), make(map[token.Pos]*ast.File) + insp.Preorder([]ast.Node{(*ast.CallExpr)(nil)}, func(n ast.Node) { call, ok := n.(*ast.CallExpr) if !ok { return } - pos := pass.Fset.PositionFor(call.Pos(), false) if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { return @@ -84,8 +93,6 @@ func run(pass *analysis.Pass) (any, error) { if nolint.HasDirectiveForLinter(pos, noLintIndex, "sprintfbool") { return } - - // Match fmt.Sprintf(format, arg) with exactly two arguments. sel, ok := call.Fun.(*ast.SelectorExpr) if !ok || sel.Sel.Name != "Sprintf" { return @@ -96,45 +103,29 @@ func run(pass *analysis.Pass) (any, error) { if len(call.Args) != 2 { return } - - // The format argument must be the string literal "%t". formatLit, ok := call.Args[0].(*ast.BasicLit) if !ok || formatLit.Kind != token.STRING || formatLit.Value != `"%t"` { return } - - // The value argument must have the exact type bool. arg := call.Args[1] argType := pass.TypesInfo.TypeOf(arg) - if argType == nil { + if argType == nil || argType != types.Typ[types.Bool] { return } - if argType != types.Typ[types.Bool] { - return - } - file := fileForPos(pass.Files, call.Pos()) if file != nil { targetCallsByFile[file.Pos()]++ filesByPos[file.Pos()] = file } - - candidates = append(candidates, candidate{ - call: call, - arg: arg, - file: file, - }) + candidates = append(candidates, candidate{call: call, arg: arg, file: file}) }) + return candidates, targetCallsByFile, filesByPos +} - replacements := make([]replacement, len(candidates)) - for i, c := range candidates { - repl := replacementForCall(pass, c.call, c.arg, c.file) - replacements[i] = repl - if repl.canFix && c.file != nil { - fixableCallsByFile[c.file.Pos()]++ - } - } - +// computeOrphanFmtStatus returns a map of file positions where fmt is imported +// solely for the flagged Sprintf calls (and thus can be removed after the fix). +func computeOrphanFmtStatus(pass *analysis.Pass, filesByPos map[token.Pos]*ast.File, targetCallsByFile, fixableCallsByFile map[token.Pos]int) map[token.Pos]bool { + orphanFmtByFile := make(map[token.Pos]bool) for filePos, targetCalls := range targetCallsByFile { file := filesByPos[filePos] if file == nil { @@ -151,7 +142,11 @@ func run(pass *analysis.Pass) (any, error) { countPkgUsesInFile(pass, file, fmtPkg) == targetCalls && fixableCallsByFile[filePos] == targetCalls } + return orphanFmtByFile +} +// reportSprintfBoolDiagnostics emits one diagnostic per candidate. +func reportSprintfBoolDiagnostics(pass *analysis.Pass, candidates []candidate, replacements []replacement, seenImportFiles map[token.Pos]bool, orphanFmtByFile map[token.Pos]bool) { for i, c := range candidates { repl := replacements[i] argText := repl.argText @@ -161,20 +156,10 @@ func run(pass *analysis.Pass) (any, error) { if argText == "" { argText = "b" } - var fixes []analysis.SuggestedFix if repl.canFix { - fixes = buildFormatBoolFix( - pass, - c.call, - repl.argText, - repl.qualifier, - c.file, - seenImportFiles, - orphanFmtByFile, - ) + fixes = buildFormatBoolFix(pass, c.call, repl.argText, repl.qualifier, c.file, seenImportFiles, orphanFmtByFile) } - pass.Report(analysis.Diagnostic{ Pos: c.call.Pos(), End: c.call.End(), @@ -182,8 +167,6 @@ func run(pass *analysis.Pass) (any, error) { SuggestedFixes: fixes, }) } - - return nil, nil } func buildFormatBoolFix( diff --git a/pkg/linters/sprintferrdot/sprintferrdot.go b/pkg/linters/sprintferrdot/sprintferrdot.go index 92fa8be5f57..61df56659f6 100644 --- a/pkg/linters/sprintferrdot/sprintferrdot.go +++ b/pkg/linters/sprintferrdot/sprintferrdot.go @@ -57,58 +57,60 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} + insp.Preorder(nodeFilter, func(n ast.Node) { + analyzeErrDotCall(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} + +// analyzeErrDotCall checks whether a call expression is a fmt format function +// that passes .Error() on an error argument redundantly. +func analyzeErrDotCall(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return } - insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } + formatArgIdx, variadicStart, ok := fmtFormatCallInfo(pass, call) + if !ok { + return + } + if formatArgIdx >= len(call.Args) || variadicStart > len(call.Args) { + return + } - formatArgIdx, variadicStart, ok := fmtFormatCallInfo(pass, call) - if !ok { - return - } - if formatArgIdx >= len(call.Args) || variadicStart > len(call.Args) { - return - } + formatStr, ok := extractStringLit(call.Args[formatArgIdx]) + if !ok { + return + } - formatStr, ok := extractStringLit(call.Args[formatArgIdx]) - if !ok { - return - } + verbs := parseSimpleFormatVerbs(formatStr) + if verbs == nil { + return + } - verbs := parseSimpleFormatVerbs(formatStr) - if verbs == nil { - return + variadicArgs := call.Args[variadicStart:] + for i, arg := range variadicArgs { + if i >= len(verbs) { + break } - - variadicArgs := call.Args[variadicStart:] - for i, arg := range variadicArgs { - if i >= len(verbs) { - break - } - if verbs[i] != 's' && verbs[i] != 'v' { + if verbs[i] != 's' && verbs[i] != 'v' { + continue + } + if isErrorDotCall(pass, arg) { + if nolint.HasDirectiveForLinter(pass.Fset.PositionFor(arg.Pos(), false), noLintIndex, "sprintferrdot") { continue } - if isErrorDotCall(pass, arg) { - if nolint.HasDirectiveForLinter(pass.Fset.PositionFor(arg.Pos(), false), noLintIndex, "sprintferrdot") { - continue - } - pass.Reportf(arg.Pos(), - "redundant .Error() call: pass the error value directly with %%%c", verbs[i]) - } + pass.Reportf(arg.Pos(), + "redundant .Error() call: pass the error value directly with %%%c", verbs[i]) } - }) - - return nil, nil + } } // fmtFormatCallInfo returns the format-string argument index and the diff --git a/pkg/linters/sprintfint/sprintfint.go b/pkg/linters/sprintfint/sprintfint.go index 99b53ce0f79..c587291b072 100644 --- a/pkg/linters/sprintfint/sprintfint.go +++ b/pkg/linters/sprintfint/sprintfint.go @@ -43,69 +43,58 @@ func run(pass *analysis.Pass) (any, error) { if err != nil { return nil, err } - - // seenImportFiles tracks files that have already received an import edit in - // this pass, preventing duplicate overlapping edits when a single file - // contains multiple flagged calls. seenImportFiles := make(map[token.Pos]bool) - - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } - - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "sprintfint") { - return - } - - // Match fmt.Sprintf(format, arg) with exactly two arguments. - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Sprintf" { - return - } - if !astutil.IsPkgSelector(pass, sel, "fmt") { - return - } - if len(call.Args) != 2 { - return - } - - // The format argument must be the string literal "%d". - formatLit, ok := call.Args[0].(*ast.BasicLit) - if !ok || formatLit.Kind != token.STRING || formatLit.Value != `"%d"` { - return - } - - // The value argument must have the exact type int (not int64, uint, etc.). - arg := call.Args[1] - argType := pass.TypesInfo.TypeOf(arg) - if argType == nil { - return - } - if argType != types.Typ[types.Int] { - return - } - - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: `use strconv.Itoa(x) instead of fmt.Sprintf("%d", x)`, - SuggestedFixes: buildItoaFix(pass, call, arg, seenImportFiles), - }) + analyzeSprintfInt(pass, n, generatedFiles, noLintIndex, seenImportFiles) }) - return nil, nil } +// analyzeSprintfInt checks whether a call expression is fmt.Sprintf("%d", x) +// with an int argument and reports a diagnostic to use strconv.Itoa instead. +func analyzeSprintfInt(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex, seenImportFiles map[token.Pos]bool) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "sprintfint") { + return + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Sprintf" { + return + } + if !astutil.IsPkgSelector(pass, sel, "fmt") { + return + } + if len(call.Args) != 2 { + return + } + formatLit, ok := call.Args[0].(*ast.BasicLit) + if !ok || formatLit.Kind != token.STRING || formatLit.Value != `"%d"` { + return + } + arg := call.Args[1] + argType := pass.TypesInfo.TypeOf(arg) + if argType == nil { + return + } + if argType != types.Typ[types.Int] { + return + } + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: `use strconv.Itoa(x) instead of fmt.Sprintf("%d", x)`, + SuggestedFixes: buildItoaFix(pass, call, arg, seenImportFiles), + }) +} + // buildItoaFix returns a SuggestedFix rewriting fmt.Sprintf("%d", x) → // strconv.Itoa(x). It also emits import TextEdits to add "strconv" and, when // the flagged call is the file's only "fmt" reference, to remove the now- diff --git a/pkg/linters/strconvparseignorederror/strconvparseignorederror.go b/pkg/linters/strconvparseignorederror/strconvparseignorederror.go index 782091cb404..08cb2ff233f 100644 --- a/pkg/linters/strconvparseignorederror/strconvparseignorederror.go +++ b/pkg/linters/strconvparseignorederror/strconvparseignorederror.go @@ -47,53 +47,53 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.AssignStmt)(nil), - } - + nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - assign, ok := n.(*ast.AssignStmt) - if !ok { - return - } - // We need exactly 2 LHS targets where the second is _ - if len(assign.Lhs) != 2 || len(assign.Rhs) != 1 { - return - } - // Check second LHS is blank identifier - blank, ok := assign.Lhs[1].(*ast.Ident) - if !ok || blank.Name != "_" { - return - } - // Check RHS is a call to strconv.ParseXxx or strconv.Atoi - call, ok := assign.Rhs[0].(*ast.CallExpr) - if !ok { - return - } - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return - } - if !strconvParseFuncs[sel.Sel.Name] { - return - } - // Verify the receiver is the strconv package - if ident, ok := sel.X.(*ast.Ident); ok { - obj := pass.TypesInfo.Uses[ident] - if pkgName, ok := obj.(*types.PkgName); ok { - if pkgName.Imported().Path() == "strconv" { - position := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(position, nolintIndex, "strconvparseignorederror") { - return - } - pass.ReportRangef(call, "error return from strconv.%s is discarded; parse failures produce zero values silently", sel.Sel.Name) - } - } - } + analyzeStrconvAssign(pass, n, generatedFiles, nolintIndex) }) - return nil, nil } + +// analyzeStrconvAssign checks whether an assignment discards the error return +// from a strconv parsing function and reports a diagnostic if so. +func analyzeStrconvAssign(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, nolintIndex nolint.DirectiveIndex) { + assign, ok := n.(*ast.AssignStmt) + if !ok { + return + } + if len(assign.Lhs) != 2 || len(assign.Rhs) != 1 { + return + } + blank, ok := assign.Lhs[1].(*ast.Ident) + if !ok || blank.Name != "_" { + return + } + call, ok := assign.Rhs[0].(*ast.CallExpr) + if !ok { + return + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return + } + if !strconvParseFuncs[sel.Sel.Name] { + return + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return + } + obj := pass.TypesInfo.Uses[ident] + pkgName, ok := obj.(*types.PkgName) + if !ok || pkgName.Imported().Path() != "strconv" { + return + } + position := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(position, nolintIndex, "strconvparseignorederror") { + return + } + pass.ReportRangef(call, "error return from strconv.%s is discarded; parse failures produce zero values silently", sel.Sel.Name) +} diff --git a/pkg/linters/stringscountcontains/stringscountcontains.go b/pkg/linters/stringscountcontains/stringscountcontains.go index 48011e733bd..d28e4c6e7c7 100644 --- a/pkg/linters/stringscountcontains/stringscountcontains.go +++ b/pkg/linters/stringscountcontains/stringscountcontains.go @@ -41,55 +41,53 @@ func run(pass *analysis.Pass) (any, error) { } nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)} - insp.Preorder(nodeFilter, func(n ast.Node) { - expr, ok := n.(*ast.BinaryExpr) - if !ok { - return - } - - pos := pass.Fset.PositionFor(expr.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringscountcontains") { - return - } - - countCall, negated, matched := matchCountComparison(pass, expr) - if !matched { - return - } - - if len(countCall.Args) != 2 { - return - } - - sText := astutil.NodeText(pass.Fset, countCall.Args[0]) - subText := astutil.NodeText(pass.Fset, countCall.Args[1]) - pkgText := astutil.CallQualifierText(pass.Fset, countCall) - if sText == "" || subText == "" || pkgText == "" { - return - } - - var msg string - if negated { - msg = fmt.Sprintf("use !strings.Contains(%s, %s) instead of strings.Count comparison", sText, subText) - } else { - msg = fmt.Sprintf("use strings.Contains(%s, %s) instead of strings.Count comparison", sText, subText) - } - - pass.Report(analysis.Diagnostic{ - Pos: expr.Pos(), - End: expr.End(), - Message: msg, - SuggestedFixes: astutil.BuildContainsFix(expr, pkgText, sText, subText, negated, "Replace strings.Count comparison with strings.Contains"), - }) + analyzeCountContains(pass, n, generatedFiles, noLintIndex) }) - return nil, nil } +// analyzeCountContains checks whether a binary expression is a strings.Count +// comparison with 0 or 1 that should use strings.Contains. +func analyzeCountContains(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + expr, ok := n.(*ast.BinaryExpr) + if !ok { + return + } + pos := pass.Fset.PositionFor(expr.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringscountcontains") { + return + } + countCall, negated, matched := matchCountComparison(pass, expr) + if !matched { + return + } + if len(countCall.Args) != 2 { + return + } + sText := astutil.NodeText(pass.Fset, countCall.Args[0]) + subText := astutil.NodeText(pass.Fset, countCall.Args[1]) + pkgText := astutil.CallQualifierText(pass.Fset, countCall) + if sText == "" || subText == "" || pkgText == "" { + return + } + var msg string + if negated { + msg = fmt.Sprintf("use !strings.Contains(%s, %s) instead of strings.Count comparison", sText, subText) + } else { + msg = fmt.Sprintf("use strings.Contains(%s, %s) instead of strings.Count comparison", sText, subText) + } + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: msg, + SuggestedFixes: astutil.BuildContainsFix(expr, pkgText, sText, subText, negated, "Replace strings.Count comparison with strings.Contains"), + }) +} + // matchCountComparison reports whether expr is a strings.Count comparison with // 0 or 1 that can be replaced with strings.Contains. // diff --git a/pkg/linters/stringsindexcontains/stringsindexcontains.go b/pkg/linters/stringsindexcontains/stringsindexcontains.go index c92f71e9870..f892edbfa13 100644 --- a/pkg/linters/stringsindexcontains/stringsindexcontains.go +++ b/pkg/linters/stringsindexcontains/stringsindexcontains.go @@ -40,63 +40,54 @@ func run(pass *analysis.Pass) (any, error) { } nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)} - insp.Preorder(nodeFilter, func(n ast.Node) { - expr, ok := n.(*ast.BinaryExpr) - if !ok { - return - } - - pos := pass.Fset.PositionFor(expr.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsindexcontains") { - return - } - - // Match patterns: - // strings.Index(s, sub) != -1 → strings.Contains(s, sub) - // strings.Index(s, sub) >= 0 → strings.Contains(s, sub) - // strings.Index(s, sub) == -1 → !strings.Contains(s, sub) - // strings.Index(s, sub) < 0 → !strings.Contains(s, sub) - // (and yoda variants: -1 != strings.Index(...), etc.) - - indexCall, negated, matched := matchIndexComparison(pass, expr) - if !matched { - return - } - - if len(indexCall.Args) != 2 { - return - } - - sText := astutil.NodeText(pass.Fset, indexCall.Args[0]) - subText := astutil.NodeText(pass.Fset, indexCall.Args[1]) - pkgText := astutil.CallQualifierText(pass.Fset, indexCall) - if sText == "" || subText == "" || pkgText == "" { - return - } - - var msg string - if negated { - msg = "use !strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison" - } else { - msg = "use strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison" - } - - fix := astutil.BuildContainsFix(expr, pkgText, sText, subText, negated, "Replace strings.Index comparison with strings.Contains") - pass.Report(analysis.Diagnostic{ - Pos: expr.Pos(), - End: expr.End(), - Message: msg, - SuggestedFixes: fix, - }) + analyzeIndexContains(pass, n, generatedFiles, noLintIndex) }) - return nil, nil } +// analyzeIndexContains checks whether a binary expression is a strings.Index +// comparison with -1 or 0 that should use strings.Contains. +func analyzeIndexContains(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + expr, ok := n.(*ast.BinaryExpr) + if !ok { + return + } + pos := pass.Fset.PositionFor(expr.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringsindexcontains") { + return + } + indexCall, negated, matched := matchIndexComparison(pass, expr) + if !matched { + return + } + if len(indexCall.Args) != 2 { + return + } + sText := astutil.NodeText(pass.Fset, indexCall.Args[0]) + subText := astutil.NodeText(pass.Fset, indexCall.Args[1]) + pkgText := astutil.CallQualifierText(pass.Fset, indexCall) + if sText == "" || subText == "" || pkgText == "" { + return + } + var msg string + if negated { + msg = "use !strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison" + } else { + msg = "use strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison" + } + fix := astutil.BuildContainsFix(expr, pkgText, sText, subText, negated, "Replace strings.Index comparison with strings.Contains") + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: msg, + SuggestedFixes: fix, + }) +} + // matchIndexComparison reports whether expr is a strings.Index comparison with -1 or 0. // It returns the strings.Index call, whether the result is negated (i.e., checks for absence), // and whether the pattern matched. diff --git a/pkg/linters/timeafterleak/timeafterleak.go b/pkg/linters/timeafterleak/timeafterleak.go index 33bcf1bb0de..f0bf176b59f 100644 --- a/pkg/linters/timeafterleak/timeafterleak.go +++ b/pkg/linters/timeafterleak/timeafterleak.go @@ -124,26 +124,8 @@ func isInsideLoopSelectComm(cur inspector.Cursor) bool { return false } - // If the enclosing SelectStmt has no other CommClause (no other channel case - // and no default), the timer must fire — it cannot be preempted, so there is - // no accumulation. A default clause (CommClause with nil Comm) can preempt - // the timer and is still flagged. - for selCur := range clauseCur.Enclosing((*ast.SelectStmt)(nil)) { - sel, ok := selCur.Node().(*ast.SelectStmt) - if !ok { - break - } - hasOther := false - for _, stmt := range sel.Body.List { - if other, isComm := stmt.(*ast.CommClause); isComm && other != cc { - hasOther = true - break - } - } - if !hasOther { - return false - } - break + if isSingleCaseSelect(clauseCur, cc) { + return false } // Walk up from the CommClause to find an enclosing for or range loop, @@ -162,3 +144,24 @@ func isInsideLoopSelectComm(cur inspector.Cursor) bool { } return false } + +// isSingleCaseSelect reports whether the CommClause cc is the only clause in +// its enclosing SelectStmt. Single-case selects are not flagged because the +// timer must fire — no accumulation is possible. +// A default clause (CommClause with nil Comm) can preempt the timer and is +// not counted as "another case" here; thus such selects are still flagged. +func isSingleCaseSelect(clauseCur inspector.Cursor, cc *ast.CommClause) bool { + for selCur := range clauseCur.Enclosing((*ast.SelectStmt)(nil)) { + sel, ok := selCur.Node().(*ast.SelectStmt) + if !ok { + break + } + for _, stmt := range sel.Body.List { + if other, isComm := stmt.(*ast.CommClause); isComm && other != cc { + return false + } + } + return true + } + return false +} diff --git a/pkg/linters/timenowsub/timenowsub.go b/pkg/linters/timenowsub/timenowsub.go index 011c9cf7598..fd9613cd477 100644 --- a/pkg/linters/timenowsub/timenowsub.go +++ b/pkg/linters/timenowsub/timenowsub.go @@ -38,68 +38,69 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - outer, ok := n.(*ast.CallExpr) - if !ok { - return - } + analyzeTimeNowSub(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - // Match .Sub() where is time.Now(). - sel, ok := outer.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Sub" { - return - } - if len(outer.Args) != 1 { - return - } +// analyzeTimeNowSub checks whether a call is a time.Now().Sub(t) that can be +// simplified to time.Since(t) and reports a diagnostic if so. +func analyzeTimeNowSub(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + outer, ok := n.(*ast.CallExpr) + if !ok { + return + } + + // Match .Sub() where is time.Now(). + sel, ok := outer.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Sub" { + return + } + if len(outer.Args) != 1 { + return + } - // Verify the receiver is a call to time.Now(). - nowCall, ok := sel.X.(*ast.CallExpr) - if !ok { - return - } - qualifier, ok := timeNowQualifier(pass, nowCall) - if !ok { - return - } - if !isSafeSinceArg(outer.Args[0]) { - return - } + nowCall, ok := sel.X.(*ast.CallExpr) + if !ok { + return + } + qualifier, ok := timeNowQualifier(pass, nowCall) + if !ok { + return + } + if !isSafeSinceArg(outer.Args[0]) { + return + } - pos := pass.Fset.PositionFor(outer.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "timenowsub") { - return - } + pos := pass.Fset.PositionFor(outer.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "timenowsub") { + return + } - argText := astutil.NodeText(pass.Fset, outer.Args[0]) - if argText == "" { - return - } - sinceText := qualifier + ".Since(" + argText + ")" + argText := astutil.NodeText(pass.Fset, outer.Args[0]) + if argText == "" { + return + } + sinceText := qualifier + ".Since(" + argText + ")" - pass.Report(analysis.Diagnostic{ - Pos: outer.Pos(), - End: outer.End(), - Message: fmt.Sprintf("%s.Now().Sub(%s) can be simplified to %s", qualifier, argText, sinceText), - SuggestedFixes: []analysis.SuggestedFix{{ - Message: fmt.Sprintf("Replace %s.Now().Sub(%s) with %s", qualifier, argText, sinceText), - TextEdits: []analysis.TextEdit{{ - Pos: outer.Pos(), - End: outer.End(), - NewText: []byte(sinceText), - }}, + pass.Report(analysis.Diagnostic{ + Pos: outer.Pos(), + End: outer.End(), + Message: fmt.Sprintf("%s.Now().Sub(%s) can be simplified to %s", qualifier, argText, sinceText), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: fmt.Sprintf("Replace %s.Now().Sub(%s) with %s", qualifier, argText, sinceText), + TextEdits: []analysis.TextEdit{{ + Pos: outer.Pos(), + End: outer.End(), + NewText: []byte(sinceText), }}, - }) + }}, }) - - return nil, nil } // timeNowQualifier reports the imported identifier used for time.Now(). diff --git a/pkg/linters/trimleftright/trimleftright.go b/pkg/linters/trimleftright/trimleftright.go index e78d5ec1860..4075b400b68 100644 --- a/pkg/linters/trimleftright/trimleftright.go +++ b/pkg/linters/trimleftright/trimleftright.go @@ -45,63 +45,64 @@ func run(pass *analysis.Pass) (any, error) { return nil, err } - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } + analyzeTrimLeftRight(pass, n, generatedFiles, noLintIndex) + }) + return nil, nil +} - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "trimleftright") { - return - } +// analyzeTrimLeftRight checks whether a call is a strings.TrimLeft or +// strings.TrimRight with a suspicious multi-character cutset and reports a +// diagnostic suggesting TrimPrefix or TrimSuffix. +func analyzeTrimLeftRight(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return - } - funcName := sel.Sel.Name - if funcName != "TrimLeft" && funcName != "TrimRight" { - return - } - if !astutil.IsPkgSelector(pass, sel, "strings") { - return - } - if len(call.Args) != 2 { - return - } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "trimleftright") { + return + } - // Only flag suspicious cutsets: multi-rune all-alphanumeric literals - // that do not look like intentional character-set trimming. - cutset, isCutset := stringLitValue(call.Args[1]) - if !isCutset || !looksSuspiciousCutset(cutset) { - return - } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return + } + funcName := sel.Sel.Name + if funcName != "TrimLeft" && funcName != "TrimRight" { + return + } + if !astutil.IsPkgSelector(pass, sel, "strings") { + return + } + if len(call.Args) != 2 { + return + } - var suggested string - switch funcName { - case "TrimLeft": - suggested = "TrimPrefix" - case "TrimRight": - suggested = "TrimSuffix" - } + cutset, isCutset := stringLitValue(call.Args[1]) + if !isCutset || !looksSuspiciousCutset(cutset) { + return + } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: "strings." + funcName + " with a multi-character cutset treats each character independently; " + - "use strings." + suggested + " if you intend to remove the exact string", - }) - }) + var suggested string + switch funcName { + case "TrimLeft": + suggested = "TrimPrefix" + case "TrimRight": + suggested = "TrimSuffix" + } - return nil, nil + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: "strings." + funcName + " with a multi-character cutset treats each character independently; " + + "use strings." + suggested + " if you intend to remove the exact string", + }) } // stringLitValue returns the unquoted string value of a string-literal AST node. diff --git a/pkg/linters/writebytestring/writebytestring.go b/pkg/linters/writebytestring/writebytestring.go index 4a47f363279..ec93bab78a6 100644 --- a/pkg/linters/writebytestring/writebytestring.go +++ b/pkg/linters/writebytestring/writebytestring.go @@ -66,84 +66,91 @@ func run(pass *analysis.Pass) (any, error) { } filesWithImportEdit := make(map[token.Pos]bool) - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - + nodeFilter := []ast.Node{(*ast.CallExpr)(nil)} insp.Preorder(nodeFilter, func(n ast.Node) { - call, ok := n.(*ast.CallExpr) - if !ok { - return - } + analyzeWriteCall(pass, n, generatedFiles, noLintIndex, filesWithImportEdit) + }) + return nil, nil +} - // Match .Write() - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Write" { - return - } - if len(call.Args) != 1 { - return - } +// analyzeWriteCall checks whether a call expression is a w.Write([]byte(s)) +// pattern that can be replaced with io.WriteString(w, s). +func analyzeWriteCall(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex, filesWithImportEdit map[token.Pos]bool) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } - pos := pass.Fset.PositionFor(call.Pos(), false) - if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "writebytestring") { - return - } + // Match .Write() + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Write" { + return + } + if len(call.Args) != 1 { + return + } - // The single argument must be a []byte(s) conversion where s is a string. - conv, ok := call.Args[0].(*ast.CallExpr) - if !ok { - return - } - if !astutil.IsByteSliceConversion(pass, conv) { - return - } - strArg := conv.Args[0] - if !astutil.IsStringType(pass, strArg) { - return - } + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(pos, noLintIndex, "writebytestring") { + return + } - // The receiver must implement io.Writer. - if !implementsWriter(pass, sel.X) { - return - } + // The single argument must be a []byte(s) conversion where s is a string. + conv, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return + } + if !astutil.IsByteSliceConversion(pass, conv) { + return + } + strArg := conv.Args[0] + if !astutil.IsStringType(pass, strArg) { + return + } - sText := astutil.NodeText(pass.Fset, strArg) - wText := astutil.NodeText(pass.Fset, sel.X) - if sText == "" || wText == "" { - return - } + // The receiver must implement io.Writer. + if !implementsWriter(pass, sel.X) { + return + } - // io.WriteString requires an exact predeclared string argument. If the - // argument is a named string type (e.g. type MyStr string), wrap it with - // string(...) so the emitted fix compiles. - sExpr := sText - if st := pass.TypesInfo.TypeOf(strArg); st != nil && !isExactString(st) { - sExpr = "string(" + sText + ")" - } + sText := astutil.NodeText(pass.Fset, strArg) + wText := astutil.NodeText(pass.Fset, sel.X) + if sText == "" || wText == "" { + return + } - // When the receiver is an addressable value whose Write method lives on - // the pointer type (e.g. var buf bytes.Buffer), io.WriteString requires - // the pointer form so that the interface conversion compiles. - writerArg := wText - if t := pass.TypesInfo.TypeOf(sel.X); t != nil && - !types.Implements(t, writerIface) && - types.Implements(types.NewPointer(t), writerIface) { - writerArg = "&" + wText - } + sExpr := buildStringExpr(pass, strArg, sText) + writerArg := buildWriterArg(pass, sel.X, wText) - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to potentially avoid a []byte allocation if the writer implements io.StringWriter", wText, sText, writerArg, sExpr), - SuggestedFixes: buildFix(pass, call, writerArg, sExpr, filesWithImportEdit), - }) + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to potentially avoid a []byte allocation if the writer implements io.StringWriter", wText, sText, writerArg, sExpr), + SuggestedFixes: buildFix(pass, call, writerArg, sExpr, filesWithImportEdit), }) +} - return nil, nil +// buildStringExpr returns the expression text for the io.WriteString string argument. +// If the argument is a named string type, it wraps it in string(...) for type safety. +func buildStringExpr(pass *analysis.Pass, strArg ast.Expr, sText string) string { + if st := pass.TypesInfo.TypeOf(strArg); st != nil && !isExactString(st) { + return "string(" + sText + ")" + } + return sText +} + +// buildWriterArg returns the expression text for the io.WriteString writer argument. +// When the receiver's Write method lives on the pointer type, it returns "&wText". +func buildWriterArg(pass *analysis.Pass, recv ast.Expr, wText string) string { + if t := pass.TypesInfo.TypeOf(recv); t != nil && + !types.Implements(t, writerIface) && + types.Implements(types.NewPointer(t), writerIface) { + return "&" + wText + } + return wText } // isExactString reports whether t is the predeclared string type, not a named @@ -184,13 +191,7 @@ func implementsWriter(pass *analysis.Pass, expr ast.Expr) bool { // still reported). func buildFix(pass *analysis.Pass, call *ast.CallExpr, writerArg, sText string, filesWithImportEdit map[token.Pos]bool) []analysis.SuggestedFix { // Find the file containing this call. - var file *ast.File - for _, f := range pass.Files { - if f.Pos() <= call.Pos() && call.Pos() <= f.End() { - file = f - break - } - } + file := fileForPos(pass.Files, call.Pos()) // Determine the local qualifier for "io": use the alias when the package // is already imported under a different name, or the default name when it @@ -232,24 +233,13 @@ func buildFix(pass *analysis.Pass, call *ast.CallExpr, writerArg, sText string, // file containing pos, unless "io" is already imported in that file or an // import edit for this file has already been emitted in this pass. func addIOImportEdit(pass *analysis.Pass, pos token.Pos, filesWithImportEdit map[token.Pos]bool) (analysis.TextEdit, bool) { - var file *ast.File - for _, f := range pass.Files { - if f.Pos() <= pos && pos <= f.End() { - file = f - break - } - } + file := fileForPos(pass.Files, pos) if file == nil { return analysis.TextEdit{}, false } - if filesWithImportEdit[file.Pos()] { return analysis.TextEdit{}, false } - markAndReturn := func(edit analysis.TextEdit) (analysis.TextEdit, bool) { - filesWithImportEdit[file.Pos()] = true - return edit, true - } for _, imp := range file.Imports { if imp.Path.Value == `"`+ioPkg+`"` { @@ -257,21 +247,33 @@ func addIOImportEdit(pass *analysis.Pass, pos token.Pos, filesWithImportEdit map } } + edit, ok := buildIOImportTextEdit(pass, file) + if ok { + filesWithImportEdit[file.Pos()] = true + } + return edit, ok +} + +// buildIOImportTextEdit constructs a TextEdit that adds an "io" import to file. +// It handles three cases: appending to an existing grouped import block, +// converting a single ungrouped import to a grouped block, or inserting a new +// standalone import after the package declaration. +func buildIOImportTextEdit(pass *analysis.Pass, file *ast.File) (analysis.TextEdit, bool) { + // Case 1: existing grouped import block — append to its closing paren. for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { continue } - // Keep the edit minimal by appending at the end of the grouped import; - // ordering/formatting can be normalized by formatters if desired. - return markAndReturn(analysis.TextEdit{ + return analysis.TextEdit{ Pos: genDecl.Rparen, End: genDecl.Rparen, NewText: []byte(importSpecIndent + `"` + ioPkg + `"` + "\n"), - }) + }, true } - var singleUngroupedImportDecl *ast.GenDecl + // Case 2: exactly one ungrouped import — rebuild as a grouped block. + var singleDecl *ast.GenDecl importDeclCount := 0 for _, decl := range file.Decls { genDecl, ok := decl.(*ast.GenDecl) @@ -280,27 +282,35 @@ func addIOImportEdit(pass *analysis.Pass, pos token.Pos, filesWithImportEdit map } importDeclCount++ if !genDecl.Lparen.IsValid() && len(genDecl.Specs) == 1 { - singleUngroupedImportDecl = genDecl + singleDecl = genDecl } } - if importDeclCount == 1 && singleUngroupedImportDecl != nil { - specText := astutil.NodeText(pass.Fset, singleUngroupedImportDecl.Specs[0]) + if importDeclCount == 1 && singleDecl != nil { + specText := astutil.NodeText(pass.Fset, singleDecl.Specs[0]) if specText != "" { - // Rebuild as a grouped import while preserving the existing import spec - // text and adding "io". - return markAndReturn(analysis.TextEdit{ - Pos: singleUngroupedImportDecl.Pos(), - End: singleUngroupedImportDecl.End(), + return analysis.TextEdit{ + Pos: singleDecl.Pos(), + End: singleDecl.End(), NewText: []byte("import (\n" + importSpecIndent + specText + "\n" + importSpecIndent + `"` + ioPkg + `"` + "\n)"), - }) + }, true } - // If we fail to render the existing import spec, fall back to a - // standalone import insertion below rather than emitting a broken edit. + // Fall through to the standalone insertion below. } - return markAndReturn(analysis.TextEdit{ + // Case 3: no existing import block — insert a standalone import after the package name. + return analysis.TextEdit{ Pos: file.Name.End(), End: file.Name.End(), NewText: []byte("\n\nimport \"" + ioPkg + "\""), - }) + }, true +} + +// fileForPos returns the *ast.File from files that contains pos, or nil if not found. +func fileForPos(files []*ast.File, pos token.Pos) *ast.File { + for _, f := range files { + if f.Pos() <= pos && pos <= f.End() { + return f + } + } + return nil } From 0e5b29eda5409772e8bb55e7a18ccee0151c336b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 08:43:57 +0000 Subject: [PATCH 3/5] fix: remove unused inner variable in bytesbufferstring analyzer Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/bytesbufferstring/bytesbufferstring.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/linters/bytesbufferstring/bytesbufferstring.go b/pkg/linters/bytesbufferstring/bytesbufferstring.go index 132ed898c95..7f7626b615d 100644 --- a/pkg/linters/bytesbufferstring/bytesbufferstring.go +++ b/pkg/linters/bytesbufferstring/bytesbufferstring.go @@ -75,11 +75,10 @@ func analyzeStringBytesCall(pass *analysis.Pass, n ast.Node, generatedFiles file return } - inner, sel, ok := matchBufBytesArg(pass, call) + _, sel, ok := matchBufBytesArg(pass, call) if !ok { return } - _ = inner receiverText := astutil.NodeText(pass.Fset, sel.X) if receiverText == "" { From 8287100801afdfa16dbde944990b025acdcf2cbd Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 21 Jul 2026 11:18:04 +0000 Subject: [PATCH 4/5] docs(adr): add draft ADR-47022 for analyzer helper extraction Records the architectural decision to extract large inline function literals and cursor-loop bodies into named package-level helpers in pkg/linters to resolve 33 largefunc lint violations. Co-Authored-By: Claude Sonnet 4.6 --- ...rs-to-resolve-large-function-violations.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/47022-extract-analyzer-helpers-to-resolve-large-function-violations.md diff --git a/docs/adr/47022-extract-analyzer-helpers-to-resolve-large-function-violations.md b/docs/adr/47022-extract-analyzer-helpers-to-resolve-large-function-violations.md new file mode 100644 index 00000000000..ee60d63db38 --- /dev/null +++ b/docs/adr/47022-extract-analyzer-helpers-to-resolve-large-function-violations.md @@ -0,0 +1,50 @@ +# ADR-47022: Extract Named Helper Functions from Large Analyzer `run` Bodies + +**Date**: 2026-07-21 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +`make golint-custom` reported 33 large-function violations (>60 body lines) across `pkg/linters` analyzer files. The violations were concentrated in `run` functions and a small number of related helpers that accumulated traversal, type-checking, and fix-generation logic inline — both as `insp.Preorder` anonymous callback bodies and as `for cur := range insp.Root().Preorder(...)` loop bodies. Because the large-function linter is a required quality gate that runs in CI, all 33 violations had to be resolved before the branch could merge. No change to analyzer semantics (diagnostics, positions, or suggested-fix `TextEdit`s) was permissible. + +### Decision + +We will extract the body of each oversized inline function literal or cursor loop body into a named package-level function (e.g., `analyzeWriteCall`, `checkHTTPCall`, `checkHardcodedFilePath`). Each `run` function will be reduced to setup, index construction, and a single-line delegation call. Constants that were previously scoped to `run` and needed by the extracted helper will be promoted to package-level. No behavioral changes are introduced: diagnostic messages, positions, and `SuggestedFix` values are identical before and after extraction. + +### Alternatives Considered + +#### Alternative 1: Suppress violations with `//nolint:largefunc` directives + +Add per-function `//nolint` directives to silence the 33 findings without restructuring code. This would pass the gate but would accumulate permanent suppression debt, mask future growth of these functions, and set a precedent that `pkg/linters` functions are exempt from the quality rule. + +#### Alternative 2: Raise or remove the large-function threshold for `pkg/linters` + +Adjust `.golangci.yml` or the custom linter configuration to either increase the line-length limit (e.g., to 120 lines) or exclude the `pkg/linters` directory entirely. This resolves the gate failures without code changes but defeats the purpose of the linter for exactly the code that defines the linter rules — an especially bad precedent — and does not improve readability. + +#### Alternative 3: Restructure analyzers as method sets on analyzer structs + +Replace the current `func run(pass *analysis.Pass)` pattern with a per-analyzer struct that implements sub-passes as methods, eliminating the need to thread `pass`, `generatedFiles`, and `noLintIndex` as parameters to every extracted function. This is a deeper architectural change that would affect all 25 files uniformly and could be a worthwhile future refactor, but it is out of scope for a targeted lint-compliance fix: it introduces higher risk of behavior regression and a much larger diff. + +### Consequences + +#### Positive +- All 33 `largefunc` violations in `pkg/linters` are cleared; `make golint-custom` passes. +- Each extracted function has a single, named responsibility, making stack traces and profiling output more informative. +- Reviewers can read the `run` function as a high-level outline and drill into named helpers independently. +- The extraction pattern is uniform and mechanical, minimizing review surface. + +#### Negative +- Extracted functions require all context to be passed explicitly (`pass`, `generatedFiles`, `noLintIndex`, and in some cases `seenImportFiles` or `inspector.Cursor`), increasing parameter counts relative to closures that captured variables implicitly. +- Two files (`execcommandwithoutcontext`, `nilctxpassed`) required a new import of `"golang.org/x/tools/go/ast/inspector"` to expose the `inspector.Cursor` type in the extracted function signatures, adding a minor import-set change. + +#### Neutral +- The `formatArgOffset` constant in `errorfwrapv` was promoted from a function-local `const` to package-level to eliminate a redundant parameter; this has no behavioral effect but changes the constant's visibility scope. +- All diagnostic messages, positions, and suggested-fix `TextEdit`s are unchanged — this is a structural refactor only. +- The 25 changed files share a consistent before/after pattern, which means the diff is large in line count but low in cognitive complexity per file. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 74f142ec4330836a099d49291728ba53776e507a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 11:38:36 +0000 Subject: [PATCH 5/5] fix: address review feedback on doc comments and errorfwrapv nolint deferral MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - timeafterleak: fix isSingleCaseSelect doc — default clause IS counted as another clause, making timer+default selects reportable - ioutildeprecated: expand checkDotImportIoutilUsage doc to say 'identifiers (functions and variables)' and mention import path - largefunc: restore inline comment explaining -1 in line-count formula - errorfwrapv: defer nolint lookup to just before each ReportRangef Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/errorfwrapv/errorfwrapv.go | 5 ++--- pkg/linters/ioutildeprecated/ioutildeprecated.go | 3 ++- pkg/linters/largefunc/largefunc.go | 2 +- pkg/linters/timeafterleak/timeafterleak.go | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/linters/errorfwrapv/errorfwrapv.go b/pkg/linters/errorfwrapv/errorfwrapv.go index 14df6b0e4a5..328eca532a1 100644 --- a/pkg/linters/errorfwrapv/errorfwrapv.go +++ b/pkg/linters/errorfwrapv/errorfwrapv.go @@ -103,12 +103,11 @@ func analyzeFmtErrorfCall(pass *analysis.Pass, n ast.Node, generatedFiles filech return } - suppressed := nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") verbs := parseFormatVerbs(lit.Value) errorArgVerbs, wrappedErrorArgs, hasVerbV := classifyErrorArgs(pass, call, verbs) if hasVerbV { - if suppressed { + if nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") { return } pass.ReportRangef(call, "fmt.Errorf formats an error argument with %%v; use %%w to preserve the error chain") @@ -135,7 +134,7 @@ func analyzeFmtErrorfCall(pass *analysis.Pass, n ast.Node, generatedFiles filech continue } } - if suppressed { + if nolint.HasDirectiveForLinter(position, noLintIndex, "errorfwrapv") { return } pass.ReportRangef(call, "fmt.Errorf passes an error argument without %%w; use %%w to preserve the error chain") diff --git a/pkg/linters/ioutildeprecated/ioutildeprecated.go b/pkg/linters/ioutildeprecated/ioutildeprecated.go index fbfd47934b2..137b8ef0d96 100644 --- a/pkg/linters/ioutildeprecated/ioutildeprecated.go +++ b/pkg/linters/ioutildeprecated/ioutildeprecated.go @@ -93,7 +93,8 @@ func checkQualifiedIoutilUsage(pass *analysis.Pass, root inspector.Cursor, gener } } -// checkDotImportIoutilUsage reports bare ioutil function names used via dot-imports. +// checkDotImportIoutilUsage reports bare ioutil identifiers (functions and variables) +// used via dot-imports (import . "io/ioutil"). func checkDotImportIoutilUsage(pass *analysis.Pass, root inspector.Cursor, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { for cur := range root.Preorder((*ast.Ident)(nil)) { ident, ok := cur.Node().(*ast.Ident) diff --git a/pkg/linters/largefunc/largefunc.go b/pkg/linters/largefunc/largefunc.go index bd4720acea9..821f33cecdb 100644 --- a/pkg/linters/largefunc/largefunc.go +++ b/pkg/linters/largefunc/largefunc.go @@ -88,7 +88,7 @@ func checkFuncBodyLength(pass *analysis.Pass, n ast.Node, generatedFiles fileche start := pass.Fset.Position(body.Lbrace) end := pass.Fset.Position(body.Rbrace) - lines := end.Line - start.Line - 1 + lines := end.Line - start.Line - 1 // subtract 1: exclude the closing brace line, count only body lines if lines > maxLines { if nolint.HasDirectiveForLinter(position, noLintIndex, "largefunc") { diff --git a/pkg/linters/timeafterleak/timeafterleak.go b/pkg/linters/timeafterleak/timeafterleak.go index f0bf176b59f..72be0800a39 100644 --- a/pkg/linters/timeafterleak/timeafterleak.go +++ b/pkg/linters/timeafterleak/timeafterleak.go @@ -148,8 +148,8 @@ func isInsideLoopSelectComm(cur inspector.Cursor) bool { // isSingleCaseSelect reports whether the CommClause cc is the only clause in // its enclosing SelectStmt. Single-case selects are not flagged because the // timer must fire — no accumulation is possible. -// A default clause (CommClause with nil Comm) can preempt the timer and is -// not counted as "another case" here; thus such selects are still flagged. +// A default clause (CommClause with nil Comm) is counted as another clause, +// so a select with a timer case plus a default returns false and is reportable. func isSingleCaseSelect(clauseCur inspector.Cursor, cc *ast.CommClause) bool { for selCur := range clauseCur.Enclosing((*ast.SelectStmt)(nil)) { sel, ok := selCur.Node().(*ast.SelectStmt)