[linter-miner] feat(linters): add mapclearloop linter — replace range-delete loops with clear(m)#46060
Conversation
Detects range-over-map loops whose sole body statement is delete(m, k) which can be replaced by the built-in clear(m) introduced in Go 1.21. The linter: - Verifies the range expression is a map type via type-info - Requires the value variable to be absent or blank (_) - Requires exactly one body statement: the builtin delete(m, k) - Checks delete targets the same map and key as the range - Emits a suggested fix rewriting the loop to clear(m) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds the mapclearloop Go analyzer to recommend clear(m) for simple range-delete loops.
Changes:
- Implements and registers the analyzer.
- Adds analysistest fixtures and suggested-fix golden output.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers the analyzer. |
pkg/linters/mapclearloop/mapclearloop.go |
Implements detection and fixes. |
pkg/linters/mapclearloop/mapclearloop_test.go |
Runs analyzer tests. |
pkg/linters/mapclearloop/testdata/src/mapclearloop/mapclearloop.go |
Provides test cases. |
pkg/linters/mapclearloop/testdata/src/mapclearloop/mapclearloop.go.golden |
Defines expected fixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Medium
| TextEdits: []analysis.TextEdit{{ | ||
| Pos: rangeStmt.Pos(), | ||
| End: rangeStmt.End(), | ||
| NewText: []byte("clear(" + mText + ")"), |
| keyObj := pass.TypesInfo.Defs[keyIdent] | ||
| if keyObj == nil { | ||
| return | ||
| } |
| Pos: rangeStmt.Pos(), | ||
| End: rangeStmt.End(), | ||
| NewText: []byte("clear(" + mText + ")"), |
| // Only ranging over value – not flagged. | ||
| for _, v := range m { |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
Notes: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review: mapclearloop linter
The implementation is well-structured and follows repo conventions. One non-blocking gap:
Missing positive test for the blank-value range form
for k, _ := range m { delete(m, k) } is handled by the code (lines 77-81 accept a blank value), but there is no test case exercising this path. Without it, a future regression could silently remove the blank-value support.
Everything else looks correct: map-type check via types.Underlying(), builtin-delete verification via TypesInfo.Uses, same-object identity via TypesInfo.Uses, sameObject covers both simple idents and selector expressions, SuggestedFix produces a valid replacement, and all negative cases are covered.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 37.5 AIC · ⌖ 4.38 AIC · ⊞ 5K
|
|
||
| // The key variable must be present (not blank or absent). | ||
| keyIdent, ok := rangeStmt.Key.(*ast.Ident) | ||
| if !ok || keyIdent.Name == "_" { |
There was a problem hiding this comment.
Missing test case: for k, _ := range m { delete(m, k) }
The value-blank branch (lines 77-81) that permits for k, _ := range m is never exercised by the test fixture — only the no-value form for k := range m is tested. Add a positive test case with an explicit blank value to testdata/src/mapclearloop/mapclearloop.go and its .golden counterpart.
@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.
📋 Key Themes & Highlights
Key Themes
- SuggestedFix drops interior comments (line 132): the
TextEditspan covers the whole loop, so any comment inside the body is silently deleted on autofix. Add a fixture case to document (or fix) this. - SelectorExpr positive test missing (line 6):
sameObjecthas aSelectorExprbranch that is never exercised by a// wantcase — add abadSelectorfixture. - Blank-key / keyless range not in fixture (line 20): add an explicit negative case for
for range m {}to prevent future regressions. sameObjectscope undocumented (line 145): the conservativedefault: falsefallback is safe but should be noted so future maintainers don't widen it accidentally.
Positive Highlights
- ✅ Type-aware analysis — correctly uses
*types.Builtinto guard against shadoweddelete - ✅ Follows the established linter package structure (filecheck, nolint, astutil)
- ✅ Clean, high signal-to-noise diagnostic message with accurate suggested fix
- ✅ Negative cases for multi-statement body, different-map delete, and slice range are well-chosen
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51.2 AIC · ⌖ 4.78 AIC · ⊞ 6.7K
Comment /matt to run again
| SuggestedFixes: []analysis.SuggestedFix{{ | ||
| Message: "Replace range-delete loop with clear", | ||
| TextEdits: []analysis.TextEdit{{ | ||
| Pos: rangeStmt.Pos(), |
There was a problem hiding this comment.
[/tdd] The SuggestedFix replaces the full range statement span, silently dropping any comments inside the loop body. Add a fixture case with an interior comment to document (and test) this behaviour.
💡 Example
// fixture – should this be preserved?
for k := range m {
// clear all cache entries
delete(m, k)
}After autofix the comment is gone. Consider prepending it before clear(m) or at least noting this in the diagnostic message.
@copilot please address this.
| m := map[string]int{"a": 1} | ||
|
|
||
| // Only ranging over value – not flagged. | ||
| for _, v := range m { |
There was a problem hiding this comment.
[/tdd] Missing negative test case: a range with a blank key (for _ := range m) is correctly skipped, but so is for range m { delete(m, k) } (no key variable). Confirm both are covered or add fixture lines to make the intent explicit.
💡 Suggested additions to the good() fixture
// No key variable – not flagged.
for range m {
}This guards against future regressions if the blank-key check is accidentally widened.
@copilot please address this.
|
|
||
| // sameObject reports whether expr refers to the same declared object as ref. | ||
| // ref is expected to be an *ast.Ident or *ast.SelectorExpr. | ||
| func sameObject(pass *analysis.Pass, expr, ref ast.Expr) bool { |
There was a problem hiding this comment.
[/codebase-design] sameObject only handles *ast.Ident and *ast.SelectorExpr. If the range expression is an index expression (e.g. maps[i]) or a function call returning a map, sameObject silently returns false — which is safe (no false positive) but worth documenting with a comment.
💡 Suggested inline comment
// sameObject only handles the common cases (plain identifiers and qualified
// selectors like pkg.Var or s.field). Complex expressions (index, call, etc.)
// return false conservatively — no false positive risk.
func sameObject(...) bool {@copilot please address this.
| func bad() { | ||
| m := map[string]int{"a": 1, "b": 2} | ||
|
|
||
| for k := range m { // want `range-delete loop over map can be replaced with clear\(m\)` |
There was a problem hiding this comment.
[/tdd] No positive test for a struct-field map (s.m) or a package-level map (pkg.M). The sameObject selector path is exercised by negative cases but never fires a true positive — add at least one // want line for the SelectorExpr path to confirm the fix emits clear(s.m) correctly.
💡 Suggested fixture addition
type S struct{ m map[string]int }
func badSelector(s S) {
for k := range s.m { // want `range-delete loop over map can be replaced with clear\(s\.m\)`
delete(s.m, k)
}
}@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 4 issues must be addressed before merge
The linter logic is sound for the common case, but there are correctness gaps that can produce broken suggested fixes and silent false negatives.
Blocking issues
- No Go version guard —
SuggestedFixemitsclear(m)regardless of the module'sgodirective; applying it on ago 1.20module breaks the build. NodeTextempty-string not guarded — an internal printer error producesclear()as the fix text, which is syntactically invalid Go.DefsvsUsesasymmetry on range key — loops that re-assign an outer-scope key variable (for k = range m) are silently dropped as false negatives.- Untested code paths — the explicit-blank-value branch and the entire
sameObjectSelectorExpr branch have zero test coverage; bugs there are invisible.
🔎 Code quality review by PR Code Quality Reviewer · 58.2 AIC · ⌖ 4.87 AIC · ⊞ 5.6K
Comment /review to run again
|
|
||
| // Analyzer is the map-clear-loop analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "mapclearloop", |
There was a problem hiding this comment.
No Go version guard — the suggested fix produces uncompilable code on modules targeting Go < 1.21.
💡 Details and fix
clear() was introduced in Go 1.21. The analyzer fires unconditionally regardless of the module's go directive. On a go 1.20 (or earlier) module, the emitted SuggestedFix rewrites the loop to clear(m) — silently breaking the caller's build when applied.
Add a version check in run(), or add a minimum-version annotation on the Analyzer:
var Analyzer = &analysis.Analyzer{
// ...
// clear() requires Go 1.21; skip on older module targets.
}Or guard inside run() before pass.Report:
if pass.Module != nil && !moduleRequiresAtLeast(pass.Module.GoVersion, 1, 21) {
return nil, nil
}At minimum, document the Go 1.21 requirement prominently in the Doc string.
| switch r := ref.(type) { | ||
| case *ast.Ident: | ||
| e, ok := expr.(*ast.Ident) | ||
| if !ok { |
There was a problem hiding this comment.
NodeText can return an empty string — clear() (no args) is invalid Go and will break the caller's build if the suggested fix is applied.
💡 Details and fix
astutil.NodeText calls printer.Fprint internally and returns "" on error. If mText is empty, the diagnostic message becomes range-delete loop over map can be replaced with clear() and the SuggestedFix emits clear() — syntactically invalid Go.
Guard before reporting:
mText := astutil.NodeText(pass.Fset, rangeStmt.X)
if mText == "" {
return
}
// ... pass.Report(...)| return | ||
| } | ||
| delIdent, ok := callExpr.Fun.(*ast.Ident) | ||
| if !ok || delIdent.Name != "delete" { |
There was a problem hiding this comment.
False negative: TypesInfo.Defs[keyIdent] is nil when the loop key is an existing variable (not freshly declared), silently skipping valid clear candidates.
💡 Details
TypesInfo.Defs maps an identifier to the object it defines. For a for k := range m where k is a fresh declaration, Defs[keyIdent] returns the *types.Var. But when k was declared in an enclosing scope and the range clause re-assigns it (e.g. for k = range m), Defs[keyIdent] is nil — the guard at line 98 returns early, dropping the diagnostic even though clear(m) is correct.
The same issue appears in the delete-key comparison: you compare delKeyObj != keyObj where keyObj came from Defs and delKeyObj came from Uses. For the re-assignment case keyObj is nil so the early return fires, but conceptually the comparison should use Uses[keyIdent] as a fallback.
keyObj := pass.TypesInfo.Defs[keyIdent]
if keyObj == nil {
keyObj = pass.TypesInfo.Uses[keyIdent] // re-assigned existing var
}
if keyObj == nil {
return
}| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
There was a problem hiding this comment.
Two handled code paths in mapclearloop.go have zero test coverage: explicit blank value (for k, _ := range m) and struct-field maps (sameObject SelectorExpr branch).
💡 Missing test cases
1. Explicit blank value (lines 103–108 of the implementation)
The code explicitly handles rangeStmt.Value != nil with a blank identifier. This path is never exercised:
// should be flagged
for k, _ := range m { // want `range-delete loop...`
delete(m, k)
}2. Struct-field map — sameObject SelectorExpr branch (lines 179–185)
There is no test that exercises the selector path, so any bug there is invisible:
type S struct{ m map[string]int }
var s S
// should be flagged
for k := range s.m { // want `range-delete loop...`
delete(s.m, k)
}
// should NOT be flagged (different receiver)
var s2 S
for k := range s.m {
delete(s2.m, k)
}Add both positive and negative cases to the testdata fixture and corresponding golden output.
Documents the architectural decision to add the mapclearloop analyzer, which replaces range-delete loops with clear(m) introduced in Go 1.21.
Design Decision Gate: ADR RequiredThis PR makes significant changes to core business logic (269 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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. 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
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented a PR-finisher pass and pushed fixes in commits 4ac1747, 8bd6db9, f0e3733, and 597819b. Addressed mapclearloop review findings (assignment-form ranges, shadowed clear safety, comment-preserving fix behavior, fixture coverage updates, and follow-up hardening from validation). |
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new
mapclearloopGo static analysis linter that detectsfor k := range m { delete(m, k) }patterns and suggests replacing them with the Go 1.21 built-inclear(m).Changes
cmd/linters/main.gomapclearloop.Analyzerin the linter runnerpkg/linters/mapclearloop/mapclearloop.gopkg/linters/mapclearloop/mapclearloop_test.goanalysistestharnesspkg/linters/mapclearloop/testdata/src/mapclearloop/mapclearloop.gopkg/linters/mapclearloop/testdata/src/mapclearloop/mapclearloop.go.goldendocs/adr/46060-add-mapclearloop-linter.mdAnalyzer Behaviour
Triggers when a
for-range loop over a map has exactly one body statement:delete(m, k)wheremandkare the same objects as in the range expression.Does not trigger when:
deleteis shadowed by a local functionclearis shadowed at the call sitefilecheck)nolint:mapclearloopdirective is presentAuto-fix: emits a
SuggestedFixrewriting the entire loop toclear(m). The fix is suppressed if any comment overlaps the loop source range, to avoid silently discarding user comments.Key Implementation Details
sameObject— comparesgo/typesobjects (not just identifier names) to verify the map and key references indelete(m, k)are the same declarations as in the range header. Handles both plain identifiers and selector expressions.builtinVisibleAtPos— walks the scope chain at the loop position to confirmclearresolves to the built-in, not a local shadow.hasOverlappingComment— tests comment group positions by range intersection across all files in the package; a prior bug (premature return on first non-overlapping file) was fixed during review.Fix Commits (post-initial)
SuggestedFixwhen comments overlap the range statementTest Coverage
Bad (flagged): basic range-delete, explicit blank value (
for k, _ := range), assignment form (for k = range), case with comment overlap (diagnostic emitted, no suggested fix).Good (not flagged): value variable used, multi-statement body, delete on different map, shadowed
delete, shadowedclear, range over slice.References
docs/adr/46060-add-mapclearloop-linter.mdpkg/linters/mapdeletecheck