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.

The /* nolint:errwrap */ comment can be used to disable the linter
inline.

Release note: None

BUILD.bazel

@@ -240,6 +240,7 @@ nogo(
             "//pkg/testutils/lint/passes/descriptormarshal",
             "//pkg/testutils/lint/passes/errcheck",
             "//pkg/testutils/lint/passes/errcmp",
+            "//pkg/testutils/lint/passes/errwrap",
             "//pkg/testutils/lint/passes/fmtsafe",
             "//pkg/testutils/lint/passes/grpcclientconnclose",
             "//pkg/testutils/lint/passes/grpcstatuswithdetails",

build/bazelutil/nogo_config.json

@@ -56,6 +56,15 @@
             "github.com/cockroachdb/cockroach/.*$": "first-party code"
         }
     },
+    "errwrap": {
+      "exclude_files": {
+        ".*\\.pb\\.go$": "generated code",
+        ".*\\.pb\\.gw\\.go$": "generated code"
+      },
+      "only_files": {
+        "github.com/cockroachdb/cockroach/.*$": "first-party code"
+      }
+    },
     "fmtsafe": {
         "exclude_files": {
             "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/logger/log\\.go$": "format argument is not a constant expression",

pkg/BUILD.bazel

@@ -327,6 +327,7 @@ ALL_TESTS = [
     "//pkg/storage:storage_test",
     "//pkg/testutils/floatcmp:floatcmp_test",
     "//pkg/testutils/keysutils:keysutils_test",
+    "//pkg/testutils/lint/passes/errwrap:errwrap_test",
     "//pkg/testutils/lint/passes/fmtsafe:fmtsafe_test",
     "//pkg/testutils/lint/passes/forbiddenmethod:forbiddenmethod_test",
     "//pkg/testutils/lint/passes/hash:hash_test",

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,29 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "errwrap",
+    srcs = [
+        "errwrap.go",
+        "functions.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",
+    ],
+)
+
+go_test(
+    name = "errwrap_test",
+    srcs = ["errwrap_test.go"],
+    data = glob(["testdata/**"]),
+    deps = [
+        ":errwrap",
+        "//pkg/build/bazel",
+        "//pkg/testutils",
+        "//pkg/testutils/skip",
+        "@org_golang_x_tools//go/analysis/analysistest",
+    ],
+)

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

@@ -0,0 +1,210 @@
+// 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/types"
+	"regexp"
+	"strings"
+
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/passesutil"
+	"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 to opt the format/message string
+out of the linter using /* nolint:errwrap */ on or before the line
+that creates the error.`
+
+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
+		}
+		fnName := stripVendor(fn.FullName())
+
+		// Check that none of the arguments are err.Error()
+		if _, found := ErrorFnFormatStringIndex[fnName]; found {
+			for i := range callExpr.Args {
+				if isErrorStringCall(pass, callExpr.Args[i]) {
+					// If the argument is opting out of the linter with a special
+					// comment, tolerate that.
+					if passesutil.HasNolintComment(pass, sel, "errwrap") {
+						continue
+					}
+
+					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.
+		formatStringIdx, ok := ErrorFnFormatStringIndex[fnName]
+		if !ok || formatStringIdx < 0 {
+			// 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 passesutil.HasNolintComment(pass, sel, "errwrap") {
+					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 stripVendor(s string) string {
+	if i := strings.Index(s, "/vendor/"); i != -1 {
+		s = s[i+len("/vendor/"):]
+	}
+	return s
+}

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 TestErrwrap(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)
+		}
+	}
+}