go: proposal: x/tools/go/analysis/passes/errorstringcompare: new check that warns when programmers compare against Error() strings

Problem

On GitHub, there are too many projects that strictly compare error strings outside of test code: `.Error() == “…”

Go 1 and the Future of Go Programs does not explicitly promise that error strings are protected, nor do we want to encourage this amongst third-party libraries. See @neild’s https://github.com/golang/go/issues/49172#issuecomment-954071349.

Proposed Implementation

Now that error wrapping was introduced in Working with Errors in Go 1.13 - go.dev, we should encourage developers to use errors.Is and errors.As whenever possible.

Linting is one way of providing this hint. I propose writing an optional analysis pass that detects when errors strings are compared for equality, substrings, or with regular expressions. This analysis pass should not be incorporated into go vet, because any reported errors are not guaranteed to be bugs.

1. Add x/tools/go/analysis/passes/errorstringcompare that implements the following Analysis.Doc string:

   const Doc = `report code that compares error strings when alternatives exist
   
   The errorstringcompare analysis reports when code compares error strings,
   when the underlying error supports the errors.Is interface.
   
   Examples of such comparisons include:
   
   	if err.Error() == "..." { }
   	if err.Error()[i:j] == "..." { }
   	if err.Error()[i] == '.' { }
   	if strings.Contains(err.Error(), "...") { }
   	if strings.HasPrefix(err.Error(), "...") { }
   	if strings.HasSuffix(err.Error(), "...") { }
   	if re.MatchString(err.Error()) { }
   	if re.FindString(err.Error()) != "" { }
   
   The -ignorepkgs flag specifies a comma-separated list of packages which
   will be ignored by the analysis.
   `
   

   Proposed algorithm:

   1. Find error string comparisons in the current package, where the Error method is compared against another string. If the developer has assigned the error string to a variable or used fmt.Sprintf, assume they know what they’re doing.
   2. For each of the errors that had their Error method called, try to determine the underlying type:
      1. Determine if the error was directly initialized, or created by concrete type assertion, and record its actual sum type. Errors wrapped with fmt.Errorf can be handled in this step.
      2. Determine if the error was the result of a static function call, and add it to the list of errors to inspect.
      3. If the type of the error is indeterminate, drop the error from the analysis, since we want to avoid false positives.
   3. After resolving the sum type for each error, keep the errors that can only be exported error sentinels or that implement the errors.Is interface. The Is method implies that there are exported error sentinels to check against.
   4. Report these remaining errors with a message like fmt.Sprintf("prefer using errors.Is for %s", err.Name()).

2. Add this to gopls Analyzers, after deciding whether to enable it by default.

3. Encourage metalinters like golangci-lint to add this to their corpus of checks.

Concerns

• Some of these warnings might be encounter too many false positives. We should run errorstringcompare against a representative corpus of Go code to see how noisy it would be.
• Some packages may have tests that actually do promise backwards-compatibility for their APIs, which is why the -ignorepkgs flag exists.

See also

• #49358 which can catch more edge cases if they are covered by tests.