Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/go/analysis/passes/nilness: detect wrapping of nil errors #32808

Open
ainar-g opened this issue Jun 27, 2019 · 3 comments
Open

x/tools/go/analysis/passes/nilness: detect wrapping of nil errors #32808

ainar-g opened this issue Jun 27, 2019 · 3 comments

Comments

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Jun 27, 2019

As of 4874f863, the nilness check doesn't seem to trigger on fmt.Errorf calls with an error that is proven nil. Example:

func f() error {
	exists, err := g()
	if err != nil {
		return errors.Wrap(err, "error1")
	}

	if !exists {
		return fmt.Errorf("nothing: %w", err)
	}

	return nil
}

Same with the %v verb. I feel like most people would not want to actually produce a nothing: <nil> or a nothing: %!w(<nil>) message here.

Inspired by dominikh/go-tools#529.

@andybons
Copy link
Member

@andybons andybons commented Jun 28, 2019

@andybons andybons added this to the Unreleased milestone Jun 28, 2019
@jba
Copy link
Contributor

@jba jba commented Jul 5, 2019

Currently the nilness check only knows about the Go language, not the standard library. We could extend it to check static calls to known functions, maybe along the lines of https://golang.org/cl/184939. Then we could borrow format-parsing code from the printf check to perform the %w check.

I don't know if any of this is a good idea or not. @matloob, @ianthehat, @alandonovan, your thoughts?

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Aug 8, 2019

The nilness check could be made to return a result value to other analyses through which they could query whether a function call argument is nil. The Printf checker (and others) could then use it to refine their heuristics.

However, that would make the printf check depend on nilness, and the standard vet contains only the watered-down nilfunc check, not full nilness, because the latter depends on golang.org/x/tools/go/ssa, which is more expensive to compute. We could evaluate whether the benefit of better checks exceeds the cost of computing full SSA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.