Skip to content

errstringmatch: extend brittle error-string detection to HasPrefix/HasSuffix/EqualFold/Index/LastIndex/Compare #40248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jun 19, 2026
+73 −12
Merged

errstringmatch: extend brittle error-string detection to HasPrefix/HasSuffix/EqualFold/Index/LastIndex/Compare #40248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ba0075
Initial plan
Copilot Jun 19, 2026
fe6b1fd
errstringmatch: extend to HasPrefix/HasSuffix/EqualFold/Index/LastInd…
Copilot Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions pkg/linters/errstringmatch/errstringmatch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Package errstringmatch implements a Go analysis linter that flags
// calls to strings.Contains(err.Error(), "literal") that perform brittle
// substring matching on error messages instead of using errors.Is or errors.As.
// Package errstringmatch implements a Go analysis linter that flags calls to
// strings.Contains/HasPrefix/HasSuffix/EqualFold/Index/LastIndex/Compare on
// err.Error() with a string literal — all perform brittle substring matching on
// error messages instead of using errors.Is or errors.As.
Comment on lines +1 to +4
package errstringmatch

import (
Expand All @@ -18,12 +19,24 @@ import (
// Analyzer is the err-string-match analysis pass.
var Analyzer = &analysis.Analyzer{
Name: "errstringmatch",
Doc: "reports strings.Contains(err.Error(), \"...\") calls that perform brittle substring matching on error messages",
Doc: "reports strings.Contains/HasPrefix/HasSuffix/EqualFold/Index/LastIndex/Compare(err.Error(), \"...\") calls that perform brittle substring matching on error messages",
URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/errstringmatch",
Requires: []*analysis.Analyzer{inspect.Analyzer},
Run: run,
}

// brittleErrStringFuncs is the set of strings package functions that perform
// brittle error-message matching when their first argument is err.Error().
var brittleErrStringFuncs = map[string]bool{
"Contains": true,
"HasPrefix": true,
"HasSuffix": true,
"EqualFold": true,

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] EqualFold and Compare are semantically symmetric — strings.EqualFold(someString, err.Error()) and strings.Compare(someString, err.Error()) == 0 are equally brittle patterns, but they would not be flagged because only outer.Args[0] is checked for err.Error(). This is consistent with the existing behaviour for Contains, so it is not a regression introduced here.

Worth adding a brief comment (or a TODO) to the package doc or isErrDotError acknowledging this known gap, so future contributors do not confuse the absence of second-arg detection with intentional design.

"Index": true,
"LastIndex": true,
"Compare": true,

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EqualFold and Compare with swapped arguments silently bypass the linter, creating a false-confidence gap for the two symmetric functions this PR introduces.

💡 Details

strings.EqualFold and strings.Compare are symmetric (argument order does not change the equality result). The linter only checks outer.Args[0] against err.Error(), so:

strings.EqualFold("timeout", err.Error())   // not flagged — false negative
strings.Compare("EOF", err.Error()) == 0    // not flagged — false negative

For the non-symmetric functions (HasPrefix, HasSuffix, Index, LastIndex) the reversed order has a different semantic, so skipping it is defensible. But EqualFold and Compare are genuinely equivalent in either order when used for equality testing.

Consider extending the check for these two specifically:

// for EqualFold and Compare, either arg can be err.Error()
isSymmetric := funcName == "EqualFold" || funcName == "Compare"
arg0Err := isErrDotError(pass, outer.Args[0])
arg1Err := isErrDotError(pass, outer.Args[1])
if !arg0Err && !(isSymmetric && arg1Err) {
    return
}

}

func run(pass *analysis.Pass) (any, error) {
insp, err := astutil.Inspector(pass)
if err != nil {
Expand All @@ -45,8 +58,9 @@ func run(pass *analysis.Pass) (any, error) {
return
}

// Match strings.Contains(X, Y)
if !isStringsContains(outer) {
// Match strings.<BrittleFunc>(X, Y)
funcName, matched := brittleErrStringFuncName(outer)
if !matched {
return
}
if len(outer.Args) != 2 {
Expand All @@ -66,23 +80,30 @@ func run(pass *analysis.Pass) (any, error) {
return
}

pass.ReportRangef(outer, "avoid strings.Contains(err.Error(), ...) — use errors.Is, errors.As, or a sentinel error instead")
pass.ReportRangef(outer, "avoid strings.%s(err.Error(), ...) — use errors.Is, errors.As, or a sentinel error instead", funcName)
})

return nil, nil
}

// isStringsContains returns true for strings.Contains(...) call expressions.
func isStringsContains(call *ast.CallExpr) bool {
// brittleErrStringFuncName returns the matched strings function name and true
// when call is a strings.<BrittleFunc>(...) call expression.
func brittleErrStringFuncName(call *ast.CallExpr) (string, bool) {
sel, ok := call.Fun.(*ast.SelectorExpr)
if !ok {
return false
return "", false
}
ident, ok := sel.X.(*ast.Ident)
if !ok {
return false
return "", false
}
if ident.Name != "strings" {
return "", false
}
if brittleErrStringFuncs[sel.Sel.Name] {
return sel.Sel.Name, true
}
return ident.Name == "strings" && sel.Sel.Name == "Contains"
return "", false
}

// isErrDotError returns true when expr is a method call of the form <expr>.Error()
Expand Down
40 changes: 40 additions & 0 deletions pkg/linters/errstringmatch/testdata/src/errstringmatch/errstringmatch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,43 @@ func checkIgnoredPreviousLine(err error) bool {
func checkIgnoredSameLine(err error) bool {
return strings.Contains(err.Error(), "already merged") //nolint:errstringmatch // gh CLI merge status is only available as text.
}

// flagged: strings.HasPrefix on err.Error() with a string literal
func checkHasPrefix(err error) bool {
return strings.HasPrefix(err.Error(), "connection refused") // want `avoid strings\.HasPrefix\(err\.Error\(\)`
}

// flagged: strings.HasSuffix on err.Error() with a string literal
func checkHasSuffix(err error) bool {
return strings.HasSuffix(err.Error(), "not found") // want `avoid strings\.HasSuffix\(err\.Error\(\)`
}

// flagged: strings.EqualFold on err.Error() with a string literal
func checkEqualFold(err error) bool {
return strings.EqualFold(err.Error(), "timeout") // want `avoid strings\.EqualFold\(err\.Error\(\)`
}

// flagged: strings.Index on err.Error() with a string literal
func checkIndex(err error) bool {
return strings.Index(err.Error(), "denied") >= 0 // want `avoid strings\.Index\(err\.Error\(\)`
}

// flagged: strings.LastIndex on err.Error() with a string literal
func checkLastIndex(err error) bool {
return strings.LastIndex(err.Error(), "denied") >= 0 // want `avoid strings\.LastIndex\(err\.Error\(\)`
}

// flagged: strings.Compare on err.Error() with a string literal
func checkCompare(err error) bool {
return strings.Compare(err.Error(), "timeout") == 0 // want `avoid strings\.Compare\(err\.Error\(\)`
}

// not flagged: strings.HasPrefix on a plain string, not err.Error()

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Negative test coverage is incomplete: Index, LastIndex, and Compare have no "not flagged" test cases for non-error string arguments.

💡 Details

Lines 69–77 add negative cases only for HasPrefix and EqualFold with non-error string arguments. Index, LastIndex, and Compare have no corresponding negative tests. A regression where the linter incorrectly flags strings.Index(someString, "x") would go undetected. Add:

func checkIndexString(s string) bool {
    return strings.Index(s, "denied") >= 0
}
func checkLastIndexString(s string) bool {
    return strings.LastIndex(s, "denied") >= 0
}
func checkCompareString(s string) bool {
    return strings.Compare(s, "timeout") == 0
}

func checkHasPrefixString(s string) bool {
return strings.HasPrefix(s, "prefix")
}

// not flagged: strings.EqualFold on a plain string, not err.Error()
func checkEqualFoldString(s string) bool {
return strings.EqualFold(s, "value")
}

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Only HasPrefix and EqualFold have negative ("not flagged") test cases; HasSuffix, Index, LastIndex, and Compare are missing equivalent coverage. If isErrDotError were inadvertently broken for those functions, no test would catch it.

💡 Suggested additions 
// not flagged: strings.HasSuffix on a plain string, not err.Error()
func checkHasSuffixString(s string) bool {
	return strings.HasSuffix(s, "suffix")
}

// not flagged: strings.Index on a plain string, not err.Error()
func checkIndexString(s string) bool {
	return strings.Index(s, "sub") >= 0
}

// not flagged: strings.LastIndex on a plain string, not err.Error()
func checkLastIndexString(s string) bool {
	return strings.LastIndex(s, "sub") >= 0
}

// not flagged: strings.Compare on a plain string, not err.Error()
func checkCompareString(s string) bool {
	return strings.Compare(s, "value") == 0
}

One negative test per function makes the testdata file a self-documenting specification of what is and is not flagged.

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No nolint:errstringmatch suppression test covers any of the six newly added functions, leaving the nolint path untested for the expanded diagnostic message format.

💡 Details

The existing nolint tests at lines 30–37 verify suppression only for strings.Contains. None of the new functions (HasPrefix, HasSuffix, EqualFold, Index, LastIndex, Compare) have a corresponding suppression test. The diagnostic message changed from a hardcoded string to a fmt.Sprintf-formatted string ("avoid strings.%s(err.Error(), ..."). If the format regresses, the nolint directive lookup could silently break for any of these six new functions without a test catching it.

Add at least one suppression test for a representative new function:

func checkHasPrefixNolint(err error) bool {
    (nolint/redacted):errstringmatch
    return strings.HasPrefix(err.Error(), "connection refused")
}

Loading