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..e78d5ec1860 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,77 @@ 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. +// 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 _, ok := seen[r]; ok { - return true + if r < '0' || r > '9' { + return false } - seen[r] = struct{}{} } + return true +} + +// 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) { + case 'a', 'e', 'i', 'o', 'u': + default: + return false + } + } + return true +} - return false +// 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. +// 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 + 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..8fb3a1848d9 100644 --- a/pkg/linters/trimleftright/trimleftright_internal_test.go +++ b/pkg/linters/trimleftright/trimleftright_internal_test.go @@ -10,12 +10,49 @@ 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). + // "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 {