From b2a90c404ec4ba85cd59c5a9d543f4053c599039 Mon Sep 17 00:00:00 2001 From: Yan Yi Goh Date: Mon, 27 Apr 2026 12:46:31 +0800 Subject: [PATCH 1/2] isolint: add skip-fields config for collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some uppercase 2- and 3-letter strings overlap with ISO codes by accident — most notably card scheme abbreviations like "MC" (MasterCard, also Monaco) and "AX" (American Express, also Aland Islands). Until now the only escape hatch was per-line //nolint:isolint, which clutters real test code and offers no guard against new collisions. skip-fields names struct fields whose ISO-like values should be ignored (e.g. CardSchemes). The check is syntactic — it walks the AST stack for an enclosing assignment and matches by field name — so the analyzer stays on LoadModeSyntax and a real pq.StringArray of country codes assigned to a non-skipped field is still flagged. This was preferred over a flat allow-values allowlist (too coarse, suppresses genuine country usages) and over type-based matching (would force LoadModeTypesInfo on the whole golangci-lint batch). Settings flow through both the module plugin (decoded via register.DecodeSettings) and the standalone CLI (via a new -skip-fields flag on the analyzer's FlagSet). Co-Authored-By: Claude Opus 4.7 (1M context) --- linters/isolint/AGENTS.md | 9 +- linters/isolint/README.md | 43 +++++ linters/isolint/analyzer.go | 157 ++++++++++++++++-- linters/isolint/analyzer_test.go | 7 + linters/isolint/docs/decisions.md | 15 ++ linters/isolint/docs/testing.md | 4 + linters/isolint/flags_test.go | 45 +++++ linters/isolint/nil_pkg_test.go | 2 +- linters/isolint/plugin.go | 26 ++- .../testdata/skipfields/skip_fields.go | 41 +++++ .../testdata/skipfields/still_flags.go | 30 ++++ 11 files changed, 359 insertions(+), 20 deletions(-) create mode 100644 linters/isolint/flags_test.go create mode 100644 linters/isolint/testdata/skipfields/skip_fields.go create mode 100644 linters/isolint/testdata/skipfields/still_flags.go diff --git a/linters/isolint/AGENTS.md b/linters/isolint/AGENTS.md index ca2137f..5514969 100644 --- a/linters/isolint/AGENTS.md +++ b/linters/isolint/AGENTS.md @@ -1,14 +1,14 @@ # isolint -golangci-lint module plugin that flags uppercase ISO code string literals (`"USD"`, `"SG"`) and recommends `currency.USD` / `site.SG` package constants instead. Lowercase strings (`"usd"`, `"sg"`) are ignored — only uppercase is considered an intentional ISO reference. Suppress remaining false positives with `//nolint:isolint`. +golangci-lint module plugin that flags uppercase ISO code string literals (`"USD"`, `"SG"`) and recommends `currency.USD` / `site.SG` package constants instead. Lowercase strings (`"usd"`, `"sg"`) are ignored — only uppercase is considered an intentional ISO reference. Configurable via [`Settings.SkipFields`](analyzer.go) for domain collisions (e.g. card scheme `"MC"` vs Monaco). Suppress remaining false positives with `//nolint:isolint`. ## Structure -- `analyzer.go` — entry point; walks `*ast.BasicLit` nodes with `inspector.WithStack` for parent context +- `analyzer.go` — entry point; walks `*ast.BasicLit` nodes with `inspector.WithStack` for parent context. Defines `Settings`, `NewAnalyzer`, and the `-skip-fields` flag. - `codes.go` — delegates to `currency.IsISO4217()` and `site.Currency()` for code validation (uppercase only) - `report.go` — diagnostic messages and `SuggestedFix` text edits -- `plugin.go` — golangci-lint module plugin registration -- `cmd/isolint/` — standalone CLI +- `plugin.go` — golangci-lint module plugin registration; decodes YAML settings via `register.DecodeSettings[Settings]` +- `cmd/isolint/` — standalone CLI; settings reach the analyzer via `-skip-fields` - `testdata/` — separate Go module with test fixtures and `.golden` files ## Commands @@ -38,6 +38,7 @@ See [docs/decisions.md](docs/decisions.md) for rationale behind each decision. - **Code validation delegates to source packages** — `currency.IsISO4217()` and `site.Currency()`, no hardcoded maps - **Skips definition packages** — [`skipPackages`](analyzer.go) in `analyzer.go` - **Skips import paths and call arguments** — [`skipMethods`](analyzer.go) in `analyzer.go` +- **Skips configured field assignments** — [`Settings.SkipFields`](analyzer.go) handles domain collisions like card scheme `"MC"` - **Load mode is `LoadModeSyntax`** — only needs string literal values, not type info ## Performance diff --git a/linters/isolint/README.md b/linters/isolint/README.md index d401375..77f9e35 100644 --- a/linters/isolint/README.md +++ b/linters/isolint/README.md @@ -108,6 +108,49 @@ if code == currency.USD { ... } if siteCode == site.SG { ... } ``` +## Configuration + +### `skip-fields` + +Some uppercase 2- or 3-letter strings collide with ISO codes by accident. The +canonical example is the card scheme abbreviation `"MC"` (MasterCard), which +shares its form with the ISO 3166-1 alpha-2 code for Monaco. To avoid manual +`//nolint:isolint` on every site, configure `skip-fields` with the struct field +names that hold these domain values. The linter then skips literals assigned to +those fields, in either of these shapes: + +```go +a.CardSchemes = pq.StringArray{"MC"} // receiver-field assignment +_ = entity.CardAvailability{CardSchemes: pq.StringArray{"MC"}} // composite-literal field +``` + +Assignments to other fields are still flagged — the skip is field-scoped, not a +global value allowlist. + +#### As a golangci-lint module plugin + +```yaml +version: "2" + +linters: + enable: + - isolint + settings: + custom: + isolint: + type: module + description: "Enforces currency/site package constant usage" + settings: + skip-fields: + - CardSchemes +``` + +#### As a standalone CLI + +```bash +isolint -skip-fields=CardSchemes,IssuerCountries ./... +``` + ## Auto-fix The linter provides suggested fixes that can be applied automatically: diff --git a/linters/isolint/analyzer.go b/linters/isolint/analyzer.go index 79e41b2..492d9f2 100644 --- a/linters/isolint/analyzer.go +++ b/linters/isolint/analyzer.go @@ -4,15 +4,32 @@ package isolint import ( + "flag" "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" ) +// Settings configures the isolint analyzer. It is populated from the +// `linters.settings.custom.isolint.settings` block in .golangci.yml when +// running as a module plugin, or from CLI flags when running standalone. +type Settings struct { + // SkipFields is a list of struct field names whose ISO-like string + // literal values should not be flagged. This handles domain collisions + // where a card scheme abbreviation (e.g. "MC" for MasterCard) shares + // the form of an ISO 3166-1 alpha-2 code (Monaco). + // + // Two assignment shapes are matched syntactically: + // - x.Field = ... (AssignStmt with SelectorExpr LHS) + // - StructType{Field: ...} (KeyValueExpr inside a CompositeLit) + SkipFields []string `json:"skip-fields"` +} + // skipPackages are package import paths whose source files should not be // checked. These packages define the constants themselves and must use raw // string literals. @@ -44,16 +61,67 @@ var skipMethods = map[string]bool{ "NotEquals": true, } -// Analyzer is the isolint analyzer that checks for raw ISO code string literals. -var Analyzer = &analysis.Analyzer{ - Name: "isolint", - Doc: "recommends using currency/site package constants over raw ISO code string literals", - URL: "https://github.com/wego/pkg/linters/isolint", - Run: run, - Requires: []*analysis.Analyzer{inspect.Analyzer}, +// Analyzer is the default isolint analyzer with empty settings. The +// standalone CLI in cmd/isolint and existing analystest drivers consume +// this. Plugin and configured-CLI callers should use NewAnalyzer. +var Analyzer = NewAnalyzer(Settings{}) + +// NewAnalyzer returns an isolint analyzer configured with the given +// settings. It is the entry point for the module plugin and any caller +// that needs per-invocation configuration. +// +// The returned analyzer also exposes a -skip-fields flag (comma-separated) +// on its FlagSet, seeded from s.SkipFields. The standalone CLI consumes +// this flag via singlechecker. Plugin and analystest callers do not parse +// flags, so the seeded default is what they see. +func NewAnalyzer(s Settings) *analysis.Analyzer { + skipFields := stringSliceFlag(append([]string(nil), s.SkipFields...)) + + a := &analysis.Analyzer{ + Name: "isolint", + Doc: "recommends using currency/site package constants over raw ISO code string literals", + URL: "https://github.com/wego/pkg/linters/isolint", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + } + a.Flags.Var(&skipFields, "skip-fields", + "comma-separated struct field names whose ISO-like string values should not be flagged "+ + "(e.g. CardSchemes for card scheme abbreviations like \"MC\")") + a.Run = func(pass *analysis.Pass) (any, error) { + set := make(map[string]bool, len(skipFields)) + for _, f := range skipFields { + if f != "" { + set[f] = true + } + } + return run(pass, set) + } + return a +} + +// stringSliceFlag implements flag.Value over a comma-separated list, +// preserving order. Empty Set ("") clears the slice. +type stringSliceFlag []string + +func (s *stringSliceFlag) String() string { + if s == nil { + return "" + } + return strings.Join(*s, ",") } -func run(pass *analysis.Pass) (any, error) { +func (s *stringSliceFlag) Set(v string) error { + if v == "" { + *s = nil + return nil + } + *s = strings.Split(v, ",") + return nil +} + +// Compile-time assertion that stringSliceFlag implements flag.Value. +var _ flag.Value = (*stringSliceFlag)(nil) + +func run(pass *analysis.Pass, skipFields map[string]bool) (any, error) { // Skip packages that define the constants themselves. // pass.Pkg may be nil under LoadModeSyntax in golangci-lint. if pass.Pkg != nil && skipPackages[pass.Pkg.Path()] { @@ -92,12 +160,17 @@ func run(pass *analysis.Pass) (any, error) { } // String arguments to ORM, HTTP, and filter methods are column - // or parameter names (e.g. db.Select("id"), c.Query("to")), - // not ISO code values. + // or parameter names (e.g. db.Select("to")), not ISO codes. if isArgToSkipMethod(stack) { return true } + // String literals assigned to configured fields (e.g. CardSchemes) + // are domain values that happen to share the form of an ISO code. + if isAssignToSkipField(stack, skipFields) { + return true + } + value, err := strconv.Unquote(lit.Value) if err != nil { return true @@ -145,6 +218,70 @@ func isArgToSkipMethod(stack []ast.Node) bool { return skipMethods[callName(call)] } +// isAssignToSkipField reports whether the BasicLit at the top of the stack +// is part of a value assigned to a struct field whose name appears in +// skipFields. +// +// Two assignment shapes are recognized: +// +// x.Field = ... // AssignStmt with SelectorExpr LHS +// StructType{Field: ...} // KeyValueExpr inside a CompositeLit +// +// In both shapes the literal may be nested inside a CompositeLit RHS +// (e.g. pq.StringArray{"MC"}), so the stack is walked from the top down +// to find the nearest enclosing assignment. +func isAssignToSkipField(stack []ast.Node, skipFields map[string]bool) bool { + if len(skipFields) == 0 { + return false + } + // Walk ancestors from nearest to farthest. We stop at function + // boundaries because a literal beyond a FuncLit/FuncDecl is no + // longer "inside" the original assignment. + for i := len(stack) - 2; i >= 0; i-- { + switch parent := stack[i].(type) { + case *ast.KeyValueExpr: + if name, ok := identName(parent.Key); ok && skipFields[name] { + return true + } + // A KeyValueExpr binds the field; no need to keep walking. + return false + case *ast.AssignStmt: + for _, lhs := range parent.Lhs { + if name, ok := fieldNameFromLHS(lhs); ok && skipFields[name] { + return true + } + } + return false + case *ast.FuncLit, *ast.FuncDecl: + return false + } + } + return false +} + +// identName returns the name of an *ast.Ident expression. +func identName(expr ast.Expr) (string, bool) { + id, ok := expr.(*ast.Ident) + if !ok { + return "", false + } + return id.Name, true +} + +// fieldNameFromLHS extracts a field name from an assignment LHS expression. +// For x.Field it returns "Field"; for plain identifiers it returns the +// identifier name (covers cases where the LHS is itself a struct field +// shadowed by a local var, which is rare but cheap to handle). +func fieldNameFromLHS(expr ast.Expr) (string, bool) { + switch e := expr.(type) { + case *ast.SelectorExpr: + return e.Sel.Name, true + case *ast.Ident: + return e.Name, true + } + return "", false +} + // callName extracts the method or function name from a call expression. // For selector expressions (x.Method), it returns the method name. // For plain identifiers (funcName), it returns the function name. diff --git a/linters/isolint/analyzer_test.go b/linters/isolint/analyzer_test.go index 672278e..8d15fb4 100644 --- a/linters/isolint/analyzer_test.go +++ b/linters/isolint/analyzer_test.go @@ -17,3 +17,10 @@ func TestAnalyzerWithFixes(t *testing.T) { testdata := analysistest.TestData() analysistest.RunWithSuggestedFixes(t, testdata, isolint.Analyzer, "./example") } + +func TestAnalyzerWithSkipFields(t *testing.T) { + analyzer := isolint.NewAnalyzer(isolint.Settings{ + SkipFields: []string{"CardSchemes"}, + }) + analysistest.Run(t, analysistest.TestData(), analyzer, "./skipfields") +} diff --git a/linters/isolint/docs/decisions.md b/linters/isolint/docs/decisions.md index 4ada5bb..9c55e92 100644 --- a/linters/isolint/docs/decisions.md +++ b/linters/isolint/docs/decisions.md @@ -27,6 +27,21 @@ Import paths like `import "io"` are syntactically `*ast.BasicLit` strings. The l String arguments to ORM, HTTP, and filter methods (e.g. `db.Select("SG")`, `c.Query("TH")`) are skipped. The linter inspects the parent `*ast.CallExpr` and checks the callee name against [`skipMethods`](../analyzer.go). To skip a new method, add its name there. +### Field-name skip ([`Settings.SkipFields`](../analyzer.go)) + +Uppercase 2- and 3-letter strings sometimes collide with ISO codes by accident — most notably card scheme abbreviations like `"MC"` (MasterCard, also Monaco) and `"AX"` (American Express, also Åland Islands). When a literal is assigned to a struct field whose name appears in [`Settings.SkipFields`](../analyzer.go), the linter skips it. + +Two assignment shapes are recognized syntactically — no type information is needed: + +- `*ast.AssignStmt` whose `Lhs[0]` is a `*ast.SelectorExpr` (covers `a.CardSchemes = pq.StringArray{"MC"}`) +- `*ast.KeyValueExpr` whose `Key` is an `*ast.Ident` (covers `Foo{CardSchemes: pq.StringArray{"MC"}}`) + +The walk in [`isAssignToSkipField`](../analyzer.go) stops at function boundaries (`*ast.FuncLit`/`*ast.FuncDecl`) so a literal beyond a closure boundary is no longer treated as part of the original assignment. + +**Why not type-based?** Matching by the destination field's _type_ (e.g. "skip everything assigned to a `pq.StringArray`") would require `pass.TypesInfo`, which forces escalation to `LoadModeTypesInfo`. That has a project-wide cost (see below) and is also coarser — a real `pq.StringArray` of site codes would also be skipped. Field-name targeting is precise and stays within `LoadModeSyntax`. + +**Why not a flat value allowlist (e.g. `allow-values: [MC]`)?** A global allowlist suppresses the same literal everywhere, including in genuine country contexts. Field-name targeting suppresses it only where the field semantics warrant it, leaving the linter useful elsewhere. + ## `LoadModeSyntax` This is the cheapest load mode — the type-checker never runs. The linter only needs string literal values, not type information, so `LoadModeSyntax` is sufficient. diff --git a/linters/isolint/docs/testing.md b/linters/isolint/docs/testing.md index 12e97ff..57576bc 100644 --- a/linters/isolint/docs/testing.md +++ b/linters/isolint/docs/testing.md @@ -22,6 +22,10 @@ Rule 2 is the crucial one. It means any `.go` file in `testdata/` that does _not | [`testdata/example/valid.go`](../testdata/example/valid.go) | Negative test — code that resembles flaggable patterns but must NOT be flagged | | [`testdata/example/valid_contexts.go`](../testdata/example/valid_contexts.go) | Negative test — call-expression contexts (ORM, HTTP, filter methods) that must NOT be flagged | | `testdata/example/*.go.golden` | Expected auto-fix output for positive test files | +| [`testdata/skipfields/skip_fields.go`](../testdata/skipfields/skip_fields.go) | Negative test — assignments to a configured skip field must NOT be flagged | +| [`testdata/skipfields/still_flags.go`](../testdata/skipfields/still_flags.go) | Positive test — same package, same value, but assigned to a different field is still flagged | + +The `./skipfields` package runs under a dedicated `TestAnalyzerWithSkipFields` driver that constructs an analyzer with `Settings{SkipFields: ["CardSchemes"]}` via [`isolint.NewAnalyzer`](../analyzer.go). Because each `analystest.Run` call constructs its own analyzer, configured fixtures are isolated from `./example`'s zero-config assertions. ### Why `valid.go` and `valid_contexts.go` matter diff --git a/linters/isolint/flags_test.go b/linters/isolint/flags_test.go new file mode 100644 index 0000000..f86b4ff --- /dev/null +++ b/linters/isolint/flags_test.go @@ -0,0 +1,45 @@ +package isolint + +import ( + "testing" +) + +// TestSkipFieldsFlag verifies that the -skip-fields flag exposed on the +// analyzer's FlagSet accepts a comma-separated list and updates the +// closure-captured state used by the run function. +func TestSkipFieldsFlag(t *testing.T) { + a := NewAnalyzer(Settings{}) + + flag := a.Flags.Lookup("skip-fields") + if flag == nil { + t.Fatal("expected -skip-fields flag to be registered on analyzer") + } + + if err := flag.Value.Set("CardSchemes,IssuerCountries"); err != nil { + t.Fatalf("flag.Value.Set returned error: %v", err) + } + + got := flag.Value.String() + want := "CardSchemes,IssuerCountries" + if got != want { + t.Errorf("flag.Value.String() = %q, want %q", got, want) + } +} + +// TestSkipFieldsFlagSeededFromSettings verifies that constructor-provided +// settings populate the flag's default value, so plugin and analystest +// callers (which do not parse flags) still get the configured behavior. +func TestSkipFieldsFlagSeededFromSettings(t *testing.T) { + a := NewAnalyzer(Settings{SkipFields: []string{"CardSchemes"}}) + + flag := a.Flags.Lookup("skip-fields") + if flag == nil { + t.Fatal("expected -skip-fields flag to be registered on analyzer") + } + + got := flag.Value.String() + want := "CardSchemes" + if got != want { + t.Errorf("seeded flag.Value.String() = %q, want %q", got, want) + } +} diff --git a/linters/isolint/nil_pkg_test.go b/linters/isolint/nil_pkg_test.go index d8e1a6a..7fa07fc 100644 --- a/linters/isolint/nil_pkg_test.go +++ b/linters/isolint/nil_pkg_test.go @@ -39,7 +39,7 @@ func f() { } // Must not panic. - _, err = run(pass) + _, err = run(pass, nil) if err != nil { t.Fatalf("run() returned unexpected error: %v", err) } diff --git a/linters/isolint/plugin.go b/linters/isolint/plugin.go index bd19f6b..5bc7711 100644 --- a/linters/isolint/plugin.go +++ b/linters/isolint/plugin.go @@ -11,14 +11,30 @@ func init() { // New is the entry point for the golangci-lint module plugin system. // The signature must be: func New(any) (register.LinterPlugin, error). -func New(_ any) (register.LinterPlugin, error) { - return plugin{}, nil +// +// Settings are decoded from the .golangci.yml block: +// +// linters: +// settings: +// custom: +// isolint: +// type: module +// settings: +// skip-fields: [CardSchemes] +func New(settings any) (register.LinterPlugin, error) { + s, err := register.DecodeSettings[Settings](settings) + if err != nil { + return nil, err + } + return plugin{settings: s}, nil } -type plugin struct{} +type plugin struct { + settings Settings +} -func (plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { - return []*analysis.Analyzer{Analyzer}, nil +func (p plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{NewAnalyzer(p.settings)}, nil } func (plugin) GetLoadMode() string { diff --git a/linters/isolint/testdata/skipfields/skip_fields.go b/linters/isolint/testdata/skipfields/skip_fields.go new file mode 100644 index 0000000..552a90c --- /dev/null +++ b/linters/isolint/testdata/skipfields/skip_fields.go @@ -0,0 +1,41 @@ +package skipfields + +// String literals assigned to fields whose names appear in the configured +// skip-fields list must not be flagged, even when the value matches a valid +// ISO code. This file asserts zero diagnostics under +// Settings{SkipFields: ["CardSchemes"]}. +// +// Two assignment shapes are exercised: +// - Pattern A: receiver-field assignment (a.CardSchemes = ...) +// - Pattern B: composite-literal struct field (Foo{CardSchemes: ...}) +// +// "MC" (Monaco) and "AX" (Aland Islands) are valid ISO 3166-1 alpha-2 +// site codes — but here they denote MasterCard and American Express card +// schemes, not countries. + +type stringArray []string + +type cardAvailability struct { + CardSchemes stringArray + IssuerCountries stringArray +} + +// Pattern A — receiver-field assignment. +func patternA() { + a := &cardAvailability{} + a.CardSchemes = stringArray{"MC"} + a.CardSchemes = stringArray{"AX"} + a.CardSchemes = stringArray{"VISA", "MC"} + a.CardSchemes = stringArray{"MC", "AX", "JCB"} +} + +// Pattern B — composite-literal struct field. +func patternB() { + _ = &cardAvailability{ + CardSchemes: stringArray{"MC"}, + } + _ = &cardAvailability{ + CardSchemes: stringArray{"MC", "AX"}, + } + _ = cardAvailability{CardSchemes: stringArray{"AX"}} +} diff --git a/linters/isolint/testdata/skipfields/still_flags.go b/linters/isolint/testdata/skipfields/still_flags.go new file mode 100644 index 0000000..fc0486f --- /dev/null +++ b/linters/isolint/testdata/skipfields/still_flags.go @@ -0,0 +1,30 @@ +package skipfields + +// The skip-fields setting is targeted: only assignments to the configured +// field names are skipped. Assignments to OTHER fields with ISO-like values +// must still be flagged. This file proves that targeting and runs under +// Settings{SkipFields: ["CardSchemes"]}. + +func notInSkipList() { + // IssuerCountries is not in the skip list — site codes here are real + // countries and should be flagged. + a := &cardAvailability{} + a.IssuerCountries = stringArray{"SG"} // want `use site\.SG instead of "SG"` + a.IssuerCountries = stringArray{"US"} // want `use site\.US instead of "US"` + + // Same field name in a different shape (KeyValueExpr) — also flagged. + _ = &cardAvailability{ + IssuerCountries: stringArray{"JP"}, // want `use site\.JP instead of "JP"` + } +} + +func bareLiterals() { + // Bare assignments without a struct field still flag. + x := "MY" // want `use site\.MY instead of "MY"` + _ = x + + // Currency literals are unaffected by skip-fields when they are not + // inside a configured field assignment. + y := "USD" // want `use currency\.USD instead of "USD"` + _ = y +} From 0f60c766e933596b0e0b7387d4874c2e926c6e2b Mon Sep 17 00:00:00 2001 From: Yan Yi Goh Date: Mon, 27 Apr 2026 14:06:50 +0800 Subject: [PATCH 2/2] isolint: tighten skip-fields targeting Pre-merge review on PR #201 surfaced three correctness gaps in the field-name skip logic: - Tuple assignments over-suppressed. The AssignStmt branch iterated every Lhs element and matched if ANY was in the skip list. For `a.CardSchemes, a.IssuerCountries = stringArray{"MC"}, stringArray{"SG"}` the literal "SG" (destined for IssuerCountries, not in the skip list) was silently skipped because CardSchemes appeared elsewhere on the LHS. Without type information the analyzer cannot correlate which RHS literal belongs to which LHS target, so tuple assignments now fall through and flag every literal as usual. - Local variables shadowing skip-field names were silently skipped. fieldNameFromLHS accepted plain *ast.Ident on top of *ast.SelectorExpr, so `CardSchemes := "MC"` matched the skip-fields set even though the LHS is a local variable, not a struct field. The original GoDoc hand-waved this as "rare but cheap to handle", but it expanded the linter's silent-skip surface beyond the documented design and was uncovered by tests. - decisions.md drifted from the implementation. The doc described the AssignStmt shape as `Lhs[0]` while the code iterated all Lhs. The numbered "Guard ordering" list also did not include isAssignToSkipField, leaving future extenders with a stale picture. The fix narrows the AssignStmt branch to single-target SelectorExpr LHS only and aligns the doc, including explicit notes on the tuple and local-var carve-outs so the design intent is auditable from the repo alone. Co-Authored-By: Claude Opus 4.7 (1M context) --- linters/isolint/analyzer.go | 34 ++++++++++--------- linters/isolint/docs/decisions.md | 11 +++--- .../testdata/skipfields/still_flags.go | 20 +++++++++++ 3 files changed, 45 insertions(+), 20 deletions(-) diff --git a/linters/isolint/analyzer.go b/linters/isolint/analyzer.go index 492d9f2..f923680 100644 --- a/linters/isolint/analyzer.go +++ b/linters/isolint/analyzer.go @@ -224,12 +224,15 @@ func isArgToSkipMethod(stack []ast.Node) bool { // // Two assignment shapes are recognized: // -// x.Field = ... // AssignStmt with SelectorExpr LHS +// x.Field = ... // AssignStmt with single SelectorExpr LHS // StructType{Field: ...} // KeyValueExpr inside a CompositeLit // // In both shapes the literal may be nested inside a CompositeLit RHS // (e.g. pq.StringArray{"MC"}), so the stack is walked from the top down -// to find the nearest enclosing assignment. +// to find the nearest enclosing assignment. Tuple assignments +// (len(Lhs) > 1) are intentionally not handled — without type information +// the analyzer cannot correlate which RHS literal belongs to which LHS +// target, so it falls through and flags every literal as usual. func isAssignToSkipField(stack []ast.Node, skipFields map[string]bool) bool { if len(skipFields) == 0 { return false @@ -246,10 +249,11 @@ func isAssignToSkipField(stack []ast.Node, skipFields map[string]bool) bool { // A KeyValueExpr binds the field; no need to keep walking. return false case *ast.AssignStmt: - for _, lhs := range parent.Lhs { - if name, ok := fieldNameFromLHS(lhs); ok && skipFields[name] { - return true - } + if len(parent.Lhs) != 1 { + return false + } + if name, ok := fieldNameFromLHS(parent.Lhs[0]); ok && skipFields[name] { + return true } return false case *ast.FuncLit, *ast.FuncDecl: @@ -268,18 +272,16 @@ func identName(expr ast.Expr) (string, bool) { return id.Name, true } -// fieldNameFromLHS extracts a field name from an assignment LHS expression. -// For x.Field it returns "Field"; for plain identifiers it returns the -// identifier name (covers cases where the LHS is itself a struct field -// shadowed by a local var, which is rare but cheap to handle). +// fieldNameFromLHS extracts a struct field name from an assignment LHS. +// Only *ast.SelectorExpr (e.g. a.CardSchemes) is matched — bare identifiers +// are local variables, not struct fields, and a literal assigned to a local +// var that happens to share a skip-field name should still be flagged. func fieldNameFromLHS(expr ast.Expr) (string, bool) { - switch e := expr.(type) { - case *ast.SelectorExpr: - return e.Sel.Name, true - case *ast.Ident: - return e.Name, true + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return "", false } - return "", false + return sel.Sel.Name, true } // callName extracts the method or function name from a call expression. diff --git a/linters/isolint/docs/decisions.md b/linters/isolint/docs/decisions.md index 9c55e92..dd0c631 100644 --- a/linters/isolint/docs/decisions.md +++ b/linters/isolint/docs/decisions.md @@ -33,9 +33,11 @@ Uppercase 2- and 3-letter strings sometimes collide with ISO codes by accident Two assignment shapes are recognized syntactically — no type information is needed: -- `*ast.AssignStmt` whose `Lhs[0]` is a `*ast.SelectorExpr` (covers `a.CardSchemes = pq.StringArray{"MC"}`) +- Single-target `*ast.AssignStmt` whose sole `Lhs` element is a `*ast.SelectorExpr` (covers `a.CardSchemes = pq.StringArray{"MC"}`). Tuple assignments (`len(Lhs) > 1`) fall through to the default flag behavior because the analyzer cannot correlate which RHS literal belongs to which LHS target without type information. - `*ast.KeyValueExpr` whose `Key` is an `*ast.Ident` (covers `Foo{CardSchemes: pq.StringArray{"MC"}}`) +Bare local variables that happen to share a skip-field name (e.g. `CardSchemes := "MC"`) are intentionally NOT skipped — a local variable is not a struct field, and broadening to plain identifiers would silently expand the linter's blind spot beyond the documented design. + The walk in [`isAssignToSkipField`](../analyzer.go) stops at function boundaries (`*ast.FuncLit`/`*ast.FuncDecl`) so a literal beyond a closure boundary is no longer treated as part of the original assignment. **Why not type-based?** Matching by the destination field's _type_ (e.g. "skip everything assigned to a `pq.StringArray`") would require `pass.TypesInfo`, which forces escalation to `LoadModeTypesInfo`. That has a project-wide cost (see below) and is also coarser — a real `pq.StringArray` of site codes would also be skipped. Field-name targeting is precise and stays within `LoadModeSyntax`. @@ -58,9 +60,10 @@ Guards in the callback are ordered by cost (cheapest first): 2. `len(lit.Value)` — length check (integer) 3. [`isImportPath`](../analyzer.go) — stack walk (already loaded) 4. [`isArgToSkipMethod`](../analyzer.go) — stack walk + string comparison -5. `strconv.Unquote` — first allocation -6. Code validation — delegates to `currency`/`site` packages -7. `fmt.Sprintf` — only on the reporting path +5. [`isAssignToSkipField`](../analyzer.go) — stack walk + string comparison; no-ops when `SkipFields` is empty +6. `strconv.Unquote` — first allocation +7. Code validation — delegates to `currency`/`site` packages +8. `fmt.Sprintf` — only on the reporting path This ordering ensures the hot path (non-string, wrong-length, import-path literals) exits before any allocation occurs. diff --git a/linters/isolint/testdata/skipfields/still_flags.go b/linters/isolint/testdata/skipfields/still_flags.go index fc0486f..972a365 100644 --- a/linters/isolint/testdata/skipfields/still_flags.go +++ b/linters/isolint/testdata/skipfields/still_flags.go @@ -28,3 +28,23 @@ func bareLiterals() { y := "USD" // want `use currency\.USD instead of "USD"` _ = y } + +// The skip is single-target. Tuple assignments (len(Lhs) > 1) fall through +// to the default flag behavior — even when one of the LHS targets is in +// the skip list, the linter cannot reliably correlate which RHS literal +// belongs to which LHS without type information, so it flags everything. +func tupleAssign() { + a := &cardAvailability{} + a.CardSchemes, a.IssuerCountries = stringArray{"MC"}, stringArray{"SG"} // want `use site\.MC instead of "MC"` `use site\.SG instead of "SG"` +} + +// The skip is targeted at struct-field assignments. A bare local variable +// that happens to share its name with a configured skip field is NOT a +// struct field — it should still flag. +func localVarShadowing() { + CardSchemes := "MC" // want `use site\.MC instead of "MC"` + _ = CardSchemes + + var IssuerCountries = "SG" // want `use site\.SG instead of "SG"` + _ = IssuerCountries +}