[linter-miner] feat(linters): add seenmapbool linter — flag map[string]bool used as a set#36313
Conversation
Flags map[string]bool variables used purely as sets (values always true),
suggesting map[string]struct{} to avoid allocating a bool per entry.
Evidence from code scanning: 18 occurrences across 18 files in pkg/ and cmd/.
Corroborated by issues #36022, #36127, #36160 flagging hand-rolled set
patterns as refactoring targets.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Go go/analysis linter (seenmapbool) intended to detect map[string]bool variables used purely as membership sets (only assigning the literal true) and recommend map[string]struct{} to reduce per-entry memory overhead.
Changes:
- Introduces the
seenmapboolanalyzer implementation. - Adds
analysistest-based unit tests + fixtures for expected diagnostics. - Registers the analyzer in the
cmd/lintersmultichecker binary.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/seenmapbool/seenmapbool.go |
Implements the analyzer that identifies set-like map[string]bool usage. |
pkg/linters/seenmapbool/seenmapbool_test.go |
Adds analysistest runner and basic analyzer-field assertions. |
pkg/linters/seenmapbool/testdata/src/seenmapbool/seenmapbool.go |
Provides fixtures for “bad” and “good” patterns using // want annotations. |
cmd/linters/main.go |
Registers seenmapbool.Analyzer in the multichecker list. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 11
| _, _ = seen["x"] | ||
| } |
| case *ast.FuncLit: | ||
| if fn.Body == nil { | ||
| return | ||
| } | ||
| body = fn.Body | ||
| } |
| return isMapStringBoolTypeExpr(e.Args[0]) | ||
| case *ast.CompositeLit: | ||
| return isMapStringBoolTypeExpr(e.Type) | ||
| } | ||
| return false |
| for _, name := range valSpec.Names { | ||
| if name.Name == "_" { | ||
| continue | ||
| } | ||
| obj := pass.TypesInfo.ObjectOf(name) | ||
| if obj == nil { | ||
| continue | ||
| } | ||
| if isMapStringBool(pass.TypesInfo.TypeOf(name)) { | ||
| candidates[obj] = name | ||
| } | ||
| } |
|
|
||
| // Second pass: check that every write to these maps only assigns true. | ||
| // If any non-true assignment is found, remove the map from candidates. | ||
| nonSetMaps := make(map[types.Object]bool) |
| for obj, declNode := range candidates { | ||
| if nonSetMaps[obj] { | ||
| continue | ||
| } |
| package seenmapbool | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
| ) | ||
|
|
||
| func TestSeenMapBool(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, Analyzer, "seenmapbool") | ||
| } |
| func TestAnalyzerFields(t *testing.T) { | ||
| if Analyzer.Name != "seenmapbool" { | ||
| t.Errorf("expected Name %q, got %q", "seenmapbool", Analyzer.Name) | ||
| } | ||
| if Analyzer.Doc == "" { | ||
| t.Error("Doc must not be empty") | ||
| } | ||
| if Analyzer.URL == "" { | ||
| t.Error("URL must not be empty") | ||
| } |
| ast.Inspect(body, func(n ast.Node) bool { | ||
| switch stmt := n.(type) { |
| ast.Inspect(body, func(n ast.Node) bool { | ||
| assign, ok := n.(*ast.AssignStmt) | ||
| if !ok { | ||
| return true | ||
| } |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (287 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does — in this case, why a homegrown per-package analysis-pass linter was chosen over an off-the-shelf aggregator. 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
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — requesting changes on test coverage gaps and a correctness edge case.
📋 Key Themes & Highlights
Key Themes
- Test fixture gaps: Three code paths (
DeclStmt/var,FuncLit/closure) are implemented but lack// wantannotations in the testdata, leaving them unguarded against regressions. - Alias false positive: Maps aliased to a second variable bypass the second-pass disqualification check; a write of
falsethrough the alias will not prevent the original candidate from being flagged. :=vsvarasymmetry: Short-variable assignments requireisMapStringBoolExprin addition to the type check;vardeclarations do not. The asymmetry is undocumented and creates a coverage gap for maps returned from functions.- Implementation irony:
nonSetMapsis itself amap[types.Object]boolused purely as a set — swap it tomap[types.Object]struct{}for consistency.
Positive Highlights
- ✅ Clean two-pass design: collect then filter — easy to reason about
- ✅ Test fixture correctly uses
analysistestwith// wantannotations - ✅ Test-file exclusion via
filecheck.IsTestFileis a good safeguard - ✅ Registering in the multichecker is the right pattern; the change is minimal
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.5M
| } | ||
| if isMapStringBool(pass.TypesInfo.TypeOf(name)) { | ||
| candidates[obj] = name | ||
| } |
There was a problem hiding this comment.
[/tdd] nonSetMaps itself uses map[types.Object]bool — a minor irony in a linter designed to eliminate that pattern. Using map[types.Object]struct{} here would make the implementation consistent with its own advice and serve as a self-documenting example.
💡 Suggested change
// Before
nonSetMaps := make(map[types.Object]bool)
// ...
nonSetMaps[obj] = true
// ...
if nonSetMaps[obj] {
// After
nonSetMaps := make(map[types.Object]struct{})
// ...
nonSetMaps[obj] = struct{}{}
// ...
if _, ok := nonSetMaps[obj]; ok {| return true | ||
| }) | ||
|
|
||
| if len(candidates) == 0 { |
There was a problem hiding this comment.
[/diagnose] Alias aliasing is a false-positive vector: if a second variable references the same map and writes false through that alias, the linter will not disqualify the original candidate and will incorrectly flag it.
💡 Example scenario
seen := make(map[string]bool) // flagged — but should not be
alias := seen
alias["disabled"] = false // write of false through alias; not seen by second pass
seen["enabled"] = trueThe second pass only checks IndexExpr.X against the known candidate objects. An alias has a different types.Object and is not a candidate, so alias["disabled"] = false is silently ignored.
In practice this pattern is uncommon in the seen-set idiom, but documenting the limitation in a comment near the second-pass loop would help future maintainers, and a test fixture covering it would prevent silent regressions.
| } | ||
| for i, lhs := range stmt.Lhs { | ||
| if i >= len(stmt.Rhs) { | ||
| break |
There was a problem hiding this comment.
[/tdd] The DeclStmt / var seen map[string]bool path is implemented but has no corresponding test fixture. A // want annotation in the testdata file is the spec for this code path — without it, a future refactor could silently break it.
💡 Suggested test fixture addition
Add to testdata/src/seenmapbool/seenmapbool.go:
func BadVarDecl() {
var seen map[string]bool // want `map\[string\]bool "seen" used as a set`
seen = make(map[string]bool)
seen["a"] = true
_ = seen
}Also consider a negative-case: var flags map[string]bool with no writes should either be flagged (type annotation is the intent) or explicitly documented as out-of-scope.
| (*ast.FuncLit)(nil), | ||
| } | ||
|
|
||
| insp.Preorder(nodeFilter, func(n ast.Node) { |
There was a problem hiding this comment.
[/tdd] FuncLit bodies are inspected but the test fixture only exercises FuncDecl. Closures that capture a map[string]bool and use it as a set are a real pattern (e.g. inside t.Run callbacks) — without a // want fixture, this code path is not regression-tested.
💡 Suggested test fixture addition
func BadSetInClosure() {
var results []string
process := func(items []string) {
seen := make(map[string]bool) // want `map\[string\]bool "seen" used as a set`
for _, item := range items {
if !seen[item] {
seen[item] = true
results = append(results, item)
}
}
}
process([]string{"a", "b", "a"})
_ = results
}| // First pass: collect declarations of map[string]bool locals. | ||
| ast.Inspect(body, func(n ast.Node) bool { | ||
| switch stmt := n.(type) { | ||
| case *ast.AssignStmt: |
There was a problem hiding this comment.
[/tdd] For := assignments, the linter requires both a type match (isMapStringBool) AND an expression match (isMapStringBoolExpr), so seen := getSeenMap() is silently skipped even if getSeenMap() returns map[string]bool. The var path (line 78) only requires the type match, creating an inconsistency. Consider documenting this intentional asymmetry — or extending the := path to also catch return-value assignments.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness issues must be fixed before merge.
Blocking issues
Blocking
FuncLittest-file exclusion missing (line 48) — closures in test files bypass thefilecheck.IsTestFileguard and will produce spurious diagnostics.- False positive on
var-declared maps with no writes (line 108) —var seen map[string]boolwith only reads (or no use) is reported as a set even though it has no set-style writes.
Non-blocking
nonSetMapsself-violation (line 123) — the internal tracking map is itself amap[types.Object]boolused as a set.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.7M
| pos := pass.Fset.PositionFor(fn.Pos(), false) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Test-file exclusion is skipped for FuncLit nodes, so closures inside test files will still be analyzed and can emit spurious diagnostics.
💡 Suggested fix
Resolve the file position for FuncLit the same way it is done for FuncDecl:
case *ast.FuncLit:
if fn.Body == nil {
return
}
pos := pass.Fset.PositionFor(fn.Pos(), false)
if filecheck.IsTestFile(pos.Filename) {
return
}
body = fn.BodyWithout this, any map[string]bool set-pattern in a test-file closure will generate a false diagnostic even though test files are supposed to be excluded.
| continue | ||
| } | ||
| obj := pass.TypesInfo.ObjectOf(name) | ||
| if obj == nil { |
There was a problem hiding this comment.
False positive: var seen map[string]bool with no index writes is reported as a set, but it is not one. A nil map that is only read (or never used) will be flagged because the second pass never finds a disqualifying write.
💡 Example and fix
var seen map[string]bool
if seen["x"] { // only a read — no map[k]=true writes
...
}
// linter fires here incorrectlyFor var-declared maps, require at least one observed index-write before adding to candidates, or filter candidates after the second pass to only include those that have at least one confirmed map[k] = true assignment:
// Only flag if the map actually has set-style writes
seenWrites := make(map[types.Object]bool) // track maps with at least one true-write
// ... in second pass, record seenWrites[obj] = true when isBoolTrue(rhs)
// ... in report loop: if !seenWrites[obj] { continue }This also covers the analogous case of :=-declared maps that are never written to after declaration.
| if len(candidates) == 0 { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
nonSetMaps is itself a map[types.Object]bool used purely as a set — the exact anti-pattern this linter exists to flag.
💡 Suggested fix
nonSetMaps := make(map[types.Object]struct{})
// ...
nonSetMaps[obj] = struct{}{}
// ...
if _, disqualified := nonSetMaps[obj]; disqualified {
continue
}Not a correctness issue, but the self-contradiction undermines confidence in the linter and would be caught by the linter itself if it analyzed its own package.
|
@copilot apply copilot-review skill |
Applied the copilot-review skill for this pass and evaluated the feedback scope. No new code change was requested in this mention. |
Add
seenmapboolstatic analysis linterSummary
Introduces a new Go static-analysis linter,
seenmapbool, that detectsmap[string]boolseen-maps whose values are always assignedtrueand flags them for replacement with the zero-allocationmap[string]struct{}pattern. The linter is registered in the central linter list, covered by fixture-drivenanalysistesttests, and documented with an Architecture Decision Record.What changed and why
pkg/linters/seenmapbool/seenmapbool.go(added, high impact)Core linter implementation.
inspectpass and type information:map[string]boolvariable declarations.true; any map that ever assignsfalseis excluded from diagnostics (it is a genuine bool map, not a seen-map).filecheck.IsTestFileto avoid false positives in test helpers.pkg/linters/seenmapbool/seenmapbool_test.go(added, medium impact)analysistest-based test suite.TestSeenMapBool– fixture-driven test exercising all four cases (see fixtures below).TestAnalyzerFields– metadata validation (name, documentation, flags).pkg/linters/seenmapbool/testdata/src/seenmapbool/seenmapbool.go(added, low impact)Test fixtures covering the four canonical cases:
BadSetBool(makeform)BadSetBoolLiteral(composite literal form)GoodSetStruct(map[string]struct{})GoodBoolMapWithFalse(assignsfalse)cmd/linters/main.go(modified, medium impact)Registered
seenmapboolin the central analyzer list so it runs with all other linters in the pipeline.docs/adr/36313-add-seenmapbool-linter.md(added, low impact)Draft ADR documenting the motivation (zero-allocation seen-maps), alternatives considered, and the decision to implement this as a static-analysis pass.
Impact assessment
map[string]boolseen-mapsNotes for reviewers
true-only writes) from a genuine boolean map without first collecting all write sites.filecheck.IsTestFilecovers all test-file conventions used in this repo (e.g._test.gosuffix and anytestdata/conventions).GoodBoolMapWithFalsefixture is the key correctness guard — ensure the exclusion logic is preserved if the linter is extended.