From 8a9b677c40848df629e8990cedb7995c583d59cf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 07:02:07 +0000 Subject: [PATCH 1/6] Initial plan From 7baefaffd099bf6821adfca27d1ae0b192071f6f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 07:17:56 +0000 Subject: [PATCH 2/6] fix(tolowerequalfold): guard against case-mismatched literals and mixed ToLower/ToUpper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The linter previously flagged comparisons like: strings.ToLower(x) == "UPPER" // always false, not EqualFold strings.ToUpper(x) == "lower" // always false, not EqualFold strings.ToLower(a) == strings.ToUpper(b) // mismatched functions and emitted a suggested fix that silently changed behavior by rewriting them to EqualFold. The fix adds: - caseConvFuncAndArg: combined func-name + arg extraction - caseConvFuncName: returns "ToLower" or "ToUpper" for a call node - stringLitValue: unquotes a BasicLit string - literalCaseMatchesConv: checks literal is already in the correct case - caseConvIsCompatible: main guard for direct calls - caseConvAliasIsCompatible: guard for aliased variables The trigger condition now only fires when: 1. Literal operand: the literal is already in the correct case for the conversion (ToLower→lowercase, ToUpper→uppercase). 2. Two conversions: both sides use the same function (ToLower==ToLower or ToUpper==ToUpper). Also adds the caseConvAliasInfo struct so the alias tracking map stores both the function name and the argument, enabling the alias guard. New negative test fixtures lock in the fixed behavior: strings.ToLower(name) == "ALICE" → no diagnostic strings.ToUpper(name) == "alice" → no diagnostic "ALICE" == strings.ToLower(name) → no diagnostic strings.ToLower(name) == strings.ToUpper(name) → no diagnostic lowerName == "ALICE" (alias case) → no diagnostic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../src/tolowerequalfold/alias_shadow.go | 11 ++ .../tolowerequalfold/alias_shadow.go.golden | 11 ++ .../src/tolowerequalfold/tolowerequalfold.go | 15 ++ .../tolowerequalfold.go.golden | 15 ++ .../tolowerequalfold/tolowerequalfold.go | 154 ++++++++++++++---- 5 files changed, 177 insertions(+), 29 deletions(-) diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go index b8d89ff8f04..5f5d1f3aac4 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go @@ -21,6 +21,17 @@ func aliasImportTrackedExamples() { _ = "ALICE" == y // want `use strings\.EqualFold` } +func aliasImportMismatchedExamples() { + a := "Alice" + + // Alias with case-mismatched literal — must not be rewritten to EqualFold. + x := str.ToLower(a) + _ = x == "ALICE" + + y := str.ToUpper(a) + _ = "alice" == y +} + type shadowStrings struct{} func (shadowStrings) ToLower(s string) string { diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden index f386f6b7f8f..be1b00ae4c2 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden @@ -21,6 +21,17 @@ func aliasImportTrackedExamples() { _ = "ALICE" == y // want `use strings\.EqualFold` } +func aliasImportMismatchedExamples() { + a := "Alice" + + // Alias with case-mismatched literal — must not be rewritten to EqualFold. + x := str.ToLower(a) + _ = x == "ALICE" + + y := str.ToUpper(a) + _ = "alice" == y +} + type shadowStrings struct{} func (shadowStrings) ToLower(s string) string { diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go index bd9d1061340..8972d1f3475 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go @@ -31,6 +31,21 @@ func okExamples() { lower := strings.ToLower(name) _ = lower == name + + // Case-mismatched literal: ToLower output can never equal an uppercase + // literal, so the comparison is always false — not a case-insensitive + // equality check and must not be rewritten to EqualFold. + _ = strings.ToLower(name) == "ALICE" + _ = strings.ToUpper(name) == "alice" + _ = "ALICE" == strings.ToLower(name) + + // Mixed ToLower/ToUpper: lower(a)==upper(b) is false for any letters, + // not a case-insensitive equality — must not be rewritten to EqualFold. + _ = strings.ToLower(name) == strings.ToUpper(name) + + // Alias with case-mismatched literal — same reasoning as above. + lowerName := strings.ToLower(name) + _ = lowerName == "ALICE" } func suppressedExamples() { diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden index f5a61bdb01e..df283c20938 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden @@ -31,6 +31,21 @@ func okExamples() { lower := strings.ToLower(name) _ = lower == name + + // Case-mismatched literal: ToLower output can never equal an uppercase + // literal, so the comparison is always false — not a case-insensitive + // equality check and must not be rewritten to EqualFold. + _ = strings.ToLower(name) == "ALICE" + _ = strings.ToUpper(name) == "alice" + _ = "ALICE" == strings.ToLower(name) + + // Mixed ToLower/ToUpper: lower(a)==upper(b) is false for any letters, + // not a case-insensitive equality — must not be rewritten to EqualFold. + _ = strings.ToLower(name) == strings.ToUpper(name) + + // Alias with case-mismatched literal — same reasoning as above. + lowerName := strings.ToLower(name) + _ = lowerName == "ALICE" } func suppressedExamples() { diff --git a/pkg/linters/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/tolowerequalfold.go index 9bd0a693c4f..d4ed915bbe3 100644 --- a/pkg/linters/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/tolowerequalfold.go @@ -8,6 +8,8 @@ import ( "go/ast" "go/token" "go/types" + "strconv" + "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -64,9 +66,10 @@ func run(pass *analysis.Pass) (any, error) { return } - if isCaseConvCall(pass, expr.X) || isCaseConvCall(pass, expr.Y) || - (isCaseConvAlias(pass, expr.X, caseConvAliases) && astutil.IsStringLiteral(expr.Y)) || - (isCaseConvAlias(pass, expr.Y, caseConvAliases) && astutil.IsStringLiteral(expr.X)) { + if (isCaseConvCall(pass, expr.X) && caseConvIsCompatible(pass, expr.X, expr.Y)) || + (isCaseConvCall(pass, expr.Y) && caseConvIsCompatible(pass, expr.Y, expr.X)) || + (isCaseConvAlias(pass, expr.X, caseConvAliases) && astutil.IsStringLiteral(expr.Y) && caseConvAliasIsCompatible(pass, expr.X, expr.Y, caseConvAliases)) || + (isCaseConvAlias(pass, expr.Y, caseConvAliases) && astutil.IsStringLiteral(expr.X) && caseConvAliasIsCompatible(pass, expr.Y, expr.X, caseConvAliases)) { if nolint.HasDirective(pass.Fset.PositionFor(expr.Pos(), false), noLintLinesByFile) { return } @@ -128,8 +131,15 @@ func buildEqualFoldFix(pass *analysis.Pass, expr *ast.BinaryExpr) []analysis.Sug }} } -func collectCaseConvAliases(pass *analysis.Pass) map[types.Object]ast.Expr { - aliases := make(map[types.Object]ast.Expr) +// caseConvAliasInfo records the case-conversion function and its argument for +// a local variable that aliases a strings.ToLower/ToUpper call. +type caseConvAliasInfo struct { + funcName string // "ToLower" or "ToUpper" + arg ast.Expr +} + +func collectCaseConvAliases(pass *analysis.Pass) map[types.Object]caseConvAliasInfo { + aliases := make(map[types.Object]caseConvAliasInfo) for _, file := range pass.Files { ast.Inspect(file, func(node ast.Node) bool { switch n := node.(type) { @@ -153,7 +163,7 @@ func collectCaseConvAliases(pass *analysis.Pass) map[types.Object]ast.Expr { return aliases } -func collectAliasesFromAssignStmt(pass *analysis.Pass, stmt *ast.AssignStmt, aliases map[types.Object]ast.Expr) { +func collectAliasesFromAssignStmt(pass *analysis.Pass, stmt *ast.AssignStmt, aliases map[types.Object]caseConvAliasInfo) { for i, lhs := range stmt.Lhs { ident, ok := lhs.(*ast.Ident) if !ok || ident.Name == "_" { @@ -175,18 +185,19 @@ func collectAliasesFromAssignStmt(pass *analysis.Pass, stmt *ast.AssignStmt, ali delete(aliases, obj) continue } - if arg, ok := caseConvArg(pass, rhs); ok { - aliases[obj] = arg - } else { + funcName, arg, ok := caseConvFuncAndArg(pass, rhs) + if !ok { delete(aliases, obj) + continue } + aliases[obj] = caseConvAliasInfo{funcName: funcName, arg: arg} case token.ASSIGN: delete(aliases, obj) } } } -func collectAliasesFromValueSpec(pass *analysis.Pass, spec *ast.ValueSpec, aliases map[types.Object]ast.Expr) { +func collectAliasesFromValueSpec(pass *analysis.Pass, spec *ast.ValueSpec, aliases map[types.Object]caseConvAliasInfo) { for i, name := range spec.Names { if name.Name == "_" { continue @@ -200,15 +211,16 @@ func collectAliasesFromValueSpec(pass *analysis.Pass, spec *ast.ValueSpec, alias delete(aliases, obj) continue } - if arg, ok := caseConvArg(pass, rhs); ok { - aliases[obj] = arg - } else { + funcName, arg, ok := caseConvFuncAndArg(pass, rhs) + if !ok { delete(aliases, obj) + continue } + aliases[obj] = caseConvAliasInfo{funcName: funcName, arg: arg} } } -func deleteAliasForExpr(pass *analysis.Pass, aliases map[types.Object]ast.Expr, expr ast.Expr) { +func deleteAliasForExpr(pass *analysis.Pass, aliases map[types.Object]caseConvAliasInfo, expr ast.Expr) { ident, ok := expr.(*ast.Ident) if !ok { return @@ -222,12 +234,12 @@ func isCaseConvCall(pass *analysis.Pass, n ast.Node) bool { return ok } -func isCaseConvAlias(pass *analysis.Pass, expr ast.Expr, aliases map[types.Object]ast.Expr) bool { +func isCaseConvAlias(pass *analysis.Pass, expr ast.Expr, aliases map[types.Object]caseConvAliasInfo) bool { _, ok := caseConvAliasArg(pass, expr, aliases) return ok } -func caseConvAliasArg(pass *analysis.Pass, expr ast.Expr, aliases map[types.Object]ast.Expr) (ast.Expr, bool) { +func caseConvAliasArg(pass *analysis.Pass, expr ast.Expr, aliases map[types.Object]caseConvAliasInfo) (ast.Expr, bool) { ident, ok := expr.(*ast.Ident) if !ok { return nil, false @@ -236,33 +248,117 @@ func caseConvAliasArg(pass *analysis.Pass, expr ast.Expr, aliases map[types.Obje if obj == nil { return nil, false } - arg, ok := aliases[obj] + info, ok := aliases[obj] if !ok { return nil, false } - return arg, true + return info.arg, true +} + +// caseConvFuncAndArg returns the function name ("ToLower" or "ToUpper") and +// the argument when n is a direct strings.ToLower/ToUpper call. +func caseConvFuncAndArg(pass *analysis.Pass, n ast.Node) (funcName string, arg ast.Expr, ok bool) { + call, callOK := n.(*ast.CallExpr) + if !callOK { + return "", nil, false + } + if len(call.Args) != 1 { + return "", nil, false + } + sel, selOK := call.Fun.(*ast.SelectorExpr) + if !selOK { + return "", nil, false + } + if !astutil.IsPkgSelector(pass, sel, "strings") { + return "", nil, false + } + if sel.Sel.Name != "ToLower" && sel.Sel.Name != "ToUpper" { + return "", nil, false + } + return sel.Sel.Name, call.Args[0], true } // caseConvArg returns the argument when n is strings.ToLower/ToUpper(). func caseConvArg(pass *analysis.Pass, n ast.Node) (ast.Expr, bool) { - call, ok := n.(*ast.CallExpr) + _, arg, ok := caseConvFuncAndArg(pass, n) + return arg, ok +} + +// caseConvFuncName returns the function name ("ToLower" or "ToUpper") when n +// is a direct strings.ToLower/ToUpper call. +func caseConvFuncName(pass *analysis.Pass, n ast.Node) (string, bool) { + name, _, ok := caseConvFuncAndArg(pass, n) + return name, ok +} + +// stringLitValue returns the unquoted string value of a string-literal AST node. +func stringLitValue(expr ast.Expr) (string, bool) { + lit, ok := expr.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return "", false + } + s, err := strconv.Unquote(lit.Value) + if err != nil { + return "", false + } + return s, true +} + +// literalCaseMatchesConv reports whether lit is already in the correct case for +// funcName (i.e. applying the conversion to lit is a no-op). Only when this +// holds is "conv(x) == lit" semantically equivalent to "EqualFold(x, lit)". +func literalCaseMatchesConv(funcName, lit string) bool { + switch funcName { + case "ToLower": + return strings.ToLower(lit) == lit + case "ToUpper": + return strings.ToUpper(lit) == lit + } + return true +} + +// caseConvIsCompatible reports whether it is safe to rewrite a comparison +// where convSide is a case-conversion call and otherSide is the other operand. +// Returns false when: +// - otherSide is a string literal whose case does not match the conversion, or +// - otherSide is a case-conversion call using a different function. +func caseConvIsCompatible(pass *analysis.Pass, convSide ast.Node, otherSide ast.Expr) bool { + funcName, ok := caseConvFuncName(pass, convSide) if !ok { - return nil, false + return false } - if len(call.Args) != 1 { - return nil, false + // String-literal operand: the literal must already be in the correct case. + if lit, ok := stringLitValue(otherSide); ok { + return literalCaseMatchesConv(funcName, lit) } - sel, ok := call.Fun.(*ast.SelectorExpr) + // Case-conversion operand: both sides must use the same function. + if otherFunc, ok := caseConvFuncName(pass, otherSide); ok { + return funcName == otherFunc + } + // Variable or other expression: safe to rewrite. + return true +} + +// caseConvAliasIsCompatible reports whether it is safe to rewrite a comparison +// where aliasExpr is a case-conversion alias and litExpr is a string literal. +func caseConvAliasIsCompatible(pass *analysis.Pass, aliasExpr ast.Expr, litExpr ast.Expr, aliases map[types.Object]caseConvAliasInfo) bool { + ident, ok := aliasExpr.(*ast.Ident) if !ok { - return nil, false + return false } - if !astutil.IsPkgSelector(pass, sel, "strings") { - return nil, false + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + return false } - if sel.Sel.Name != "ToLower" && sel.Sel.Name != "ToUpper" { - return nil, false + info, ok := aliases[obj] + if !ok { + return false + } + lit, ok := stringLitValue(litExpr) + if !ok { + return false } - return call.Args[0], true + return literalCaseMatchesConv(info.funcName, lit) } func caseConvPkgName(pass *analysis.Pass, n ast.Node) (string, bool) { From 3aaf78cc0aac24635e620ab7dcbd7deb66be5d68 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 07:19:08 +0000 Subject: [PATCH 3/6] refactor(tolowerequalfold): address code review feedback - Change literalCaseMatchesConv default to return false (fail-safe for unknown function names) - Extract trigger condition into isEquivalentToEqualFold helper for readability Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../tolowerequalfold/tolowerequalfold.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/pkg/linters/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/tolowerequalfold.go index d4ed915bbe3..33d133f1f31 100644 --- a/pkg/linters/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/tolowerequalfold.go @@ -66,10 +66,7 @@ func run(pass *analysis.Pass) (any, error) { return } - if (isCaseConvCall(pass, expr.X) && caseConvIsCompatible(pass, expr.X, expr.Y)) || - (isCaseConvCall(pass, expr.Y) && caseConvIsCompatible(pass, expr.Y, expr.X)) || - (isCaseConvAlias(pass, expr.X, caseConvAliases) && astutil.IsStringLiteral(expr.Y) && caseConvAliasIsCompatible(pass, expr.X, expr.Y, caseConvAliases)) || - (isCaseConvAlias(pass, expr.Y, caseConvAliases) && astutil.IsStringLiteral(expr.X) && caseConvAliasIsCompatible(pass, expr.Y, expr.X, caseConvAliases)) { + if isEquivalentToEqualFold(pass, expr, caseConvAliases) { if nolint.HasDirective(pass.Fset.PositionFor(expr.Pos(), false), noLintLinesByFile) { return } @@ -314,7 +311,18 @@ func literalCaseMatchesConv(funcName, lit string) bool { case "ToUpper": return strings.ToUpper(lit) == lit } - return true + return false +} + +// isEquivalentToEqualFold reports whether the == or != comparison expr is +// semantically equivalent to a strings.EqualFold rewrite. It returns true only +// when at least one side is a case-conversion call (or alias) and the other +// operand is case-compatible with that conversion. +func isEquivalentToEqualFold(pass *analysis.Pass, expr *ast.BinaryExpr, caseConvAliases map[types.Object]caseConvAliasInfo) bool { + return (isCaseConvCall(pass, expr.X) && caseConvIsCompatible(pass, expr.X, expr.Y)) || + (isCaseConvCall(pass, expr.Y) && caseConvIsCompatible(pass, expr.Y, expr.X)) || + (isCaseConvAlias(pass, expr.X, caseConvAliases) && astutil.IsStringLiteral(expr.Y) && caseConvAliasIsCompatible(pass, expr.X, expr.Y, caseConvAliases)) || + (isCaseConvAlias(pass, expr.Y, caseConvAliases) && astutil.IsStringLiteral(expr.X) && caseConvAliasIsCompatible(pass, expr.Y, expr.X, caseConvAliases)) } // caseConvIsCompatible reports whether it is safe to rewrite a comparison From d4abe8d88c719acdc4f196109f0cb860d9303b21 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 11 Jul 2026 07:35:02 +0000 Subject: [PATCH 4/6] docs(adr): draft ADR-44888 for tolowerequalfold case-compatibility guard --- ...lowerequalfold-case-compatibility-guard.md | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 docs/adr/44888-tolowerequalfold-case-compatibility-guard.md diff --git a/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md b/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md new file mode 100644 index 00000000000..5beb15b5a28 --- /dev/null +++ b/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md @@ -0,0 +1,59 @@ +# ADR-44888: Guard tolowerequalfold Linter Against Case-Incompatible Rewrites + +**Date**: 2026-07-11 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The `tolowerequalfold` custom linter flags comparisons of the form `strings.ToLower(x) == y` and rewrites them to `strings.EqualFold(x, y)`. The original trigger condition was purely structural: it fired whenever one operand was a `strings.ToLower` or `strings.ToUpper` call (or a local variable aliasing such a call) and the other was any expression. This caused two classes of incorrect rewrites: + +1. **Case-mismatched literals**: `strings.ToLower(name) == "ALICE"` is always false (the output of `ToLower` can never equal an uppercase string), so it is dead code — not a case-insensitive equality check. Rewriting it to `strings.EqualFold(name, "ALICE")` silently converts dead code into a live check. +2. **Mixed conversion functions**: `strings.ToLower(a) == strings.ToUpper(b)` is always false for any string containing letters, for the same reason. Rewriting to `EqualFold` introduces a behavioral change. + +Both rewrites violate the invariant that an autofix must be behavior-preserving. + +### Decision + +We will add semantic compatibility guards to the linter's trigger condition. A comparison is flagged for EqualFold rewriting only when the operands are genuinely case-equivalent: + +- If the non-conversion operand is a string literal, the literal must already be in the correct case for the conversion function (all-lowercase for `ToLower`, all-uppercase for `ToUpper`). +- If both operands are case-conversion calls, they must use the same function (`ToLower`/`ToLower` or `ToUpper`/`ToUpper`). +- These guards apply to both direct calls and local-variable aliases. + +The internal alias map type is changed from `map[types.Object]ast.Expr` to `map[types.Object]caseConvAliasInfo` to carry the function name alongside the argument, enabling alias-level guards. + +### Alternatives Considered + +#### Alternative 1: Suppress only mixed-case literals + +Guard only against literals that contain both upper and lowercase characters (e.g., "Alice"). All-caps or all-lowercase literals paired with the wrong conversion function would still trigger the diagnostic. + +Rejected because it does not cover the primary bug: `strings.ToLower(name) == "ALICE"` uses an all-uppercase literal and would still be incorrectly rewritten. The guard must be based on whether the literal's case is invariant under the conversion, not on whether it is "mixed case." + +#### Alternative 2: Require explicit nolint suppression for edge cases + +Keep the current permissive trigger and require users to annotate problematic patterns with a `//nolint:tolowerequalfold` directive when they know the comparison is dead code. + +Rejected because it puts the burden of correctness on every consumer of the linter rather than making the linter correct by default. A linter whose autofix can silently introduce bugs is more dangerous than no linter at all, especially in automated fix workflows. + +### Consequences + +#### Positive +- The linter no longer silently converts always-false (dead-code) comparisons into live case-insensitive checks. +- Mixed `ToLower`/`ToUpper` comparisons are correctly excluded from the diagnostic, preserving existing behavior. +- Negative test fixtures lock in the new behavior and prevent regression. + +#### Negative +- The trigger condition is now more complex: the new `isEquivalentToEqualFold` function delegates to several helper functions (`caseConvIsCompatible`, `caseConvAliasIsCompatible`, `literalCaseMatchesConv`, `stringLitValue`, `caseConvFuncAndArg`), adding ~90 lines to the linter. +- The `caseConvAliasInfo` struct change is a breaking internal refactor: all functions accepting `map[types.Object]ast.Expr` must be updated to `map[types.Object]caseConvAliasInfo`. + +#### Neutral +- The helper `caseConvFuncAndArg` is introduced as a single source of truth for extracting both the function name and argument from a conversion call; the existing `caseConvArg` and new `caseConvFuncName` become thin delegates to it. +- The guard uses Go's own `strings.ToLower`/`strings.ToUpper` at analysis time to determine whether a literal is in the correct case, ensuring the check is always consistent with the runtime behavior being linted. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 59457f204f65feb05b52fcddcb86e8aa90051053 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 08:05:43 +0000 Subject: [PATCH 5/6] fix(tolowerequalfold): fail closed on non-literal and unicode-unsafe rewrites Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- ...lowerequalfold-case-compatibility-guard.md | 28 ++++++++++--------- .../src/tolowerequalfold/alias_shadow.go | 10 +++++-- .../tolowerequalfold/alias_shadow.go.golden | 10 +++++-- .../src/tolowerequalfold/tolowerequalfold.go | 7 +++-- .../tolowerequalfold.go.golden | 6 +++- .../tolowerequalfold/tolowerequalfold.go | 24 ++++++++-------- 6 files changed, 54 insertions(+), 31 deletions(-) diff --git a/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md b/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md index 5beb15b5a28..f464da6a806 100644 --- a/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md +++ b/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md @@ -1,8 +1,8 @@ # ADR-44888: Guard tolowerequalfold Linter Against Case-Incompatible Rewrites **Date**: 2026-07-11 -**Status**: Draft -**Deciders**: Unknown +**Status**: Accepted +**Deciders**: gh-aw maintainers --- @@ -17,11 +17,12 @@ Both rewrites violate the invariant that an autofix must be behavior-preserving. ### Decision -We will add semantic compatibility guards to the linter's trigger condition. A comparison is flagged for EqualFold rewriting only when the operands are genuinely case-equivalent: +We will make the `tolowerequalfold` rewrite **fail closed** and only permit cases we can prove equivalent. A comparison is flagged for EqualFold rewriting only when: -- If the non-conversion operand is a string literal, the literal must already be in the correct case for the conversion function (all-lowercase for `ToLower`, all-uppercase for `ToUpper`). -- If both operands are case-conversion calls, they must use the same function (`ToLower`/`ToLower` or `ToUpper`/`ToUpper`). -- These guards apply to both direct calls and local-variable aliases. +- one side is a `strings.ToLower`/`strings.ToUpper` call (or tracked alias), and +- the other side is an **ASCII string literal** that is already in the matching case (`ToLower` ↔ lowercase literal, `ToUpper` ↔ uppercase literal). + +All other operand shapes are rejected (unknown variables, other function calls, and conversion-vs-conversion comparisons, including same-function pairs). This conservative choice avoids Unicode simple-fold edge cases (for example Greek sigma forms) where `ToLower`/`ToUpper` equality can diverge from `strings.EqualFold`. The internal alias map type is changed from `map[types.Object]ast.Expr` to `map[types.Object]caseConvAliasInfo` to carry the function name alongside the argument, enabling alias-level guards. @@ -33,27 +34,28 @@ Guard only against literals that contain both upper and lowercase characters (e. Rejected because it does not cover the primary bug: `strings.ToLower(name) == "ALICE"` uses an all-uppercase literal and would still be incorrectly rewritten. The guard must be based on whether the literal's case is invariant under the conversion, not on whether it is "mixed case." -#### Alternative 2: Require explicit nolint suppression for edge cases +#### Alternative 2: Allow same-function conversion pairs (`ToLower(a)==ToLower(b)`) -Keep the current permissive trigger and require users to annotate problematic patterns with a `//nolint:tolowerequalfold` directive when they know the comparison is dead code. +Treat matching conversion functions as sufficient evidence of equivalence and continue rewriting those comparisons. -Rejected because it puts the burden of correctness on every consumer of the linter rather than making the linter correct by default. A linter whose autofix can silently introduce bugs is more dangerous than no linter at all, especially in automated fix workflows. +Rejected because Go Unicode semantics make this unsafe in general (`strings.ToLower("ς") != strings.ToLower("σ")` while `strings.EqualFold("ς", "σ")` is true). This would still permit behavior-changing rewrites. ### Consequences #### Positive - The linter no longer silently converts always-false (dead-code) comparisons into live case-insensitive checks. -- Mixed `ToLower`/`ToUpper` comparisons are correctly excluded from the diagnostic, preserving existing behavior. +- Mixed `ToLower`/`ToUpper` and same-function conversion-pair comparisons are excluded, preserving behavior across Unicode edge cases. - Negative test fixtures lock in the new behavior and prevent regression. #### Negative -- The trigger condition is now more complex: the new `isEquivalentToEqualFold` function delegates to several helper functions (`caseConvIsCompatible`, `caseConvAliasIsCompatible`, `literalCaseMatchesConv`, `stringLitValue`, `caseConvFuncAndArg`), adding ~90 lines to the linter. +- The rule becomes stricter and emits fewer diagnostics than before, intentionally favoring correctness over aggressiveness. +- The trigger condition is now more complex: `isEquivalentToEqualFold` delegates to compatibility helpers (`caseConvIsCompatible`, `caseConvAliasIsCompatible`, `literalCaseMatchesConv`, `stringLitValue`, `caseConvFuncAndArg`). - The `caseConvAliasInfo` struct change is a breaking internal refactor: all functions accepting `map[types.Object]ast.Expr` must be updated to `map[types.Object]caseConvAliasInfo`. #### Neutral - The helper `caseConvFuncAndArg` is introduced as a single source of truth for extracting both the function name and argument from a conversion call; the existing `caseConvArg` and new `caseConvFuncName` become thin delegates to it. -- The guard uses Go's own `strings.ToLower`/`strings.ToUpper` at analysis time to determine whether a literal is in the correct case, ensuring the check is always consistent with the runtime behavior being linted. +- The literal guard uses ASCII-only matching so the analyzer does not need full Unicode fold-class reasoning. --- -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* +*Finalized for PR #44888.* diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go index 5f5d1f3aac4..4902d2f16e4 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go @@ -6,8 +6,8 @@ func aliasImportExamples() { a := "Alice" b := "alice" - _ = str.ToLower(a) == str.ToLower(b) // want `use strings\.EqualFold` - _ = str.ToUpper(a) == str.ToUpper(b) // want `use strings\.EqualFold` + _ = str.ToLower(a) == str.ToLower(b) + _ = str.ToUpper(a) == str.ToUpper(b) } func aliasImportTrackedExamples() { @@ -23,6 +23,7 @@ func aliasImportTrackedExamples() { func aliasImportMismatchedExamples() { a := "Alice" + b := "Bob" // Alias with case-mismatched literal — must not be rewritten to EqualFold. x := str.ToLower(a) @@ -30,6 +31,11 @@ func aliasImportMismatchedExamples() { y := str.ToUpper(a) _ = "alice" == y + + // Alias-vs-alias with mismatched conversion functions must not be rewritten. + lower := str.ToLower(a) + upper := str.ToUpper(b) + _ = lower == upper } type shadowStrings struct{} diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden index be1b00ae4c2..4902d2f16e4 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/alias_shadow.go.golden @@ -6,8 +6,8 @@ func aliasImportExamples() { a := "Alice" b := "alice" - _ = str.EqualFold(a, b) // want `use strings\.EqualFold` - _ = str.EqualFold(a, b) // want `use strings\.EqualFold` + _ = str.ToLower(a) == str.ToLower(b) + _ = str.ToUpper(a) == str.ToUpper(b) } func aliasImportTrackedExamples() { @@ -23,6 +23,7 @@ func aliasImportTrackedExamples() { func aliasImportMismatchedExamples() { a := "Alice" + b := "Bob" // Alias with case-mismatched literal — must not be rewritten to EqualFold. x := str.ToLower(a) @@ -30,6 +31,11 @@ func aliasImportMismatchedExamples() { y := str.ToUpper(a) _ = "alice" == y + + // Alias-vs-alias with mismatched conversion functions must not be rewritten. + lower := str.ToLower(a) + upper := str.ToUpper(b) + _ = lower == upper } type shadowStrings struct{} diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go index 8972d1f3475..a08bae5dcd0 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go @@ -11,7 +11,6 @@ func flaggedExamples() { _ = strings.ToUpper(name) == "ALICE" // want `use strings\.EqualFold` _ = "alice" == strings.ToLower(name) // want `use strings\.EqualFold` _ = strings.ToLower(name) != "alice" // want `use strings\.EqualFold` - _ = strings.ToLower(name) == strings.ToLower("alice") // want `use strings\.EqualFold` lower := strings.ToLower(name) _ = lower == "alice" // want `use strings\.EqualFold` @@ -42,14 +41,18 @@ func okExamples() { // Mixed ToLower/ToUpper: lower(a)==upper(b) is false for any letters, // not a case-insensitive equality — must not be rewritten to EqualFold. _ = strings.ToLower(name) == strings.ToUpper(name) + _ = strings.ToLower(name) == strings.ToLower("alice") // Alias with case-mismatched literal — same reasoning as above. lowerName := strings.ToLower(name) _ = lowerName == "ALICE" + + // Unicode literals are conservatively excluded because ToLower/ToUpper + // equality may diverge from EqualFold semantics (e.g. Greek sigma forms). + _ = strings.ToLower(name) == "σ" } func suppressedExamples() { name := "Alice" _ = strings.ToLower(name) == "alice" //nolint:tolowerequalfold } - diff --git a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden index df283c20938..c6ce44333c7 100644 --- a/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden +++ b/pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go.golden @@ -11,7 +11,6 @@ func flaggedExamples() { _ = strings.EqualFold(name, "ALICE") // want `use strings\.EqualFold` _ = strings.EqualFold("alice", name) // want `use strings\.EqualFold` _ = !strings.EqualFold(name, "alice") // want `use strings\.EqualFold` - _ = strings.EqualFold(name, "alice") // want `use strings\.EqualFold` lower := strings.ToLower(name) _ = lower == "alice" // want `use strings\.EqualFold` @@ -42,10 +41,15 @@ func okExamples() { // Mixed ToLower/ToUpper: lower(a)==upper(b) is false for any letters, // not a case-insensitive equality — must not be rewritten to EqualFold. _ = strings.ToLower(name) == strings.ToUpper(name) + _ = strings.ToLower(name) == strings.ToLower("alice") // Alias with case-mismatched literal — same reasoning as above. lowerName := strings.ToLower(name) _ = lowerName == "ALICE" + + // Unicode literals are conservatively excluded because ToLower/ToUpper + // equality may diverge from EqualFold semantics (e.g. Greek sigma forms). + _ = strings.ToLower(name) == "σ" } func suppressedExamples() { diff --git a/pkg/linters/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/tolowerequalfold.go index 33d133f1f31..7a8c351c637 100644 --- a/pkg/linters/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/tolowerequalfold.go @@ -10,6 +10,7 @@ import ( "go/types" "strconv" "strings" + "unicode/utf8" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -302,9 +303,12 @@ func stringLitValue(expr ast.Expr) (string, bool) { } // literalCaseMatchesConv reports whether lit is already in the correct case for -// funcName (i.e. applying the conversion to lit is a no-op). Only when this -// holds is "conv(x) == lit" semantically equivalent to "EqualFold(x, lit)". +// funcName and uses ASCII-only letters. This conservative guard avoids Unicode +// simple-fold mismatches where ToLower/ToUpper equality and EqualFold differ. func literalCaseMatchesConv(funcName, lit string) bool { + if !isASCIIString(lit) { + return false + } switch funcName { case "ToLower": return strings.ToLower(lit) == lit @@ -314,6 +318,10 @@ func literalCaseMatchesConv(funcName, lit string) bool { return false } +func isASCIIString(s string) bool { + return utf8.ValidString(s) && len(s) == utf8.RuneCountInString(s) +} + // isEquivalentToEqualFold reports whether the == or != comparison expr is // semantically equivalent to a strings.EqualFold rewrite. It returns true only // when at least one side is a case-conversion call (or alias) and the other @@ -327,9 +335,8 @@ func isEquivalentToEqualFold(pass *analysis.Pass, expr *ast.BinaryExpr, caseConv // caseConvIsCompatible reports whether it is safe to rewrite a comparison // where convSide is a case-conversion call and otherSide is the other operand. -// Returns false when: -// - otherSide is a string literal whose case does not match the conversion, or -// - otherSide is a case-conversion call using a different function. +// Returns true only for ASCII string literals whose case already matches the +// conversion function. All other forms fail closed. func caseConvIsCompatible(pass *analysis.Pass, convSide ast.Node, otherSide ast.Expr) bool { funcName, ok := caseConvFuncName(pass, convSide) if !ok { @@ -339,12 +346,7 @@ func caseConvIsCompatible(pass *analysis.Pass, convSide ast.Node, otherSide ast. if lit, ok := stringLitValue(otherSide); ok { return literalCaseMatchesConv(funcName, lit) } - // Case-conversion operand: both sides must use the same function. - if otherFunc, ok := caseConvFuncName(pass, otherSide); ok { - return funcName == otherFunc - } - // Variable or other expression: safe to rewrite. - return true + return false } // caseConvAliasIsCompatible reports whether it is safe to rewrite a comparison From 47265dc69883b1926dbfbdeac8f9a8f364d73bbb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 08:14:00 +0000 Subject: [PATCH 6/6] docs(adr): finalize ADR-44888 and clarify conservative ASCII guard Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../44888-tolowerequalfold-case-compatibility-guard.md | 2 +- pkg/linters/tolowerequalfold/tolowerequalfold.go | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md b/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md index f464da6a806..3004a3eb02c 100644 --- a/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md +++ b/docs/adr/44888-tolowerequalfold-case-compatibility-guard.md @@ -38,7 +38,7 @@ Rejected because it does not cover the primary bug: `strings.ToLower(name) == "A Treat matching conversion functions as sufficient evidence of equivalence and continue rewriting those comparisons. -Rejected because Go Unicode semantics make this unsafe in general (`strings.ToLower("ς") != strings.ToLower("σ")` while `strings.EqualFold("ς", "σ")` is true). This would still permit behavior-changing rewrites. +Rejected because Go Unicode semantics make this unsafe in general (for example, `strings.ToLower("ς")` is `"ς"` and `strings.ToLower("σ")` is `"σ"`, while `strings.EqualFold("ς", "σ")` is `true`). This would still permit behavior-changing rewrites. ### Consequences diff --git a/pkg/linters/tolowerequalfold/tolowerequalfold.go b/pkg/linters/tolowerequalfold/tolowerequalfold.go index 7a8c351c637..5db3d03961c 100644 --- a/pkg/linters/tolowerequalfold/tolowerequalfold.go +++ b/pkg/linters/tolowerequalfold/tolowerequalfold.go @@ -10,7 +10,6 @@ import ( "go/types" "strconv" "strings" - "unicode/utf8" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -303,7 +302,7 @@ func stringLitValue(expr ast.Expr) (string, bool) { } // literalCaseMatchesConv reports whether lit is already in the correct case for -// funcName and uses ASCII-only letters. This conservative guard avoids Unicode +// funcName and uses ASCII-only characters. This conservative guard avoids Unicode // simple-fold mismatches where ToLower/ToUpper equality and EqualFold differ. func literalCaseMatchesConv(funcName, lit string) bool { if !isASCIIString(lit) { @@ -319,7 +318,12 @@ func literalCaseMatchesConv(funcName, lit string) bool { } func isASCIIString(s string) bool { - return utf8.ValidString(s) && len(s) == utf8.RuneCountInString(s) + for _, b := range []byte(s) { + if b > 0x7f { + return false + } + } + return true } // isEquivalentToEqualFold reports whether the == or != comparison expr is