Add packagelevelmutableslicemap custom linter - #52920
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed the packagelevelmutableslicemap linter PR for over-engineering; its structure mirrors sibling analyzers (mapdeletecheck, mapclearloop) in size and pattern, uses shared internal helpers (analyzerutil, filecheck, nolint) consistently, and has no speculative abstractions, unused flexibility, or hand-rolled stdlib logic.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Pull request overview
Adds a custom Go analyzer for detecting mutations of package-level slices and maps.
Changes:
- Implements and registers the analyzer.
- Adds analysistest coverage and fixtures.
- Updates analyzer documentation and counts.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/packagelevelmutableslicemap/packagelevelmutableslicemap.go |
Implements mutation detection. |
pkg/linters/packagelevelmutableslicemap/packagelevelmutableslicemap_test.go |
Runs analyzer tests. |
pkg/linters/packagelevelmutableslicemap/testdata/src/packagelevelmutableslicemap/packagelevelmutableslicemap.go |
Provides test cases. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/spec_test.go |
Adds documentation consistency coverage. |
pkg/linters/doc.go |
Documents the analyzer. |
pkg/linters/README.md |
Updates public analyzer listings. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| if len(stmt.Lhs) != 1 || len(stmt.Rhs) != 1 { | ||
| return "", false |
| func localSliceIsFine() { | ||
| s := []int{1, 2} | ||
| s = append(s, 3) | ||
| _ = s | ||
| } | ||
|
|
||
| func localMapIsFine() { | ||
| m := map[string]int{} | ||
| m["a"] = 1 | ||
| delete(m, "a") | ||
| _ = m | ||
| } |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
This analyzer misses common package-level mutations, so it will silently let unsafe shared state through while looking authoritative.
Blocking themes
- Method-call mutations on named map/slice wrapper types are not detected.
- The tests only cover built-in
[]T/map[K]Vsyntax, so this blind spot would ship unnoticed.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.28 AIC · ⌖ 6.82 AIC · ⊞ 6.9K
Comment /review to run again
| obj := pass.TypesInfo.Defs[name] | ||
| if obj == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
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
collectPackageLevelSliceMapVars only keeps vars whose Underlying() type is *types.Slice or *types.Map. That works for var m map[string]int, but not for common wrappers like type cache map[string]int; var global cache. In those cases Underlying() is *types.Named, so the variable is dropped before we ever inspect global[k] = v, delete(global, k), or global = 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.
t := obj.Type()
for {
named, ok := t.(*types.Named)
if !ok {
break
}
t = named.Underlying()
}
switch t.(type) {
case *types.Slice, *types.Map:
targets[obj] = name.Name
}
Generated by the Design Decision Gate: PR #52920 adds >100 lines of business logic without a linked ADR.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (282 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review: packagelevelmutableslicemap linter
The overall implementation is clean and follows existing linter conventions well. Two correctness issues need addressing before merge.
Blocking issues
-
init()functions produce false positives — mutating package-level state ininit()is idiomatic and intentional in Go. The analyzer has no guard for this case and will fire on common, correct initialization patterns. (See inline comment on line 47.) -
matchAppendReassignhas a logic gap — the check that the firstappendargument equals the LHS object is overly strict and misses the caseglobalSlice = append(otherSlice, ...), which still mutates the package-level variable. (See inline comment on line 148.)
Suggestions (non-blocking)
- The testdata only covers the three mutation kinds for a single variable at a time. Adding a test case for
init()(expect no diagnostic) and one forglobalSlice = append(localSlice, x)(expect diagnostic) would lock down both edge cases and prevent regressions.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.1 AIC · ⌖ 7.32 AIC · ⊞ 5.6K
| (*ast.AssignStmt)(nil), | ||
| (*ast.ExprStmt)(nil), | ||
| } | ||
| return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { |
There was a problem hiding this comment.
False positive: init() functions are not exempt
The analyzer walks all function bodies including init(), but mutating package-level slices/maps inside init() is idiomatic Go — it is the standard way to initialize shared state before any other code runs. The linter will fire on correct, conventional code like:
var registry []handler
func init() {
registry = append(registry, newHandler()) // false positive
}Fix: in the Preorder walk, check the enclosing *ast.FuncDecl; if it is named init with no parameters and no return values, skip the mutation report.
@copilot please address this.
| if !tracked { | ||
| return "", false | ||
| } | ||
| call, ok := stmt.Rhs[0].(*ast.CallExpr) |
There was a problem hiding this comment.
matchAppendReassign only catches the self-append pattern, missing other mutations
Currently the function requires the first argument to append to be the same object as the LHS:
if pass.TypesInfo.Uses[firstArgIdent] != lhsObj {
return "", false
}But this misses the equally mutating case where a different slice is appended to a package-level variable:
var globalSlice []int
func populateGlobal(other []int) {
globalSlice = append(other, 1) // not caught — LHS is global, first arg is local
}The core invariant should be: the LHS is a tracked package-level variable and the RHS is an append() call — the first argument is irrelevant to whether globalSlice is being mutated.
Fix: remove the firstArgIdent check entirely; report whenever the LHS is a tracked target and the RHS is a call to the built-in append.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage gaps and one correctness issue.
📋 Key Themes & Highlights
Key Themes
- Silent false negative in
matchAppendReassign: the first-argument identity guard meansglobalSlice = append(otherSlice, v)is never flagged, even though the mutation still happens. Document or fix. - Missing test coverage: no testdata case for slice index mutation (
globalSlice[i] = v) and no coverage for multi-LHS assignment (_, globalMap[k] = fn()). - Named type scope boundary undocumented:
var x MySlicewheretype MySlice = []intis silently skipped; a comment stating the known limit would prevent future confusion.
Positive Highlights
- ✅ Correct use of
types.Objectidentity for shadowing — local vars are properly excluded. - ✅ Consistent with sibling analyzers:
nolintsuppression,filecheck,analyzerutil.Newwiring. - ✅ Thorough first-pass testdata: append, index, delete, nolint, read-only, and local variable cases are all covered.
- ✅ Doc/registry/spec sync is clean.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 44.2 AIC · ⌖ 8.73 AIC · ⊞ 7.7K
Comment /matt to run again
| return "", false | ||
| } | ||
| lhsIdent, ok := stmt.Lhs[0].(*ast.Ident) | ||
| if !ok { |
There was a problem hiding this comment.
[/tdd] False negative: s = append(differentSlice, v) — any reassignment of a tracked package-level var where the first argument is not the same object will be silently missed. The mutation still occurs, so the linter's diagnostic goal is only partially met.
💡 Suggested fix or documentation
Either broaden matchAppendReassign to flag any trackedVar = append(...) regardless of the first argument:
// tracked var on LHS and append() on RHS — flag regardless of first arg
return name, trueOr document the constraint explicitly in the package comment so future maintainers are not surprised by the gap.
@copilot please address this.
| } | ||
| builtin, ok := pass.TypesInfo.Uses[fnIdent].(*types.Builtin) | ||
| if !ok || builtin.Name() != "append" { | ||
| return "", false |
There was a problem hiding this comment.
[/tdd] Missing coverage: matchIndexAssign handles slice index assignment (s[i] = v) as well as map assignment, but the testdata fixture has no case for a slice index mutation (globalSlice[0] = 99). Without a golden test, a future regression removing the slice branch will go undetected.
💡 Suggested test case
Add to testdata/src/packagelevelmutableslicemap/packagelevelmutableslicemap.go:
func setSliceIndex() {
globalSlice[0] = 99 // want `package-level slice/map variable globalSlice is mutated via index assignment; ...`
}@copilot please address this.
| t := obj.Type() | ||
| if t == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
[/codebase-design] collectPackageLevelSliceMapVars only scans pass.Files — these are the files in the package under analysis, not all transitive dependencies. That is correct. However, the function silently skips const/type blocks (expected), but it also misses typed aliases (type MySlice = []int) where the underlying type is a slice. This is likely acceptable scope, but documenting the known boundary prevents future confusion.
💡 Suggestion
Add 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.
| case *ast.ExprStmt: | ||
| analyzeExprStmt(pass, stmt, targets, generatedFiles, noLintIndex) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] The analyzeAssignStmt function only reports one diagnostic per AssignStmt — if both matchAppendReassign and matchIndexAssign could apply to the same statement (unlikely but possible with multi-LHS), the second mutation would be silently skipped due to the early return. More practically: a multi-assignment like a, globalMap[k] = fn() where one LHS is a tracked index expression will be flagged correctly, but if the first LHS also matched an append pattern, the early return would suppress it. There is no test covering multi-LHS assignment.
💡 Suggested test case
Add to testdata:
func multiLHS(k string) {
var x int
x, globalMap[k] = 1, 2 // want `...mutated via index assignment...`
_ = x
}@copilot please address this.
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it, address the remaining reviewer feedback, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
…mutations Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All four outstanding items are addressed:
Fixtures also now shadow the package-level names locally so the |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it and run the The latest author update says the prior linter feedback and ADR-link follow-ups were addressed. Outstanding maintainer-facing follow-up:
Failed checks from the compact candidate set:
Run context: https://github.com/github/gh-aw/actions/runs/31899148351
|
|
🎉 This pull request is included in a new release. Release: |
Package-level mutable slices/maps are shared across every goroutine and every call into the package, risking data races and cross-call state leaks when mutated from inside a function body instead of via struct state or fresh return values.
New analyzer
pkg/linters/packagelevelmutableslicemap, ago/analysislinter that tracks package-scopevardeclarations with slice/map underlying types and flags mutations via:append()re-assignment:s = append(s, ...)m[k] = vdelete(m, k)types.Objectidentity, so local variables that shadow a package-level name are correctly left unflagged.//nolint:packagelevelmutableslicemapsuppression, consistent with sibling linters.Registration & docs
pkg/linters/registry.go.pkg/linters/doc.go,pkg/linters/README.md, andpkg/linters/spec_test.goto keep the documented analyzer count (now 65) and subpackage lists in sync.Tests
packagelevelmutableslicemap_test.gowith ananalysistestfixture covering append-mutation, index-assignment,delete(), a read-only slice (no diagnostic), a//nolint-suppressed case, and locally shadowed variables.