fix(tolowerequalfold): reject case-mismatched literals and mixed ToLower/ToUpper comparisons#44888
Conversation
…ed ToLower/ToUpper 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>
- 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>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Adds compatibility guards to prevent unsafe strings.EqualFold diagnostics and autofixes.
Changes:
- Tracks conversion function names for direct calls and aliases.
- Adds literal-case and conversion-function compatibility checks.
- Adds negative fixtures for mismatched literals and conversions.
Show a summary per file
| File | Description |
|---|---|
tolowerequalfold.go |
Implements compatibility checks and alias metadata. |
tolowerequalfold.go.golden |
Adds expected outputs for negative cases. |
tolowerequalfold.go testdata |
Adds direct-call negative fixtures. |
alias_shadow.go.golden |
Adds alias-import expected outputs. |
alias_shadow.go |
Adds alias mismatch fixtures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Medium
| // Variable or other expression: safe to rewrite. | ||
| return true |
| func literalCaseMatchesConv(funcName, lit string) bool { | ||
| switch funcName { | ||
| case "ToLower": | ||
| return strings.ToLower(lit) == lit |
| // Case-conversion operand: both sides must use the same function. | ||
| if otherFunc, ok := caseConvFuncName(pass, otherSide); ok { | ||
| return funcName == otherFunc |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-structured. The three new guard functions (literalCaseMatchesConv, caseConvIsCompatible, caseConvAliasIsCompatible) cleanly separate the compatibility check from the detection logic, and the negative fixtures lock in the intended non-flagging behavior. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 22.8 AIC · ⌖ 4.93 AIC · ⊞ 4.8K
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (+185 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes for two correctness gaps found in the new guard logic.
📋 Key Themes & Highlights
Issues
- Alias-vs-alias fallthrough bug (line 347):
caseConvIsCompatibledoes not receive the aliases map, so a direct call compared to a mismatched alias (e.g.strings.ToLower(a) == upperAlias) takes the permissivereturn truepath — the same class of silent-dead-code-to-live-logic bug this PR is fixing. - Missing alias-vs-alias test (line 324–325): the alias branch in
isEquivalentToEqualFoldonly handles alias vs. string-literal;x := ToLower(a); y := ToUpper(b); _ = x == yis untested and its behavior unverified. - Missing same-function positive regression test (line 301): no fixture confirms that
ToLower(a) == ToLower(b)still fires the diagnostic after the refactor.
Positive Highlights
- ✅
literalCaseMatchesConv+caseConvFuncAndArgare clean, well-named, and easy to test in isolation - ✅ The
.goldenfiles correctly mirror source fixtures, validating autofix output - ✅ Negative fixture comments are thorough and explain why each pattern must not be rewritten
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 57.8 AIC · ⌖ 4.76 AIC · ⊞ 6.6K
Comment /matt to run again
| return funcName == otherFunc | ||
| } | ||
| // Variable or other expression: safe to rewrite. | ||
| return true |
There was a problem hiding this comment.
[/diagnosing-bugs] The fallthrough return true on this line is incorrect when otherSide is a case-conversion alias with a different function. For example, lowerX == y where y stores a strings.ToUpper call will reach here and be flagged as safe, silently converting a dead comparison into a live EqualFold call — the very bug this PR is fixing.
💡 Fix: check alias compatibility before returning true
// Variable or other expression: safe to rewrite — but reject a case-conv alias
// using a different function (e.g. lower == aliasUpper).
if isCaseConvAlias(pass, otherSide, /* aliases needed here */) {
aliasInfo, ok := /* look up alias info */
if ok && aliasInfo.funcName != funcName {
return false
}
}
return truecaseConvIsCompatible doesn't currently receive the aliases map, so this requires threading it through as a parameter. Alternatively, consider merging the alias-check into caseConvIsCompatible so all compatibility logic lives in one place.
@copilot please address this.
| 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)) |
There was a problem hiding this comment.
[/diagnosing-bugs] The alias branch in isEquivalentToEqualFold (line 324–325) only handles alias vs. string-literal comparisons — it misses alias vs. alias. For example, x := ToLower(a); y := ToUpper(b); _ = x == y would fall through this guard unchecked and never reach the alias conditions, so it would not fire the diagnostic. That case is different from direct calls on line 322–323.
💡 Suggested test to lock in the gap
Add to alias_shadow.go:
func aliasBothMismatchedExample() {
a, b := "Alice", "Bob"
x := str.ToLower(a)
y := str.ToUpper(b)
_ = x == y // must NOT be rewritten to EqualFold
}If this test emits a want comment the behavior is correct; if it doesn't, a missing check needs to be added to the alias-vs-alias path.
@copilot please address this.
| if err != nil { | ||
| return "", false | ||
| } | ||
| return s, true |
There was a problem hiding this comment.
[/tdd] There is no test case covering strings.ToLower(a) == strings.ToLower(b) (both direct calls, same function) to confirm it still fires the diagnostic after this refactor. The PR adds tests only for mismatched functions and literal mismatches; a regression here would be silent.
💡 Suggested fixture line to add to tolowerequalfold.go
// Same-function direct calls — must still fire.
_ = strings.ToLower(name) == strings.ToLower(other) // want `use strings\.EqualFold`@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — the compatibility guards introduced here are not sufficient to make the EqualFold rewrite semantically safe.
Blocking issues
literalCaseMatchesConv uses Go runtime case as a proxy for EqualFold equivalence (lines 274/276)
Go runtime strings.ToLower diverges from strings.EqualFold for certain Unicode pairs. For example, final sigma (U+03C2, "ς") and sigma (U+03C3, "σ") are EqualFold-equivalent but strings.ToLower("ς") == "σ" is false. A literal that passes the fixed-point check is not guaranteed to be EqualFold-equivalent to the conversion result.
Same-conversion guard is not sufficient (lines 311–312)
funcName == otherFunc ("both sides use ToLower") does not guarantee EqualFold safety. strings.ToLower("ς") != strings.ToLower("σ") yet strings.EqualFold("ς", "σ") is true. The rewrite changes semantics for Unicode inputs.
return true fallthrough reaches unknown call expressions (lines 314–315)
Any otherSide that is neither a string literal nor a strings.ToLower/ToUpper call silently falls through to return true, including unicode.ToLower(y), bytes.ToLower(...), and user-defined converters. These should fail safe rather than enable the diagnostic.
🔎 Code quality review by PR Code Quality Reviewer · 60.9 AIC · ⌖ 4.54 AIC · ⊞ 5.4K
Comment /review to run again
| return strings.ToUpper(lit) == lit | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Misleading fallthrough: non-strings-package call expressions are silently treated as safe to rewrite, which can produce incorrect EqualFold diagnostics.
💡 Details
The comment says "Variable or other expression: safe to rewrite" but this branch is reached for any otherSide that is not a string literal and not a strings.ToLower/ToUpper call — including calls to other packages (e.g. unicode.ToLower(y), user-defined converters).
For example:
strings.ToLower(x) == unicode.ToLower(y)caseConvFuncName(pass, otherSide) returns false for unicode.ToLower, so execution falls through to return true. The diagnostic fires and suggests rewriting to strings.EqualFold(x, y), which has different semantics.
Suggested fix — guard unknown calls:
if _, isCall := otherSide.(*ast.CallExpr); isCall {
// Unknown call — cannot determine case semantics; skip.
return false
}
return true
…rewrites Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Updated in commits 59457f2 and 47265dc.
I attempted to update the PR body with the ADR link from this environment, but |
|
🎉 This pull request is included in a new release. Release: |
The linter's trigger and autofix were not behavior-preserving:
strings.ToLower(x) == "UPPER"(always false) andstrings.ToLower(a) == strings.ToUpper(b)(mismatched functions) were both rewritten tostrings.EqualFold(...), silently converting dead code into live case-insensitive checks.Changes
Case-compatibility guard (
tolowerequalfold.go)caseConvFuncAndArg— combined extraction of function name + argument from a case-conversion call;caseConvArgand newcaseConvFuncNamedelegate to itstringLitValue— unquotes aBasicLitstring for runtime case comparisonliteralCaseMatchesConv— checks that a literal is already in the correct case for the conversion (ToLower→all-lowercase, ToUpper→all-uppercase); returnsfalsefor unknown function names (fail-safe)caseConvIsCompatible— fires only when: literal operand is already in the correct case, or the other side is the same conversion functioncaseConvAliasIsCompatible— same guard for alias variablesisEquivalentToEqualFold— encapsulates all four trigger sub-conditions for readabilityAlias map changed from
map[types.Object]ast.Exprtomap[types.Object]caseConvAliasInfoto store the function name alongside the argument, enabling the alias-level guard.Before / after
Negative fixtures added for all three shapes (direct calls and alias variant) to lock in the behavior.