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

[carnott-snap]

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]

CC @neild @jba

[neild]

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]

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]

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.

[carnott-snap]

This is not a breaking change.

[rsc]

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]

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]

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

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.)

[carnott-snap]

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

[rsc]

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

[carnott-snap]

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.