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.

Sign up for GitHub

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

Jump to bottom

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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal Proposal-Accepted release-blocker
Milestone
Go1.13

Comments

@jba
Copy link
Contributor

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
@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 12, 2019
@andybons
Copy link
Member

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?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 19, 2019
@jba
Copy link
Contributor Author

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 Issue is not actionable because of missing required information, which needs to be provided. label May 7, 2019
@rsc
Copy link
Contributor

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
Copy link
Contributor

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 added Proposal-Accepted NeedsFix The path to resolution is known, but the work has not been done. labels May 7, 2019
@rsc rsc modified the milestones: Proposal, Go1.13 May 7, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 7, 2019
@rsc rsc added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels May 7, 2019
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label May 7, 2019
@rsc rsc self-assigned this May 9, 2019
@jba
Copy link
Contributor Author

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
Copy link

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

@gopherbot
Copy link

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
cmd/vet: include the errors.As check from upstream x/tools
9f76566 
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>
@golang golang locked and limited conversation to collaborators Jun 11, 2020
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal Proposal-Accepted release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@rsc @andybons @bcmills @gopherbot @jba