From 17139110b961035d91ee3af48a4a39fd9355d455 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 06:16:22 +0000 Subject: [PATCH 1/5] Initial plan From c416be69a731c46a8949be1e327764129baa9870 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 06:35:16 +0000 Subject: [PATCH 2/5] trimleftright: widen looksSuspiciousCutset to catch distinct-char prefix/suffix bugs Replace the repeated-rune proxy with a smarter heuristic: - Remove the repeated-rune requirement that was missing common bugs like TrimLeft(s, "http"), TrimLeft(s, "abc"), TrimRight(s, "txt") - Add three narrow exceptions for intentional character-class trimming: 1. Pure decimal-digit sets (e.g. "0123456789") 2. Pure ASCII-vowel sets (e.g. "aeiou") 3. Complete hex-letter alphabet (all 6 of a-f present in any case, optionally with digits, e.g. "abcdef", "0123456789abcdef") - Partial hex subsets like "abc" are correctly flagged Update testdata with new bad cases (http, abc, txt, v1) and good cases (digits, vowels, hex alphabets). Expand unit tests for looksSuspiciousCutset. Update Analyzer.Doc string and ADR to reflect the revised decision. Closes #46526 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- docs/adr/46465-add-trimleftright-linter.md | 10 ++- .../src/trimleftright/trimleftright.go | 40 +++++++-- pkg/linters/trimleftright/trimleftright.go | 85 ++++++++++++++++--- .../trimleftright_internal_test.go | 35 +++++++- 4 files changed, 149 insertions(+), 21 deletions(-) diff --git a/docs/adr/46465-add-trimleftright-linter.md b/docs/adr/46465-add-trimleftright-linter.md index 2ce372d7f45..ef33e04fad9 100644 --- a/docs/adr/46465-add-trimleftright-linter.md +++ b/docs/adr/46465-add-trimleftright-linter.md @@ -12,7 +12,13 @@ ### Decision -We will add a new `trimleftright` analyzer at `pkg/linters/trimleftright/` with a conservative heuristic: flag `strings.TrimLeft`/`strings.TrimRight` only when the cutset is a string literal that is (1) multi-rune, (2) fully alphanumeric, and (3) contains at least one repeated rune (for example `"foo"` or `"barr"`). This sharply reduces false positives for idiomatic character-set trimming such as whitespace classes (`" \t\n\r"`), punctuation sets, or unique-rune sets (`"aeiou"`). +We will add a new `trimleftright` analyzer at `pkg/linters/trimleftright/` with a heuristic that flags `strings.TrimLeft`/`strings.TrimRight` when the cutset is a string literal that is (1) multi-rune, (2) fully alphanumeric, and (3) does not match a recognised intentional character-class idiom. The recognised exceptions are: + +- **Pure decimal-digit sets** (e.g. `"0123456789"`, `"012"`) — digit-class trimming. +- **Pure ASCII-vowel sets** (e.g. `"aeiou"`, `"aei"`) — vowel-class trimming. +- **Complete hex-letter alphabet** (all six of `a`–`f` present, in any case, optionally mixed with digits; e.g. `"abcdef"`, `"ABCDEF"`, `"0123456789abcdef"`) — hex-class trimming. + +All other multi-rune alphanumeric cutsets — including common prefix/suffix bugs such as `"http"`, `"bar"`, `"abc"`, `"v1"`, `"0x"` — are flagged. This is a broader trigger than the original repeated-rune heuristic, which systematically missed the most common form of the bug (distinct-character cutsets). The analyzer will emit diagnostics only (no `SuggestedFix`) because replacing `TrimLeft`/`TrimRight` with `TrimPrefix`/`TrimSuffix` changes semantics and cannot be proven safe from syntax alone. Generated files are excluded and callers can suppress intentional cases with `//nolint:trimleftright`. The analyzer is registered in `cmd/linters/main.go` alongside existing analyzers. @@ -40,7 +46,7 @@ Not chosen because: documentation-only approaches do not catch new instances at #### Negative - Any intentional multi-character cutset usage (e.g., trimming a set of punctuation characters) must be explicitly annotated with `//nolint:trimleftright`, adding a small maintenance burden for valid usages. - The analyzer adds one more entry to `cmd/linters/main.go` and must be kept in sync with future refactors of the linter registry. -- The conservative heuristic may miss some genuine mistakes (for example, literal prefixes without repeated runes such as `"bar"`). +- Partial hex-letter subsets (e.g. `"abc"`, `"abce"`) are flagged even when the intent is genuinely hex-class trimming; such cases should be annotated with `//nolint:trimleftright`. #### Neutral - The linter applies only to string literals; dynamic cutset values (variables, expressions) are not flagged, which is a deliberate scope limitation—inferring intent from non-literal arguments is not feasible at analysis time. diff --git a/pkg/linters/trimleftright/testdata/src/trimleftright/trimleftright.go b/pkg/linters/trimleftright/testdata/src/trimleftright/trimleftright.go index 2bd2be9a246..b86a4ad3a28 100644 --- a/pkg/linters/trimleftright/testdata/src/trimleftright/trimleftright.go +++ b/pkg/linters/trimleftright/testdata/src/trimleftright/trimleftright.go @@ -13,6 +13,26 @@ func badTrimRight(s string) string { return strings.TrimRight(s, "barr") // want `strings\.TrimRight with a multi-character cutset` } +// bad: distinct-char prefix — no repeated rune but still a prefix bug +func badHTTP(s string) string { + return strings.TrimLeft(s, "http") // want `strings\.TrimLeft with a multi-character cutset` +} + +// bad: distinct-char prefix with no repeated rune +func badABC(s string) string { + return strings.TrimLeft(s, "abc") // want `strings\.TrimLeft with a multi-character cutset` +} + +// bad: distinct-char suffix +func badTXT(s string) string { + return strings.TrimRight(s, "txt") // want `strings\.TrimRight with a multi-character cutset` +} + +// bad: mixed letter+digit prefix +func badV1(s string) string { + return strings.TrimLeft(s, "v1") // want `strings\.TrimLeft with a multi-character cutset` +} + // good: TrimLeft with single character — valid cutset usage func goodSingleChar(s string) string { return strings.TrimLeft(s, " ") @@ -38,14 +58,24 @@ func goodUnicodeRune(s string) string { return strings.TrimLeft(s, "→") } -// good: intentional multi-character cutset -func goodIntentionalCutset(s string) string { +// good: intentional vowel-class cutset +func goodVowels(s string) string { return strings.TrimLeft(s, "aeiou") } -// good: alphanumeric literal with no repeated runes -func goodNoRepeatedAlnum(s string) string { - return strings.TrimLeft(s, "abc") +// good: intentional decimal-digit class +func goodDigits(s string) string { + return strings.TrimLeft(s, "0123456789") +} + +// good: complete hex-letter alphabet — intentional hex-class trimming +func goodHexLetters(s string) string { + return strings.TrimLeft(s, "abcdef") +} + +// good: full lowercase hex alphabet including digits +func goodFullHex(s string) string { + return strings.TrimLeft(s, "0123456789abcdef") } // suppressed: nolint directive suppresses the diagnostic diff --git a/pkg/linters/trimleftright/trimleftright.go b/pkg/linters/trimleftright/trimleftright.go index ec4e6fa03c6..f82737d6d33 100644 --- a/pkg/linters/trimleftright/trimleftright.go +++ b/pkg/linters/trimleftright/trimleftright.go @@ -25,7 +25,7 @@ import ( // Analyzer is the trimleftright analysis pass. var Analyzer = &analysis.Analyzer{ Name: "trimleftright", - Doc: "reports likely mistaken strings.TrimLeft/TrimRight calls using repeated alphanumeric literal cutsets", + Doc: "reports likely mistaken strings.TrimLeft/TrimRight calls using multi-character alphanumeric literal cutsets", URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/trimleftright", Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer}, Run: run, @@ -78,9 +78,8 @@ func run(pass *analysis.Pass) (any, error) { return } - // Only flag suspicious cutsets where a multi-rune alphanumeric literal - // contains repeated runes (e.g., "foo"). This avoids flagging common, - // intentional character-set trimming like whitespace classes. + // Only flag suspicious cutsets: multi-rune all-alphanumeric literals + // that do not look like intentional character-set trimming. cutset, isCutset := stringLitValue(call.Args[1]) if !isCutset || !looksSuspiciousCutset(cutset) { return @@ -119,9 +118,19 @@ func stringLitValue(expr ast.Expr) (string, bool) { } // looksSuspiciousCutset reports likely TrimPrefix/TrimSuffix confusion. -// We require a multi-rune alphanumeric cutset with at least one repeated rune -// so valid character-set trimming (whitespace, punctuation, unique rune sets) -// is not flagged. +// It returns true for any multi-rune all-alphanumeric cutset that does not +// look like an intentional character-class trimmer. +// +// Recognised character-class exceptions (returned false): +// - Pure decimal-digit sets (e.g. "0123456789", "012") — digit trimming. +// - Pure ASCII-vowel sets (e.g. "aeiou", "aei") — vowel trimming. +// - Complete hex-letter alphabet in any case (all six of a–f present, +// mixed with optional digits, e.g. "abcdef", "ABCDEF", +// "0123456789abcdef") — hex trimming. +// +// Single-rune cutsets and cutsets containing non-alphanumeric runes are +// returned false because they are either trivially correct or already +// covered by other idioms (whitespace trimming, punctuation sets). func looksSuspiciousCutset(cutset string) bool { runes := []rune(cutset) if len(runes) <= 1 { @@ -134,13 +143,65 @@ func looksSuspiciousCutset(cutset string) bool { } } - seen := make(map[rune]struct{}) + // Exception: intentional decimal-digit set. + if isAllASCIIDigits(runes) { + return false + } + + // Exception: intentional ASCII-vowel set. + if isAllASCIIVowels(runes) { + return false + } + + // Exception: complete hex-letter alphabet (all six of a–f in any case, + // optionally accompanied by decimal digits). + if isCompleteHexAlphabet(runes) { + return false + } + + return true +} + +// isAllASCIIDigits reports whether every rune is an ASCII decimal digit. +func isAllASCIIDigits(runes []rune) bool { for _, r := range runes { - if _, ok := seen[r]; ok { - return true + if r < '0' || r > '9' { + return false } - seen[r] = struct{}{} } + return true +} - return false +// isAllASCIIVowels reports whether every rune is an ASCII vowel (a e i o u), +// case-insensitive. +func isAllASCIIVowels(runes []rune) bool { + for _, r := range runes { + switch unicode.ToLower(r) { + case 'a', 'e', 'i', 'o', 'u': + default: + return false + } + } + return true +} + +// isCompleteHexAlphabet reports whether runes consist solely of ASCII decimal +// digits and/or ASCII hex letters (a–f, A–F), and include all six hex-letter +// code points (a b c d e f, case-insensitively). This matches intentional +// hex-class trimming like "abcdef", "ABCDEF", or "0123456789abcdef". +// Partial hex-letter subsets such as "abc" are not matched and return false. +func isCompleteHexAlphabet(runes []rune) bool { + seen := make(map[rune]struct{}) + for _, r := range runes { + lower := unicode.ToLower(r) + switch { + case lower >= 'a' && lower <= 'f': + seen[lower] = struct{}{} + case r >= '0' && r <= '9': + // decimal digits are permissible alongside hex letters + default: + return false + } + } + return len(seen) == 6 } diff --git a/pkg/linters/trimleftright/trimleftright_internal_test.go b/pkg/linters/trimleftright/trimleftright_internal_test.go index a9019555d2c..e606358122f 100644 --- a/pkg/linters/trimleftright/trimleftright_internal_test.go +++ b/pkg/linters/trimleftright/trimleftright_internal_test.go @@ -10,12 +10,43 @@ func TestLooksSuspiciousCutset(t *testing.T) { cutset string want bool }{ + // Trivially safe: too short or non-alphanumeric. {name: "empty", cutset: "", want: false}, {name: "single rune", cutset: "a", want: false}, - {name: "repeated alnum", cutset: "foo", want: true}, - {name: "unique alnum", cutset: "aeiou", want: false}, {name: "contains whitespace", cutset: " \t", want: false}, {name: "contains punctuation", cutset: "a!", want: false}, + + // Suspicious: repeated rune (legacy coverage). + {name: "repeated alnum", cutset: "foo", want: true}, + {name: "repeated suffix alnum", cutset: "barr", want: true}, + + // Suspicious: distinct-char prefixes/suffixes — the main gap fixed. + {name: "http prefix", cutset: "http", want: true}, + {name: "abc prefix", cutset: "abc", want: true}, + {name: "txt suffix", cutset: "txt", want: true}, + {name: "bar prefix", cutset: "bar", want: true}, + {name: "v1 version prefix", cutset: "v1", want: true}, + {name: "mixed alpha digit", cutset: "0x", want: true}, + + // Safe: intentional decimal-digit class. + {name: "all digits", cutset: "0123456789", want: false}, + {name: "digit subset", cutset: "012", want: false}, + + // Safe: intentional ASCII-vowel class. + {name: "all vowels", cutset: "aeiou", want: false}, + {name: "vowel subset", cutset: "aei", want: false}, + {name: "vowels uppercase", cutset: "AEIOU", want: false}, + + // Safe: complete hex-letter alphabet (all six of a–f present). + {name: "hex letters lower", cutset: "abcdef", want: false}, + {name: "hex letters upper", cutset: "ABCDEF", want: false}, + {name: "full hex with digits lower", cutset: "0123456789abcdef", want: false}, + {name: "full hex with digits upper", cutset: "0123456789ABCDEF", want: false}, + {name: "hex mixed case", cutset: "abcDEF", want: false}, + + // Suspicious: partial hex-letter subset (not the complete alphabet). + {name: "partial hex abc", cutset: "abc", want: true}, + {name: "partial hex abce", cutset: "abce", want: true}, } for _, tt := range tests { From 43b221c40ff7c308a782adab10685f095979cc2a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 06:41:35 +0000 Subject: [PATCH 3/5] =?UTF-8?q?trimleftright:=20address=20review=20comment?= =?UTF-8?q?s=20=E2=80=94=20remove=20duplicate=20test,=20document=20empty-s?= =?UTF-8?q?lice=20edge=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/trimleftright/trimleftright.go | 4 ++++ pkg/linters/trimleftright/trimleftright_internal_test.go | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/linters/trimleftright/trimleftright.go b/pkg/linters/trimleftright/trimleftright.go index f82737d6d33..0355c985b5b 100644 --- a/pkg/linters/trimleftright/trimleftright.go +++ b/pkg/linters/trimleftright/trimleftright.go @@ -163,6 +163,8 @@ func looksSuspiciousCutset(cutset string) bool { } // isAllASCIIDigits reports whether every rune is an ASCII decimal digit. +// An empty slice returns true (vacuous truth); callers must apply the length +// guard in looksSuspiciousCutset before invoking this helper. func isAllASCIIDigits(runes []rune) bool { for _, r := range runes { if r < '0' || r > '9' { @@ -174,6 +176,8 @@ func isAllASCIIDigits(runes []rune) bool { // isAllASCIIVowels reports whether every rune is an ASCII vowel (a e i o u), // case-insensitive. +// An empty slice returns true (vacuous truth); callers must apply the length +// guard in looksSuspiciousCutset before invoking this helper. func isAllASCIIVowels(runes []rune) bool { for _, r := range runes { switch unicode.ToLower(r) { diff --git a/pkg/linters/trimleftright/trimleftright_internal_test.go b/pkg/linters/trimleftright/trimleftright_internal_test.go index e606358122f..c3288483893 100644 --- a/pkg/linters/trimleftright/trimleftright_internal_test.go +++ b/pkg/linters/trimleftright/trimleftright_internal_test.go @@ -45,7 +45,8 @@ func TestLooksSuspiciousCutset(t *testing.T) { {name: "hex mixed case", cutset: "abcDEF", want: false}, // Suspicious: partial hex-letter subset (not the complete alphabet). - {name: "partial hex abc", cutset: "abc", want: true}, + // "abc" is tested above via "abc prefix"; here we confirm the + // 5-letter subset also escapes the hex exception. {name: "partial hex abce", cutset: "abce", want: true}, } From 219a638a60b8e0047fb2a57b0adc8e1592cd57d1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 07:29:37 +0000 Subject: [PATCH 4/5] =?UTF-8?q?trimleftright:=20fix=20comment=20=E2=80=94?= =?UTF-8?q?=20abce=20is=20a=204-letter=20subset,=20not=205-letter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/trimleftright/trimleftright_internal_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/linters/trimleftright/trimleftright_internal_test.go b/pkg/linters/trimleftright/trimleftright_internal_test.go index c3288483893..7a9db2a6004 100644 --- a/pkg/linters/trimleftright/trimleftright_internal_test.go +++ b/pkg/linters/trimleftright/trimleftright_internal_test.go @@ -46,7 +46,7 @@ func TestLooksSuspiciousCutset(t *testing.T) { // Suspicious: partial hex-letter subset (not the complete alphabet). // "abc" is tested above via "abc prefix"; here we confirm the - // 5-letter subset also escapes the hex exception. + // 4-letter subset also escapes the hex exception. {name: "partial hex abce", cutset: "abce", want: true}, } From 322ddb3f8463bcdec92416ee8025d7c20911a411 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Jul 2026 07:44:07 +0000 Subject: [PATCH 5/5] trimleftright: fix regression in isCompleteHexAlphabet, add boundary tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/linters/trimleftright/trimleftright.go | 8 ++++++++ pkg/linters/trimleftright/trimleftright_internal_test.go | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/pkg/linters/trimleftright/trimleftright.go b/pkg/linters/trimleftright/trimleftright.go index 0355c985b5b..e78d5ec1860 100644 --- a/pkg/linters/trimleftright/trimleftright.go +++ b/pkg/linters/trimleftright/trimleftright.go @@ -194,12 +194,20 @@ func isAllASCIIVowels(runes []rune) bool { // code points (a b c d e f, case-insensitively). This matches intentional // hex-class trimming like "abcdef", "ABCDEF", or "0123456789abcdef". // Partial hex-letter subsets such as "abc" are not matched and return false. +// Duplicate hex letters (e.g. "aabcdef") are also rejected and return false; +// a repeated hex letter is almost certainly a bug rather than a character class. func isCompleteHexAlphabet(runes []rune) bool { + if len(runes) > 16 { // 6 unique hex letters (a–f, case-folded to lowercase) + 10 digits + return false + } seen := make(map[rune]struct{}) for _, r := range runes { lower := unicode.ToLower(r) switch { case lower >= 'a' && lower <= 'f': + if _, ok := seen[lower]; ok { + return false // repeated hex letter → suspicious + } seen[lower] = struct{}{} case r >= '0' && r <= '9': // decimal digits are permissible alongside hex letters diff --git a/pkg/linters/trimleftright/trimleftright_internal_test.go b/pkg/linters/trimleftright/trimleftright_internal_test.go index 7a9db2a6004..8fb3a1848d9 100644 --- a/pkg/linters/trimleftright/trimleftright_internal_test.go +++ b/pkg/linters/trimleftright/trimleftright_internal_test.go @@ -48,6 +48,11 @@ func TestLooksSuspiciousCutset(t *testing.T) { // "abc" is tested above via "abc prefix"; here we confirm the // 4-letter subset also escapes the hex exception. {name: "partial hex abce", cutset: "abce", want: true}, + // Boundary: all six hex letters plus a non-hex character. + {name: "almost hex abcdefg", cutset: "abcdefg", want: true}, + // Regression: repeated hex letters look like a bug, not a character class. + {name: "repeated hex letters", cutset: "aabbccddeeff", want: true}, + {name: "hex with duplicate letter", cutset: "aabcdef", want: true}, } for _, tt := range tests {