Merge #71877

71877: lint: add new errwrap linter r=ajwerner,knz a=rafiss

fixes #42510

This linter checks if we don't correctly wrap errors.

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

See individual commits for mistakes this linter caught.
It had already caught a few in #72353, #72352, #72351, #72350, and #72349.

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>

BUILD.bazel

@@ -241,6 +241,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

@@ -329,6 +329,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/ccl/backupccl/backup_test.go

@@ -1665,11 +1665,11 @@ func TestBackupRestoreCheckpointing(t *testing.T) {
 		checkpointPath = filepath.Join(ip.dir, ip.name, backupManifestCheckpointName)
 		checkpointDescBytes, err := ioutil.ReadFile(checkpointPath)
 		if err != nil {
-			return errors.Errorf("%+v", err)
+			return errors.Wrap(err, "error while reading checkpoint")
 		}
 		var checkpointDesc BackupManifest
 		if err := protoutil.Unmarshal(checkpointDescBytes, &checkpointDesc); err != nil {
-			return errors.Errorf("%+v", err)
+			return errors.Wrap(err, "error while unmarshalling checkpoint")
 		}
 		if len(checkpointDesc.Files) == 0 {
 			return errors.Errorf("empty backup checkpoint descriptor")

pkg/ccl/changefeedccl/sink_kafka.go

@@ -581,7 +581,7 @@ func buildKafkaConfig(u sinkURL, opts map[string]string) (*sarama.Config, error)
 		if dialConfig.clientCert != nil && dialConfig.clientKey != nil {
 			cert, err := tls.X509KeyPair(dialConfig.clientCert, dialConfig.clientKey)
 			if err != nil {
-				return nil, errors.Errorf(`invalid client certificate data provided: %s`, err)
+				return nil, errors.Wrap(err, `invalid client certificate data provided`)
 			}
 			config.Net.TLS.Config.Certificates = []tls.Certificate{cert}
 		}

pkg/ccl/importccl/read_import_mysql.go

@@ -761,7 +761,16 @@ func mysqlColToCockroach(
 				exprString := mysql.String(col.Default)
 				expr, err := parser.ParseExpr(exprString)
 				if err != nil {
-					return nil, unimplemented.Newf("import.mysql.default", "unsupported default expression %q for column %q: %v", exprString, name, err)
+					// There is currently no way to wrap an error with an unimplemented
+					// error.
+					// nolint:errwrap
+					return nil, errors.Wrapf(
+						unimplemented.Newf("import.mysql.default", "unsupported default expression"),
+						"error parsing %q for column %q: %v",
+						exprString,
+						name,
+						err,
+					)
 				}
 				def.DefaultExpr.Expr = expr
 			}

pkg/ccl/serverccl/diagnosticsccl/reporter_test.go

@@ -121,7 +121,7 @@ func TestServerReport(t *testing.T) {
 				fmt.Sprintf(`ALTER %s CONFIGURE ZONE = '%s'`, cmd.resource, cmd.config),
 			); err != nil {
 				// Work around gossip asynchronicity.
-				return errors.Errorf("error applying zone config %q to %q: %v", cmd.config, cmd.resource, err)
+				return errors.Wrapf(err, "error applying zone config %q to %q", cmd.config, cmd.resource)
 			}
 			return nil
 		})

pkg/cli/clierrorplus/decorate_error.go

@@ -75,25 +75,35 @@ func MaybeDecorateError(
 		}()
 
 		connFailed := func() error {
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
 			const format = "cannot dial server.\n" +
 				"Is the server running?\n" +
 				"If the server is running, check --host client-side and --advertise server-side.\n\n%v"
 			return errors.Errorf(format, err)
 		}
 
 		connSecurityHint := func() error {
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
 			const format = "SSL authentication error while connecting.\n%v"
 			return errors.Errorf(format, err)
 		}
 
 		connInsecureHint := func() error {
-			return errors.Errorf("cannot establish secure connection to insecure server.\n"+
-				"Maybe use --insecure?\n\n%v", err)
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
+			const format = "cannot establish secure connection to insecure server.\n" +
+				"Maybe use --insecure?\n\n%v"
+			return errors.Errorf(format, err)
 		}
 
 		connRefused := func() error {
-			return errors.Errorf("server closed the connection.\n"+
-				"Is this a CockroachDB node?\n%v", err)
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
+			const format = "server closed the connection.\n" +
+				"Is this a CockroachDB node?\n%v"
+			return errors.Errorf(format, err)
 		}
 
 		// Is this an "unable to connect" type of error?
@@ -105,9 +115,11 @@ func MaybeDecorateError(
 		}
 
 		if wErr := (*security.Error)(nil); errors.As(err, &wErr) {
-			return errors.Errorf("cannot load certificates.\n"+
-				"Check your certificate settings, set --certs-dir, or use --insecure for insecure clusters.\n\n%v",
-				err)
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
+			const format = "cannot load certificates.\n" +
+				"Check your certificate settings, set --certs-dir, or use --insecure for insecure clusters.\n\n%v"
+			return errors.Errorf(format, err)
 		}
 
 		if wErr := (*x509.UnknownAuthorityError)(nil); errors.As(err, &wErr) {
@@ -181,7 +193,10 @@ func MaybeDecorateError(
 		}
 
 		opTimeout := func() error {
-			return errors.Errorf("operation timed out.\n\n%v", err)
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
+			const format = "operation timed out.\n\n%v"
+			return errors.Errorf(format, err)
 		}
 
 		// Is it a plain context cancellation (i.e. timeout)?
@@ -201,7 +216,10 @@ func MaybeDecorateError(
 			return fmt.Errorf(
 				"incompatible client and server versions (likely server version: v1.0, required: >=v1.1)")
 		} else if grpcutil.IsClosedConnection(err) {
-			return errors.Errorf("connection lost.\n\n%v", err)
+			// Avoid errors.Wrapf here so that we have more control over the
+			// formatting of the message with error text.
+			const format = "connection lost.\n\n%v"
+			return errors.Errorf(format, err)
 		}
 
 		// Does the server require GSSAPI authentication?

pkg/cli/clisqlclient/conn_test.go

@@ -69,7 +69,7 @@ func TestConnRecover(t *testing.T) {
 				t.Fatal(closeErr)
 			}
 		} else if !errors.Is(err, driver.ErrBadConn) {
-			return errors.Newf("expected ErrBadConn, got %v", err)
+			return errors.Newf("expected ErrBadConn, got %v", err) // nolint:errwrap
 		}
 		return nil
 	})
@@ -89,7 +89,7 @@ func TestConnRecover(t *testing.T) {
 	// Ditto from Query().
 	testutils.SucceedsSoon(t, func() error {
 		if err := conn.Exec(`SELECT 1`, nil); !errors.Is(err, driver.ErrBadConn) {
-			return errors.Newf("expected ErrBadConn, got %v", err)
+			return errors.Newf("expected ErrBadConn, got %v", err) // nolint:errwrap
 		}
 		return nil
 	})

pkg/cli/node.go

@@ -305,7 +305,7 @@ func parseNodeIDs(strNodeIDs []string) ([]roachpb.NodeID, error) {
 	for _, str := range strNodeIDs {
 		i, err := strconv.ParseInt(str, 10, 32)
 		if err != nil {
-			return nil, errors.Errorf("unable to parse %s: %s", str, err)
+			return nil, errors.Wrapf(err, "unable to parse %s", str)
 		}
 		nodeIDs = append(nodeIDs, roachpb.NodeID(i))
 	}

pkg/cli/testutils.go

@@ -441,7 +441,7 @@ func GetCsvNumCols(csvStr string) (cols int, err error) {
 	reader := csv.NewReader(strings.NewReader(csvStr))
 	records, err := reader.Read()
 	if err != nil {
-		return 0, errors.Errorf("error reading csv input: \n %v\n errors:%s", csvStr, err)
+		return 0, errors.Wrapf(err, "error reading csv input:\n %v\n", csvStr)
 	}
 	return len(records), nil
 }
@@ -451,8 +451,8 @@ func GetCsvNumCols(csvStr string) (cols int, err error) {
 func MatchCSV(csvStr string, matchColRow [][]string) (err error) {
 	defer func() {
 		if err != nil {
-			err = errors.Errorf("csv input:\n%v\nexpected:\n%s\nerrors:%s",
-				csvStr, pretty.Sprint(matchColRow), err)
+			err = errors.Wrapf(err, "csv input:\n%v\nexpected:\n%s\n",
+				csvStr, pretty.Sprint(matchColRow))
 		}
 	}()
 
@@ -482,8 +482,8 @@ func MatchCSV(csvStr string, matchColRow [][]string) (err error) {
 			pat, str := matchColRow[i][j], records[i][j]
 			re := regexp.MustCompile(pat)
 			if !re.MatchString(str) {
-				err = errors.Errorf("%v\nrow #%d, col #%d: found %q which does not match %q",
-					err, i+1, j+1, str, pat)
+				err = errors.Wrapf(err, "row #%d, col #%d: found %q which does not match %q",
+					i+1, j+1, str, pat)
 			}
 		}
 	}

pkg/cloud/amazon/s3_storage.go

@@ -433,9 +433,10 @@ func (s *s3Storage) openStreamAt(
 			switch aerr.Code() {
 			// Relevant 404 errors reported by AWS.
 			case s3.ErrCodeNoSuchBucket, s3.ErrCodeNoSuchKey:
-				return nil, errors.WithMessagef(
+				// nolint:errwrap
+				return nil, errors.Wrapf(
 					errors.Wrap(cloud.ErrFileDoesNotExist, "s3 object does not exist"),
-					"%s",
+					"%v",
 					err.Error(),
 				)
 			}

pkg/cloud/azure/azure_storage.go

@@ -156,9 +156,10 @@ func (s *azureStorage) ReadFileAt(
 			switch azerr.ServiceCode() {
 			// TODO(adityamaru): Investigate whether both these conditions are required.
 			case azblob.ServiceCodeBlobNotFound, azblob.ServiceCodeResourceNotFound:
-				return nil, 0, errors.WithMessagef(
+				// nolint:errwrap
+				return nil, 0, errors.Wrapf(
 					errors.Wrap(cloud.ErrFileDoesNotExist, "azure blob does not exist"),
-					"%s",
+					"%v",
 					err.Error(),
 				)
 			}

pkg/cloud/gcp/gcs_storage.go

@@ -196,9 +196,10 @@ func (g *gcsStorage) ReadFileAt(
 			// if file does not exist.  Regardless why we couldn't open the stream
 			// (whether its invalid bucket or file doesn't exist),
 			// return our internal ErrFileDoesNotExist.
-			err = errors.WithMessagef(
+			// nolint:errwrap
+			err = errors.Wrapf(
 				errors.Wrap(cloud.ErrFileDoesNotExist, "gcs object does not exist"),
-				"%s",
+				"%v",
 				err.Error(),
 			)
 		}

pkg/cloud/httpsink/http_storage.go

@@ -266,9 +266,10 @@ func (h *httpStorage) req(
 		_ = resp.Body.Close()
 		err := errors.Errorf("error response from server: %s %q", resp.Status, body)
 		if err != nil && resp.StatusCode == 404 {
-			err = errors.WithMessagef(
+			// nolint:errwrap
+			err = errors.Wrapf(
 				errors.Wrap(cloud.ErrFileDoesNotExist, "http storage file does not exist"),
-				"%s",
+				"%v",
 				err.Error(),
 			)
 		}

pkg/cloud/nodelocal/nodelocal_storage.go

@@ -144,6 +144,7 @@ func (l *localFileStorage) ReadFileAt(
 		// The local store returns a golang native ErrNotFound, whereas the remote
 		// store returns a gRPC native NotFound error.
 		if oserror.IsNotExist(err) || status.Code(err) == codes.NotFound {
+			// nolint:errwrap
 			return nil, 0, errors.WithMessagef(
 				errors.Wrap(cloud.ErrFileDoesNotExist, "nodelocal storage file does not exist"),
 				"%s",

pkg/cmd/github-post/main.go

@@ -503,8 +503,8 @@ func getFileLine(
 	// ../ccl/storageccl/export_test.go:31:func TestExportCmd(t *testing.T) {
 	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return "", "", errors.Errorf("couldn't find test %s in %s: %s %s",
-			testName, packageName, err, string(out))
+		return "", "", errors.Wrapf(err, "couldn't find test %s in %s: %s\n",
+			testName, packageName, string(out))
 	}
 	re := regexp.MustCompile(`(.*):(.*):`)
 	// The first 2 :-delimited fields are the filename and line number.

pkg/cmd/roachtest/tests/encryption.go

@@ -53,7 +53,7 @@ func registerEncryption(r registry.Registry) {
 		testCLIGenKey := func(size int) error {
 			// Generate encryption store key through `./cockroach gen encryption-key -s=size aes-size.key`.
 			if err := c.RunE(ctx, c.Node(nodes), fmt.Sprintf("./cockroach gen encryption-key -s=%[1]d aes-%[1]d.key", size)); err != nil {
-				return errors.Errorf("failed to generate aes key with size %d through CLI, got err %s", size, err)
+				return errors.Wrapf(err, "failed to generate AES key with size %d through CLI", size)
 			}
 
 			// Check the size of generated aes key has expected size.

pkg/cmd/roachtest/tests/schema_change_database_version_upgrade.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/version"
 	"github.com/cockroachdb/errors"
@@ -91,7 +92,7 @@ func runSchemaChangeDatabaseVersionUpgrade(
 	assertDatabaseNotResolvable := func(ctx context.Context, db *gosql.DB, dbName string) error {
 		_, err = db.ExecContext(ctx, fmt.Sprintf(`SELECT table_name FROM [SHOW TABLES FROM %s]`, dbName))
 		if err == nil || err.Error() != "pq: target database or schema does not exist" {
-			return errors.AssertionFailedf("unexpected error: %s", err)
+			return errors.Newf("unexpected error: %s", pgerror.FullError(err))
 		}
 		return nil
 	}

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/col/coldatatestutils/random_testutils.go

@@ -216,7 +216,7 @@ func setNull(rng *rand.Rand, vec coldata.Vec, i int) {
 	case types.DecimalFamily:
 		_, err := vec.Decimal()[i].SetFloat64(rng.Float64())
 		if err != nil {
-			colexecerror.InternalError(errors.AssertionFailedf("%v", err))
+			colexecerror.InternalError(errors.NewAssertionErrorWithWrappedErrf(err, "could not set decimal"))
 		}
 	case types.IntervalFamily:
 		vec.Interval()[i] = duration.MakeDuration(rng.Int63(), rng.Int63(), rng.Int63())

pkg/geo/geos/geos.go

@@ -654,11 +654,11 @@ func PrepareGeometry(a geopb.EWKB) (PreparedGeometry, error) {
 func PreparedGeomDestroy(a PreparedGeometry) {
 	g, err := ensureInitInternal()
 	if err != nil {
-		panic(errors.AssertionFailedf("trying to destroy PreparedGeometry with no GEOS: %v", err))
+		panic(errors.NewAssertionErrorWithWrappedErrf(err, "trying to destroy PreparedGeometry with no GEOS"))
 	}
 	ap := (*C.CR_GEOS_PreparedGeometry)(unsafe.Pointer(a))
 	if err := statusToError(C.CR_GEOS_PreparedGeometryDestroy(g, ap)); err != nil {
-		panic(errors.AssertionFailedf("PreparedGeometryDestroy returned an error: %v", err))
+		panic(errors.NewAssertionErrorWithWrappedErrf(err, "PreparedGeometryDestroy returned an error"))
 	}
 }
 

pkg/gossip/client.go

@@ -263,8 +263,8 @@ func (c *client) handleResponse(ctx context.Context, g *Gossip, reply *Response)
 		// The certificates (if any) may only be valid for the unresolved
 		// address.
 		if _, err := reply.AlternateAddr.Resolve(); err != nil {
-			return errors.Errorf("unable to resolve alternate address %s for n%d: %s",
-				reply.AlternateAddr, reply.AlternateNodeID, err)
+			return errors.Wrapf(err, "unable to resolve alternate address %s for n%d",
+				reply.AlternateAddr, reply.AlternateNodeID)
 		}
 		c.forwardAddr = reply.AlternateAddr
 		return errors.Errorf("received forward from n%d to n%d (%s)",

pkg/jobs/job_scheduler_test.go

@@ -616,7 +616,7 @@ func TestJobSchedulerDaemonUsesSystemTables(t *testing.T) {
 		var count int
 		if err := db.QueryRow(
 			"SELECT count(*) FROM defaultdb.foo").Scan(&count); err != nil || count != 3 {
-			return errors.Newf("expected 3 rows, got %d (err=%+v)", count, err)
+			return errors.Newf("expected 3 rows, got %d (err=%+v)", count, err) // nolint:errwrap
 		}
 		return nil
 	})