lint: add new errwrap linter

This linter checks if we don't correctly wrap errors. It checks two
cases:
1. if err.Error() is used as an argument to a function that is meant to
   wrap an error.
2. if an error is passed as a string format parameter with a format verb
   of %s or %v.

Release note: None

pkg/cmd/roachvet/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
     visibility = ["//visibility:private"],
     deps = [
         "//pkg/testutils/lint/passes/errcmp",
+        "//pkg/testutils/lint/passes/errwrap",
         "//pkg/testutils/lint/passes/fmtsafe",
         "//pkg/testutils/lint/passes/forbiddenmethod",
         "//pkg/testutils/lint/passes/hash",

pkg/cmd/roachvet/main.go

@@ -15,6 +15,7 @@ package main
 
 import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errcmp"
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errwrap"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/hash"
@@ -65,6 +66,7 @@ func main() {
 		fmtsafe.Analyzer,
 		errcmp.Analyzer,
 		nilness.Analyzer,
+		errwrap.Analyzer,
 	)
 
 	// Standard go vet analyzers:

pkg/testutils/lint/passes/errwrap/BUILD.bazel

@@ -0,0 +1,13 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "errwrap",
+    srcs = ["errwrap.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errwrap",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@org_golang_x_tools//go/analysis",
+        "@org_golang_x_tools//go/analysis/passes/inspect",
+        "@org_golang_x_tools//go/ast/inspector",
+    ],
+)

pkg/testutils/lint/passes/errwrap/errwrap.go

@@ -0,0 +1,265 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package errwrap
+
+import (
+	"fmt"
+	"go/ast"
+	"go/constant"
+	"go/token"
+	"go/types"
+	"regexp"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+const Doc = `checks for unwrapped errors.
+
+This linter checks that:
+
+- err.Error() is not passed as an argument to an error-creating
+  function.
+
+- the '%s', '%v', and '%+v' format verbs are not used to format
+  errors when creating a new error.
+
+In both cases, an error-wrapping function can be used to correctly
+preserve the chain of errors so that user-directed hints, links to
+documentation issues, and telemetry data are all propagated.
+
+It is possible for a call site *in a test file* to opt the format/message
+string out of the linter using /* nolint:errwrap */ after the format
+argument. This escape hatch is not available in non-test code.`
+
+var errorType = types.Universe.Lookup("error").Type().String()
+
+// Analyzer checks for improperly wrapped errors.
+var Analyzer = &analysis.Analyzer{
+	Name:     "errwrap",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspctr := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+
+	inspctr.Preorder(nodeFilter, func(n ast.Node) {
+		// Catch-all for possible bugs in the linter code.
+		defer func() {
+			if r := recover(); r != nil {
+				if err, ok := r.(error); ok {
+					pass.Reportf(n.Pos(), "internal linter error: %v", err)
+					return
+				}
+				panic(r)
+			}
+		}()
+
+		callExpr, ok := n.(*ast.CallExpr)
+		if !ok {
+			return
+		}
+		if pass.TypesInfo.TypeOf(callExpr).String() != errorType {
+			return
+		}
+		sel, ok := callExpr.Fun.(*ast.SelectorExpr)
+		if !ok {
+			return
+		}
+		obj, ok := pass.TypesInfo.Uses[sel.Sel]
+		if !ok {
+			return
+		}
+		fn, ok := obj.(*types.Func)
+		if !ok {
+			return
+		}
+		pkg := obj.Pkg()
+		if pkg == nil {
+			return
+		}
+
+		// Skip files generated by go-bindata.
+		file := pass.Fset.File(n.Pos())
+		if strings.HasSuffix(file.Name(), "/embedded.go") {
+			return
+		}
+
+		// Check that none of the arguments are err.Error()
+		if (pkg.Name() == "fmt" && fn.Name() == "Errorf") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "Wrapf") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "Wrap") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "WrapWithDepthf") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "Newf") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "New") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "NewWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "Wrap") ||
+			(pkg.Name() == "errors" && fn.Name() == "Wrapf") ||
+			(pkg.Name() == "errors" && fn.Name() == "New") ||
+			(pkg.Name() == "errors" && fn.Name() == "Newf") ||
+			(pkg.Name() == "errors" && fn.Name() == "WrapWithDepth") ||
+			(pkg.Name() == "errors" && fn.Name() == "WrapWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "NewWithDepth") ||
+			(pkg.Name() == "errors" && fn.Name() == "NewWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "AssertionFailedf") ||
+			(pkg.Name() == "errors" && fn.Name() == "AssertionFailedWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "NewAssertionErrorWithWrappedErrf") {
+			for i := range callExpr.Args {
+				if isErrorStringCall(pass, callExpr.Args[i]) {
+					pass.Report(analysis.Diagnostic{
+						Pos: n.Pos(),
+						Message: fmt.Sprintf(
+							"err.Error() is passed to %s.%s; use pgerror.Wrap/errors.Wrap/errors.CombineErrors/"+
+								"errors.WithSecondaryError/errors.NewAssertionErrorWithWrappedErrf instead",
+							pkg.Name(), fn.Name()),
+					})
+				}
+			}
+		}
+
+		// Check that the format string does not use %s or %v for an error.
+		var formatStringIdx int
+		if (pkg.Name() == "fmt" && fn.Name() == "Errorf") ||
+			(pkg.Name() == "errors" && fn.Name() == "Newf") ||
+			(pkg.Name() == "errors" && fn.Name() == "AssertionFailedf") {
+			formatStringIdx = 0
+		} else if (pkg.Name() == "pgerror" && fn.Name() == "Newf") ||
+			(pkg.Name() == "errors" && fn.Name() == "Wrapf") ||
+			(pkg.Name() == "errors" && fn.Name() == "NewWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "AssertionFailedWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "NewAssertionErrorWithWrappedErrf") {
+			formatStringIdx = 1
+		} else if (pkg.Name() == "pgerror" && fn.Name() == "Wrapf") ||
+			(pkg.Name() == "pgerror" && fn.Name() == "NewWithDepthf") ||
+			(pkg.Name() == "errors" && fn.Name() == "WrapWithDepthf") {
+			formatStringIdx = 2
+		} else if pkg.Name() == "pgerror" && fn.Name() == "WrapWithDepthf" {
+			formatStringIdx = 3
+		} else {
+			// Not an error formatting function.
+			return
+		}
+
+		// Find all % fields in the format string.
+		formatVerbs, ok := getFormatStringVerbs(pass, callExpr, formatStringIdx)
+		if !ok {
+			return
+		}
+
+		// For any arguments that are errors, check whether the wrapping verb
+		// is %s or %v.
+		args := callExpr.Args[formatStringIdx+1:]
+		for i := 0; i < len(args) && i < len(formatVerbs); i++ {
+			if pass.TypesInfo.TypeOf(args[i]).String() != errorType {
+				continue
+			}
+
+			if formatVerbs[i] == "%v" || formatVerbs[i] == "%+v" || formatVerbs[i] == "%s" {
+				// If the argument is opting out of the linter with a special
+				// comment, tolerate that.
+				if hasNoLintComment(pass, callExpr, formatStringIdx+1+i) {
+					continue
+				}
+
+				pass.Report(analysis.Diagnostic{
+					Pos: n.Pos(),
+					Message: fmt.Sprintf(
+						"non-wrapped error is passed to %s.%s; use pgerror.Wrap/errors.Wrap/errors.CombineErrors/"+
+							"errors.WithSecondaryError/errors.NewAssertionErrorWithWrappedErrf instead",
+						pkg.Name(), fn.Name(),
+					),
+				})
+			}
+		}
+	})
+
+	return nil, nil
+}
+
+// isErrorStringCall tests whether the expression is a string expression that
+// is the result of an `(error).Error()` method call.
+func isErrorStringCall(pass *analysis.Pass, expr ast.Expr) bool {
+	if call, ok := expr.(*ast.CallExpr); ok {
+		if pass.TypesInfo.TypeOf(call).String() == "string" {
+			if callSel, ok := call.Fun.(*ast.SelectorExpr); ok {
+				fun := pass.TypesInfo.Uses[callSel.Sel].(*types.Func)
+				return fun.Type().String() == "func() string" && fun.Name() == "Error"
+			}
+		}
+	}
+	return false
+}
+
+// formatVerbRegexp naively matches format string verbs. This does not take
+// modifiers such as padding into account.
+var formatVerbRegexp = regexp.MustCompile(`%([^%+]|\+v)`)
+
+// getFormatStringVerbs return an array of all `%` format verbs from the format
+// string argument of a function call.
+// Based on https://github.com/polyfloyd/go-errorlint/blob/e4f368f0ae6983eb40821ba4f88dc84ac51aef5b/errorlint/lint.go#L88
+func getFormatStringVerbs(
+	pass *analysis.Pass, call *ast.CallExpr, formatStringIdx int,
+) ([]string, bool) {
+	if len(call.Args) <= formatStringIdx {
+		return nil, false
+	}
+	strLit, ok := call.Args[formatStringIdx].(*ast.BasicLit)
+	if !ok {
+		// Ignore format strings that are not literals.
+		return nil, false
+	}
+	formatString := constant.StringVal(pass.TypesInfo.Types[strLit].Value)
+
+	return formatVerbRegexp.FindAllString(formatString, -1), true
+}
+
+func hasNoLintComment(pass *analysis.Pass, call *ast.CallExpr, idx int) bool {
+	fPos, f := findContainingFile(pass, call)
+
+	if !strings.HasSuffix(fPos.Name(), "_test.go") {
+		// The nolint: escape hatch is only supported in test files.
+		return false
+	}
+
+	startPos := call.Args[idx].End()
+	endPos := call.Rparen
+	if idx < len(call.Args)-1 {
+		endPos = call.Args[idx+1].Pos()
+	}
+	for _, cg := range f.Comments {
+		if cg.Pos() > endPos || cg.End() < startPos {
+			continue
+		}
+		for _, c := range cg.List {
+			if strings.Contains(c.Text, "nolint:errwrap") {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func findContainingFile(pass *analysis.Pass, n ast.Node) (*token.File, *ast.File) {
+	fPos := pass.Fset.File(n.Pos())
+	for _, f := range pass.Files {
+		if pass.Fset.File(f.Pos()) == fPos {
+			return fPos, f
+		}
+	}
+	panic(fmt.Errorf("cannot file file for %v", n))
+}

pkg/testutils/lint/passes/errwrap/errwrap_test.go

@@ -0,0 +1,39 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package errwrap_test
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/build/bazel"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errwrap"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func init() {
+	if bazel.BuiltWithBazel() {
+		bazel.SetGoEnv()
+	}
+}
+
+func Test(t *testing.T) {
+	skip.UnderStress(t)
+	testdata := testutils.TestDataPath(t)
+	analysistest.TestData = func() string { return testdata }
+	results := analysistest.Run(t, testdata, errwrap.Analyzer, "a")
+	for _, r := range results {
+		for _, d := range r.Diagnostics {
+			t.Logf("%s: %v: %s", r.Pass.Analyzer.Name, d.Pos, d.Message)
+		}
+	}
+}