From 68e246610ccc764d969f5ccc3d85ae1f12521457 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 21 Jun 2026 13:07:12 +0000 Subject: [PATCH 1/3] Initial plan From ffe3499bc71a27fba549c6ade012ab391249733b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 21 Jun 2026 14:00:58 +0000 Subject: [PATCH 2/3] feat(lenstringzero): extend linter to flag relational len comparisons on strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- pkg/linters/lenstringzero/lenstringzero.go | 177 ++++++++++++++---- .../src/lenstringzero/lenstringzero.go | 66 ++++++- .../src/lenstringzero/lenstringzero.go.golden | 66 ++++++- 3 files changed, 272 insertions(+), 37 deletions(-) diff --git a/pkg/linters/lenstringzero/lenstringzero.go b/pkg/linters/lenstringzero/lenstringzero.go index 1ee55231b36..7eb6aa06408 100644 --- a/pkg/linters/lenstringzero/lenstringzero.go +++ b/pkg/linters/lenstringzero/lenstringzero.go @@ -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,21 +70,14 @@ 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, }) }) @@ -98,14 +85,112 @@ func run(pass *analysis.Pass) (any, error) { 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) { + 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: + 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 { diff --git a/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go b/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go index ad23e80e31a..9b917e04a2c 100644 --- a/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go +++ b/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go @@ -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 } @@ -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` +} + 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 } + diff --git a/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden b/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden index a942e1171c7..c9daab3f0b5 100644 --- a/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden +++ b/pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden @@ -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 s != "" // want `use s != "" to check for non-empty string instead of len\(s\) > 0` +} + +func lenGreaterOrEqualOne(s string) bool { + return s != "" // want `use s != "" to check for non-empty string instead of len\(s\) >= 1` +} + +func lenLessThanOne(s string) bool { + return s == "" // want `use s == "" to check for empty string instead of len\(s\) < 1` +} + +func lenLessOrEqualZero(s string) bool { + return s == "" // want `use s == "" to check for empty string instead of len\(s\) <= 0` +} + +func yodaLenGreaterThanZero(s string) bool { + return s != "" // want `use s != "" to check for non-empty string instead of len\(s\) > 0` +} + +func yodaLenGreaterOrEqualOne(s string) bool { + return s != "" // want `use s != "" to check for non-empty string instead of len\(s\) >= 1` +} + +func yodaLenLessThanOne(s string) bool { + return s == "" // want `use s == "" to check for empty string instead of len\(s\) < 1` +} + +func yodaLenLessOrEqualZero(s string) bool { + return 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 } @@ -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` +} + 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 } + From 0a9208039b95db6bc4b9b60ddfa54dde284e7b28 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 21 Jun 2026 14:31:42 +0000 Subject: [PATCH 3/3] docs(adr): add draft ADR-40621 for lenstringzero relational comparisons --- ...ro-to-relational-len-string-comparisons.md | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 docs/adr/40621-extend-lenstringzero-to-relational-len-string-comparisons.md diff --git a/docs/adr/40621-extend-lenstringzero-to-relational-len-string-comparisons.md b/docs/adr/40621-extend-lenstringzero-to-relational-len-string-comparisons.md new file mode 100644 index 00000000000..4f4b32721ce --- /dev/null +++ b/docs/adr/40621-extend-lenstringzero-to-relational-len-string-comparisons.md @@ -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.*