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: 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
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Aug 10, 2020

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)
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 10, 2020

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

@neild 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

@carnott-snap carnott-snap commented Aug 11, 2020

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 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 Aug 12, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 12, 2020

This is not a breaking change.

@rsc
Copy link
Contributor

@rsc 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.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 14, 2020

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

@ianlancetaylor ianlancetaylor commented Aug 14, 2020

@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 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 Aug 26, 2020
@rsc rsc closed this Aug 26, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 26, 2020

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

@rsc
Copy link
Contributor

@rsc 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

@carnott-snap carnott-snap commented Aug 27, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
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.