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

proposal: cmd/go: run the errors.As vet check as part of `go test` #31213

Closed
jba opened this issue Apr 2, 2019 · 7 comments
Closed

proposal: cmd/go: run the errors.As vet check as part of `go test` #31213

jba opened this issue Apr 2, 2019 · 7 comments

Comments

@jba
Copy link
Contributor

@jba jba commented Apr 2, 2019

This new vet check validates that the second argument to errors.As is a pointer to an interface or a type implementing error.

It can generate a false positive if a valid value is held in a different static type, such as

var e error
var i interface{} = &e
... errors.As(..., i) ...

but that's unlikely.

@andybons andybons changed the title cmd/go: run the errors.As vet check as part of `go test` proposal: cmd/go: run the errors.As vet check as part of `go test` Apr 3, 2019
@gopherbot gopherbot added this to the Proposal milestone Apr 3, 2019
@gopherbot gopherbot added the Proposal label Apr 3, 2019
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Apr 17, 2019

Is there a way to get more data on how often this occurs?

Also can we make this more reliable by ignoring the empty interface like in your example above?

@jba

This comment has been minimized.

Copy link
Contributor Author

@jba jba commented May 7, 2019

Is there a way to get more data on how often this occurs?

Unfortunately not, until 1.13 is in general use.

Also can we make this more reliable by ignoring the empty interface like in your example above?

I did this in https://go-review.googlesource.com/c/tools/+/175717.

@andybons andybons removed the WaitingForInfo label May 7, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented May 7, 2019

This is analogous to the existing json.Unmarshal/xml.Unmarshal checks; we know that was a common mistake for those. It tends not to get into production code, although here it might be even more likely. The check is also cheap.

Now that we've decided to add errors.As for Go 1.13, accepting this proposal as well.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented May 7, 2019

Re:

var i interface{} = &e
... errors.As(..., i) ...

we should probably look into what the json.Unmarshal check does, and do the same. (I would expect this code is OK.)

@rsc rsc modified the milestones: Proposal, Go1.13 May 7, 2019
@gopherbot gopherbot removed the NeedsDecision label May 7, 2019
@gopherbot gopherbot removed the NeedsFix label May 7, 2019
@rsc rsc self-assigned this May 9, 2019
@jba

This comment has been minimized.

Copy link
Contributor Author

@jba jba commented May 20, 2019

The json.Unmarshal check doesn't report a problem if the argument is an interface type (any interface type).

Even with the above PR, this check is stricter: it won't report on interface{}, but will on interface{ M() } or any other non-empty interface. That can result in false positives for code like

type Error ...
func(Error) Error() string
func(*Error) M()

var e Error
var i interface{M()} =&e
... errors.As(..., i) ...
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 1, 2019

Change https://golang.org/cl/179977 mentions this issue: internal/test: add -errorsas to default vet checks

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 11, 2019

Change https://golang.org/cl/181717 mentions this issue: cmd/vet: include the errors.As check from upstream x/tools

gopherbot pushed a commit that referenced this issue Jun 11, 2019
This change revendors golang.org/x/tools to include the check and
modifies cmd/vet to add it to the command.

CL 179977 will enable the check by default for 'go test'.

Commands run (starting in GOROOT/src):
	cd cmd
	emacs vet/main.go
	go get -u=patch golang.org/x/tools/go/analysis/passes/errorsas@latest
	go mod tidy
	go mod vendor
	cd ..
	./make.bash
	go test all

Updates #31213

Change-Id: Ic2ba9bd2d31c4c5fd9e7c42ca14e8dc38520c93b
Reviewed-on: https://go-review.googlesource.com/c/go/+/181717
Reviewed-by: Jonathan Amsterdam <jba@google.com>
@gopherbot gopherbot closed this in 0c2953e Jun 12, 2019
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
5 participants
You can’t perform that action at this time.