feat(lenstringzero): extend linter to cover relational len-string comparisons#40621
Conversation
… on strings Extend the lenstringzero linter to flag the relational equivalents of len(s) == 0 / len(s) != 0 on string values: len(s) > 0 → s != "" (non-empty) len(s) >= 1 → s != "" (non-empty) len(s) < 1 → s == "" (empty) len(s) <= 0 → s == "" (empty) Yoda forms (0 < len(s), 1 <= len(s), etc.) and alias forms (n := len(s); n > 0) are also handled. Tautologies (len(s) >= 0) and contradictions (len(s) < 0) are deliberately not flagged. Non-string slices/arrays remain unflagged via the existing *types.Basic String guard. The suggested fix rewrites to the correct == "" or != "" form. Closes #40581 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the lenstringzero Go analysis linter so it also flags relational comparisons against len(s) that are semantically equivalent to empty/non-empty string checks, and produces correct == "" / != "" fixes for direct len(...) comparisons.
Changes:
- Expanded detection from
== 0/!= 0to all six comparison operators, normalizing yoda-style expressions (e.g.0 < len(s)→len(s) > 0). - Added operator/literal normalization and fix resolution so relational comparisons map to the correct
==/!=replacement (excluding tautologies/contradictions likelen(s) >= 0). - Updated testdata and golden outputs to cover the new comparison patterns and non-flagged cases.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/lenstringzero/lenstringzero.go |
Adds relational-operator matching, normalization, and correct fix/operator mapping for empty/non-empty string comparisons. |
pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go |
Adds new test cases for relational and yoda-order patterns, plus non-flagged tautology/contradiction and non-string cases. |
pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden |
Updates golden output to reflect the expected suggested fixes and diagnostics for the new patterns. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
|
✅ 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. No test files were added or modified in this PR. The PR modifies pkg/linters/lenstringzero/lenstringzero.go (production code) and expands analysistest fixture files (testdata/src/lenstringzero/lenstringzero.go and its .golden counterpart) with new // want annotation cases for relational len-string comparisons. The test harness lenstringzero_test.go was not changed. Test Quality Sentinel skipped — no _test.go, *.test.cjs, or *.test.js files were added or modified. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (272 new lines under 📄 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. The choice to keep 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![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 /grill-with-docs — no blocking issues, commenting with improvements.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: The yoda-order × alias combination (
0 < nwheren := len(s)) is the one untested path throughmatchLenLiteralExpr. - Missing "not flagged" cases: Non-string relational tests cover only
> 0;>= 1,< 1,<= 0for slices are absent.len(s) != 1also lacks a negative test. resolveFixOpdoc: Silent fall-through forGTR/LEQwithlit == 1and forEQL/NEQwithlit == 1are unexplained, unlike the explicit tautology/contradiction comments already present.- Unit test for
resolveFixOp: This function is the semantic kernel; a table-driven test would pin correctness directly.
Positive Highlights
- ✅ Semantic correctness is solid — all six operator expansions map correctly, tautologies and contradictions are properly excluded.
- ✅ Yoda-order normalization via
flipOpis clean and fully tested for directlen()calls. - ✅ The two-function decomposition (
matchLenLiteralExpr+resolveFixOp) is a clear improvement over the original inline logic. - ✅
buildLenStringFixcorrectly uses the decoupledfixOprather than the original expression operator — this was the key correctness risk for relational operators. - ✅ PR description is excellent: the coverage table makes the intent immediately clear.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 111.8 AIC · ⌖ 9.42 AIC · ⊞ 6.9K
| func aliasLessOrEqualZero(s string) bool { | ||
| n := len(s) | ||
| return n <= 0 // want `use s == "" to check for empty string instead of len\(s\) <= 0` | ||
| } |
There was a problem hiding this comment.
[/tdd] Yoda-order alias paths are handled in matchLenLiteralExpr (implementation lines 128–136) but never exercised by the test matrix — e.g. 0 < n or 1 <= n where n := len(s). This is the only (alias × yoda) combination without coverage.
💡 Suggested additions
func aliasYodaGreaterThanZero(s string) bool {
n := len(s)
return 0 < n // want `use s != "" to check for non-empty string instead of len\(s\) > 0`
}
func aliasYodaGreaterOrEqualOne(s string) bool {
n := len(s)
return 1 <= n // want `use s != "" to check for non-empty string instead of len\(s\) >= 1`
}
// + yoda alias variants for < and <=Adding all four yoda-alias variants closes the only untested code path in the new matchLenLiteralExpr.
|
|
||
| // Non-string slice must not be flagged for relational operators. | ||
| func sliceGreaterThanZeroNotFlagged(s []byte) bool { | ||
| return len(s) > 0 |
There was a problem hiding this comment.
[/tdd] The "not flagged" non-string guard is tested only for > 0, but the new operators add three more relational paths through the type check. A len(slice) >= 1, len(slice) < 1, and len(slice) <= 0 should also be present to pin that the types.String guard holds for all new operators.
💡 Suggested additions
func sliceGreaterOrEqualOneNotFlagged(s []byte) bool {
return len(s) >= 1
}
func sliceLessThanOneNotFlagged(s []byte) bool {
return len(s) < 1
}
func sliceLessOrEqualZeroNotFlagged(s []byte) bool {
return len(s) <= 0
}Without these, a future regression in the type-guard branch for GEQ/LSS/LEQ would go undetected.
| // len(s) < 1 → s == "" (empty) | ||
| // len(s) <= 0 → s == "" (empty) | ||
| // | ||
| // Tautologies (len(s) >= 0) and contradictions (len(s) < 0) are not flagged. |
There was a problem hiding this comment.
[/grill-with-docs] The doc comment correctly calls out tautologies and contradictions, but does not explain that len(s) == 1 and len(s) != 1 — while matched by matchLenLiteralExpr — are intentionally dropped because they do not reduce to a single string comparison. A reader tracing why NEQ + lit=1 falls through will have to reason through both functions to confirm this is deliberate.
💡 Suggested wording
// Tautologies (len(s) >= 0) and contradictions (len(s) < 0) are not flagged.
// Comparisons against 1 that are not equivalent to empty/non-empty (e.g.
// len(s) == 1, len(s) != 1, len(s) <= 1) are also intentionally excluded.| return token.EQL, "empty", true | ||
| } | ||
| // lit == 0 → len(s) < 0 is always false; do not flag. | ||
| case token.LEQ: |
There was a problem hiding this comment.
[/grill-with-docs] GEQ and LSS each have an inline comment explaining their silent fall-through (>= 0 is always true, < 0 is always false). LEQ with lit == 1 (len(s) <= 1) and GTR with lit == 1 (len(s) > 1) also fall through silently but without explanation, making the asymmetry look like an omission.
💡 Suggested additions
In the GTR case (line ~165):
case token.GTR:
if lit == 0 {
return token.NEQ, "non-empty", true
}
// lit == 1 → len(s) > 1 means at least 2 chars; not equivalent to non-empty.In the LEQ case (line ~178):
case token.LEQ:
if lit == 0 {
return token.EQL, "empty", true
}
// lit == 1 → len(s) <= 1 means 0 or 1 chars; not equivalent to empty.| // len(s) <= 0 → s == "" (empty) | ||
| // | ||
| // Tautologies (len(s) >= 0) and contradictions (len(s) < 0) are not flagged. | ||
| func resolveFixOp(normalOp token.Token, lit int) (fixOp token.Token, cmpVerb string, ok bool) { |
There was a problem hiding this comment.
[/tdd] resolveFixOp is the correctness kernel of this change — it encodes the full (operator, literal) → fixOp mapping table. It is currently tested only indirectly via analysistest.RunWithSuggestedFixes. A small table-driven unit test would both document the contract and catch any future accidental deletion of a case.
💡 Sketch
func TestResolveFixOp(t *testing.T) {
tests := []struct {
op token.Token
lit int
wantOk bool
wantFix token.Token
}{
{token.EQL, 0, true, token.EQL},
{token.NEQ, 0, true, token.NEQ},
{token.GTR, 0, true, token.NEQ},
{token.GEQ, 1, true, token.NEQ},
{token.LSS, 1, true, token.EQL},
{token.LEQ, 0, true, token.EQL},
// non-flagged cases
{token.GEQ, 0, false, 0}, // tautology
{token.LSS, 0, false, 0}, // contradiction
{token.EQL, 1, false, 0}, // len == 1
{token.NEQ, 1, false, 0}, // len != 1
}
for _, tc := range tests {
fix, _, ok := resolveFixOp(tc.op, tc.lit)
// assert ok == tc.wantOk, fix == tc.wantFix
}
}
lenstringzeroflaggedlen(s) == 0andlen(s) != 0but silently missed the semantically identical relational forms — most notablylen(s) > 0, the most common non-empty string idiom in Go.What changed
lenstringzero.goEQL/NEQto all six comparison operators.matchLenLiteralExpr: normalizes direct and yoda-order comparisons against literals0or1, for both directlen()calls and stored aliases.resolveFixOp: maps normalized(operator, literal)pairs to the correct fix op (==/!=) and verb (empty/non-empty). Deliberately excludes tautologies (len(s) >= 0) and contradictions (len(s) < 0).flipOpandisIntOnehelpers.buildLenStringFixnow takes an explicitfixOpinstead of reusingexpr.Op, so relational operators produce the correct==/!=replacement.len(s) > 0) even for yoda input.Analyzer.Docand package comment.Testdata / golden file
Coverage
len(s) > 0s != ""len(s) >= 1s != ""len(s) < 1s == ""len(s) <= 0s == ""0 < len(s)(yoda)s != ""len(s) >= 0(tautology)len(slice) > 0(non-string)