go-tools icon indicating copy to clipboard operation
go-tools copied to clipboard

staticcheck: detect `error.Wrap` on `nil` error

Open bartekn opened this issue 6 years ago • 8 comments

github.com/pkg/errors is a very popular package that provides helper methods for creating errors. One of the methods is errors.Wrap(err error, message string) error however, it's easy to forget that:

If err is nil, Wrap returns nil.

Recently, while reviewing PRs by two different developers, I noticed a code like:

	exists, object, err := GetObject(id)
	if err != nil {
		return errors.Wrap(err, "error1")
	}
	// err == nil at this point
	if !exists {
		return errors.Wrap(err, "error2")
	}
	// ...more code using `object` here...

You can see that the second return always returns nil (to fix this, developer should use errors.New instead of errors.Wrap).

Would be great to add a check that finds instances of this. I couldn't find any contribution guide, do you have any? Are you be willing to accept a PR with a new check? This is really connected to a single package so I wasn't sure you want to include such check.

bartekn avatar Jun 27 '19 13:06 bartekn

I expected the nilness tool to catch it, but surprisingly it didn't. Neither did it catch it with fmt.Errorf with the new %w verb. I've filed a separate issue there.

ainar-g avatar Jun 27 '19 15:06 ainar-g

Are you be willing to accept a PR with a new check?

Normally I am open to PRs, but not so much in this case. This check (and checks related to it) are something I want to actively experiment with myself.

dominikh avatar Jun 28 '19 01:06 dominikh

Sounds good. Looking forward to checking it!

bartekn avatar Jun 28 '19 09:06 bartekn

Are you be willing to accept a PR with a new check?

Normally I am open to PRs, but not so much in this case. This check (and checks related to it) are something I want to actively experiment with myself.

It has been almost 5 years by now and this feature still appears to be missing. This kind of issue crashed our server recently - a linter could have prevented that.

Any plans to finally check this?

JannikGM avatar Feb 26 '24 12:02 JannikGM

@JannikGM I switched to CodeQL for finding this specific issue. Check this query: https://github.com/github/codeql/blob/main/go/ql/src/InconsistentCode/WrappedErrorAlwaysNil.ql

bartekn avatar Feb 26 '24 12:02 bartekn

I don't think CodeQL works for us (license wise). We need to be able to run it locally and on CI for private repositories.

I also tried to use CodeQL locally (for evaluation wether it might be worth it to purchase it) but couldn't even figure out how to use it.

I'd definitely prefer a solution in staticcheck or golangci-lint.

JannikGM avatar Feb 26 '24 13:02 JannikGM

CodeQL is released under MIT license: https://github.com/github/codeql/blob/main/LICENSE

bartekn avatar Feb 26 '24 13:02 bartekn

That's only the queries, but not the engine and database tooling to use them (which is far more expensive and restrictive).

From: https://github.com/github/codeql?tab=readme-ov-file#license

The CodeQL CLI (including the CodeQL engine) is hosted in a different repository and is licensed separately. If you'd like to use the CodeQL CLI to analyze closed-source code, you will need a separate commercial license; please contact us for further help.

https://github.com/github/codeql-cli-binaries/blob/main/LICENSE.md

JannikGM avatar Feb 26 '24 13:02 JannikGM