tolowerequalfold: avoid false positives on ToLower(x) == x / != x case-detection idioms#36855
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
ToLower(x) == x / != x case-detection idioms
| - `.github/aw/github-agentic-workflows.md` | ||
| - `.github/aw/github-mcp-server.md` | ||
| - `.github/aw/llms.md` | ||
| - `.github/aw/mcp-clis.md` |
There was a problem hiding this comment.
@copilot determine why this line was deleted, something is wrong with the file list generator
There was a problem hiding this comment.
Root cause: those two lines were unintentionally included in my earlier Plan linter precision fix commit (b6d2a50) due an unrelated pre-existing local diff in this file, not from the tolowerequalfold change itself. I restored the file to its pre-change state in fa9c4a8.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the tolowerequalfold Go analyzer to avoid reporting (and auto-suggesting strings.EqualFold) on common case-detection idioms like strings.ToLower(x) == x / != x, which are not intended to be case-insensitive equality checks.
Changes:
- Add a self-comparison guard that skips diagnostics when comparing
strings.ToLower/ToUpper(E)against the same identifierE(matched viatypes.Objectidentity). - Refactor case-conversion call detection into a reusable helper (
caseConvArg(...)) and reuse it fromisCaseConvCall(...). - Extend test fixtures to include the
ToLower(name) == nameandToLower(name) != nameidioms as OK examples.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/tolowerequalfold/tolowerequalfold.go | Adds a guard to avoid false positives for ToLower/ToUpper(x) == x / != x and refactors call detection. |
| pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.go | Adds fixture coverage for the case-detection idiom to prevent regressions. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| if isCaseConvCall(expr.X) || isCaseConvCall(expr.Y) { | ||
| pass.ReportRangef(expr, | ||
| "use strings.EqualFold for case-insensitive comparison instead of strings.ToLower/ToUpper with ==") | ||
| } |
tolowerequalfoldwas flagging comparisons where one side isstrings.ToLower/ToUpperand the other is the same operand, e.g.strings.ToLower(x) != x. Those are case-detection checks, not case-insensitive equality checks, and rewriting tostrings.EqualFoldcan produce incorrect logic.Analyzer precision: self-comparison guard
pkg/linters/tolowerequalfold/tolowerequalfold.goto skip diagnostics when:strings.ToLower/ToUpper(E), andE(matched viatypes.Objectidentity).caseConvArg(...)and reused it fromisCaseConvCall(...).Fixture coverage for the missed idiom
pkg/linters/tolowerequalfold/testdata/src/tolowerequalfold/tolowerequalfold.gookExampleswith:strings.ToLower(name) == namestrings.ToLower(name) != name