Add packagelevelmutableslicemap custom linter #52920
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # ADR-52920: Add packagelevelmutableslicemap Static Analysis Linter | ||
|
|
||
| **Date**: 2026-08-15 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| Package-level `var` declarations of slices and maps in Go are shared across every goroutine and every call into the package for the lifetime of the process. When a function body mutates such a variable (via `append()` re-assignment, index assignment, or `delete()`), it risks data races under concurrent access and can leak state across otherwise-unrelated calls (e.g. between test runs or concurrent request handlers). | ||
|
|
||
| Issue #52683 identified a concrete instance of this pattern in the codebase — a module-level mutable array (`_allResults`) reset and appended to across `main()` invocations, which is a latent cross-run data-corruption risk if the calling architecture ever changes to parallel dispatch. A broader codebase scan found additional package-level slice/map `var` declarations in `pkg/` that exhibit the same structural risk. The project's existing `go/analysis`-based linter suite in `pkg/linters/` already enforces similar structural constraints (e.g. `manualmutexunlock`, `goroutinemissingrecover`) and provides shared infrastructure (`analyzerutil`, `filecheck`, `nolint`) that makes adding a new checker straightforward. | ||
|
|
||
| No built-in Go tool or existing third-party linter in the project's toolchain specifically targets this class of mutation at static analysis time. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will add a new custom `go/analysis` linter, `packagelevelmutableslicemap`, under `pkg/linters/packagelevelmutableslicemap/`. The analyzer scans package-scope `var` declarations with slice or map underlying types — including named wrapper types such as `type registry map[string]int` — and flags any mutation of those variables from inside a function body via `append()` re-assignment (including parallel assignments and appends whose source is a different slice), index assignment (`m[k] = v`, including nested `m[a][b] = v`), or `delete(m, k)`. Object identity via `types.Object` is used to correctly exempt local variables that shadow a package-level name. Mutations inside a top-level `init()` function are exempt, since `init` runs exactly once before any other code and is the idiomatic place to populate package-level state. A `//nolint:packagelevelmutableslicemap` directive on the mutating line suppresses the diagnostic, consistent with sibling linters. The analyzer is registered in `pkg/linters/registry.go` alongside the existing 64 analyzers. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Rely on Go's built-in race detector (`-race`) | ||
|
|
||
| The race detector (`go test -race` / `go run -race`) detects data races dynamically at runtime. It is thorough but requires actual concurrent execution of the conflicting code paths during the test run. Mutations of package-level slices/maps that happen sequentially — or that are only exercised under production load — will not trigger it. It also does not catch the cross-call state-leak pattern (where sequential mutations corrupt state for a later call), which is a distinct risk from a data race. Static analysis at lint time catches the structural smell unconditionally, regardless of how tests are structured. | ||
|
|
||
| #### Alternative 2: Code review convention and documentation | ||
|
|
||
| The team could document a convention prohibiting mutable package-level slice/map state and rely on human reviewers to enforce it. This has zero tooling cost but scales poorly: conventions drift, reviewers miss cases under time pressure, and new contributors are unaware of the rule until they encounter a review comment. Given that the project already invests in automated linting for analogous structural constraints, automation is the consistent choice. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Package-level mutable slice/map bugs are caught at lint time — before they manifest as intermittent data races or cross-test contamination in production or CI. | ||
| - The implementation follows established patterns in `pkg/linters/`, reusing `analyzerutil`, `filecheck`, and `nolint` infrastructure; the incremental cost per new analyzer is low, and the approach is familiar to contributors. | ||
| - The `//nolint:packagelevelmutableslicemap` escape hatch allows intentionally synchronized global state (e.g., a mutex-protected registry) to opt out without refactoring. | ||
|
|
||
| #### Negative | ||
| - The analyzer will produce false positives for package-level state that is intentionally and correctly synchronized (e.g., a `sync.Mutex`-protected global cache). Each such site requires a `//nolint` directive or a refactor; if there are many such sites in the codebase, this creates short-term maintenance work. | ||
| - The linter cannot detect all mutable-shared-state hazards — it only covers `var` declarations with `slice` or `map` underlying types mutated directly. Aliased mutations (e.g., passing the global slice to a helper that appends internally) are not flagged. | ||
|
|
||
| #### Neutral | ||
| - The analyzer count in `pkg/linters/doc.go` and `README.md` increases from 64 to 65; the `spec_test.go` count assertion is updated accordingly. This is a minor bookkeeping change with no behavioral impact. | ||
| - Existing code in the repository that triggers the new linter will begin failing lint checks once the analyzer is active; a sweep of existing violations may be needed before enabling the linter in CI. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,244 @@ | ||
| // Package packagelevelmutableslicemap implements a Go analysis linter that | ||
| // flags package-level (file/package-scope) var declarations of slices or maps | ||
| // that are mutated from inside a function body via append re-assignment, | ||
| // index assignment, or delete(). | ||
| // | ||
| // Package-level mutable slices/maps are shared across every goroutine and | ||
| // every call into the package for the lifetime of the process. Mutating one | ||
| // from inside a function — rather than storing the state on a struct or | ||
| // returning fresh values — risks data races under concurrent access and can | ||
| // leak state between unrelated calls. | ||
| // | ||
| // Mutations inside a top-level init() function are not reported: init runs | ||
| // exactly once before any other code, so it is the idiomatic place to | ||
| // populate package-level state. | ||
| package packagelevelmutableslicemap | ||
|
|
||
| import ( | ||
| "go/ast" | ||
| "go/token" | ||
| "go/types" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/internal/analyzerutil" | ||
| "github.com/github/gh-aw/pkg/linters/internal/astutil" | ||
| "github.com/github/gh-aw/pkg/linters/internal/filecheck" | ||
| "github.com/github/gh-aw/pkg/linters/internal/nolint" | ||
| ) | ||
|
|
||
| // Analyzer is the package-level-mutable-slice-map analysis pass. | ||
| var Analyzer = analyzerutil.New("packagelevelmutableslicemap", "reports mutation of package-level slice/map variables from inside function bodies, which risks data races and cross-call state leaks", run) | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp, err := astutil.Inspector(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| noLintIndex, err := nolint.Index(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| generatedFiles, err := filecheck.Index(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| targets := collectPackageLevelSliceMapVars(pass) | ||
| if len(targets) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| for cur := range insp.Root().Preorder((*ast.AssignStmt)(nil), (*ast.ExprStmt)(nil)) { | ||
| if isInInitFunction(cur) { | ||
| continue | ||
| } | ||
| analyzeNode(pass, cur.Node(), targets, generatedFiles, noLintIndex) | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| // isInInitFunction reports whether cur is inside a top-level init() function. | ||
| // Only top-level (no receiver) init functions are recognized; methods named | ||
| // init are ordinary methods and are not exempt. | ||
| func isInInitFunction(cur inspector.Cursor) bool { | ||
| for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { | ||
| decl, ok := encl.Node().(*ast.FuncDecl) | ||
| if !ok { | ||
| // Innermost enclosing function is a literal (e.g. a goroutine | ||
| // started from init), which is not exempt. | ||
| return false | ||
| } | ||
| return decl.Recv == nil && decl.Name != nil && decl.Name.Name == "init" | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // collectPackageLevelSliceMapVars scans the top-level declarations of every | ||
| // file in the package and returns the set of package-scope var objects whose | ||
| // underlying type is a slice or a map (including named wrapper types such as | ||
| // `type registry map[string]int`), keyed by their declared name. | ||
| func collectPackageLevelSliceMapVars(pass *analysis.Pass) map[types.Object]string { | ||
| targets := make(map[types.Object]string) | ||
| for _, file := range pass.Files { | ||
| for _, decl := range file.Decls { | ||
| genDecl, ok := decl.(*ast.GenDecl) | ||
| if !ok || genDecl.Tok != token.VAR { | ||
| continue | ||
| } | ||
| for _, spec := range genDecl.Specs { | ||
| valueSpec, ok := spec.(*ast.ValueSpec) | ||
| if !ok { | ||
| continue | ||
| } | ||
| for _, name := range valueSpec.Names { | ||
| if name.Name == "_" { | ||
| continue | ||
| } | ||
| obj := pass.TypesInfo.Defs[name] | ||
| if obj == nil { | ||
| continue | ||
| } | ||
| t := obj.Type() | ||
| if t == nil { | ||
| continue | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 SuggestionAdd a comment to the function clarifying that named type aliases with slice/map underlying types are not in scope, so the intent is explicit: // Note: var declarations using a named type alias (e.g. type T = []int; var x T)
// are not currently tracked; only direct slice/map type declarations are flagged.@copilot please address this. |
||
| switch t.Underlying().(type) { | ||
| case *types.Slice, *types.Map: | ||
| targets[obj] = name.Name | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return targets | ||
| } | ||
|
|
||
| func analyzeNode(pass *analysis.Pass, n ast.Node, targets map[types.Object]string, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { | ||
| switch stmt := n.(type) { | ||
| case *ast.AssignStmt: | ||
| analyzeAssignStmt(pass, stmt, targets, generatedFiles, noLintIndex) | ||
| case *ast.ExprStmt: | ||
| analyzeExprStmt(pass, stmt, targets, generatedFiles, noLintIndex) | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The 💡 Suggested test caseAdd to testdata: func multiLHS(k string) {
var x int
x, globalMap[k] = 1, 2 // want `...mutated via index assignment...`
_ = x
}@copilot please address this. |
||
|
|
||
| func analyzeAssignStmt(pass *analysis.Pass, stmt *ast.AssignStmt, targets map[types.Object]string, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { | ||
| if name, ok := matchAppendReassign(pass, stmt, targets); ok { | ||
| report(pass, stmt.Pos(), name, "append() re-assignment", generatedFiles, noLintIndex) | ||
| return | ||
| } | ||
| if name, ok := matchIndexAssign(pass, stmt, targets); ok { | ||
| report(pass, stmt.Pos(), name, "index assignment", generatedFiles, noLintIndex) | ||
| } | ||
| } | ||
|
|
||
| func analyzeExprStmt(pass *analysis.Pass, stmt *ast.ExprStmt, targets map[types.Object]string, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { | ||
| call, ok := stmt.X.(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } | ||
| if !isBuiltinCall(pass, call, "delete") || len(call.Args) != 2 { | ||
| return | ||
| } | ||
| name, ok := targetBaseName(pass, call.Args[0], targets) | ||
| if !ok { | ||
| return | ||
| } | ||
| report(pass, stmt.Pos(), name, "delete()", generatedFiles, noLintIndex) | ||
| } | ||
|
|
||
| // matchAppendReassign reports whether stmt re-assigns a tracked package-level | ||
| // target from an append() call, returning its declared name. Every LHS/RHS | ||
| // pair is inspected so parallel assignments such as | ||
| // `globalSlice, err = append(globalSlice, v), nil` are matched too, and the | ||
| // append arguments are not required to reference the target itself, so | ||
| // `globalSlice = append(otherSlice, v)` is matched as well. | ||
| func matchAppendReassign(pass *analysis.Pass, stmt *ast.AssignStmt, targets map[types.Object]string) (string, bool) { | ||
| for i, lhs := range stmt.Lhs { | ||
| lhsIdent, ok := lhs.(*ast.Ident) | ||
| if !ok { | ||
| continue | ||
| } | ||
| name, tracked := targets[pass.TypesInfo.Uses[lhsIdent]] | ||
| if !tracked { | ||
| continue | ||
| } | ||
| rhs, ok := astutil.RhsExprForIndex(stmt.Rhs, i) | ||
| if !ok { | ||
| continue | ||
| } | ||
| call, ok := rhs.(*ast.CallExpr) | ||
| if !ok || len(call.Args) == 0 { | ||
| continue | ||
| } | ||
| if isBuiltinCall(pass, call, "append") { | ||
| return name, true | ||
| } | ||
| } | ||
| return "", false | ||
| } | ||
|
|
||
| // matchIndexAssign reports whether stmt assigns into m[k] for a tracked | ||
| // package-level map/slice target m. | ||
| func matchIndexAssign(pass *analysis.Pass, stmt *ast.AssignStmt, targets map[types.Object]string) (string, bool) { | ||
| for _, lhs := range stmt.Lhs { | ||
| idxExpr, ok := lhs.(*ast.IndexExpr) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if name, ok := targetBaseName(pass, idxExpr, targets); ok { | ||
| return name, true | ||
| } | ||
| } | ||
| return "", false | ||
| } | ||
|
|
||
| // isBuiltinCall reports whether call invokes the named Go builtin. | ||
| func isBuiltinCall(pass *analysis.Pass, call *ast.CallExpr, name string) bool { | ||
| ident, ok := call.Fun.(*ast.Ident) | ||
| if !ok || ident.Name != name { | ||
| return false | ||
| } | ||
| builtin, ok := pass.TypesInfo.Uses[ident].(*types.Builtin) | ||
| return ok && builtin.Name() == name | ||
| } | ||
|
|
||
| // targetBaseName reports whether expr is rooted at an identifier referring to | ||
| // a tracked package-level target, returning its declared name. Nested index | ||
| // expressions such as nested[a][b] resolve to their base identifier so | ||
| // mutations of nested collections are reported too. | ||
| func targetBaseName(pass *analysis.Pass, expr ast.Expr, targets map[types.Object]string) (string, bool) { | ||
| for { | ||
| switch e := expr.(type) { | ||
| case *ast.Ident: | ||
| obj := pass.TypesInfo.Uses[e] | ||
| if obj == nil { | ||
| return "", false | ||
| } | ||
| name, ok := targets[obj] | ||
| return name, ok | ||
| case *ast.IndexExpr: | ||
| expr = e.X | ||
| case *ast.ParenExpr: | ||
| expr = e.X | ||
| default: | ||
| return "", false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func report(pass *analysis.Pass, pos token.Pos, varName, kind string, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { | ||
| position := pass.Fset.PositionFor(pos, false) | ||
| if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { | ||
| return | ||
| } | ||
| if nolint.HasDirectiveForLinter(position, noLintIndex, "packagelevelmutableslicemap") { | ||
| return | ||
| } | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: pos, | ||
| Message: "package-level slice/map variable " + varName + " is mutated via " + kind + "; mutating shared package state risks data races and can leak state across calls", | ||
| }) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| //go:build !integration | ||
|
|
||
| package packagelevelmutableslicemap_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/packagelevelmutableslicemap" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, packagelevelmutableslicemap.Analyzer, "packagelevelmutableslicemap") | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This analyzer skips mutations on package-level named slice/map types, so it will miss exactly the same shared-state bug once that map or slice is wrapped in a domain type.
💡 Why this blocks
collectPackageLevelSliceMapVarsonly keeps vars whoseUnderlying()type is*types.Sliceor*types.Map. That works forvar m map[string]int, but not for common wrappers liketype cache map[string]int; var global cache. In those casesUnderlying()is*types.Named, so the variable is dropped before we ever inspectglobal[k] = v,delete(global, k), orglobal = append(global, x).That leaves a large false-negative hole in a linter whose whole point is catching shared mutable package state. Please unwrap named types before the kind check and add analysistest coverage for both named map and named slice aliases/wrappers.