-
Notifications
You must be signed in to change notification settings - Fork 425
feat(lenstringzero): extend linter to cover relational len-string comparisons #40621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # ADR-40621: Extend `lenstringzero` linter to relational len-string comparisons | ||
|
|
||
| **Date**: 2026-06-21 | ||
| **Status**: Draft | ||
|
|
||
| ## Context | ||
|
|
||
| The `lenstringzero` linter flags `len(s) == 0` and `len(s) != 0` comparisons on string values, suggesting the clearer `s == ""` / `s != ""` forms. It silently missed the semantically identical *relational* idioms — most notably `len(s) > 0`, which is arguably the most common non-empty-string test in Go, plus `len(s) >= 1`, `len(s) < 1`, and `len(s) <= 0`, including their yoda-order variants (`0 < len(s)`). This left a real coverage gap: the linter's stated purpose (steer string emptiness checks toward direct comparison) was only partially enforced. The implementation matches strings syntactically over the AST and must avoid flagging non-string slices, tautologies (`len(s) >= 0`), and contradictions (`len(s) < 0`). | ||
|
|
||
| ## Decision | ||
|
|
||
| We will extend `lenstringzero` to recognize all six comparison operators (`==`, `!=`, `>`, `>=`, `<`, `<=`) against the integer literals `0` and `1`, in both normal and yoda operand order. A new `matchLenLiteralExpr` normalizes each comparison so `len` is on the left and reports the literal; `resolveFixOp` maps the normalized `(operator, literal)` pair to the correct fix (`==`/`!=`) and verb (`empty`/`non-empty`), and explicitly excludes tautologies and contradictions. `buildLenStringFix` now takes an explicit `fixOp` so relational inputs still emit a correct direct-comparison suggested fix, and diagnostics report the normalized form even for yoda input. This keeps the linter's syntactic, fix-emitting design while closing the idiom gap. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| ### Alternative 1: Keep scope limited to `==` / `!=` | ||
| Leave the linter as-is and rely on reviewers to catch relational forms. Rejected because `len(s) > 0` is the single most common non-empty idiom, so the linter was missing its highest-value target while appearing complete. | ||
|
|
||
| ### Alternative 2: Type/SSA-based semantic analysis instead of syntactic pattern tables | ||
| Use a more general value-analysis approach to recognize emptiness comparisons regardless of literal or operator shape. Rejected as disproportionate: it adds significant complexity and analysis cost for a linter whose value comes from a small, well-known set of literal idioms that an explicit `(operator, literal)` table covers precisely and predictably. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| - Catches the most common non-empty-string idiom (`len(s) > 0`) and its relatives, including yoda order. | ||
| - Every flagged relational case still ships a correct auto-applicable suggested fix. | ||
|
|
||
| ### Negative | ||
| - More surface area and branching (`matchLenLiteralExpr`, `resolveFixOp`, `flipOp`, `isIntOne`) to maintain and reason about. | ||
| - Coverage is deliberately bounded to literals `0` and `1`; other meaningful thresholds (e.g. `len(s) > 1`) are out of scope and not flagged. | ||
|
|
||
| ### Neutral | ||
| - Diagnostic messages now normalize yoda input to `len`-on-left form, so reported text can differ from the source spelling. | ||
| - Tautologies (`len(s) >= 0`) and contradictions (`len(s) < 0`) are intentionally left unflagged rather than rewritten. | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27907011489) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| // Package lenstringzero implements a Go analysis linter that flags len(s) == 0 | ||
| // and len(s) != 0 comparisons on string values that should use == "" or != "" instead. | ||
| // Package lenstringzero implements a Go analysis linter that flags len(s) == 0, | ||
| // len(s) != 0, and equivalent relational comparisons (len(s) > 0, len(s) >= 1, | ||
| // len(s) < 1, len(s) <= 0) on string values that should use == "" or != "" instead. | ||
| package lenstringzero | ||
|
|
||
| import ( | ||
|
|
@@ -16,8 +17,10 @@ import ( | |
| ) | ||
|
|
||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "lenstringzero", | ||
| Doc: "reports len(s) == 0 and len(s) != 0 comparisons on string values that should use == \"\" or != \"\" instead", | ||
| Name: "lenstringzero", | ||
| Doc: "reports len(s) == 0, len(s) != 0, and equivalent relational comparisons " + | ||
| "(len(s) > 0, len(s) >= 1, len(s) < 1, len(s) <= 0) on string values " + | ||
| "that should use == \"\" or != \"\" instead", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/lenstringzero", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
|
|
@@ -37,7 +40,9 @@ func run(pass *analysis.Pass) (any, error) { | |
| if !ok { | ||
| return | ||
| } | ||
| if expr.Op != token.EQL && expr.Op != token.NEQ { | ||
| switch expr.Op { | ||
| case token.EQL, token.NEQ, token.GTR, token.GEQ, token.LSS, token.LEQ: | ||
| default: | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -46,24 +51,13 @@ func run(pass *analysis.Pass) (any, error) { | |
| return | ||
| } | ||
|
|
||
| var lenArg ast.Expr | ||
| isDirect := false | ||
| if isLenCall(expr.X) && isIntZero(expr.Y) { | ||
| lenArg = lenCallArg(expr.X) | ||
| isDirect = true | ||
| } else if isIntZero(expr.X) && isLenCall(expr.Y) { | ||
| lenArg = lenCallArg(expr.Y) | ||
| isDirect = true | ||
| } else if isIntZero(expr.Y) { | ||
| if arg, ok := lenAliasArg(pass, expr.X, lenStringAliases); ok { | ||
| lenArg = arg | ||
| } | ||
| } else if isIntZero(expr.X) { | ||
| if arg, ok := lenAliasArg(pass, expr.Y, lenStringAliases); ok { | ||
| lenArg = arg | ||
| } | ||
| lenArg, isDirect, normalOp, lit, matched := matchLenLiteralExpr(pass, expr, lenStringAliases) | ||
| if !matched { | ||
| return | ||
| } | ||
| if lenArg == nil { | ||
|
|
||
| fixOp, cmpVerb, valid := resolveFixOp(normalOp, lit) | ||
| if !valid { | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -76,36 +70,127 @@ func run(pass *analysis.Pass) (any, error) { | |
| return | ||
| } | ||
|
|
||
| op := expr.Op.String() | ||
| var cmpVerb string | ||
| if expr.Op == token.EQL { | ||
| cmpVerb = "empty" | ||
| } else { | ||
| cmpVerb = "non-empty" | ||
| } | ||
| var fixes []analysis.SuggestedFix | ||
| if isDirect { | ||
| fixes = buildLenStringFix(pass, expr, lenArg) | ||
| fixes = buildLenStringFix(pass, expr, lenArg, fixOp) | ||
| } | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: expr.Pos(), | ||
| End: expr.End(), | ||
| Message: fmt.Sprintf(`use s %s "" to check for %s string instead of len(s) %s 0`, op, cmpVerb, op), | ||
| Message: fmt.Sprintf(`use s %s "" to check for %s string instead of len(s) %s %d`, fixOp, cmpVerb, normalOp, lit), | ||
| SuggestedFixes: fixes, | ||
| }) | ||
| }) | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| // buildLenStringFix returns a SuggestedFix that rewrites a direct len(s) == 0 | ||
| // or len(s) != 0 comparison to s == "" or s != "". | ||
| func buildLenStringFix(pass *analysis.Pass, expr *ast.BinaryExpr, lenArg ast.Expr) []analysis.SuggestedFix { | ||
| // matchLenLiteralExpr tries to match len(s)/alias OP literal or literal OP len(s)/alias. | ||
| // Returns (lenArg, isDirect, normalOp, lit, matched) where: | ||
| // - lenArg is the string expression passed to len() | ||
| // - isDirect indicates a direct len() call (true) vs a stored alias (false) | ||
| // - normalOp is the operator normalized so that len is on the left side | ||
| // - lit is the integer literal value (0 or 1) | ||
| // - matched indicates whether a valid pattern was found | ||
| func matchLenLiteralExpr(pass *analysis.Pass, expr *ast.BinaryExpr, aliases map[types.Object]ast.Expr) (lenArg ast.Expr, isDirect bool, normalOp token.Token, lit int, ok bool) { | ||
| op := expr.Op | ||
|
|
||
| // Normal order: len/alias on the left, literal on the right. | ||
| if isLenCall(expr.X) { | ||
| if isIntZero(expr.Y) { | ||
| return lenCallArg(expr.X), true, op, 0, true | ||
| } | ||
| if isIntOne(expr.Y) { | ||
| return lenCallArg(expr.X), true, op, 1, true | ||
| } | ||
| } | ||
| if isIntZero(expr.Y) { | ||
| if arg, ok2 := lenAliasArg(pass, expr.X, aliases); ok2 { | ||
| return arg, false, op, 0, true | ||
| } | ||
| } | ||
| if isIntOne(expr.Y) { | ||
| if arg, ok2 := lenAliasArg(pass, expr.X, aliases); ok2 { | ||
| return arg, false, op, 1, true | ||
| } | ||
| } | ||
|
|
||
| // Yoda order: literal on the left, len/alias on the right. | ||
| // Flip the operator so the normalized form has len on the left. | ||
| if isLenCall(expr.Y) { | ||
| if isIntZero(expr.X) { | ||
| return lenCallArg(expr.Y), true, flipOp(op), 0, true | ||
| } | ||
| if isIntOne(expr.X) { | ||
| return lenCallArg(expr.Y), true, flipOp(op), 1, true | ||
| } | ||
| } | ||
| if isIntZero(expr.X) { | ||
| if arg, ok2 := lenAliasArg(pass, expr.Y, aliases); ok2 { | ||
| return arg, false, flipOp(op), 0, true | ||
| } | ||
| } | ||
| if isIntOne(expr.X) { | ||
| if arg, ok2 := lenAliasArg(pass, expr.Y, aliases); ok2 { | ||
| return arg, false, flipOp(op), 1, true | ||
| } | ||
| } | ||
|
|
||
| return nil, false, 0, 0, false | ||
| } | ||
|
|
||
| // resolveFixOp returns the fix operator and comparison verb for a normalized | ||
| // comparison where len is on the left side. | ||
| // Only the semantically meaningful cases are flagged: | ||
| // | ||
| // len(s) == 0 → s == "" (empty) | ||
| // len(s) != 0 → s != "" (non-empty) | ||
| // len(s) > 0 → s != "" (non-empty) | ||
| // len(s) >= 1 → s != "" (non-empty) | ||
| // len(s) < 1 → s == "" (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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] 💡 Sketchfunc 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
}
} |
||
| switch normalOp { | ||
| case token.EQL: | ||
| if lit == 0 { | ||
| return token.EQL, "empty", true | ||
| } | ||
| case token.NEQ: | ||
| if lit == 0 { | ||
| return token.NEQ, "non-empty", true | ||
| } | ||
| case token.GTR: | ||
| if lit == 0 { | ||
| return token.NEQ, "non-empty", true | ||
| } | ||
| case token.GEQ: | ||
| if lit == 1 { | ||
| return token.NEQ, "non-empty", true | ||
| } | ||
| // lit == 0 → len(s) >= 0 is always true; do not flag. | ||
| case token.LSS: | ||
| if lit == 1 { | ||
| return token.EQL, "empty", true | ||
| } | ||
| // lit == 0 → len(s) < 0 is always false; do not flag. | ||
| case token.LEQ: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/grill-with-docs] 💡 Suggested additionsIn the 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 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. |
||
| if lit == 0 { | ||
| return token.EQL, "empty", true | ||
| } | ||
| } | ||
| return 0, "", false | ||
| } | ||
|
|
||
| // buildLenStringFix returns a SuggestedFix that rewrites a direct len(s) comparison | ||
| // to a direct string comparison using fixOp (== or !=). | ||
| func buildLenStringFix(pass *analysis.Pass, expr *ast.BinaryExpr, lenArg ast.Expr, fixOp token.Token) []analysis.SuggestedFix { | ||
| text := astutil.NodeText(pass.Fset, lenArg) | ||
| if text == "" { | ||
| return nil | ||
| } | ||
| replacement := fmt.Sprintf(`%s %s ""`, text, expr.Op.String()) | ||
| replacement := fmt.Sprintf(`%s %s ""`, text, fixOp.String()) | ||
| return []analysis.SuggestedFix{{ | ||
| Message: "Replace with direct string comparison", | ||
| TextEdits: []analysis.TextEdit{{ | ||
|
|
@@ -116,6 +201,23 @@ func buildLenStringFix(pass *analysis.Pass, expr *ast.BinaryExpr, lenArg ast.Exp | |
| }} | ||
| } | ||
|
|
||
| // flipOp returns the comparison operator adjusted for swapping left and right operands. | ||
| // For example, when converting "0 < len(s)" to the normalized "len(s) > 0", LSS becomes GTR. | ||
| func flipOp(op token.Token) token.Token { | ||
| switch op { | ||
| case token.LSS: | ||
| return token.GTR | ||
| case token.GTR: | ||
| return token.LSS | ||
| case token.LEQ: | ||
| return token.GEQ | ||
| case token.GEQ: | ||
| return token.LEQ | ||
| default: | ||
| return op | ||
| } | ||
| } | ||
|
|
||
| func isLenCall(expr ast.Expr) bool { | ||
| call, ok := expr.(*ast.CallExpr) | ||
| if !ok || len(call.Args) != 1 { | ||
|
|
@@ -138,6 +240,11 @@ func isIntZero(expr ast.Expr) bool { | |
| return ok && lit.Kind == token.INT && lit.Value == "0" | ||
| } | ||
|
|
||
| func isIntOne(expr ast.Expr) bool { | ||
| lit, ok := expr.(*ast.BasicLit) | ||
| return ok && lit.Kind == token.INT && lit.Value == "1" | ||
| } | ||
|
|
||
| func collectLenStringAliases(pass *analysis.Pass) map[types.Object]ast.Expr { | ||
| aliases := make(map[types.Object]ast.Expr) | ||
| for _, file := range pass.Files { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,50 @@ func arrayNotFlagged(s [1]byte) bool { | |
| return len(s) != 0 | ||
| } | ||
|
|
||
| func lenNotZeroOp(s string) bool { | ||
| func lenGreaterThanZero(s string) bool { | ||
| return len(s) > 0 // want `use s != "" to check for non-empty string instead of len\(s\) > 0` | ||
| } | ||
|
|
||
| func lenGreaterOrEqualOne(s string) bool { | ||
| return len(s) >= 1 // want `use s != "" to check for non-empty string instead of len\(s\) >= 1` | ||
| } | ||
|
|
||
| func lenLessThanOne(s string) bool { | ||
| return len(s) < 1 // want `use s == "" to check for empty string instead of len\(s\) < 1` | ||
| } | ||
|
|
||
| func lenLessOrEqualZero(s string) bool { | ||
| return len(s) <= 0 // want `use s == "" to check for empty string instead of len\(s\) <= 0` | ||
| } | ||
|
|
||
| func yodaLenGreaterThanZero(s string) bool { | ||
| return 0 < len(s) // want `use s != "" to check for non-empty string instead of len\(s\) > 0` | ||
| } | ||
|
|
||
| func yodaLenGreaterOrEqualOne(s string) bool { | ||
| return 1 <= len(s) // want `use s != "" to check for non-empty string instead of len\(s\) >= 1` | ||
| } | ||
|
|
||
| func yodaLenLessThanOne(s string) bool { | ||
| return 1 > len(s) // want `use s == "" to check for empty string instead of len\(s\) < 1` | ||
| } | ||
|
|
||
| func yodaLenLessOrEqualZero(s string) bool { | ||
| return 0 >= len(s) // want `use s == "" to check for empty string instead of len\(s\) <= 0` | ||
| } | ||
|
|
||
| // len(s) >= 0 is always true — must not be flagged. | ||
| func lenAlwaysTrue(s string) bool { | ||
| return len(s) >= 0 | ||
| } | ||
|
|
||
| // len(s) < 0 is always false — must not be flagged. | ||
| func lenAlwaysFalse(s string) bool { | ||
| return len(s) < 0 | ||
| } | ||
|
|
||
| // Non-string slice must not be flagged for relational operators. | ||
| func sliceGreaterThanZeroNotFlagged(s []byte) bool { | ||
| return len(s) > 0 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The "not flagged" non-string guard is tested only for 💡 Suggested additionsfunc 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 |
||
| } | ||
|
|
||
|
|
@@ -50,6 +93,26 @@ func aliasNotEmpty(s string) bool { | |
| return n != 0 // want `use s != "" to check for non-empty string instead of len\(s\) != 0` | ||
| } | ||
|
|
||
| func aliasGreaterThanZero(s string) bool { | ||
| n := len(s) | ||
| return n > 0 // want `use s != "" to check for non-empty string instead of len\(s\) > 0` | ||
| } | ||
|
|
||
| func aliasGreaterOrEqualOne(s string) bool { | ||
| n := len(s) | ||
| return n >= 1 // want `use s != "" to check for non-empty string instead of len\(s\) >= 1` | ||
| } | ||
|
|
||
| func aliasLessThanOne(s string) bool { | ||
| n := len(s) | ||
| return n < 1 // want `use s == "" to check for empty string instead of len\(s\) < 1` | ||
| } | ||
|
|
||
| func aliasLessOrEqualZero(s string) bool { | ||
| n := len(s) | ||
| return n <= 0 // want `use s == "" to check for empty string instead of len\(s\) <= 0` | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Yoda-order alias paths are handled in 💡 Suggested additionsfunc 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 |
||
|
|
||
| func aliasReassignedNotFlagged(s string) bool { | ||
| n := len(s) | ||
| n = 1 | ||
|
|
@@ -71,3 +134,4 @@ func arrayAliasNotFlagged(s [1]byte) bool { | |
| n := len(s) | ||
| return n == 0 | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The doc comment correctly calls out tautologies and contradictions, but does not explain that
len(s) == 1andlen(s) != 1— while matched bymatchLenLiteralExpr— are intentionally dropped because they do not reduce to a single string comparison. A reader tracing whyNEQ + lit=1falls through will have to reason through both functions to confirm this is deliberate.💡 Suggested wording