Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions docs/adr/37750-track-len-string-aliases-in-lenstringzero.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# ADR-37750: Track local `len(string)` aliases in the `lenstringzero` linter

**Date**: 2026-06-08
**Status**: Draft

## Context

The `lenstringzero` analysis-pass linter (introduced in [ADR-37618](37618-add-lenstringzero-linter.md)) flags the non-idiomatic `len(s) == 0` / `len(s) != 0` pattern on **string** values, recommending `s == ""` / `s != ""` instead. ADR-37618 explicitly recorded a known false-negative as a Negative consequence: an equivalent length stored in an intermediate variable (`n := len(s); n == 0`) was not detected. This PR closes that gap. The challenge is that an aliased length is only equivalent to the direct form until the alias is mutated, so naive aliasing would produce false positives on reassigned or incremented variables.

## Decision

We will extend `lenstringzero` with a pre-pass that records local objects initialized from `len(<string-expr>)` — via short declarations (`:=`) and `var` specs (`ValueSpec`) — into a `map[types.Object]ast.Expr` keyed by the resolved type-checker object. The existing binary-expression matcher then treats a tracked alias compared against the integer literal `0` (in both operand orders) the same as a direct `len(s)` comparison, emitting identical diagnostics. To preserve soundness, aliases are invalidated (deleted from the map) on any mutation: reassignment (`=`), increment/decrement (`++`/`--`), and range-assignment targets. Type restriction to underlying `string` is reused so non-string aliases (`[]byte`, arrays) remain unflagged.

## Alternatives Considered

### Alternative 1: Full dataflow / SSA-based analysis
A control-flow-aware analysis (e.g. `go/ssa`) could track value provenance precisely across branches and scopes. It was not chosen because it is substantially heavier than the existing single-`inspect`-pass design of every linter in `pkg/linters/`, and the targeted alias-then-compare pattern is handled adequately by a syntactic pre-pass with conservative mutation invalidation.

### Alternative 2: Leave the limitation documented and unaddressed
ADR-37618 already disclosed the alias false-negative as an accepted trade-off, so doing nothing was a valid option. It was not chosen because the alias form is a common, easily-detectable variant whose omission noticeably weakens the linter's value, and the conservative pre-pass approach adds the coverage without meaningful false-positive risk.

## Consequences

### Positive
- Detects the previously-missed `n := len(s); n == 0` alias form, closing the false-negative documented in ADR-37618.
- Mutation invalidation (`=`, `++`/`--`, range targets) prevents stale-alias false positives after the value changes.
- Reuses the existing underlying-`string` type check, so `[]byte`/array aliases stay correctly unflagged; diagnostic wording is unchanged.

### Negative
- Adds ~150 lines of alias-collection and invalidation logic to maintain, increasing the linter's surface area and complexity.
- The pre-pass is syntactic, not flow-sensitive: it does not model branch-conditional reassignment or aliasing through additional intermediate variables, so some edge cases remain out of scope.
- Alias tracking is whole-file and keyed by `types.Object`; correctness depends on accurate object resolution and the completeness of the mutation-invalidation cases.

### Neutral
- Detection remains string-only and limited to comparisons against the literal `0`; other operators and non-zero literals are still intentionally excluded.
- The diagnostic stays report-only (no suggested-fix/autofix is emitted).
- Test fixtures were extended with positive (alias `== 0`/`!= 0`) and negative (reassigned, incremented, non-string) cases, consistent with sibling linters.

---

*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27118730160) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*
152 changes: 152 additions & 0 deletions pkg/linters/lenstringzero/lenstringzero.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func run(pass *analysis.Pass) (any, error) {
if !ok {
return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
}
lenStringAliases := collectLenStringAliases(pass)

nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)}

Expand All @@ -50,6 +51,14 @@ func run(pass *analysis.Pass) (any, error) {
lenArg = lenCallArg(expr.X)
} else if isIntZero(expr.X) && isLenCall(expr.Y) {
lenArg = lenCallArg(expr.Y)
} 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
}
}
if lenArg == nil {
return
Expand Down Expand Up @@ -96,3 +105,146 @@ func isIntZero(expr ast.Expr) bool {
lit, ok := expr.(*ast.BasicLit)
return ok && lit.Kind == token.INT && lit.Value == "0"
}

func collectLenStringAliases(pass *analysis.Pass) map[types.Object]ast.Expr {
aliases := make(map[types.Object]ast.Expr)
for _, file := range pass.Files {
ast.Inspect(file, func(node ast.Node) bool {
switch n := node.(type) {
case *ast.AssignStmt:
collectLenStringAliasesFromAssignStmt(pass, n, aliases)
case *ast.ValueSpec:
collectLenStringAliasesFromValueSpec(pass, n, aliases)
case *ast.IncDecStmt:
if ident, ok := n.X.(*ast.Ident); ok {
delete(aliases, pass.TypesInfo.ObjectOf(ident))
}
case *ast.RangeStmt:
if n.Tok == token.ASSIGN {
deleteLenStringAliasForExpr(pass, aliases, n.Key)
deleteLenStringAliasForExpr(pass, aliases, n.Value)
}
}
return true
})
}
return aliases
}

func collectLenStringAliasesFromAssignStmt(pass *analysis.Pass, stmt *ast.AssignStmt, aliases map[types.Object]ast.Expr) {
for i, lhs := range stmt.Lhs {
ident, ok := lhs.(*ast.Ident)
if !ok || ident.Name == "_" {
continue
}
obj := pass.TypesInfo.ObjectOf(ident)
if obj == nil || !isLocalObject(obj) {
continue
}

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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
}

}
Comment on lines +145 to +163
}
}

func collectLenStringAliasesFromValueSpec(pass *analysis.Pass, spec *ast.ValueSpec, aliases map[types.Object]ast.Expr) {
for i, name := range spec.Names {
if name.Name == "_" {
continue
}
obj := pass.TypesInfo.ObjectOf(name)
if obj == nil || !isLocalObject(obj) {
continue
}
rhs, ok := rhsExprForIndex(spec.Values, i)
if !ok {
delete(aliases, obj)
continue
}
if arg, ok := lenStringArg(pass, rhs); ok {
aliases[obj] = arg
} else {
delete(aliases, obj)
}
}
}

func lenAliasArg(pass *analysis.Pass, expr ast.Expr, aliases map[types.Object]ast.Expr) (ast.Expr, bool) {
ident, ok := expr.(*ast.Ident)
if !ok {
return nil, false
}
obj := pass.TypesInfo.ObjectOf(ident)
if obj == nil {
return nil, false
}
arg, ok := aliases[obj]
if !ok {
return nil, false
}
return arg, true
}

func lenStringArg(pass *analysis.Pass, expr ast.Expr) (ast.Expr, bool) {
if !isLenCall(expr) {
return nil, false
}
arg := lenCallArg(expr)
t := pass.TypesInfo.TypeOf(arg)
if t == nil {
return nil, false
}
basic, ok := t.Underlying().(*types.Basic)
if !ok || basic.Kind() != types.String {
return nil, false
}
return arg, true
}

func rhsExprForIndex(rhs []ast.Expr, idx int) (ast.Expr, bool) {
switch {
case len(rhs) == 0:
return nil, false
case idx < len(rhs):
return rhs[idx], true
default:
return nil, false
}
}

func deleteLenStringAliasForExpr(pass *analysis.Pass, aliases map[types.Object]ast.Expr, expr ast.Expr) {
ident, ok := expr.(*ast.Ident)
if !ok {
return
}
delete(aliases, pass.TypesInfo.ObjectOf(ident))
}

func isLocalObject(obj types.Object) bool {
if obj == nil {
return false
}
parent := obj.Parent()
if parent == nil {
return false
}
pkg := obj.Pkg()
return pkg == nil || parent != pkg.Scope()
}
Original file line number Diff line number Diff line change
@@ -1,41 +1,73 @@
package lenstringzero

func isEmpty(s string) bool {
return len(s) == 0 // want `use s == "" to check for empty string instead of len\(s\) == 0`
return len(s) == 0 // want `use s == "" to check for empty string instead of len\(s\) == 0`
}

func isNotEmpty(s string) bool {
return len(s) != 0 // want `use s != "" to check for non-empty string instead of len\(s\) != 0`
return len(s) != 0 // want `use s != "" to check for non-empty string instead of len\(s\) != 0`
}

func flippedEmpty(s string) bool {
return 0 == len(s) // want `use s == "" to check for empty string instead of len\(s\) == 0`
return 0 == len(s) // want `use s == "" to check for empty string instead of len\(s\) == 0`
}

func flippedNotEmpty(s string) bool {
return 0 != len(s) // want `use s != "" to check for non-empty string instead of len\(s\) != 0`
return 0 != len(s) // want `use s != "" to check for non-empty string instead of len\(s\) != 0`
}

func alreadyGoodEmpty(s string) bool {
return s == ""
return s == ""
}

func alreadyGoodNotEmpty(s string) bool {
return s != ""
return s != ""
}

func sliceNotFlagged(s []byte) bool {
return len(s) == 0
return len(s) == 0
}

func arrayNotFlagged(s [1]byte) bool {
return len(s) != 0
return len(s) != 0
}

func lenNotZeroOp(s string) bool {
return len(s) > 0
return len(s) > 0
}

func lenNotComparedToZero(s string) bool {
return len(s) == 1
return len(s) == 1
}

func aliasEmpty(s string) bool {
n := len(s)
return n == 0 // want `use s == "" to check for empty string instead of len\(s\) == 0`
}

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`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 aliasReassignedNotFlagged(s string) bool {
n := len(s)
n = 1
return n == 0
}

func aliasIncrementedNotFlagged(s string) bool {
n := len(s)
n++
return n == 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 sliceAliasNotFlagged(s []byte) bool {
n := len(s)
return n == 0
}

func arrayAliasNotFlagged(s [1]byte) bool {
n := len(s)
return n == 0
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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`
}

Loading