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

cmd/vet: ensure errors implement Is, As, and Unwrap anonymous interfaces #40356

Closed
carnott-snap opened this issue Jul 22, 2020 · 11 comments
Closed

cmd/vet: ensure errors implement Is, As, and Unwrap anonymous interfaces #40356

carnott-snap opened this issue Jul 22, 2020 · 11 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals (old)
Milestone
Go1.17

Comments

@carnott-snap
Copy link

carnott-snap commented Jul 22, 2020 •
edited

The interfaces for errors.Is and errors.As seem stable and complete. Unfortunately, it requires reading through the godoc to ensure that your custom error type correctly implements interface{ As(interface{}) bool }, interface{ Is(error) bool }, or interface{ Unwrap() error }. This can lead to implementation bugs that fail pathologically.

Instead of exposing these symbols, as was proposed in #39539, we can add a vet check to ensure that errors with Is, As, or Unwrap methods implement the correct interfaces. This is not a requirement that all errors implement Is, As, and Unwrap, but simply that if the symbol exists, the syntax matches.

Technically, this is not required, as you can use an anonymous type to perform this check at compile time. But this is ungainly, not automatic, and fails to the same typographical issues due to its distributed implementation:

var (
        _ = (interface{ As(interface{}) bool })(myError{})
        _ = (interface{ Is(error) bool })(myError{})
        _ = (interface{ Unwrap() error })(myError{})
        _ = error(myError{})
@gopherbot gopherbot added this to the Proposal milestone Jul 22, 2020
@ainar-g ainar-g mentioned this issue Jul 24, 2020
Open
@carnott-snap
Copy link
Author

@ianlancetaylor: can you add this to the proposals project?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 6, 2020
@ianlancetaylor
Copy link
Contributor

@carnott-snap Done.

@urandom2 urandom2 mentioned this issue Apr 14, 2021
Closed
@urandom2
Copy link
Contributor

did this get lost in the incoming stack?

@ianlancetaylor
Copy link
Contributor

The incoming stack has more than 200 issues in it, so it's going to take a while to get through them all.

See https://github.com/golang/go/projects/1 .

@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Apr 21, 2021
@rsc rsc mentioned this issue Apr 21, 2021
Open
@neild
Copy link
Contributor

neild commented Apr 21, 2021

I don't object to cmd/vet checking the signature of As/Is/Unwrap methods for types that implement the error interface; it seems unlikely to have significant false positives. However, the best way to ensure an error implements these interfaces correctly is to write a test for the error, because a test will validate not only the method signature but the implementation.

@gopherbot
Copy link

Change https://golang.org/cl/312829 mentions this issue: passes/stdmethods: warn when an error has an unexpected Is, As, or Unwrap method.

@rsc
Copy link
Contributor

rsc commented Apr 28, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Apr 28, 2021
@timothy-king
Copy link
Contributor

Tested golang.org/cl/312829 against a large Go codebase that uses Is, As and Unwrap. No findings so no false positives.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) May 5, 2021
@rsc
Copy link
Contributor

rsc commented May 5, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/vet: ensure errors implement Is, As, and Unwrap anonymous interfaces cmd/vet: ensure errors implement Is, As, and Unwrap anonymous interfaces May 5, 2021
@rsc rsc modified the milestones: Proposal, Backlog May 5, 2021
gopherbot pushed a commit to golang/tools that referenced this issue May 5, 2021
@timothy-king
passes/stdmethods: warn when an Is, As, or Unwrap has the wrong signa…
1949673 
…ture.

Extend stdmethods to warn when a type implementing error has an Is, As, or Unwrap method with a different signature than the one expected by the errors package.

For golang/go#40356

Change-Id: Ia196bb83a685340d6dab216b87c8a4aa2846f785
Reviewed-on: https://go-review.googlesource.com/c/tools/+/312829
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/321389 mentions this issue: cmd: go get golang.org/x/tools/analysis@49064d23 && go mod vendor

@dmitshur dmitshur modified the milestones: Backlog, Go1.17 May 20, 2021
@D1CED D1CED mentioned this issue Apr 20, 2022
Closed
@golang golang locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
No open projects
Proposals (old)
Accepted
Milestone
Go1.17
Development

No branches or pull requests
8 participants
@neild @rsc @timothy-king @dmitshur @urandom2 @ianlancetaylor @gopherbot @carnott-snap