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..f923680 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,72 @@ 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 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. 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 + } + // 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: + 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: + 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 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) { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return "", false + } + return sel.Sel.Name, true +} + // 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..dd0c631 100644 --- a/linters/isolint/docs/decisions.md +++ b/linters/isolint/docs/decisions.md @@ -27,6 +27,23 @@ 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: + +- 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`. + +**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. @@ -43,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/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..972a365 --- /dev/null +++ b/linters/isolint/testdata/skipfields/still_flags.go @@ -0,0 +1,50 @@ +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 +} + +// 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 +}