diff --git a/docs/adr/38317-standardize-linter-suite-helpers-cursor-api-autofixes.md b/docs/adr/38317-standardize-linter-suite-helpers-cursor-api-autofixes.md new file mode 100644 index 00000000000..dd0fe2892dc --- /dev/null +++ b/docs/adr/38317-standardize-linter-suite-helpers-cursor-api-autofixes.md @@ -0,0 +1,42 @@ +# ADR-38317: Standardize the linter suite on shared astutil helpers, the Cursor API, and SuggestedFix autofixes + +**Date**: 2026-06-10 +**Status**: Draft + +## Context + +The `pkg/linters` suite contains 23 `golang.org/x/tools/go/analysis` analyzers. Every analyzer repeated the same three-line type assertion to extract `*inspector.Inspector` from `pass.ResultOf`, two analyzers (`regexpcompileinfunction`, `panic-in-library-code`) still traversed the AST with the older `WithStack` callback plus hand-rolled stack-scanning helpers, and several mechanically-fixable diagnostics (`fprintlnsprintf`, `tolowerequalfold`, `lenstringzero`) reported problems without offering an automatic fix. A `golang.org/x/tools` usage review flagged all three as drift from the modern, idiomatic way to write analyzers in this codebase. The constraint is that any change must be behavior-preserving for diagnostics (existing `analysistest` expectations must continue to hold) while reducing duplication and adopting the current traversal API. + +## Decision + +We will standardize the linter suite on three shared conventions. First, inspector extraction is centralized in `astutil.Inspector(pass)` (returning `(*inspector.Inspector, error)`) and reused by all 23 linters, alongside a new `astutil.NodeText(fset, node)` helper for rendering nodes during fix generation. Second, AST traversal uses the `inspector.Cursor` API (`insp.Root().Preorder()` + `cur.Enclosing()`) rather than `WithStack`, with explicit `break` in loops that only inspect the immediate enclosing node. Third, mechanically-fixable linters emit `analysis.SuggestedFix` so fixes are available through `multichecker`'s `-fix` flag and editor integrations, with fixes restricted to unambiguous direct-call patterns. + +## Alternatives Considered + +### Alternative 1: Leave per-linter boilerplate and keep `WithStack` +Keeping the status quo avoids touching 23 files at once and the review surface that comes with it. It was rejected because the duplicated assertion is a recurring copy-paste hazard, and `WithStack` is the legacy traversal API — new linters were already being written against `Cursor`, so the two stragglers were a maintenance inconsistency. + +### Alternative 2: Generate the boilerplate via code generation +A `go:generate` step could stamp out the inspector-extraction block per linter. It was rejected as heavier than the problem warrants: a single exported helper function gives the same de-duplication with no build-time tooling, no generated files to keep in sync, and a clearer call site. + +### Alternative 3: Emit autofixes for all matched patterns, including alias variables +We could attempt fixes for indirect cases (e.g. `n := len(s); n == 0`, or aliased `strings.ToLower` results). This was rejected because rewriting through an intermediate variable risks changing semantics or producing incorrect edits; limiting fixes to direct call patterns keeps every emitted fix provably safe while leaving the harder cases diagnostic-only. + +## Consequences + +### Positive +- Removes 23 copies of the inspector-extraction boilerplate, centralizing the error message in one place. +- Brings every linter onto the same modern `Cursor` traversal API, easing future maintenance and onboarding. +- `fprintlnsprintf`, `tolowerequalfold`, and `lenstringzero` now offer automatic fixes via `-fix` and editor integrations. + +### Negative +- All 23 linters now depend on the `internal/astutil` package, increasing coupling to that shared helper; a change there affects every linter. +- Autofix coverage is intentionally partial — alias/indirect patterns remain diagnostic-only, which may surprise users who expect every diagnostic to be auto-fixable. + +### Neutral +- Tests for the autofixing linters switch from `analysistest.Run` to `analysistest.RunWithSuggestedFixes`, requiring `.golden` files to be kept in sync with the source testdata. +- Diagnostics that previously used `pass.Reportf`/`pass.ReportRangef` now construct `analysis.Diagnostic` values explicitly to carry `SuggestedFixes`. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27270831408) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go b/pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go index 35d34268abd..08adc8b846d 100644 --- a/pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go +++ b/pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go @@ -3,15 +3,14 @@ package contextcancelnotdeferred import ( - "fmt" "go/ast" "go/token" "go/types" "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" ) @@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } nodeFilter := []ast.Node{ diff --git a/pkg/linters/ctxbackground/ctxbackground.go b/pkg/linters/ctxbackground/ctxbackground.go index 203c1a6ff5b..6168497179a 100644 --- a/pkg/linters/ctxbackground/ctxbackground.go +++ b/pkg/linters/ctxbackground/ctxbackground.go @@ -4,14 +4,13 @@ package ctxbackground import ( - "fmt" "go/ast" "go/types" "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" ) @@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { diff --git a/pkg/linters/errormessage/errormessage.go b/pkg/linters/errormessage/errormessage.go index e8f40329281..8e99502b4ae 100644 --- a/pkg/linters/errormessage/errormessage.go +++ b/pkg/linters/errormessage/errormessage.go @@ -3,7 +3,6 @@ package errormessage import ( - "fmt" "go/ast" "go/token" "path/filepath" @@ -12,7 +11,6 @@ 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" @@ -44,9 +42,9 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "errormessage") diff --git a/pkg/linters/errstringmatch/errstringmatch.go b/pkg/linters/errstringmatch/errstringmatch.go index 26bf06cbda5..db0175faacc 100644 --- a/pkg/linters/errstringmatch/errstringmatch.go +++ b/pkg/linters/errstringmatch/errstringmatch.go @@ -4,13 +4,11 @@ package errstringmatch import ( - "fmt" "go/ast" "go/types" "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" @@ -27,9 +25,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "errstringmatch") diff --git a/pkg/linters/excessivefuncparams/excessivefuncparams.go b/pkg/linters/excessivefuncparams/excessivefuncparams.go index eb241a37dfd..d805ccdcf6f 100644 --- a/pkg/linters/excessivefuncparams/excessivefuncparams.go +++ b/pkg/linters/excessivefuncparams/excessivefuncparams.go @@ -3,12 +3,12 @@ package excessivefuncparams import ( - "fmt" "go/ast" "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" ) // DefaultMaxParams is the default maximum number of parameters allowed in a function declaration. @@ -32,9 +32,9 @@ func init() { } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } nodeFilter := []ast.Node{ diff --git a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go index d9b6ae2e472..faa4428d565 100644 --- a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go +++ b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go @@ -11,8 +11,8 @@ 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" ) @@ -26,9 +26,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { diff --git a/pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go b/pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go index 2bf1a524743..45d4435e5aa 100644 --- a/pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go +++ b/pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go @@ -3,15 +3,14 @@ package fileclosenotdeferred import ( - "fmt" "go/ast" "go/token" "go/types" "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" ) @@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } nodeFilter := []ast.Node{ diff --git a/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go index 279793c7077..72fca0ce73f 100644 --- a/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go +++ b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go @@ -4,14 +4,12 @@ package fmterrorfnoverbs import ( - "fmt" "go/ast" "go/token" "strings" "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/nolint" @@ -27,9 +25,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "fmterrorfnoverbs") diff --git a/pkg/linters/fprintlnsprintf/fprintlnsprintf.go b/pkg/linters/fprintlnsprintf/fprintlnsprintf.go index f358547dbe0..d649078f596 100644 --- a/pkg/linters/fprintlnsprintf/fprintlnsprintf.go +++ b/pkg/linters/fprintlnsprintf/fprintlnsprintf.go @@ -3,13 +3,15 @@ package fprintlnsprintf import ( - "fmt" "go/ast" + "go/token" + "strconv" + "strings" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -24,9 +26,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "fprintlnsprintf") @@ -66,12 +68,69 @@ func run(pass *analysis.Pass) (any, error) { return } - pass.Reportf(call.Pos(), "use fmt.Fprintf instead of fmt.Fprintln(w, fmt.Sprintf(...))") + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: "use fmt.Fprintf instead of fmt.Fprintln(w, fmt.Sprintf(...))", + SuggestedFixes: buildFprintfFix(call, printedArg), + }) }) return nil, nil } +// buildFprintfFix returns a SuggestedFix rewriting +// fmt.Fprintln(w, fmt.Sprintf("format", args...)) to +// fmt.Fprintf(w, "format\n", args...). +// A fix is only emitted when the format argument is a plain double-quoted +// string literal; other forms (raw strings, variables) are left unfixed. +func buildFprintfFix(call *ast.CallExpr, sprintfCall *ast.CallExpr) []analysis.SuggestedFix { + if len(sprintfCall.Args) == 0 { + return nil + } + formatLit, ok := sprintfCall.Args[0].(*ast.BasicLit) + if !ok || formatLit.Kind != token.STRING { + return nil + } + raw := formatLit.Value + if len(raw) < 2 || raw[0] != '"' || raw[len(raw)-1] != '"' { + return nil // not a plain double-quoted literal + } + + // Decode the literal to check for a trailing newline so we don't produce + // a double \n when the format string already ends with one. + decoded, err := strconv.Unquote(raw) + if err != nil { + return nil + } + + // Build the replacement format literal: append \n only when not already present. + var newFormatLit []byte + if strings.HasSuffix(decoded, "\n") { + newFormatLit = []byte(raw) // already ends with \n; keep as-is + } else { + newFormatLit = []byte(raw[:len(raw)-1] + `\n"`) + } + + outerSel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return nil + } + return []analysis.SuggestedFix{{ + Message: `Replace fmt.Fprintln with fmt.Fprintf`, + TextEdits: []analysis.TextEdit{ + // 1. "Fprintln" → "Fprintf" + {Pos: outerSel.Sel.Pos(), End: outerSel.Sel.End(), NewText: []byte("Fprintf")}, + // 2. Delete "fmt.Sprintf(" — from the start of sprintfCall to after its "(" + {Pos: sprintfCall.Pos(), End: sprintfCall.Lparen + 1, NewText: nil}, + // 3. Replace the format literal (adding \n if not already present). + {Pos: formatLit.Pos(), End: formatLit.End(), NewText: newFormatLit}, + // 4. Delete the closing ")" of fmt.Sprintf(...) + {Pos: sprintfCall.Rparen, End: sprintfCall.Rparen + 1, NewText: nil}, + }, + }} +} + // isFmtFunc returns true if call is a call to fmt.. func isFmtFunc(call *ast.CallExpr, name string) bool { sel, ok := call.Fun.(*ast.SelectorExpr) diff --git a/pkg/linters/fprintlnsprintf/fprintlnsprintf_test.go b/pkg/linters/fprintlnsprintf/fprintlnsprintf_test.go index 0ad39f9617f..3e4c93f9270 100644 --- a/pkg/linters/fprintlnsprintf/fprintlnsprintf_test.go +++ b/pkg/linters/fprintlnsprintf/fprintlnsprintf_test.go @@ -11,5 +11,5 @@ import ( ) func TestAnalyzer(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), fprintlnsprintf.Analyzer, "fprintlnsprintf") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), fprintlnsprintf.Analyzer, "fprintlnsprintf") } diff --git a/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go b/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go index 86933e294ad..0e523366563 100644 --- a/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go +++ b/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go @@ -9,6 +9,10 @@ func flagged(name string) { fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name)) // want "use fmt.Fprintf" } +func flaggedAlreadyNewline(name string) { + fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s\n", name)) // want "use fmt.Fprintf" +} + func notFlagged(name string) { fmt.Fprintln(os.Stderr, "plain string") fmt.Fprintln(os.Stderr, "prefix", fmt.Sprintf("hello %s", name)) diff --git a/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go.golden b/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go.golden new file mode 100644 index 00000000000..736668e41aa --- /dev/null +++ b/pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go.golden @@ -0,0 +1,28 @@ +package fprintlnsprintf + +import ( + "fmt" + "os" +) + +func flagged(name string) { + fmt.Fprintf(os.Stderr, "hello %s\n", name) // want "use fmt.Fprintf" +} + +func flaggedAlreadyNewline(name string) { + fmt.Fprintf(os.Stderr, "hello %s\n", name) // want "use fmt.Fprintf" +} + +func notFlagged(name string) { + fmt.Fprintln(os.Stderr, "plain string") + fmt.Fprintln(os.Stderr, "prefix", fmt.Sprintf("hello %s", name)) + fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name), "suffix") + fmt.Fprintln(os.Stderr) + fmt.Fprintf(os.Stderr, "hello %s\n", name) +} + +func suppressed(name string) { + //nolint:fprintlnsprintf + fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name)) + fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name)) //nolint:fprintlnsprintf +} diff --git a/pkg/linters/internal/astutil/astutil.go b/pkg/linters/internal/astutil/astutil.go index 7e1bbefbb7e..c257006f210 100644 --- a/pkg/linters/internal/astutil/astutil.go +++ b/pkg/linters/internal/astutil/astutil.go @@ -2,11 +2,16 @@ package astutil import ( + "bytes" + "fmt" "go/ast" + "go/printer" "go/token" "go/types" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" ) // IsLocalObject reports whether obj is a local (non-package-scope) object. @@ -66,3 +71,22 @@ func IsFmtErrorf(pass *analysis.Pass, call *ast.CallExpr) bool { } return pkgName.Imported().Path() == "fmt" } + +// Inspector extracts the *inspector.Inspector from pass.ResultOf. +// It returns an error if the result has an unexpected type. +func Inspector(pass *analysis.Pass) (*inspector.Inspector, error) { + insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + } + return insp, nil +} + +// NodeText formats node as Go source text using go/printer. +func NodeText(fset *token.FileSet, node ast.Node) string { + var buf bytes.Buffer + if err := printer.Fprint(&buf, fset, node); err != nil { + return "" + } + return buf.String() +} diff --git a/pkg/linters/internal/astutil/astutil_test.go b/pkg/linters/internal/astutil/astutil_test.go index 65bfb79eb1e..d8ac94422d2 100644 --- a/pkg/linters/internal/astutil/astutil_test.go +++ b/pkg/linters/internal/astutil/astutil_test.go @@ -51,3 +51,14 @@ func TestIsStringLiteral(t *testing.T) { t.Fatal("did not expect int literal to be detected as string") } } + +func TestNodeText(t *testing.T) { + t.Parallel() + + fset := token.NewFileSet() + node := &ast.Ident{Name: "myVar"} + got := NodeText(fset, node) + if got != "myVar" { + t.Fatalf("NodeText = %q, want %q", got, "myVar") + } +} diff --git a/pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go b/pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go index 713cf5b868e..f7d2e329ef7 100644 --- a/pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go +++ b/pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go @@ -3,14 +3,13 @@ package jsonmarshalignoredeerror import ( - "fmt" "go/ast" "go/types" "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/nolint" ) @@ -24,9 +23,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "jsonmarshalignoredeerror") nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)} diff --git a/pkg/linters/largefunc/largefunc.go b/pkg/linters/largefunc/largefunc.go index 768e2260a51..4d4deac5f40 100644 --- a/pkg/linters/largefunc/largefunc.go +++ b/pkg/linters/largefunc/largefunc.go @@ -3,13 +3,12 @@ package largefunc import ( - "fmt" "go/ast" "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" ) @@ -34,9 +33,9 @@ func init() { } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } nodeFilter := []ast.Node{ diff --git a/pkg/linters/lenstringzero/lenstringzero.go b/pkg/linters/lenstringzero/lenstringzero.go index 5dade900f5f..1ee55231b36 100644 --- a/pkg/linters/lenstringzero/lenstringzero.go +++ b/pkg/linters/lenstringzero/lenstringzero.go @@ -10,7 +10,6 @@ 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" @@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } lenStringAliases := collectLenStringAliases(pass) @@ -48,10 +47,13 @@ func run(pass *analysis.Pass) (any, error) { } var lenArg ast.Expr + isDirect := false if isLenCall(expr.X) && isIntZero(expr.Y) { lenArg = lenCallArg(expr.X) + isDirect = true } else if isIntZero(expr.X) && isLenCall(expr.Y) { lenArg = lenCallArg(expr.Y) + isDirect = true } else if isIntZero(expr.Y) { if arg, ok := lenAliasArg(pass, expr.X, lenStringAliases); ok { lenArg = arg @@ -81,14 +83,39 @@ func run(pass *analysis.Pass) (any, error) { } else { cmpVerb = "non-empty" } - pass.ReportRangef(expr, - `use s %s "" to check for %s string instead of len(s) %s 0`, - op, cmpVerb, op) + var fixes []analysis.SuggestedFix + if isDirect { + fixes = buildLenStringFix(pass, expr, lenArg) + } + 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 0`, op, cmpVerb, op), + SuggestedFixes: fixes, + }) }) return nil, nil } +// buildLenStringFix returns a SuggestedFix that rewrites a direct len(s) == 0 +// or len(s) != 0 comparison to s == "" or s != "". +func buildLenStringFix(pass *analysis.Pass, expr *ast.BinaryExpr, lenArg ast.Expr) []analysis.SuggestedFix { + text := astutil.NodeText(pass.Fset, lenArg) + if text == "" { + return nil + } + replacement := fmt.Sprintf(`%s %s ""`, text, expr.Op.String()) + return []analysis.SuggestedFix{{ + Message: "Replace with direct string comparison", + TextEdits: []analysis.TextEdit{{ + Pos: expr.Pos(), + End: expr.End(), + NewText: []byte(replacement), + }}, + }} +} + func isLenCall(expr ast.Expr) bool { call, ok := expr.(*ast.CallExpr) if !ok || len(call.Args) != 1 { diff --git a/pkg/linters/lenstringzero/lenstringzero_test.go b/pkg/linters/lenstringzero/lenstringzero_test.go index bf73dac3026..5d7ab5e9f62 100644 --- a/pkg/linters/lenstringzero/lenstringzero_test.go +++ b/pkg/linters/lenstringzero/lenstringzero_test.go @@ -12,5 +12,5 @@ import ( func TestLenStringZero(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, lenstringzero.Analyzer, "lenstringzero") + analysistest.RunWithSuggestedFixes(t, testdata, lenstringzero.Analyzer, "lenstringzero") } diff --git a/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden b/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden new file mode 100644 index 00000000000..a942e1171c7 --- /dev/null +++ b/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden @@ -0,0 +1,73 @@ +package lenstringzero + +func isEmpty(s string) bool { + return s == "" // want `use s == "" to check for empty string instead of len\(s\) == 0` +} + +func isNotEmpty(s string) bool { + return s != "" // want `use s != "" to check for non-empty string instead of len\(s\) != 0` +} + +func flippedEmpty(s string) bool { + return s == "" // want `use s == "" to check for empty string instead of len\(s\) == 0` +} + +func flippedNotEmpty(s string) bool { + return s != "" // want `use s != "" to check for non-empty string instead of len\(s\) != 0` +} + +func alreadyGoodEmpty(s string) bool { + return s == "" +} + +func alreadyGoodNotEmpty(s string) bool { + return s != "" +} + +func sliceNotFlagged(s []byte) bool { + return len(s) == 0 +} + +func arrayNotFlagged(s [1]byte) bool { + return len(s) != 0 +} + +func lenNotZeroOp(s string) bool { + return len(s) > 0 +} + +func lenNotComparedToZero(s string) bool { + return len(s) == 1 +} + +func aliasEmpty(s string) bool { + n := len(s) + return n == 0 // want `use s == "" to check for empty string instead of len\(s\) == 0` +} + +func aliasNotEmpty(s string) bool { + n := len(s) + return n != 0 // want `use s != "" to check for non-empty string instead of len\(s\) != 0` +} + +func aliasReassignedNotFlagged(s string) bool { + n := len(s) + n = 1 + return n == 0 +} + +func aliasIncrementedNotFlagged(s string) bool { + n := len(s) + n++ + return n == 0 +} + +func sliceAliasNotFlagged(s []byte) bool { + n := len(s) + return n == 0 +} + +func arrayAliasNotFlagged(s [1]byte) bool { + n := len(s) + return n == 0 +} diff --git a/pkg/linters/manualmutexunlock/manualmutexunlock.go b/pkg/linters/manualmutexunlock/manualmutexunlock.go index 7dd6c58e685..e10a6db87c2 100644 --- a/pkg/linters/manualmutexunlock/manualmutexunlock.go +++ b/pkg/linters/manualmutexunlock/manualmutexunlock.go @@ -4,15 +4,14 @@ package manualmutexunlock import ( - "fmt" "go/ast" "go/token" "go/types" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -27,9 +26,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "manualmutexunlock") diff --git a/pkg/linters/osexitinlibrary/osexitinlibrary.go b/pkg/linters/osexitinlibrary/osexitinlibrary.go index 7c53780b800..cc99e8ac73e 100644 --- a/pkg/linters/osexitinlibrary/osexitinlibrary.go +++ b/pkg/linters/osexitinlibrary/osexitinlibrary.go @@ -3,14 +3,13 @@ package osexitinlibrary import ( - "fmt" "go/ast" "strings" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -31,9 +30,9 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "osexitinlibrary") diff --git a/pkg/linters/ossetenvlibrary/ossetenvlibrary.go b/pkg/linters/ossetenvlibrary/ossetenvlibrary.go index dffa4293103..f95530a881b 100644 --- a/pkg/linters/ossetenvlibrary/ossetenvlibrary.go +++ b/pkg/linters/ossetenvlibrary/ossetenvlibrary.go @@ -3,15 +3,14 @@ package ossetenvlibrary import ( - "fmt" "go/ast" "go/types" "strings" "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" ) @@ -30,9 +29,9 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } nodeFilter := []ast.Node{ diff --git a/pkg/linters/panic-in-library-code/panic-in-library-code.go b/pkg/linters/panic-in-library-code/panic-in-library-code.go index df694fc96b8..0f1cf74320d 100644 --- a/pkg/linters/panic-in-library-code/panic-in-library-code.go +++ b/pkg/linters/panic-in-library-code/panic-in-library-code.go @@ -3,7 +3,6 @@ package panicinlibrarycode import ( - "fmt" "go/ast" "go/constant" "go/token" @@ -15,6 +14,7 @@ import ( "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -35,91 +35,75 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "panicinlibrarycode") - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - - insp.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { - if !push { - return true - } - - call, ok := n.(*ast.CallExpr) + for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { + call, ok := cur.Node().(*ast.CallExpr) if !ok { - return true + continue } // Skip test files if strings.HasSuffix(pkgPath, ".test") || filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { - return true + continue } // Check if this is a call to the builtin panic function ident, ok := call.Fun.(*ast.Ident) - if !ok { - return true - } - - if ident.Name != "panic" { - return true + if !ok || ident.Name != "panic" { + continue } // Verify it's the builtin panic, not a user-defined function if obj := pass.TypesInfo.Uses[ident]; obj != nil { if _, ok := obj.(*types.Builtin); !ok { - return true // Not the builtin panic + continue // Not the builtin panic } } - if shouldSkipPanic(pass, call, stack) { - return true + if shouldSkipPanic(pass, call, cur) { + continue } position := pass.Fset.PositionFor(call.Pos(), false) if nolint.HasDirective(position, noLintLinesByFile) { - return true + continue } pass.ReportRangef(call, "avoid panic in library code; return an error instead") - return true - }) + } return nil, nil } -func shouldSkipPanic(pass *analysis.Pass, call *ast.CallExpr, stack []ast.Node) bool { - return isInSyncOnceDoFuncLit(pass, stack) || +func shouldSkipPanic(pass *analysis.Pass, call *ast.CallExpr, cur inspector.Cursor) bool { + return isInSyncOnceDoFuncLit(pass, cur) || panicMessageStartsWithBUG(pass, call) || - isInInitFunction(stack) || - hasDocumentedPanicContract(stack) + isInInitFunction(cur) || + hasDocumentedPanicContract(cur) } -func isInSyncOnceDoFuncLit(pass *analysis.Pass, stack []ast.Node) bool { - for forwardIdx, node := range slices.Backward(stack) { - funcLit, ok := node.(*ast.FuncLit) - if !ok || forwardIdx == 0 { - continue +func isInSyncOnceDoFuncLit(pass *analysis.Pass, cur inspector.Cursor) bool { + for encl := range cur.Enclosing((*ast.FuncLit)(nil)) { + funcLit, ok := encl.Node().(*ast.FuncLit) + if !ok { + break } - - call, ok := stack[forwardIdx-1].(*ast.CallExpr) + parent := encl.Parent() + call, ok := parent.Node().(*ast.CallExpr) if !ok || !containsExpr(call.Args, funcLit) { continue } - sel, ok := call.Fun.(*ast.SelectorExpr) if !ok || sel.Sel.Name != "Do" { continue } - if isSyncOnceType(pass.TypesInfo.TypeOf(sel.X)) { return true } } - return false } @@ -194,29 +178,36 @@ func isFmtSprintf(pass *analysis.Pass, call *ast.CallExpr) bool { // isInInitFunction reports whether the panic is inside a top-level init() // function. Only top-level (no receiver) init functions are recognized; // methods named init are ordinary methods and are not exempt. -func isInInitFunction(stack []ast.Node) bool { - decl := enclosingFuncDecl(stack) - return decl != nil && decl.Recv == nil && decl.Name != nil && decl.Name.Name == "init" -} - -func hasDocumentedPanicContract(stack []ast.Node) bool { - decl := enclosingFuncDecl(stack) - if decl == nil || decl.Doc == nil { - return false +func isInInitFunction(cur inspector.Cursor) bool { + for encl := range cur.Enclosing((*ast.FuncDecl)(nil)) { + decl, ok := encl.Node().(*ast.FuncDecl) + if !ok { + break + } + if decl.Recv == nil && decl.Name != nil && decl.Name.Name == "init" { + return true + } + break // only check the immediate enclosing FuncDecl } - - doc := strings.ToLower(decl.Doc.Text()) - return strings.Contains(doc, "panics on") || - strings.Contains(doc, "panics if") || - strings.Contains(doc, "panic on") || - strings.Contains(doc, "panic if") + return false } -func enclosingFuncDecl(stack []ast.Node) *ast.FuncDecl { - for _, node := range slices.Backward(stack) { - if decl, ok := node.(*ast.FuncDecl); ok { - return decl +func hasDocumentedPanicContract(cur inspector.Cursor) bool { + for encl := range cur.Enclosing((*ast.FuncDecl)(nil)) { + decl, ok := encl.Node().(*ast.FuncDecl) + if !ok { + break + } + if decl.Doc != nil { + doc := strings.ToLower(decl.Doc.Text()) + if strings.Contains(doc, "panics on") || + strings.Contains(doc, "panics if") || + strings.Contains(doc, "panic on") || + strings.Contains(doc, "panic if") { + return true + } } + break // only check the immediate enclosing FuncDecl } - return nil + return false } diff --git a/pkg/linters/rawloginlib/rawloginlib.go b/pkg/linters/rawloginlib/rawloginlib.go index 5d480012494..92d80bb1a98 100644 --- a/pkg/linters/rawloginlib/rawloginlib.go +++ b/pkg/linters/rawloginlib/rawloginlib.go @@ -3,14 +3,13 @@ package rawloginlib import ( - "fmt" "go/ast" "strings" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -36,9 +35,9 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "rawloginlib") diff --git a/pkg/linters/regexpcompileinfunction/regexpcompileinfunction.go b/pkg/linters/regexpcompileinfunction/regexpcompileinfunction.go index 68e34689c98..49ed2c5defb 100644 --- a/pkg/linters/regexpcompileinfunction/regexpcompileinfunction.go +++ b/pkg/linters/regexpcompileinfunction/regexpcompileinfunction.go @@ -4,16 +4,14 @@ package regexpcompileinfunction import ( - "fmt" "go/ast" "go/token" "go/types" - "slices" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -28,48 +26,44 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "regexpcompileinfunction") - nodeFilter := []ast.Node{ - (*ast.CallExpr)(nil), - } - - insp.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { - if !push { - return true - } - - call, ok := n.(*ast.CallExpr) + for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { + call, ok := cur.Node().(*ast.CallExpr) if !ok || !isRegexpCompileCall(call) { - return true + continue } if !hasConstantStringPattern(pass, call) { - return true + continue } pos := pass.Fset.PositionFor(call.Pos(), false) if filecheck.IsTestFile(pos.Filename) { - return true + continue } - // Check if we're inside a function (not package-level) - if isInsideFunction(stack) { - if nolint.HasDirective(pos, noLintLinesByFile) { - return true - } - pass.Report(analysis.Diagnostic{ - Pos: call.Pos(), - End: call.End(), - Message: "regexp compilation inside function should be moved to package-level variable", - }) + // Check if we're inside a function (not package-level). + inside := false + for range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { + inside = true + break } - - return true - }) + if !inside { + continue + } + if nolint.HasDirective(pos, noLintLinesByFile) { + continue + } + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: "regexp compilation inside function should be moved to package-level variable", + }) + } return nil, nil } @@ -87,17 +81,6 @@ func isRegexpCompileCall(call *ast.CallExpr) bool { return ident.Name == "regexp" && (sel.Sel.Name == "MustCompile" || sel.Sel.Name == "Compile") } -// isInsideFunction checks if the current node is inside a function body. -func isInsideFunction(stack []ast.Node) bool { - for i := range slices.Backward(stack) { - switch stack[i].(type) { - case *ast.FuncDecl, *ast.FuncLit: - return true - } - } - return false -} - // hasConstantStringPattern checks whether the regexp pattern is a compile-time constant string, // such as a string literal or const identifier (but not variables/parameters). func hasConstantStringPattern(pass *analysis.Pass, call *ast.CallExpr) bool { diff --git a/pkg/linters/seenmapbool/seenmapbool.go b/pkg/linters/seenmapbool/seenmapbool.go index 13bef4fe51c..00baae409bf 100644 --- a/pkg/linters/seenmapbool/seenmapbool.go +++ b/pkg/linters/seenmapbool/seenmapbool.go @@ -4,14 +4,13 @@ package seenmapbool import ( - "fmt" "go/ast" "go/types" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -26,9 +25,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "seenmapbool") diff --git a/pkg/linters/sortslice/sortslice.go b/pkg/linters/sortslice/sortslice.go index 7579f0d9480..0ca6b90eef5 100644 --- a/pkg/linters/sortslice/sortslice.go +++ b/pkg/linters/sortslice/sortslice.go @@ -4,14 +4,13 @@ package sortslice import ( - "fmt" "go/ast" "go/types" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -26,9 +25,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "sortslice") diff --git a/pkg/linters/strconvparseignorederror/strconvparseignorederror.go b/pkg/linters/strconvparseignorederror/strconvparseignorederror.go index 5236f837b39..a1740ac4924 100644 --- a/pkg/linters/strconvparseignorederror/strconvparseignorederror.go +++ b/pkg/linters/strconvparseignorederror/strconvparseignorederror.go @@ -4,14 +4,13 @@ package strconvparseignorederror import ( - "fmt" "go/ast" "go/types" "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/nolint" ) @@ -34,9 +33,9 @@ var strconvParseFuncs = map[string]bool{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "strconvparseignorederror") diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go index 96e9b9df8ac..bd9d1061340 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go @@ -7,10 +7,10 @@ func flaggedExamples() { name := "Alice" // should use strings.EqualFold(name, "alice") - _ = strings.ToLower(name) == "alice" // want `use strings\.EqualFold` - _ = strings.ToUpper(name) == "ALICE" // want `use strings\.EqualFold` - _ = "alice" == strings.ToLower(name) // want `use strings\.EqualFold` - _ = strings.ToLower(name) != "alice" // want `use strings\.EqualFold` + _ = strings.ToLower(name) == "alice" // want `use strings\.EqualFold` + _ = strings.ToUpper(name) == "ALICE" // want `use strings\.EqualFold` + _ = "alice" == strings.ToLower(name) // want `use strings\.EqualFold` + _ = strings.ToLower(name) != "alice" // want `use strings\.EqualFold` _ = strings.ToLower(name) == strings.ToLower("alice") // want `use strings\.EqualFold` lower := strings.ToLower(name) @@ -37,3 +37,4 @@ func suppressedExamples() { name := "Alice" _ = strings.ToLower(name) == "alice" //nolint:tolowerequalfold } + diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden new file mode 100644 index 00000000000..f5a61bdb01e --- /dev/null +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden @@ -0,0 +1,39 @@ +// Package tolowerequalfold contains test fixtures for the tolowerequalfold linter. +package tolowerequalfold + +import "strings" + +func flaggedExamples() { + name := "Alice" + + // should use strings.EqualFold(name, "alice") + _ = strings.EqualFold(name, "alice") // want `use strings\.EqualFold` + _ = strings.EqualFold(name, "ALICE") // want `use strings\.EqualFold` + _ = strings.EqualFold("alice", name) // want `use strings\.EqualFold` + _ = !strings.EqualFold(name, "alice") // want `use strings\.EqualFold` + _ = strings.EqualFold(name, "alice") // want `use strings\.EqualFold` + + lower := strings.ToLower(name) + _ = lower == "alice" // want `use strings\.EqualFold` +} + +func okExamples() { + name := "Alice" + + // EqualFold is already idiomatic — no diagnostic expected + _ = strings.EqualFold(name, "alice") + + // Regular case-sensitive comparison — no diagnostic + _ = name == "Alice" + _ = strings.ToLower(name) // used standalone, not in a comparison + _ = strings.ToLower(name) == name + _ = strings.ToLower(name) != name + + lower := strings.ToLower(name) + _ = lower == name +} + +func suppressedExamples() { + name := "Alice" + _ = strings.ToLower(name) == "alice" //nolint:tolowerequalfold +} diff --git a/pkg/linters/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/tolowerequalfold.go index ef4c8148ed8..25eca475dba 100644 --- a/pkg/linters/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/tolowerequalfold.go @@ -11,7 +11,6 @@ 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,9 +27,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "tolowerequalfold") caseConvAliases := collectCaseConvAliases(pass) @@ -71,14 +70,56 @@ func run(pass *analysis.Pass) (any, error) { if nolint.HasDirective(pass.Fset.PositionFor(expr.Pos(), false), noLintLinesByFile) { return } - pass.ReportRangef(expr, - "use strings.EqualFold for case-insensitive comparison instead of strings.ToLower/ToUpper with ==") + pass.Report(analysis.Diagnostic{ + Pos: expr.Pos(), + End: expr.End(), + Message: "use strings.EqualFold for case-insensitive comparison instead of strings.ToLower/ToUpper with ==", + SuggestedFixes: buildEqualFoldFix(pass, expr), + }) } }) return nil, nil } +// buildEqualFoldFix returns a SuggestedFix that rewrites a direct +// strings.ToLower/ToUpper comparison to strings.EqualFold. +// A fix is only emitted when at least one side is a direct caseConvCall (not +// an alias variable), since alias variables may be defined at a different +// source location. +func buildEqualFoldFix(pass *analysis.Pass, expr *ast.BinaryExpr) []analysis.SuggestedFix { + leftArg, leftOK := caseConvArg(expr.X) + rightArg, rightOK := caseConvArg(expr.Y) + if !leftOK && !rightOK { + return nil + } + arg1 := expr.X + if leftOK { + arg1 = leftArg + } + arg2 := expr.Y + if rightOK { + arg2 = rightArg + } + text1 := astutil.NodeText(pass.Fset, arg1) + text2 := astutil.NodeText(pass.Fset, arg2) + if text1 == "" || text2 == "" { + return nil + } + call := fmt.Sprintf("strings.EqualFold(%s, %s)", text1, text2) + if expr.Op == token.NEQ { + call = "!" + call + } + return []analysis.SuggestedFix{{ + Message: "Replace with strings.EqualFold", + TextEdits: []analysis.TextEdit{{ + Pos: expr.Pos(), + End: expr.End(), + NewText: []byte(call), + }}, + }} +} + func collectCaseConvAliases(pass *analysis.Pass) map[types.Object]ast.Expr { aliases := make(map[types.Object]ast.Expr) for _, file := range pass.Files { diff --git a/pkg/linters/tolowerequalfold/tolowerequalfold_test.go b/pkg/linters/tolowerequalfold/tolowerequalfold_test.go index 5eb85c46597..578b4063a45 100644 --- a/pkg/linters/tolowerequalfold/tolowerequalfold_test.go +++ b/pkg/linters/tolowerequalfold/tolowerequalfold_test.go @@ -12,5 +12,5 @@ import ( func TestAnalyzer(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, tolowerequalfold.Analyzer, "tolowerequalfold") + analysistest.RunWithSuggestedFixes(t, testdata, tolowerequalfold.Analyzer, "tolowerequalfold") } diff --git a/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go b/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go index bb66dd2d605..a795b9d4923 100644 --- a/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go +++ b/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go @@ -4,13 +4,12 @@ package uncheckedtypeassertion import ( - "fmt" "go/ast" "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" "github.com/github/gh-aw/pkg/linters/internal/nolint" ) @@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { - insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - if !ok { - return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err } noLintLinesByFile := nolint.BuildLineIndex(pass, "uncheckedtypeassertion")