Improve lenstringzero precision for len(string) aliases in zero-comparisons - #37750
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
lenstringzero precision for len(string) aliases in zero-comparisons
There was a problem hiding this comment.
Pull request overview
Extends the lenstringzero Go analyzer to detect len(<string>) results stored in local variables and later compared to 0, matching the same diagnostics as direct len(s) == 0 / != 0 checks.
Changes:
- Track local aliases initialized from
len(<string-expr>)and detectalias == 0/alias != 0(including flipped0 == aliasforms). - Add alias invalidation logic for certain mutations (
=,++/--, andrangeassignment targets). - Expand analysistest fixtures to cover positive and negative alias cases.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/lenstringzero/lenstringzero.go | Adds alias collection and extends comparison matching to handle alias-vs-zero checks. |
| pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go | Updates fixtures to include alias-based comparisons and mutation-negative cases. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| if !ok { | ||
| return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) | ||
| } | ||
| lenStringAliases := collectLenStringAliases(pass) |
| switch stmt.Tok { | ||
| case token.DEFINE: | ||
| if obj.Pos() != ident.Pos() { | ||
| delete(aliases, obj) | ||
| continue | ||
| } | ||
| rhs, ok := rhsExprForIndex(stmt.Rhs, i) | ||
| if !ok { | ||
| delete(aliases, obj) | ||
| continue | ||
| } | ||
| if arg, ok := lenStringArg(pass, rhs); ok { | ||
| aliases[obj] = arg | ||
| } else { | ||
| delete(aliases, obj) | ||
| } | ||
| case token.ASSIGN: | ||
| delete(aliases, obj) | ||
| } |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The PR modifies lenstringzero.go (production code) and testdata/src/lenstringzero/lenstringzero.go (an analysistest fixture file containing // want annotations). The testdata fixture does not match the test file criteria (_test.go, *.test.cjs, *.test.js), so Test Quality Sentinel skipped scoring. Note: the testdata was updated to add new alias-tracking test cases (aliasEmpty, aliasNotEmpty, aliasReassignedNotFlagged, etc.) which provide good behavioral coverage for the new alias detection feature. |
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 (194 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. This change makes a real design choice — conservative syntactic alias tracking with mutation invalidation, rather than full dataflow analysis — that future contributors should be able to find and understand. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
ADRs live in
|
|
@copilot review all comments and review . Apply copilot-review skill |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Clean, correct refactors — no blocking issues
Both changes are logically equivalent to the code they replace and safe to merge.
Analysis summary
run() — condition reordering (lines 51-57)
Moving isIntZero(expr.Y) before lenAliasArg is semantically equivalent:
| Scenario | Old behaviour | New behaviour |
|---|---|---|
n == 0 (alias on left) |
lenAliasArg(n).ok && isIntZero(0) → lenArg = arg |
isIntZero(0) → inner lenAliasArg(n).ok → lenArg = arg ✓ |
0 == n (alias on right) |
Falls through to 4th branch | isIntZero(n)=false → 4th branch ✓ |
0 == 0 |
Neither alias branch fires | isIntZero(0)=true, lenAliasArg(0).ok=false → lenArg=nil ✓ |
As a side-effect, lenAliasArg (a map lookup + type check) is now only called when isIntZero(Y) is already true — a minor but correct performance improvement.
The else if isIntZero(expr.Y) block cannot accidentally shadow the else if isIntZero(expr.X) block for any valid alias case because isIntZero(Y)=true implies Y is a zero literal, not an alias.
rhsExprForIndex — removed redundant switch case (lines 222-223)
case len(rhs) == 1 && idx == 0 is entirely subsumed by the following case idx < len(rhs) (0 < 1 is always true when len==1, idx==0). The removed lines are dead code — correct to delete.
Existing tests (aliasEmpty, aliasNotEmpty) exercise both changed paths.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 14.5 AIC
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes for one correctness bug and three test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- False-positive bug (compound assignments):
n += 1(and all otherop=tokens) are not treated as mutations — the alias stays live in the map, so the subsequentn == 0is still reported. This is the most critical issue since a linter emitting spurious diagnostics erodes trust. - Test-coverage gaps: Three code paths lack
analysistestfixtures — flipped alias forms (0 == n), theValueSpec(var-decl) path, and the compound-assignment mutation path.
Positive Highlights
- ✅ Clean pre-pass design: alias tracking is neatly separated from the binary-expression walk
- ✅
isLocalObjectcorrectly scopes tracking to function-local variables - ✅ Mutation invalidation for
=,++/--, and range-assign targets is correct - ✅ Negative fixtures for reassignment and increment already present — a solid foundation
- ✅ The
obj.Pos() != ident.Pos()guard for:=redeclarations is a subtle but correct touch
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 228.1 AIC · ⌖ 13.9 AIC
| delete(aliases, obj) | ||
| } | ||
| case token.ASSIGN: | ||
| delete(aliases, obj) |
There was a problem hiding this comment.
[/diagnose] Compound-assignment operators (+=, -=, *=, /=, %=, ...) are not invalidated — this causes false positives.
stmt.Tok for n += 1 is token.ADD_ASSIGN, which falls through the switch stmt.Tok without calling delete(aliases, obj). The alias stays in the map, so the subsequent n == 0 is reported even though n no longer holds len(s).
💡 Suggested fix — fold ASSIGN into default
Change the switch to a default case; any non-DEFINE token (plain =, +=, -=, ...) should invalidate the alias:
switch stmt.Tok {
case token.DEFINE:
// ... existing logic ...
default:
delete(aliases, obj)
}A failing regression fixture to add in the test data:
func aliasCompoundAssignedNotFlagged(s string) bool {
n := len(s)
n += 1
return n == 0 // should NOT be reported
}|
|
||
| func aliasNotEmpty(s string) bool { | ||
| n := len(s) | ||
| return n != 0 // want `use s != "" to check for non-empty string instead of len\(s\) != 0` |
There was a problem hiding this comment.
[/tdd] The flipped alias forms 0 == n and 0 != n are handled by the code (the isIntZero(expr.X) branch in run()) but have no test fixtures.
All four direct-len flipped forms have fixtures (flippedEmpty, flippedNotEmpty); the alias path deserves parity so a future refactor cannot silently drop that branch.
💡 Suggested fixtures to add after this function
func aliasFlippedEmpty(s string) bool {
n := len(s)
return 0 == n // want `use s == "" to check for empty string instead of len\(s\) == 0`
}
func aliasFlippedNotEmpty(s string) bool {
n := len(s)
return 0 != n // want `use s != "" to check for non-empty string instead of len\(s\) != 0`
}| func aliasIncrementedNotFlagged(s string) bool { | ||
| n := len(s) | ||
| n++ | ||
| return n == 0 |
There was a problem hiding this comment.
[/tdd] No negative test for compound-assignment mutations (+=, -=, etc.).
aliasIncrementedNotFlagged covers n++ (an *ast.IncDecStmt), but compound assignments are a distinct AST node (*ast.AssignStmt with a non-=/:= token) and currently trigger a false positive (the alias is not invalidated). A fixture here would serve as a regression anchor once the bug in collectLenStringAliasesFromAssignStmt is fixed.
💡 Suggested fixture
func aliasCompoundAssignedNotFlagged(s string) bool {
n := len(s)
n += 1
return n == 0 // should NOT be reported
}| func arrayAliasNotFlagged(s [1]byte) bool { | ||
| n := len(s) | ||
| return n == 0 | ||
| } |
There was a problem hiding this comment.
[/tdd] collectLenStringAliasesFromValueSpec (the var n = len(s) path) has no test coverage.
The ValueSpec handler runs for var declarations inside functions, but no fixture exercises this code path. It is functionally symmetric to := and is worth pinning with a positive case.
💡 Suggested fixture
func aliasVarDecl(s string) bool {
var n = len(s)
return n == 0 // want `use s == "" to check for empty string instead of len\(s\) == 0`
}
lenstringzeroonly matched directlen(s) == 0/!= 0checks and missed equivalent alias forms liken := len(s); n == 0. This PR extends detection to locallen(string)aliases while preserving existing string-only scope and avoiding mutation-related false positives.Analyzer precision: track local
len(string)aliaseslen(<string-expr>)in::=)varspecs (ValueSpec)string.Alias safety: invalidate on mutation
=)++/--)for k, v = range ...)Comparison handling: support alias-vs-zero parity with direct form
alias == 0alias != 00 == alias0 != aliaslen(s)matches.Test fixtures: positive and negative alias coverage
analysistestcases for:n := len(s); n == 0andn != 0[]byte, array) remain unflagged