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

Jump to bottom

proposal: errors: allow implementing err.Is(err) == false #40675

Closed
carnott-snap opened this issue Aug 10, 2020 · 12 comments
Closed

proposal: errors: allow implementing err.Is(err) == false #40675

carnott-snap opened this issue Aug 10, 2020 · 12 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@carnott-snap
Copy link

background

Currently you can implement, func (MyError) Is(target) bool { return true }, but MyError.Is will only be called if target != nil, see #40673, and !reflectlite.TypeOf(target).Comparable() || err != target. This can be confusing to implementers of interface{ Is(error) bool }, because it disallows impure implementations and explicitly signalling that errors are not equal.

var (
        flip bool
        log []ImpureError
)

func (e ImpureError) Is(error) bool { flip = !flip; log = append(log, e); return flip }
type NonCompError string

const ErrBad NonCompError = "bad"

func (e NonCompError) Is(target error) { return e == target && target == ErrBad }

description

I propose moving the isComparable check from errors.Is down below the call to x.Is(target). This will hand full control of equality checking to interface { Is(error) bool }`, and make the user experience more consistent.

--- a/src/errors/wrap.go
+++ b/src/errors/wrap.go
@@ -43,10 +43,10 @@ func Is(err, target error) bool {
 
        isComparable := reflectlite.TypeOf(target).Comparable()
        for {
-               if isComparable && err == target {
+               if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) {
                        return true
                }
-               if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) {
+               if isComparable && err == target {
                        return true
                }
                // TODO: consider supporting target.Is(err). This would allow

costs

It is my understanding that few people have implemented interface { Is(error) bool }, such that ``((err == target) == false) == err.Is(target)`, however if you have, you are either relying upon undefined behaviour or have a bug. That being said, this is a semantic change, so we should be careful to understand what the impact is.

alternatives

If the current implementation is preferred, it should be explicitly documented. [I]f it is equal to that target or if it implements a method Is(error) bool such that Is(target) returns true does not describe precedence, short circuiting, or function purity. I think we should either document:

  • if interface{ Is(error) bool } implementations should be pure
    • if so, can we add a vet check to guarantee this?
  • what order checks happen in
    • callers writing global state need to know if they will be called
    • callers consuming global state or overriding err == target may cause ((err == target) == false) == err.Is(target)
@gopherbot gopherbot added this to the Proposal milestone Aug 10, 2020
@carnott-snap carnott-snap mentioned this issue Aug 10, 2020
Closed
@ianlancetaylor
Copy link
Contributor

CC @neild @jba

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

neild commented Aug 10, 2020

It was a deliberate choice to not permit Is to declare that errors are not equal. It would be confusing if err1 == err2, but errors.Is(err1, err2) == false.

I do not believe we should make any changes to support non-pure errors.

@carnott-snap
Copy link
Author

None of this nuance has been well conveyed to users. Bare minimum we need to update the documentation, and should add the vet check too, if this is a canonical opinion.

That being said, the interface{ Is(error) bool } syntax continues this confusion by implying that it is possible to return true and override the equality check, but instead it behaves unintuitive. (Though it is not undefined behaviour.) It was only clear when I read the errors.Is implementation, and this should not be required. Furthermore, you can get into weird edge cases because of the complexity: e.g. by adding a non-comparable field, you can skip the runtime equality check.

@rsc rsc changed the title proposal: errors: Is should check equality second proposal: errors: allow implementing Is(err1, err1) == false Aug 11, 2020
@rsc rsc changed the title proposal: errors: allow implementing Is(err1, err1) == false proposal: errors: allow implementing err.Is(err) == false Aug 11, 2020
@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

Based on the conversation above, this seems like a likely decline.
The weight of history weighs far too much against breaking all existing Go code in the world for a cleanup.

@rsc rsc moved this from Incoming to Likely Decline in Proposals (old) Aug 12, 2020
@carnott-snap
Copy link
Author

carnott-snap commented Aug 12, 2020 •
edited

This is not a breaking change.

@rsc
Copy link
Contributor

rsc commented Aug 14, 2020

Allowing errors.Is(err1, err1) == false will certainly break Go code and flies in the face of a decade of assuming that errors are equal to themselves. No.

@rsc rsc mentioned this issue Aug 14, 2020
Open
@carnott-snap
Copy link
Author

You seem to be arguing about the abstract idea of error nilness is being violated, that is orthogonal to my definition of a breaking change: e.g. "will existing code break" (either at runtime or compile time). (I also make the assumption that nobody will introduce breaking semantic changes, similar to starting to return a wrapped ErrXxx.)

I am understanding if you hold this idea as more important, but I want to be clear, I do not understand how this change is breaking. It is also definitely confusing that I can implement functionality in an interface{ Is(error) bool } method that will never be called. (Technically, documenting it would be an improvement, but actually having a syntax that disallows expressing these absurd conditions is better.)

@ianlancetaylor
Copy link
Contributor

@carnott-snap Better docs are always appreciated.

Discussions about the exact nature of a breaking change take precious time and don't seem likely to lead to a productive result. As @rsc says, allowing errors.Is(err, err) == false is a choice we're not going to make. Let's move on. If necessary, let's disagree and commit. Thanks.

@rsc
Copy link
Contributor

rsc commented Aug 26, 2020

No change in consensus, so declined.
(I'm sorry if we haven't been clear enough for you about what is and is not a breaking change, but I don't know how to be clearer at this point.)

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Aug 26, 2020
@rsc rsc closed this as completed Aug 26, 2020
@carnott-snap
Copy link
Author

No, I see your reasons to not do this, just being pedantic about what constitutes a breaking change.

@rsc
Copy link
Contributor

rsc commented Aug 26, 2020

Being pedantic when you understand what we mean is kind of a waste of everyone's time. Please don't do that.

@carnott-snap
Copy link
Author

Precision and wording is important; I understand why you are opposed to the change, but still do not know what you define as a breaking change.

@golang golang locked and limited conversation to collaborators Aug 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
5 participants
@neild @rsc @ianlancetaylor @gopherbot @carnott-snap