lint: add new errwrap linter #71877

rafiss · 2021-10-22T17:27:26Z

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

cockroach-teamcity · 2021-10-22T17:27:32Z

This change is

knz · 2021-10-27T22:01:27Z

nb: the linter framework allows you to create unit tests for your linter rules. what would be good tests for this?

knz

tests LGTM but you forgot a few cases.

Reviewed 16 of 16 files at r1, 13 of 13 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @rafiss)

pkg/server/drain_test.go, line 90 at r2 (raw file):

			return nil
		}
		return errors.Newf("server not yet refusing RPC, got %v", err /* nolint:errwrap */)

Can you explain in a comment why the linter exception is needed here?

pkg/testutils/lint/passes/errwrap/errwrap.go, line 104 at r3 (raw file):

		// Check that none of the arguments are err.Error()
		if (pkg.Name() == "fmt" && fn.Name() == "Errorf") ||

Can you implement this via a map? This would be more efficient.

Also is there a way to share this data with the map in ../fmtsafe/functions.go?
(This also reveals you're missing a few here - unimplemented, roachpb, sqlerrors etc)

postamar · 2021-12-13T13:40:49Z

Thanks for doing this! This is valuable.

andreimatei · 2021-12-13T14:51:37Z

Just in case it's not in there already, please make sure there's a way to inhibit the linter manually at code sites.

rickystewart

small request, can you please make the appropriate change for bazel too?

diff --git a/BUILD.bazel b/BUILD.bazel
index f1b5c20697..4e322ae951 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -238,6 +238,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",

rafiss

updated bazel build config

i'm giving this another run now that more error functions are being linted

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @otan)

pkg/server/drain_test.go, line 90 at r2 (raw file):

Previously, knz (kena) wrote…

Can you explain in a comment why the linter exception is needed here?

yes added; it would be incorrect to wrap the error here since this function should return an error if err is nil, and wrapping a nil error results in another nil error.

pkg/testutils/lint/passes/errwrap/errwrap.go, line 104 at r3 (raw file):

Previously, knz (kena) wrote…

Can you implement this via a map? This would be more efficient.

Also is there a way to share this data with the map in ../fmtsafe/functions.go?
(This also reveals you're missing a few here - unimplemented, roachpb, sqlerrors etc)

i made the maps backed by the same thing

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

ajwerner

mod the one testing nit.

Reviewed 1 of 12 files at r4, 10 of 17 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz, @otan, @rafiss, and @rickystewart)

pkg/testutils/lint/passes/errwrap/testdata/src/a/a_test.go, line 63 at r5 (raw file):

	_ = errors.NewAssertionErrorWithWrappedErrf(anotherErr, "format %v", wrappedErr) // want `non-wrapped error is passed to errors.NewAssertionErrorWithWrappedErrf; use pgerror.Wrap/errors.Wrap/errors.CombineErrors/errors.WithSecondaryError/errors.NewAssertionErrorWithWrappedErrf instead`

	_ = fmt.Errorf("got %s", wrappedErr /* nolint:errwrap */)  // linting is skipped.

can you test the // style comment too? It should work.

rickystewart · 2021-12-13T21:04:46Z

The Bazel build surfaced an error, I assume this is legitimate?

21:00:58     compilepkg: nogo: errors found by nogo during build-time code analysis:
21:00:58     /tmp/rules_go_work-3000709617/github.com/cockroachdb/cockroach/pkg/util/httputil/http.go:116:16: non-wrapped error is passed to errors.Errorf; use pgerror.Wrap/errors.Wrap/errors.CombineErrors/errors.WithSecondaryError/errors.NewAssertionErrorWithWrappedErrf instead (errwrap)

Release note: None

knz

Nice, a few nits and this is good to go.

Reviewed 8 of 38 files at r6, 40 of 40 files at r7, 9 of 9 files at r8, 17 of 17 files at r9, 17 of 17 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @otan, @rafiss, and @rickystewart)

pkg/ccl/importccl/read_import_mysql.go, line 767 at r10 (raw file):

					// error.
					// nolint:errwrap
					return nil, errors.Wrapf(

💯

pkg/cli/clierrorplus/decorate_error.go, line 90 at r10 (raw file):

		connInsecureHint := func() error {
			const format = "cannot establish secure connection to insecure server.\n" +

hmm 🤔
is this a legitimate change? This suggests either it would benefit from errors.Wrap, or from a linter exception.

ditto below

pkg/cloud/amazon/s3_storage.go, line 437 at r10 (raw file):

			case s3.ErrCodeNoSuchBucket, s3.ErrCodeNoSuchKey:
				// nolint:errwrap
				return nil, errors.WithMessage(

maybe errors.Wrapf(errors.Wrap(...), "%v", err) to ensure err gets embarked as a secondary error.

pkg/cloud/azure/azure_storage.go, line 160 at r10 (raw file):

			case azblob.ServiceCodeBlobNotFound, azblob.ServiceCodeResourceNotFound:
				// nolint:errwrap
				return nil, 0, errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cloud/gcp/gcs_storage.go, line 200 at r10 (raw file):

			// return our internal ErrFileDoesNotExist.
			// nolint:errwrap
			err = errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cloud/httpsink/http_storage.go, line 270 at r10 (raw file):

		if err != nil && resp.StatusCode == 404 {
			// nolint:errwrap
			err = errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cloud/nodelocal/nodelocal_storage.go, line 146 at r10 (raw file):

		// The local store returns a golang native ErrNotFound, whereas the remote
		// store returns a gRPC native NotFound error.
		// nolint:errwrap

I don't think this linter exception is on the right source code line

pkg/cloud/nodelocal/nodelocal_storage.go, line 148 at r10 (raw file):

		// nolint:errwrap
		if oserror.IsNotExist(err) || status.Code(err) == codes.NotFound {
			return nil, 0, errors.WithMessage(

ditto previous, errors.Wrapf

pkg/cmd/roachtest/tests/encryption.go, line 56 at r10 (raw file):

			// 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.Wrapf(err, "failed to generate aes key with size %d through CLI", size)

nit: aes -> AES

pkg/kv/kvserver/client_lease_test.go, line 669 at r10 (raw file):

				wait(timetoDie.Nanoseconds())
			}
			// nolint:errwrap

nit: you can add the same comment // NB: errors.Wrapf(nil, ...) returns nil. that you added elsewhere.

pkg/kv/kvserver/replica_rangefeed_test.go, line 675 at r10 (raw file):

			}
			err = repl.AdminTransferLease(ctx, roachpb.StoreID(1))
			return errors.Wrapf(err, "not raft follower: %+v, transferred lease", raftStatus)

beware, err can be nil here. I think you want the other thing instead.

pkg/security/securitytest/securitytest.go, line 80 at r10 (raw file):

			if _, dirErr := AssetDir(joined); dirErr != nil {
				errString := dirErr.Error()
				return nil, errors.Wrapf(err, "missing directory (%s)", errString)

you could take the dirErr as secondary error here:

return nil, errors.Wrapf(err, "missing directory (%v)", dirErr /* nolint */)

pkg/util/httputil/http.go, line 116 at r10 (raw file):

	if contentType := resp.Header.Get(ContentTypeHeader); !(resp.StatusCode == http.StatusOK && contentType == JSONContentType) {
		b, err := ioutil.ReadAll(resp.Body)
		// nolint:errwrap

nit: you can add the comment that explains what happens with nil errors here too

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @knz, @otan, @rafiss, and @rickystewart)

pkg/cli/clierrorplus/decorate_error.go, line 90 at r10 (raw file):

Previously, knz (kena) wrote…

hmm 🤔
is this a legitimate change? This suggests either it would benefit from errors.Wrap, or from a linter exception.

ditto below

i made this change in order to match more closely to how the other error decorating functions worked in this file. (e.g. see connSecurityHint above.) i didn't use errors.Wrap here since it seemed like these messages were carefully formatted by hand, and Wrap makes it less obvious what the resulting string will look like. however, if your preference is for Wrap also, then i'd definitely agree with that change

pkg/cloud/amazon/s3_storage.go, line 437 at r10 (raw file):

Previously, knz (kena) wrote…

maybe errors.Wrapf(errors.Wrap(...), "%v", err) to ensure err gets embarked as a secondary error.

done

pkg/cloud/azure/azure_storage.go, line 160 at r10 (raw file):

Previously, knz (kena) wrote…

ditto previous, errors.Wrapf

done

pkg/cloud/gcp/gcs_storage.go, line 200 at r10 (raw file):

Previously, knz (kena) wrote…

ditto previous, errors.Wrapf

done

pkg/cloud/httpsink/http_storage.go, line 270 at r10 (raw file):

Previously, knz (kena) wrote…

ditto previous, errors.Wrapf

done

pkg/cloud/nodelocal/nodelocal_storage.go, line 146 at r10 (raw file):

Previously, knz (kena) wrote…

I don't think this linter exception is on the right source code line

fixed

pkg/cloud/nodelocal/nodelocal_storage.go, line 148 at r10 (raw file):

Previously, knz (kena) wrote…

ditto previous, errors.Wrapf

done

pkg/cmd/roachtest/tests/encryption.go, line 56 at r10 (raw file):

Previously, knz (kena) wrote…

nit: aes -> AES

fixed

pkg/kv/kvserver/client_lease_test.go, line 669 at r10 (raw file):

Previously, knz (kena) wrote…

nit: you can add the same comment // NB: errors.Wrapf(nil, ...) returns nil. that you added elsewhere.

done

pkg/kv/kvserver/replica_rangefeed_test.go, line 675 at r10 (raw file):

Previously, knz (kena) wrote…

beware, err can be nil here. I think you want the other thing instead.

fixed; very nice catch

pkg/security/securitytest/securitytest.go, line 80 at r10 (raw file):

Previously, knz (kena) wrote…

you could take the dirErr as secondary error here:
return nil, errors.Wrapf(err, "missing directory (%v)", dirErr /* nolint */)

done

pkg/testutils/lint/passes/errwrap/testdata/src/a/a_test.go, line 63 at r5 (raw file):

Previously, ajwerner wrote…

can you test the // style comment too? It should work.

done

pkg/util/httputil/http.go, line 116 at r10 (raw file):

Previously, knz (kena) wrote…

nit: you can add the comment that explains what happens with nil errors here too

done

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @knz, @otan, @rafiss, and @rickystewart)

pkg/cli/clierrorplus/decorate_error.go, line 90 at r10 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i made this change in order to match more closely to how the other error decorating functions worked in this file. (e.g. see connSecurityHint above.) i didn't use errors.Wrap here since it seemed like these messages were carefully formatted by hand, and Wrap makes it less obvious what the resulting string will look like. however, if your preference is for Wrap also, then i'd definitely agree with that change

I don't have a strong opinion. If you keep your choice, you can explain it in a comment.

Release note: None

rafiss · 2021-12-15T18:59:39Z

thanks for your reviews!

bors r=ajwerner,knz

craig · 2021-12-15T20:12:47Z

Build failed (retrying...):

GitHub CI (Cockroach)

rafiss · 2021-12-15T20:15:50Z

bors r-

craig · 2021-12-15T20:15:53Z

Canceled.

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

rafiss · 2021-12-15T21:19:05Z

bors r=ajwerner,knz

craig · 2021-12-15T22:11:56Z

Build succeeded:

GitHub CI (Cockroach)

rafiss force-pushed the lint-errwrap branch from e9da193 to 11bb38e Compare October 22, 2021 18:17

rafiss mentioned this pull request Oct 22, 2021

sql: flattening error objects into other errors erases telemetry data and user hints #42510

Closed

rafiss force-pushed the lint-errwrap branch from 11bb38e to b895e80 Compare October 22, 2021 20:51

rafiss force-pushed the lint-errwrap branch from b895e80 to 5212a9f Compare November 17, 2021 06:15

rafiss force-pushed the lint-errwrap branch from 5212a9f to 573da3a Compare December 11, 2021 00:58

rafiss marked this pull request as ready for review December 11, 2021 01:00

rafiss requested review from a team as code owners December 11, 2021 01:00

rafiss requested a review from a team December 11, 2021 01:00

rafiss requested a review from a team as a code owner December 11, 2021 01:00

rafiss requested review from shermanCRL, otan and knz and removed request for a team and shermanCRL December 11, 2021 01:00

knz reviewed Dec 12, 2021

View reviewed changes

rickystewart requested changes Dec 13, 2021

View reviewed changes

rafiss commented Dec 13, 2021

View reviewed changes

ajwerner reviewed Dec 13, 2021

View reviewed changes

pkg/testutils/lint/passes/errwrap/errwrap.go Outdated Show resolved Hide resolved

rafiss force-pushed the lint-errwrap branch from 573da3a to 205e71e Compare December 13, 2021 20:54

ajwerner approved these changes Dec 13, 2021

View reviewed changes

sql: address instances of improper error wrapping

6049f2c

Release note: None

rafiss force-pushed the lint-errwrap branch from 205e71e to fb21750 Compare December 14, 2021 23:37

rafiss requested review from a team December 14, 2021 23:37

rafiss requested review from a team as code owners December 14, 2021 23:37

rafiss requested review from adityamaru and removed request for a team December 14, 2021 23:37

knz reviewed Dec 15, 2021

View reviewed changes

rafiss commented Dec 15, 2021

View reviewed changes

knz approved these changes Dec 15, 2021

View reviewed changes

rafiss force-pushed the lint-errwrap branch from fb21750 to defd9c8 Compare December 15, 2021 17:21

rafiss added 3 commits December 15, 2021 12:29

*: address cases of improper error wrapping

e26432b

Release note: None

server,security: address instances of improper error wrapping

eacaf91

Release note: None

kv: address instances of improper error wrapping

ac332ff

Release note: None

rafiss force-pushed the lint-errwrap branch 2 times, most recently from 335410c to 64e2c0c Compare December 15, 2021 18:59

rafiss force-pushed the lint-errwrap branch from 64e2c0c to 118a446 Compare December 15, 2021 20:24

craig bot merged commit 2c590e2 into cockroachdb:master Dec 15, 2021

rafiss deleted the lint-errwrap branch December 16, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: add new errwrap linter #71877

lint: add new errwrap linter #71877

rafiss commented Oct 22, 2021 •

edited

Loading

cockroach-teamcity commented Oct 22, 2021

knz commented Oct 27, 2021

knz left a comment

postamar commented Dec 13, 2021

andreimatei commented Dec 13, 2021

rickystewart left a comment

rafiss left a comment

ajwerner left a comment

rickystewart commented Dec 13, 2021

knz left a comment

rafiss left a comment

knz left a comment

rafiss commented Dec 15, 2021

craig bot commented Dec 15, 2021

rafiss commented Dec 15, 2021

craig bot commented Dec 15, 2021

rafiss commented Dec 15, 2021

craig bot commented Dec 15, 2021

lint: add new errwrap linter #71877

lint: add new errwrap linter #71877

Conversation

rafiss commented Oct 22, 2021 • edited Loading

cockroach-teamcity commented Oct 22, 2021

knz commented Oct 27, 2021

knz left a comment

Choose a reason for hiding this comment

postamar commented Dec 13, 2021

andreimatei commented Dec 13, 2021

rickystewart left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

rickystewart commented Dec 13, 2021

knz left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

rafiss commented Dec 15, 2021

craig bot commented Dec 15, 2021

rafiss commented Dec 15, 2021

craig bot commented Dec 15, 2021

rafiss commented Dec 15, 2021

craig bot commented Dec 15, 2021

rafiss commented Oct 22, 2021 •

edited

Loading