diff --git a/linters/isolint/AGENTS.md b/linters/isolint/AGENTS.md new file mode 100644 index 0000000..ca2137f --- /dev/null +++ b/linters/isolint/AGENTS.md @@ -0,0 +1,50 @@ +# 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`. + +## Structure + +- `analyzer.go` — entry point; walks `*ast.BasicLit` nodes with `inspector.WithStack` for parent context +- `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 +- `testdata/` — separate Go module with test fixtures and `.golden` files + +## Commands + +```bash +go test -v ./... # run all tests +go build ./... # compile +go vet ./... # static analysis +revive ./... # lint (catches unused params, etc.) +golangci-lint run --enable-only revive # alternative: revive via golangci-lint +``` + +## Testing + +Uses `golang.org/x/tools/go/analysis/analysistest`. See [docs/testing.md](docs/testing.md) for the full testing guide. + +- Files with `// want` annotations assert expected diagnostics (positive tests) +- `valid.go` / `valid_contexts.go` assert zero false positives (negative tests) +- `.go.golden` files verify auto-fix output via `RunWithSuggestedFixes` +- `testdata/` is its own Go module — run `cd testdata && go mod tidy` after changing dependencies + +## Key decisions + +See [docs/decisions.md](docs/decisions.md) for rationale behind each decision. + +- **Uppercase only** — lowercase and mixed case are ignored +- **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` +- **Load mode is `LoadModeSyntax`** — only needs string literal values, not type info + +## Performance + +See [docs/decisions.md](docs/decisions.md) for guard ordering rationale and anti-patterns. + +- **`LoadModeSyntax`** — cheapest load mode; type-checker never runs +- **Shared inspector** — `pass.ResultOf[inspect.Analyzer]` +- **Narrow node filter** — `[]ast.Node{(*ast.BasicLit)(nil)}` with `inspector.WithStack` +- **Guard order** — cheapest checks first; allocations (`strconv.Unquote`, `fmt.Sprintf`) only on the reporting path diff --git a/linters/isolint/CLAUDE.md b/linters/isolint/CLAUDE.md new file mode 100644 index 0000000..43c994c --- /dev/null +++ b/linters/isolint/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/linters/isolint/Makefile b/linters/isolint/Makefile new file mode 100644 index 0000000..d126544 --- /dev/null +++ b/linters/isolint/Makefile @@ -0,0 +1,11 @@ +.PHONY: test lint build + +test: + go test -v ./... + +lint: + revive -formatter friendly ./... + go vet ./... + +build: + go build ./... diff --git a/linters/isolint/README.md b/linters/isolint/README.md new file mode 100644 index 0000000..d401375 --- /dev/null +++ b/linters/isolint/README.md @@ -0,0 +1,153 @@ +# isolint + +A Go analyzer that detects raw ISO code string literals and recommends using `github.com/wego/pkg/currency` and `github.com/wego/pkg/iso/site` package constants instead. + +## Installation + +### Automatic way (recommended) + +This follows golangci-lint's "Automatic Way" module plugin flow. + +Requirements: Go and git. + +1. Create `.custom-gcl.yml` in your project: + +```yaml +version: v2.8.0 +plugins: + - module: github.com/wego/pkg/linters/isolint + version: v0.1.0 +``` + +2. Build custom golangci-lint: + +```bash +golangci-lint custom +``` + +3. Configure the plugin in `.golangci.yml`: + +```yaml +version: "2" + +linters: + enable: + - isolint + settings: + custom: + isolint: + type: "module" + description: "Enforces currency/site package constant usage" +``` + +4. Run the resulting custom binary: + +```bash +./custom-gcl run ./... +``` + +### As a standalone tool + +```bash +go install github.com/wego/pkg/linters/isolint/cmd/isolint@latest +isolint ./... +``` + +## What it detects + +### Currency codes (ISO 4217) + +| Pattern | Suggestion | +|---------|------------| +| `"USD"` | `currency.USD` | +| `"EUR"` | `currency.EUR` | +| `"SGD"` | `currency.SGD` | +| ... | All 182 ISO 4217 codes | + +### Site codes (ISO 3166-1 alpha-2) + +| Pattern | Suggestion | +|---------|------------| +| `"SG"` | `site.SG` | +| `"US"` | `site.US` | +| `"JP"` | `site.JP` | +| ... | All 249 ISO 3166-1 alpha-2 codes | + +### Detected positions + +The linter catches raw ISO code strings in **all** Go expression positions: + +- Comparisons: `code == "USD"`, `"SG" != code` +- Assignments: `x := "USD"`, `var x = "SG"` +- Constant/var declarations: `const c = "USD"` +- Switch/case: `case "USD":`, `case "SG":` +- Map keys/values: `map[string]int{"USD": 1}`, `m["SG"]` +- Function arguments: `foo("USD")`, `fmt.Println("SG")` +- Struct fields: `Config{Currency: "USD"}` +- Return values: `return "USD"` +- Slice/array literals: `[]string{"USD", "SGD"}` + +### What it skips + +- Files in `github.com/wego/pkg/currency` (defines the currency constants) +- Files in `github.com/wego/pkg/iso/site` (defines the site constants) +- Non-ISO strings like `"hello"`, `""`, `"test"` +- Lowercase strings like `"usd"`, `"sg"` +- Code already using constants: `currency.USD`, `site.SG` + +## Import convention + +```go +import ( + "github.com/wego/pkg/currency" + "github.com/wego/pkg/iso/site" +) + +// The linter suggests: +if code == currency.USD { ... } +if siteCode == site.SG { ... } +``` + +## Auto-fix + +The linter provides suggested fixes that can be applied automatically: + +```bash +# With golangci-lint +./custom-gcl run --fix ./... + +# With standalone tool +isolint -fix ./... +``` + +**Note**: Auto-fix replaces the string literal but does not add the import statement. You will need to: +1. Run `goimports` to add missing imports +2. Verify the correct package is imported + +## Development + +### Local tests + +The testdata directory is a standalone module. Run tests from the module root: + +```bash +go test -v ./... +``` + +### Using a commit before tagging + +If you need to consume an untagged commit from another repo, use a Go pseudo-version +instead of a raw SHA. + +```bash +go list -m -json github.com/wego/pkg/linters/isolint@ +``` + +Then use the returned `Version` value in `.custom-gcl.yml`: + +```yaml +version: v2.8.0 +plugins: + - module: github.com/wego/pkg/linters/isolint + version: v0.0.0-20260120hhmmss-abcdef123456 +``` diff --git a/linters/isolint/analyzer.go b/linters/isolint/analyzer.go new file mode 100644 index 0000000..79e41b2 --- /dev/null +++ b/linters/isolint/analyzer.go @@ -0,0 +1,160 @@ +// Package isolint provides a Go analyzer that detects raw ISO code string +// literals and recommends using github.com/wego/pkg/currency and +// github.com/wego/pkg/iso/site package constants instead. +package isolint + +import ( + "go/ast" + "go/token" + "strconv" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// skipPackages are package import paths whose source files should not be +// checked. These packages define the constants themselves and must use raw +// string literals. +var skipPackages = map[string]bool{ + "github.com/wego/pkg/currency": true, + "github.com/wego/pkg/iso/site": true, +} + +// skipMethods are method/function names whose string arguments are key or +// column names, not ISO code values. When a string literal appears as an +// argument to one of these calls, it is skipped to reduce false positives. +var skipMethods = map[string]bool{ + // HTTP framework methods — args are parameter names. + "Query": true, + "QueryParam": true, + "Param": true, + "FormValue": true, + "GetQuery": true, + "DefaultQuery": true, + "PostForm": true, + + // ORM/DB methods — args are column names or SQL fragments. + "Select": true, + "Pluck": true, + "Omit": true, + + // Custom filter methods — args are column names. + "Equals": true, + "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}, +} + +func run(pass *analysis.Pass) (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()] { + return nil, nil + } + + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.BasicLit)(nil), + } + + // WithStack gives us ancestor context so we can skip import paths + // and arguments to known key-accepting methods. + inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { + if !push { + return true + } + + lit := n.(*ast.BasicLit) + if lit.Kind != token.STRING { + return true + } + + // Fast path: currency codes are 3 chars ("XXX" = 5 bytes quoted), + // site codes are 2 chars ("XX" = 4 bytes quoted). + // Skip everything else before allocating via Unquote. + vlen := len(lit.Value) + if vlen != 4 && vlen != 5 { + return true + } + + // Import path literals (e.g. "io") are not ISO codes. + if isImportPath(stack) { + return true + } + + // 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. + if isArgToSkipMethod(stack) { + return true + } + + value, err := strconv.Unquote(lit.Value) + if err != nil { + return true + } + + // Route by length — currency and site codes can never overlap. + switch len(value) { + case 3: + if IsCurrencyCode(value) { + reportCurrencyDiagnostic(pass, lit, value) + } + case 2: + if IsSiteCode(value) { + reportSiteDiagnostic(pass, lit, value) + } + } + + return true + }) + + return nil, nil +} + +// isImportPath reports whether the BasicLit at the top of the stack is the +// path of an import declaration. +func isImportPath(stack []ast.Node) bool { + if len(stack) < 2 { + return false + } + _, ok := stack[len(stack)-2].(*ast.ImportSpec) + return ok +} + +// isArgToSkipMethod reports whether the BasicLit at the top of the stack is +// a direct argument to a call expression whose method/function name appears +// in skipMethods. +func isArgToSkipMethod(stack []ast.Node) bool { + if len(stack) < 2 { + return false + } + call, ok := stack[len(stack)-2].(*ast.CallExpr) + if !ok { + return false + } + return skipMethods[callName(call)] +} + +// 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. +// Returns "" if the pattern doesn't match. +func callName(call *ast.CallExpr) string { + switch fn := call.Fun.(type) { + case *ast.SelectorExpr: + return fn.Sel.Name + case *ast.Ident: + return fn.Name + } + return "" +} diff --git a/linters/isolint/analyzer_test.go b/linters/isolint/analyzer_test.go new file mode 100644 index 0000000..672278e --- /dev/null +++ b/linters/isolint/analyzer_test.go @@ -0,0 +1,19 @@ +package isolint_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/wego/pkg/linters/isolint" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, isolint.Analyzer, "./example") +} + +func TestAnalyzerWithFixes(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, isolint.Analyzer, "./example") +} diff --git a/linters/isolint/cmd/isolint/main.go b/linters/isolint/cmd/isolint/main.go new file mode 100644 index 0000000..673b6fd --- /dev/null +++ b/linters/isolint/cmd/isolint/main.go @@ -0,0 +1,12 @@ +// Command isolint runs the isolint analyzer. +package main + +import ( + "golang.org/x/tools/go/analysis/singlechecker" + + "github.com/wego/pkg/linters/isolint" +) + +func main() { + singlechecker.Main(isolint.Analyzer) +} diff --git a/linters/isolint/codes.go b/linters/isolint/codes.go new file mode 100644 index 0000000..4ba8692 --- /dev/null +++ b/linters/isolint/codes.go @@ -0,0 +1,69 @@ +package isolint + +import ( + "fmt" + "strings" + + "github.com/wego/pkg/currency" + "github.com/wego/pkg/iso/site" +) + +// IsCurrencyCode reports whether s is a known uppercase ISO 4217 currency +// code (e.g. "USD"). Lowercase ("usd") and mixed case ("Usd") are not +// matched — only uppercase strings are considered intentional ISO references. +func IsCurrencyCode(s string) bool { + if len(s) != 3 { + return false + } + if !isAllUpper(s) { + return false + } + return currency.IsISO4217(s) +} + +// IsSiteCode reports whether s is a known uppercase ISO 3166-1 alpha-2 site +// code (e.g. "SG"). Lowercase ("sg") and mixed case ("Sg") are not matched +// — only uppercase strings are considered intentional ISO references. +func IsSiteCode(s string) bool { + if len(s) != 2 { + return false + } + if !isAllUpper(s) { + return false + } + _, found := site.Currency(s) + return found +} + +// NormalizeCurrencyCode returns the canonical uppercase form of a currency code. +func NormalizeCurrencyCode(code string) string { + return strings.ToUpper(code) +} + +// NormalizeSiteCode returns the canonical uppercase form of a site code. +func NormalizeSiteCode(code string) string { + return strings.ToUpper(code) +} + +// CurrencyConstName returns the qualified constant name for a currency code, +// e.g., "USD" -> "currency.USD". +func CurrencyConstName(code string) string { + return fmt.Sprintf("currency.%s", NormalizeCurrencyCode(code)) +} + +// SiteConstName returns the qualified constant name for a site code, +// e.g., "SG" -> "site.SG". +func SiteConstName(code string) string { + return fmt.Sprintf("site.%s", NormalizeSiteCode(code)) +} + +// isAllUpper reports whether s contains only uppercase ASCII letters. +func isAllUpper(s string) bool { + for i := 0; i < len(s); i++ { + if s[i] < 'A' || s[i] > 'Z' { + return false + } + } + return len(s) > 0 +} + diff --git a/linters/isolint/codes_test.go b/linters/isolint/codes_test.go new file mode 100644 index 0000000..72b8042 --- /dev/null +++ b/linters/isolint/codes_test.go @@ -0,0 +1,220 @@ +package isolint + +import "testing" + +func TestIsCurrencyCode(t *testing.T) { + tests := []struct { + code string + want bool + }{ + // Uppercase (canonical) + {"USD", true}, + {"EUR", true}, + {"GBP", true}, + {"JPY", true}, + {"SGD", true}, + {"AUD", true}, + {"CNY", true}, + {"INR", true}, + {"THB", true}, + {"VND", true}, + + // Lowercase — not flagged (only uppercase is linted) + {"usd", false}, + {"eur", false}, + {"sgd", false}, + {"jpy", false}, + + // Edge currency codes + {"XAU", true}, // Gold + {"XAG", true}, // Silver + {"XXX", true}, // No currency + {"ZWG", true}, // Newest (2024) + {"BOV", true}, // Rare fund code + {"PRB", false}, // Transnistrian Ruble — constant exists but not in ISO 4217 validation + {"CHE", true}, // WIR Euro + {"USN", true}, // US Dollar next day + + // NOT currency codes + {"", false}, + {"US", false}, // Too short (site code) + {"USDD", false}, // Too long + {"Usd", false}, // Mixed case — not matched + {"uSd", false}, // Mixed case — not matched + {"hello", false}, // Random string + {"123", false}, // Numbers + {"SG", false}, // Site code, not currency + {"XYZ", false}, // Not a real code + {"ABC", false}, // Not a real code + {"FOO", false}, // Not a real code + {"foo", false}, // Not a real code (lowercase) + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + got := IsCurrencyCode(tt.code) + if got != tt.want { + t.Errorf("IsCurrencyCode(%q) = %v, want %v", tt.code, got, tt.want) + } + }) + } +} + +func TestIsSiteCode(t *testing.T) { + tests := []struct { + code string + want bool + }{ + // Uppercase (canonical) + {"SG", true}, + {"US", true}, + {"GB", true}, + {"JP", true}, + {"AU", true}, + {"DE", true}, + {"FR", true}, + {"IN", true}, + {"TH", true}, + {"VN", true}, + + // Lowercase — not flagged (only uppercase is linted) + {"sg", false}, + {"us", false}, + {"jp", false}, + {"gb", false}, + + // Edge site codes — site.Currency() only covers sites with a currency mapping + {"AQ", false}, // Antarctica — no currency mapping + {"BV", true}, // Bouvet Island — maps to NOK + {"HM", true}, // Heard Island — maps to AUD + {"UM", true}, // US Minor Outlying Islands — maps to USD + {"AX", false}, // Aland Islands — no currency mapping + {"BQ", true}, // Bonaire — maps to USD + + // NOT site codes + {"", false}, + {"USD", false}, // Too long (currency code) + {"S", false}, // Too short + {"SGD", false}, // Currency code, not site + {"Sg", false}, // Mixed case — not matched + {"hello", false}, // Random string + {"12", false}, // Numbers + {"XX", false}, // Not a real code + {"ZZ", false}, // Not a real code + {"QQ", false}, // Not a real code + {"xx", false}, // Not a real code (lowercase) + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + got := IsSiteCode(tt.code) + if got != tt.want { + t.Errorf("IsSiteCode(%q) = %v, want %v", tt.code, got, tt.want) + } + }) + } +} + +func TestCurrencyConstName(t *testing.T) { + tests := []struct { + code string + want string + }{ + {"USD", "currency.USD"}, + {"EUR", "currency.EUR"}, + {"SGD", "currency.SGD"}, + {"JPY", "currency.JPY"}, + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + got := CurrencyConstName(tt.code) + if got != tt.want { + t.Errorf("CurrencyConstName(%q) = %q, want %q", tt.code, got, tt.want) + } + }) + } +} + +func TestSiteConstName(t *testing.T) { + tests := []struct { + code string + want string + }{ + {"SG", "site.SG"}, + {"US", "site.US"}, + {"JP", "site.JP"}, + {"GB", "site.GB"}, + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + got := SiteConstName(tt.code) + if got != tt.want { + t.Errorf("SiteConstName(%q) = %q, want %q", tt.code, got, tt.want) + } + }) + } +} + +func TestNoOverlapBetweenCurrencyAndSite(t *testing.T) { + // Currency codes are 3 letters, site codes are 2 letters — no overlap possible. + currencySamples := []string{"USD", "EUR", "SGD", "JPY", "GBP", "AUD", "THB", "VND"} + siteSamples := []string{"SG", "US", "JP", "GB", "AU", "TH", "VN", "DE"} + + for _, code := range currencySamples { + if IsSiteCode(code) { + t.Errorf("currency code %q is also a site code — unexpected overlap", code) + } + } + + for _, code := range siteSamples { + if IsCurrencyCode(code) { + t.Errorf("site code %q is also a currency code — unexpected overlap", code) + } + } +} + +// Benchmarks — measure the cost of checking a string literal in the hot path. + +func BenchmarkIsCurrencyCode_Match(b *testing.B) { + for b.Loop() { + IsCurrencyCode("USD") + } +} + +func BenchmarkIsCurrencyCode_LowercaseMatch(b *testing.B) { + for b.Loop() { + IsCurrencyCode("usd") + } +} + +func BenchmarkIsCurrencyCode_NoMatch_WrongLength(b *testing.B) { + for b.Loop() { + IsCurrencyCode("error: something went wrong") + } +} + +func BenchmarkIsCurrencyCode_NoMatch_RightLength(b *testing.B) { + for b.Loop() { + IsCurrencyCode("foo") + } +} + +func BenchmarkIsSiteCode_Match(b *testing.B) { + for b.Loop() { + IsSiteCode("SG") + } +} + +func BenchmarkIsSiteCode_LowercaseMatch(b *testing.B) { + for b.Loop() { + IsSiteCode("sg") + } +} + +func BenchmarkIsSiteCode_NoMatch_WrongLength(b *testing.B) { + for b.Loop() { + IsSiteCode("this is a long string") + } +} diff --git a/linters/isolint/docs/decisions.md b/linters/isolint/docs/decisions.md new file mode 100644 index 0000000..4ada5bb --- /dev/null +++ b/linters/isolint/docs/decisions.md @@ -0,0 +1,57 @@ +# isolint — Design Decisions + +## Uppercase only + +Only uppercase string literals (`"USD"`, `"SG"`) are flagged. Lowercase (`"usd"`, `"sg"`) and mixed case (`"Usd"`, `"Sg"`) are ignored. + +Rationale: uppercase in Go source code signals an intentional ISO reference — it matches the canonical form defined by ISO 4217 (currency) and ISO 3166-1 alpha-2 (country/site). Lowercase is almost always an identifier, parameter name, or English word. Flagging it would produce excessive false positives with minimal benefit. + +## Code validation delegates to source packages + +[`currency.IsISO4217()`](../codes.go) and [`site.Currency()`](../codes.go) are called directly rather than maintaining hardcoded maps inside the linter. + +- Single source of truth — if a new currency or site is added to the domain packages, the linter picks it up automatically. +- `site.Currency()` covers only sites with currency mappings (AQ, AX, etc. without mappings won't be flagged). This is intentional — flagging a site code that has no constant to replace it with would be a false positive. + +## Skip mechanisms + +### Package skip ([`skipPackages`](../analyzer.go)) + +`pass.Pkg.Path()` is checked against [`skipPackages`](../analyzer.go). This prevents the linter from flagging ISO literals inside the packages that _define_ the constants (e.g. the `currency` or `site` package itself). Add new entries to `skipPackages` in [`analyzer.go`](../analyzer.go) when introducing new definition packages. + +### Import path skip + +Import paths like `import "io"` are syntactically `*ast.BasicLit` strings. The linter checks the parent stack (via [`inspector.WithStack`](../analyzer.go)) to skip `*ast.ImportSpec` parents. + +### Call argument skip ([`skipMethods`](../analyzer.go)) + +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. + +## `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. + +Why this matters: golangci-lint takes the [union of all enabled linters' load modes](https://golangci-lint.run/docs/contributing/architecture/). Claiming `LoadModeTypesInfo` unnecessarily forces type-checking for the entire batch. A [real-world regression in depguard](https://github.com/golangci/golangci-lint/issues/2670) was caused by claiming type info when it wasn't needed. + +**Do not change to `LoadModeTypesInfo`** unless you add logic that genuinely requires `pass.TypesInfo`. + +## Guard ordering + +Guards in the callback are ordered by cost (cheapest first): + +1. `lit.Kind` — token comparison (integer) +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 + +This ordering ensures the hot path (non-string, wrong-length, import-path literals) exits before any allocation occurs. + +## Anti-patterns to avoid when extending + +- Claiming `LoadModeTypesInfo` when only syntax is needed — forces type-checking for all linters in the batch +- Constructing `inspector.New(pass.Files)` instead of using `pass.ResultOf[inspect.Analyzer]` — pays double construction cost and loses sharing +- Passing `nil` as the node filter to `inspector.WithStack` — visits every AST node +- Calling `fmt.Sprintf` or `strconv.Unquote` on every visited node — allocates on the hot path diff --git a/linters/isolint/docs/testing.md b/linters/isolint/docs/testing.md new file mode 100644 index 0000000..12e97ff --- /dev/null +++ b/linters/isolint/docs/testing.md @@ -0,0 +1,71 @@ +# isolint — Testing Guide + +## Framework + +Tests use [`golang.org/x/tools/go/analysis/analysistest`](https://pkg.go.dev/golang.org/x/tools/go/analysis/analysistest), the standard testing harness for Go analysis passes. + +## The bidirectional contract + +`analysistest` enforces a **bidirectional contract** between test fixtures and analyzer output: + +1. **Every `// want` annotation must match a produced diagnostic** — otherwise the test fails ("missing expected diagnostic"). +2. **Every produced diagnostic must match a `// want` annotation** — otherwise the test fails ("unexpected diagnostic"). + +Rule 2 is the crucial one. It means any `.go` file in `testdata/` that does _not_ have a `// want` comment is implicitly asserting **zero diagnostics**. If the analyzer accidentally flags something in that file, the test fails. + +## Test file roles + +| File | Role | +| --------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------- | +| [`testdata/example/currency_literal.go`](../testdata/example/currency_literal.go) | Positive test — currency literals with `// want` annotations | +| [`testdata/example/site_literal.go`](../testdata/example/site_literal.go) | Positive test — site literals with `// want` annotations | +| [`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 | + +### Why `valid.go` and `valid_contexts.go` matter + +These files contain inputs that look like they could be flagged but should not be: + +- `currency.USD` — already using a constant +- `"usd"` — lowercase (not an ISO reference) +- `"Usd"` — mixed case +- `"XYZ"` — not a valid ISO code +- `db.Select("SG")` — call argument to a skipped method + +If the analyzer accidentally matches any of these, the test fails because there is no `// want` to absorb the diagnostic. This makes these files **assertions of zero false positives**. + +## Golden files + +`.go.golden` files verify auto-fix output via [`analysistest.RunWithSuggestedFixes`](https://pkg.go.dev/golang.org/x/tools/go/analysis/analysistest#RunWithSuggestedFixes). Each golden file corresponds to a positive test file and shows what the file should look like after all suggested fixes are applied. + +Golden files are only needed for files that produce fixes. `valid.go` and `valid_contexts.go` need no golden file because they produce no diagnostics. + +## How to add tests + +### Adding a positive test case (new detection) + +1. Create a new `.go` file in `testdata/example/` (or add to an existing positive test file). +2. Add `// want` annotations on lines that should produce diagnostics: + ```go + var x = "USD" // want `"USD" is a known ISO 4217 currency code` + ``` +3. Create a matching `.go.golden` file showing the expected fix: + ```go + var x = currency.USD + ``` +4. Run `go test ./...` — the test should pass with the new annotations. + +### Adding a negative test case (false positive guard) + +1. Add the case to `valid.go` (general) or `valid_contexts.go` (call contexts). +2. Do **not** add any `// want` annotation. +3. Run `go test ./...` — if the analyzer incorrectly flags your case, the test will fail with "unexpected diagnostic". + +## The `testdata/` module + +The `testdata/` directory is its own Go module with `replace` directives pointing to `../../currency` and `../../../iso/site`. After changing those dependencies, run: + +```bash +cd testdata && go mod tidy +``` diff --git a/linters/isolint/go.mod b/linters/isolint/go.mod new file mode 100644 index 0000000..e28c2e1 --- /dev/null +++ b/linters/isolint/go.mod @@ -0,0 +1,19 @@ +module github.com/wego/pkg/linters/isolint + +go 1.24.0 + +toolchain go1.24.2 + +require ( + github.com/golangci/plugin-module-register v0.1.2 + github.com/wego/pkg/currency v0.4.4 + github.com/wego/pkg/iso/site v0.1.1 + golang.org/x/tools v0.42.0 +) + +require ( + github.com/bojanz/currency v1.3.0 // indirect + github.com/cockroachdb/apd/v3 v3.2.1 // indirect + golang.org/x/mod v0.33.0 // indirect + golang.org/x/sync v0.19.0 // indirect +) diff --git a/linters/isolint/go.sum b/linters/isolint/go.sum new file mode 100644 index 0000000..e2a79da --- /dev/null +++ b/linters/isolint/go.sum @@ -0,0 +1,28 @@ +github.com/bojanz/currency v1.3.0 h1:HlgIxAaD7xMCk1RtjR5b7UKG3d5BBTPZORSPAWefhro= +github.com/bojanz/currency v1.3.0/go.mod h1:jNoZiJyRTqoU5DFoa+n+9lputxPUDa8Fz8BdDrW06Go= +github.com/cockroachdb/apd/v3 v3.2.1 h1:U+8j7t0axsIgvQUqthuNm82HIrYXodOV2iWLWtEaIwg= +github.com/cockroachdb/apd/v3 v3.2.1/go.mod h1:klXJcjp+FffLTHlhIG69tezTDvdP065naDsHzKhYSqc= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/golangci/plugin-module-register v0.1.2 h1:e5WM6PO6NIAEcij3B053CohVp3HIYbzSuP53UAYgOpg= +github.com/golangci/plugin-module-register v0.1.2/go.mod h1:1+QGTsKBvAIvPvoY/os+G5eoqxWn70HYDm2uvUyGuVw= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/lib/pq v1.10.7 h1:p7ZhMD+KsSRozJr34udlUrhboJwWAgCg34+/ZZNvZZw= +github.com/lib/pq v1.10.7/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +github.com/wego/pkg/currency v0.4.4 h1:chcer5wumfaRmdrNvz4uP6Vr2aPcjScMhT503xQcGhw= +github.com/wego/pkg/currency v0.4.4/go.mod h1:JtkbrhYTq5fqza2KBosiDSDkZJP9P7COzGugMcIDtyk= +github.com/wego/pkg/iso/site v0.1.1 h1:UCg0Dkb1slCLVAA6gnfJBnqCSKhKXdpdOUdhqc3dQ+E= +github.com/wego/pkg/iso/site v0.1.1/go.mod h1:FuIVtYUxetKWwkoc6vFBClnsA/Gf1Njvxo6hqduuBgE= +golang.org/x/mod v0.33.0 h1:tHFzIWbBifEmbwtGz65eaWyGiGZatSrT9prnU8DbVL8= +golang.org/x/mod v0.33.0/go.mod h1:swjeQEj+6r7fODbD2cqrnje9PnziFuw4bmLbBZFrQ5w= +golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= +golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/tools v0.42.0 h1:uNgphsn75Tdz5Ji2q36v/nsFSfR/9BRFvqhGBaJGd5k= +golang.org/x/tools v0.42.0/go.mod h1:Ma6lCIwGZvHK6XtgbswSoWroEkhugApmsXyrUmBhfr0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/linters/isolint/nil_pkg_test.go b/linters/isolint/nil_pkg_test.go new file mode 100644 index 0000000..d8e1a6a --- /dev/null +++ b/linters/isolint/nil_pkg_test.go @@ -0,0 +1,46 @@ +package isolint + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// TestRunWithNilPkg verifies that the analyzer does not panic when pass.Pkg is +// nil. golangci-lint may leave pass.Pkg unset under LoadModeSyntax for certain +// packages, so the analyzer must tolerate it. +func TestRunWithNilPkg(t *testing.T) { + const src = `package example + +func f() { + _ = "USD" +} +` + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "example.go", src, 0) + if err != nil { + t.Fatal(err) + } + + ins := inspector.New([]*ast.File{file}) + + pass := &analysis.Pass{ + Analyzer: Analyzer, + Fset: fset, + Files: []*ast.File{file}, + Pkg: nil, // simulate LoadModeSyntax with nil Pkg + ResultOf: map[*analysis.Analyzer]any{inspect.Analyzer: ins}, + Report: func(analysis.Diagnostic) {}, + } + + // Must not panic. + _, err = run(pass) + if err != nil { + t.Fatalf("run() returned unexpected error: %v", err) + } +} diff --git a/linters/isolint/plugin.go b/linters/isolint/plugin.go new file mode 100644 index 0000000..bd19f6b --- /dev/null +++ b/linters/isolint/plugin.go @@ -0,0 +1,26 @@ +package isolint + +import ( + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +func init() { + register.Plugin(Analyzer.Name, New) +} + +// 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 +} + +type plugin struct{} + +func (plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{Analyzer}, nil +} + +func (plugin) GetLoadMode() string { + return register.LoadModeSyntax +} diff --git a/linters/isolint/report.go b/linters/isolint/report.go new file mode 100644 index 0000000..4f1a77e --- /dev/null +++ b/linters/isolint/report.go @@ -0,0 +1,57 @@ +package isolint + +import ( + "fmt" + "go/ast" + + "golang.org/x/tools/go/analysis" +) + +const ( + currencyPkgPath = "github.com/wego/pkg/currency" + sitePkgPath = "github.com/wego/pkg/iso/site" +) + +func reportCurrencyDiagnostic(pass *analysis.Pass, lit *ast.BasicLit, code string) { + constName := CurrencyConstName(code) + + pass.Report(analysis.Diagnostic{ + Pos: lit.Pos(), + End: lit.End(), + Message: fmt.Sprintf("use %s instead of %q (import %q)", constName, code, currencyPkgPath), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("Replace with %s", constName), + TextEdits: []analysis.TextEdit{ + { + Pos: lit.Pos(), + End: lit.End(), + NewText: []byte(constName), + }, + }, + }, + }, + }) +} + +func reportSiteDiagnostic(pass *analysis.Pass, lit *ast.BasicLit, code string) { + constName := SiteConstName(code) + + pass.Report(analysis.Diagnostic{ + Pos: lit.Pos(), + End: lit.End(), + Message: fmt.Sprintf("use %s instead of %q (import %q)", constName, code, sitePkgPath), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("Replace with %s", constName), + TextEdits: []analysis.TextEdit{ + { + Pos: lit.Pos(), + End: lit.End(), + NewText: []byte(constName), + }, + }, + }, + }, + }) +} diff --git a/linters/isolint/testdata/example/currency_literal.go b/linters/isolint/testdata/example/currency_literal.go new file mode 100644 index 0000000..fb9fc98 --- /dev/null +++ b/linters/isolint/testdata/example/currency_literal.go @@ -0,0 +1,76 @@ +package example + +import "github.com/wego/pkg/currency" + +func currencyChecks() { + // Comparisons + var code string + if code == "USD" { // want `use currency\.USD instead of "USD"` + println("match") + } + if "EUR" == code { // want `use currency\.EUR instead of "EUR"` + println("match") + } + if code != "SGD" { // want `use currency\.SGD instead of "SGD"` + println("no match") + } + + // Assignments + x := "JPY" // want `use currency\.JPY instead of "JPY"` + _ = x + + // Var declarations + var y = "GBP" // want `use currency\.GBP instead of "GBP"` + _ = y + + // Switch/case + switch code { + case "THB": // want `use currency\.THB instead of "THB"` + println("thai baht") + case "VND": // want `use currency\.VND instead of "VND"` + println("dong") + } + + // Function arguments + println("IDR") // want `use currency\.IDR instead of "IDR"` + + // Map keys + m := map[string]int{ + "AUD": 1, // want `use currency\.AUD instead of "AUD"` + "NZD": 2, // want `use currency\.NZD instead of "NZD"` + } + _ = m["CAD"] // want `use currency\.CAD instead of "CAD"` + + // Slice literals + codes := []string{ + "MYR", // want `use currency\.MYR instead of "MYR"` + "PHP", // want `use currency\.PHP instead of "PHP"` + } + _ = codes + + // Struct fields + type config struct { + Currency string + } + c := config{Currency: "INR"} // want `use currency\.INR instead of "INR"` + _ = c + + // Return values (via helper) + _ = getCurrency() +} + +func getCurrency() string { + return "HKD" // want `use currency\.HKD instead of "HKD"` +} + +func lowercaseAndMixedCaseCurrency() { + // Lowercase is not flagged — only uppercase is linted + _ = "usd" + _ = "sgd" + + // Mixed case is not flagged + _ = "Usd" +} + +// Ensure currency package is used (for compilation) +var _ = currency.USD diff --git a/linters/isolint/testdata/example/currency_literal.go.golden b/linters/isolint/testdata/example/currency_literal.go.golden new file mode 100644 index 0000000..9de274d --- /dev/null +++ b/linters/isolint/testdata/example/currency_literal.go.golden @@ -0,0 +1,76 @@ +package example + +import "github.com/wego/pkg/currency" + +func currencyChecks() { + // Comparisons + var code string + if code == currency.USD { // want `use currency\.USD instead of "USD"` + println("match") + } + if currency.EUR == code { // want `use currency\.EUR instead of "EUR"` + println("match") + } + if code != currency.SGD { // want `use currency\.SGD instead of "SGD"` + println("no match") + } + + // Assignments + x := currency.JPY // want `use currency\.JPY instead of "JPY"` + _ = x + + // Var declarations + var y = currency.GBP // want `use currency\.GBP instead of "GBP"` + _ = y + + // Switch/case + switch code { + case currency.THB: // want `use currency\.THB instead of "THB"` + println("thai baht") + case currency.VND: // want `use currency\.VND instead of "VND"` + println("dong") + } + + // Function arguments + println(currency.IDR) // want `use currency\.IDR instead of "IDR"` + + // Map keys + m := map[string]int{ + currency.AUD: 1, // want `use currency\.AUD instead of "AUD"` + currency.NZD: 2, // want `use currency\.NZD instead of "NZD"` + } + _ = m[currency.CAD] // want `use currency\.CAD instead of "CAD"` + + // Slice literals + codes := []string{ + currency.MYR, // want `use currency\.MYR instead of "MYR"` + currency.PHP, // want `use currency\.PHP instead of "PHP"` + } + _ = codes + + // Struct fields + type config struct { + Currency string + } + c := config{Currency: currency.INR} // want `use currency\.INR instead of "INR"` + _ = c + + // Return values (via helper) + _ = getCurrency() +} + +func getCurrency() string { + return currency.HKD // want `use currency\.HKD instead of "HKD"` +} + +func lowercaseAndMixedCaseCurrency() { + // Lowercase is not flagged — only uppercase is linted + _ = "usd" + _ = "sgd" + + // Mixed case is not flagged + _ = "Usd" +} + +// Ensure currency package is used (for compilation) +var _ = currency.USD diff --git a/linters/isolint/testdata/example/site_literal.go b/linters/isolint/testdata/example/site_literal.go new file mode 100644 index 0000000..08bb734 --- /dev/null +++ b/linters/isolint/testdata/example/site_literal.go @@ -0,0 +1,72 @@ +package example + +import "github.com/wego/pkg/iso/site" + +func siteChecks() { + // Comparisons + var code string + if code == "SG" { // want `use site\.SG instead of "SG"` + println("match") + } + if "US" == code { // want `use site\.US instead of "US"` + println("match") + } + if code != "JP" { // want `use site\.JP instead of "JP"` + println("no match") + } + + // Assignments + x := "GB" // want `use site\.GB instead of "GB"` + _ = x + + // Switch/case + switch code { + case "TH": // want `use site\.TH instead of "TH"` + println("thailand") + case "VN": // want `use site\.VN instead of "VN"` + println("vietnam") + } + + // Function arguments + println("AE") // want `use site\.AE instead of "AE"` + + // Map keys + m := map[string]int{ + "AU": 1, // want `use site\.AU instead of "AU"` + "NZ": 2, // want `use site\.NZ instead of "NZ"` + } + _ = m["CA"] // want `use site\.CA instead of "CA"` + + // Slice literals + codes := []string{ + "MY", // want `use site\.MY instead of "MY"` + "PH", // want `use site\.PH instead of "PH"` + } + _ = codes + + // Struct fields + type config struct { + Site string + } + c := config{Site: "IN"} // want `use site\.IN instead of "IN"` + _ = c + + // Return values + _ = getSite() +} + +func getSite() string { + return "HK" // want `use site\.HK instead of "HK"` +} + +func lowercaseAndMixedCaseSite() { + // Lowercase is not flagged — only uppercase is linted + _ = "sg" + _ = "us" + + // Mixed case is not flagged + _ = "Sg" +} + +// Ensure site package is used (for compilation) +var _ = site.SG diff --git a/linters/isolint/testdata/example/site_literal.go.golden b/linters/isolint/testdata/example/site_literal.go.golden new file mode 100644 index 0000000..dbd4cba --- /dev/null +++ b/linters/isolint/testdata/example/site_literal.go.golden @@ -0,0 +1,72 @@ +package example + +import "github.com/wego/pkg/iso/site" + +func siteChecks() { + // Comparisons + var code string + if code == site.SG { // want `use site\.SG instead of "SG"` + println("match") + } + if site.US == code { // want `use site\.US instead of "US"` + println("match") + } + if code != site.JP { // want `use site\.JP instead of "JP"` + println("no match") + } + + // Assignments + x := site.GB // want `use site\.GB instead of "GB"` + _ = x + + // Switch/case + switch code { + case site.TH: // want `use site\.TH instead of "TH"` + println("thailand") + case site.VN: // want `use site\.VN instead of "VN"` + println("vietnam") + } + + // Function arguments + println(site.AE) // want `use site\.AE instead of "AE"` + + // Map keys + m := map[string]int{ + site.AU: 1, // want `use site\.AU instead of "AU"` + site.NZ: 2, // want `use site\.NZ instead of "NZ"` + } + _ = m[site.CA] // want `use site\.CA instead of "CA"` + + // Slice literals + codes := []string{ + site.MY, // want `use site\.MY instead of "MY"` + site.PH, // want `use site\.PH instead of "PH"` + } + _ = codes + + // Struct fields + type config struct { + Site string + } + c := config{Site: site.IN} // want `use site\.IN instead of "IN"` + _ = c + + // Return values + _ = getSite() +} + +func getSite() string { + return site.HK // want `use site\.HK instead of "HK"` +} + +func lowercaseAndMixedCaseSite() { + // Lowercase is not flagged — only uppercase is linted + _ = "sg" + _ = "us" + + // Mixed case is not flagged + _ = "Sg" +} + +// Ensure site package is used (for compilation) +var _ = site.SG diff --git a/linters/isolint/testdata/example/valid.go b/linters/isolint/testdata/example/valid.go new file mode 100644 index 0000000..5428f10 --- /dev/null +++ b/linters/isolint/testdata/example/valid.go @@ -0,0 +1,71 @@ +package example + +import ( + "io" + + "github.com/wego/pkg/currency" + "github.com/wego/pkg/iso/site" +) + +func validCode() { + // Already using package constants — no flags + _ = currency.USD + _ = currency.EUR + _ = currency.SGD + _ = site.SG + _ = site.US + _ = site.JP + + // Non-ISO strings — no flags + if "hello" == "world" { + println("fine") + } + + x := "not a code" + _ = x + + // Numbers and booleans — no flags + y := 42 + _ = y + + // Mixed case — no flags (only exact uppercase is matched) + _ = "Usd" + _ = "Sg" + _ = "uSd" + + // Strings that are too long or too short — no flags + _ = "USDD" + _ = "S" + _ = "" + + // Strings that look like codes but aren't real — no flags + _ = "XYZ" + _ = "ZZ" + _ = "QQ" +} + +// Import paths must not be flagged. +// "io" is a 2-char string that matches site.IO (British Indian Ocean Territory), +// but as an import path it is clearly a package name, not a country code. +func useIO() { + var r io.Reader + _ = r +} + +// Lowercase strings are never flagged — they are common in Go source as +// identifiers, parameter names, and English words. Only uppercase forms +// are considered intentional ISO code references. +func lowercaseStrings() { + _ = "id" // identifier, not Indonesia + _ = "to" // preposition, not Tonga + _ = "no" // negation, not Norway + _ = "do" // verb, not Dominican Republic + _ = "io" // I/O, not British Indian Ocean Territory + _ = "is" // verb, not Iceland + _ = "sg" // could be anything lowercase + _ = "us" // pronoun, not United States + + _ = "all" // "everything", not Albanian Lek + _ = "usd" // lowercase, not linted + _ = "sgd" // lowercase, not linted +} diff --git a/linters/isolint/testdata/example/valid_contexts.go b/linters/isolint/testdata/example/valid_contexts.go new file mode 100644 index 0000000..89b6a34 --- /dev/null +++ b/linters/isolint/testdata/example/valid_contexts.go @@ -0,0 +1,56 @@ +package example + +// String literals passed as arguments to ORM, HTTP, and filter methods are +// column or parameter names, not ISO code values. They must not be flagged +// even when the string happens to match a valid ISO code. + +// Mock types to simulate ORM and HTTP framework method signatures. + +type mockDB struct{} + +func (mockDB) Select(cols ...string) mockDB { return mockDB{} } +func (mockDB) Pluck(col string, dest any) mockDB { return mockDB{} } +func (mockDB) Omit(cols ...string) mockDB { return mockDB{} } + +type mockCtx struct{} + +func (mockCtx) Query(key string) string { return "" } +func (mockCtx) QueryParam(key string) string { return "" } +func (mockCtx) Param(key string) string { return "" } +func (mockCtx) FormValue(key string) string { return "" } +func (mockCtx) GetQuery(key string) string { return "" } +func (mockCtx) DefaultQuery(key, def string) string { return "" } +func (mockCtx) PostForm(key string) string { return "" } + +type mockFilter struct{} + +func (mockFilter) Equals(col string, val any) mockFilter { return mockFilter{} } +func (mockFilter) NotEquals(col string, val any) mockFilter { return mockFilter{} } + +func contextSkips() { + // ORM column-name arguments — not site/currency values + db := mockDB{} + db.Select("SG") // column name, not Singapore + db.Select("FR", "DE") // column names, not France/Germany + db.Pluck("TH", nil) // column name, not Thailand + db.Omit("MY", "PH") // column names, not Malaysia/Philippines + + // HTTP parameter-name arguments — not site/currency values + c := mockCtx{} + c.Query("AE") // param name, not UAE + c.QueryParam("AU") // param name, not Australia + c.Param("GB") // param name, not United Kingdom + c.FormValue("IN") // param name, not India + c.GetQuery("HK") // param name, not Hong Kong + c.DefaultQuery("JP", "") // param name, not Japan + c.PostForm("NZ") // param name, not New Zealand + + // Filter column-name arguments — not site/currency values + f := mockFilter{} + f.Equals("CA", "val") // column name, not Canada + f.NotEquals("VN", "val") // column name, not Vietnam + + // Currency codes in call context + db.Select("USD") // column name, not US Dollar + c.Query("SGD") // param name, not Singapore Dollar +} diff --git a/linters/isolint/testdata/go.mod b/linters/isolint/testdata/go.mod new file mode 100644 index 0000000..4c5d724 --- /dev/null +++ b/linters/isolint/testdata/go.mod @@ -0,0 +1,18 @@ +module example + +go 1.24 + +require ( + github.com/wego/pkg/currency v0.1.0 + github.com/wego/pkg/iso/site v0.1.0 +) + +require ( + github.com/bojanz/currency v1.3.0 // indirect + github.com/cockroachdb/apd/v3 v3.2.1 // indirect +) + +replace ( + github.com/wego/pkg/currency => ../../../currency + github.com/wego/pkg/iso/site => ../../../iso/site +) diff --git a/linters/isolint/testdata/go.sum b/linters/isolint/testdata/go.sum new file mode 100644 index 0000000..86374d4 --- /dev/null +++ b/linters/isolint/testdata/go.sum @@ -0,0 +1,14 @@ +github.com/bojanz/currency v1.3.0 h1:HlgIxAaD7xMCk1RtjR5b7UKG3d5BBTPZORSPAWefhro= +github.com/bojanz/currency v1.3.0/go.mod h1:jNoZiJyRTqoU5DFoa+n+9lputxPUDa8Fz8BdDrW06Go= +github.com/cockroachdb/apd/v3 v3.2.1 h1:U+8j7t0axsIgvQUqthuNm82HIrYXodOV2iWLWtEaIwg= +github.com/cockroachdb/apd/v3 v3.2.1/go.mod h1:klXJcjp+FffLTHlhIG69tezTDvdP065naDsHzKhYSqc= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/lib/pq v1.10.7 h1:p7ZhMD+KsSRozJr34udlUrhboJwWAgCg34+/ZZNvZZw= +github.com/lib/pq v1.10.7/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/linters/stringlint/AGENTS.md b/linters/stringlint/AGENTS.md new file mode 100644 index 0000000..5a6301d --- /dev/null +++ b/linters/stringlint/AGENTS.md @@ -0,0 +1,48 @@ +# stringlint + +golangci-lint module plugin that flags direct string comparisons (`s == ""`, `len(s) == 0`) and recommends `wegostrings.IsEmpty(s)` / `wegostrings.IsNotEmpty(s)` from `github.com/wego/pkg/strings` instead. Suppress false positives with `//nolint:stringlint`. + +## Structure + +- `analyzer.go` — entry point; filters `*ast.BinaryExpr` nodes for comparison operators +- `patterns.go` — pattern matchers for empty-string and len-based comparisons, diagnostic reporting +- `plugin.go` — golangci-lint module plugin registration +- `cmd/stringlint/` — standalone CLI +- `testdata/` — separate Go module with test fixtures and `.golden` files + +## Commands + +```bash +go test -v ./... # run all tests +go build ./... # compile +go vet ./... # static analysis +revive ./... # lint (catches unused params, etc.) +golangci-lint run --enable-only revive # alternative: revive via golangci-lint +``` + +## Testing + +Uses `golang.org/x/tools/go/analysis/analysistest`. See [docs/testing.md](docs/testing.md) for the full testing guide. + +- Files with `// want` annotations assert expected diagnostics (positive tests) +- `valid.go` asserts zero false positives (negative tests) +- `.go.golden` files verify auto-fix output via `RunWithSuggestedFixes` +- `testdata/` is its own Go module — run `cd testdata && go mod tidy` after changing dependencies + +## Key decisions + +See [docs/decisions.md](docs/decisions.md) for rationale behind each decision. + +- **Only checks `*ast.BinaryExpr`** — targets comparisons, not all string literals +- **Handles pointer dereferences** — `*ptr == ""` suggests `IsEmptyP(ptr)` (P-suffix variants) +- **Import alias `wegostrings`** — avoids conflict with stdlib `strings`; defined as [`pkgAlias`](patterns.go) in `patterns.go` +- **Load mode is `LoadModeTypesInfo`** — needs type info to distinguish `string` from `[]byte`, slices, maps + +## Performance + +See [docs/decisions.md](docs/decisions.md) for type lookup ordering rationale and anti-patterns. + +- **`LoadModeTypesInfo`** — required; no syntax-only way to distinguish `string` from `[]byte` +- **Shared inspector** — `pass.ResultOf[inspect.Analyzer]` +- **Narrow node filter** — `[]ast.Node{(*ast.BinaryExpr)(nil)}` via `inspector.Preorder` +- **Guard order** — operator check, syntactic match, type lookup, then `printer.Fprint` only on violations diff --git a/linters/stringlint/CLAUDE.md b/linters/stringlint/CLAUDE.md new file mode 100644 index 0000000..43c994c --- /dev/null +++ b/linters/stringlint/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/linters/stringlint/docs/decisions.md b/linters/stringlint/docs/decisions.md new file mode 100644 index 0000000..829ae89 --- /dev/null +++ b/linters/stringlint/docs/decisions.md @@ -0,0 +1,48 @@ +# stringlint — Design Decisions + +## `BinaryExpr`-only targeting + +The linter filters exclusively for [`*ast.BinaryExpr`](../analyzer.go) nodes. It targets comparisons (`==`, `!=`) against empty strings and `len()` checks, not all string usage. + +Rationale: the goal is to replace direct string-emptiness checks with `wegostrings.IsEmpty` / `wegostrings.IsNotEmpty`. These patterns always appear as binary expressions. Checking other node types would add complexity without catching additional violations. + +## `LoadModeTypesInfo` + +This linter requires [`LoadModeTypesInfo`](https://pkg.go.dev/golang.org/x/tools/go/analysis#Pass) — the type-checker must run. + +### Why it's necessary + +Without type information, `len(s) == 0` where `s` is a `[]byte` would be a false positive. The linter calls [`pass.TypesInfo.TypeOf(expr)`](../patterns.go) to confirm operands are `string`-typed before reporting. There is no syntax-only way to distinguish `string` from `[]byte`, slices, or interfaces. + +### Why it matters + +golangci-lint takes the [union of all enabled linters' load modes](https://golangci-lint.run/docs/contributing/architecture/). A linter claiming `LoadModeTypesInfo` forces type-checking for the entire batch. A [real-world regression in depguard](https://github.com/golangci/golangci-lint/issues/2670) was caused by claiming type info unnecessarily. This linter's claim is justified — the alternative is rampant false positives. + +## Pointer dereference handling + +`*ptr == ""` suggests `wegostrings.IsEmptyP(ptr)` (P-suffix variants). The linter recognizes `*ast.StarExpr` as a unary dereference on the comparison operand and maps it to the pointer-accepting helpers. + +This avoids forcing callers to write `wegostrings.IsEmpty(*ptr)` which would panic on nil pointers, while `IsEmptyP` handles nil safely. + +## Import alias `wegostrings` + +The suggested import uses alias `wegostrings` for `github.com/wego/pkg/strings` to avoid conflict with the stdlib `strings` package. Defined as [`pkgAlias`](../patterns.go) in [`patterns.go`](../patterns.go). + +## Type lookup ordering + +Guards in the callback are ordered by cost (cheapest first): + +1. **Operator check** — `op` against the comparison operator set (integer comparison). Eliminates arithmetic, assignment, and bitwise binary expressions immediately. +2. **Syntactic pattern match** — checks whether the expression matches `x == ""`, `x != ""`, `len(x) == 0`, or `len(x) != 0` structurally. +3. **Type lookup** — [`pass.TypesInfo.TypeOf(expr)`](../patterns.go) (pointer-keyed map lookup) confirms the operand is `string`-typed. Only called after syntactic checks pass. +4. **`printer.Fprint`** — the [`render()`](../patterns.go) helper walks a sub-AST and pretty-prints it. Only runs on confirmed violations, not in the hot loop. + +This ordering ensures the common case (non-comparison `BinaryExpr`) exits before any type lookup or allocation. + +## Anti-patterns to avoid when extending + +- Claiming `LoadModeTypesInfo` when you only need syntax — forces type-checking for all linters in the batch +- Passing `nil` as the `Preorder` node filter — visits every AST node +- Calling `fmt.Sprintf` or `printer.Fprint` on every visited node — allocates on the hot path +- Constructing `inspector.New(pass.Files)` instead of using `pass.ResultOf[inspect.Analyzer]` — pays double construction cost and loses sharing +- Calling `pass.TypesInfo.TypeOf()` before cheaper syntactic guards — do cheap checks first diff --git a/linters/stringlint/docs/testing.md b/linters/stringlint/docs/testing.md new file mode 100644 index 0000000..821f556 --- /dev/null +++ b/linters/stringlint/docs/testing.md @@ -0,0 +1,70 @@ +# stringlint — Testing Guide + +## Framework + +Tests use [`golang.org/x/tools/go/analysis/analysistest`](https://pkg.go.dev/golang.org/x/tools/go/analysis/analysistest), the standard testing harness for Go analysis passes. + +## The bidirectional contract + +`analysistest` enforces a **bidirectional contract** between test fixtures and analyzer output: + +1. **Every `// want` annotation must match a produced diagnostic** — otherwise the test fails ("missing expected diagnostic"). +2. **Every produced diagnostic must match a `// want` annotation** — otherwise the test fails ("unexpected diagnostic"). + +Rule 2 is the crucial one. It means any `.go` file in `testdata/` that does _not_ have a `// want` comment is implicitly asserting **zero diagnostics**. If the analyzer accidentally flags something in that file, the test fails. + +## Test file roles + +| File | Role | +| ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | +| [`testdata/example/empty_string.go`](../testdata/example/empty_string.go) | Positive test — `s == ""` / `s != ""` patterns with `// want` annotations | +| [`testdata/example/len_check.go`](../testdata/example/len_check.go) | Positive test — `len(s) == 0` / `len(s) != 0` patterns with `// want` annotations | +| [`testdata/example/pointer.go`](../testdata/example/pointer.go) | Positive test — `*ptr == ""` pointer dereference patterns with `// want` annotations | +| [`testdata/example/valid.go`](../testdata/example/valid.go) | Negative test — code that resembles flaggable patterns but must NOT be flagged | +| `testdata/example/*.go.golden` | Expected auto-fix output for positive test files | + +### Why `valid.go` matters + +`valid.go` contains inputs that look like they could be flagged but should not be: + +- Comparisons against non-string types (`[]byte`, `int`, etc.) +- Struct field access patterns +- Method calls returning strings +- Non-comparison binary expressions + +If the analyzer accidentally matches any of these, the test fails because there is no `// want` to absorb the diagnostic. This makes `valid.go` an **assertion of zero false positives**. + +## Golden files + +`.go.golden` files verify auto-fix output via [`analysistest.RunWithSuggestedFixes`](https://pkg.go.dev/golang.org/x/tools/go/analysis/analysistest#RunWithSuggestedFixes). Each golden file corresponds to a positive test file and shows what the file should look like after all suggested fixes are applied. + +Golden files are only needed for files that produce fixes. `valid.go` needs no golden file because it produces no diagnostics. + +## How to add tests + +### Adding a positive test case (new detection) + +1. Create a new `.go` file in `testdata/example/` (or add to an existing positive test file). +2. Add `// want` annotations on lines that should produce diagnostics: + ```go + var empty = s == "" // want `use wegostrings\.IsEmpty` + ``` +3. Create a matching `.go.golden` file showing the expected fix: + ```go + var empty = wegostrings.IsEmpty(s) + ``` +4. Run `go test ./...` — the test should pass with the new annotations. + +### Adding a negative test case (false positive guard) + +1. Add the case to `valid.go`. +2. Do **not** add any `// want` annotation. +3. Run `go test ./...` — if the analyzer incorrectly flags your case, the test will fail with "unexpected diagnostic". + +## The `testdata/` module + +The `testdata/` directory is its own Go module with a `replace` directive pointing to `../../../strings`. After changing that dependency, run: + +```bash +cd testdata && go mod tidy +```