Proposal: Optional cause

[sheenobu]

The current errors.Cause method will gladly return a nil'error if given a nil error (this is expected). However, if the bottom of your error stack is a Causer that also returns nil, Cause returns nil. Example here: https://play.golang.org/p/wQa9urzGX8

The proposed change is simple enough as checking for nil in the errors.Cause loop (If there are additional complexities I haven't considered, please let me know).

func Cause(err error) error {
    type causer interface {
        Cause() error
    }

    for err != nil {
        cause, ok := err.(causer)
        if !ok {
            break
        }

        if err2 := cause.Cause(); err2 != nil {
            err = err2
        } else {
            break
        }
    }
    return err
}

[davecheney]

Is the bug that the Cause implmentation on myCodedError returns whatever is in its err field without checking it is nil?

Could this be solved by changing the contract on the causer interface to specific that Cause should not return nil? I can see the argument for allow Cause to return nil, but i'd like to understand what the use case for this is.

[sheenobu]

Most important point: This is about errors.Cause working without any surprises. errors.Cause() returning nil when the input is non-nil is a surprise. Neither the code nor the documentation cover this.

Is the bug that the Cause implmentation on myCodedError returns whatever is in its err field without checking it is nil?

Yes.

Could this be solved by changing the contract on the causer interface to specific that Cause should not return nil? I can see the argument for allow Cause to return nil, but i'd like to understand what the use case for this is.

Yes it could be solved by changing the contract of causer. It would be a documentation issue then, correct?

The use case for this is interoping with custom error types which implement causer and whether they'll always return non-nil errors. In some cases this could be a programming mistake and other cases it could be purposeful.

Do we mandate non-nil errors via documentation or a panic? Or treat nil errors from causer as 'this causer has no underlying error, so return it EDIT: it being the causer, not the nil underlying error'.

[tscholl2]

I just ran into this, and it was a surprise to me. Here is the code I used: https://play.golang.org/p/RYXqML2VAw

Basically, gorilla's securecookie happens to have the same interface, but with different semantics. I did not realize this, and was very confused why it was returning nil.

I love this errors package, and it's never failed me. I guess my two cents would be that if an error is not nil, but it's Cause() returns nil, then errors.Cause should return the original error, if that makes sense. I understand if this goes against your original intention though.

[davecheney]

I think this would be ok; - we tighten the contract for Causer.Cause and say if it is present on a type it _must_ return an error - or, we change the contract for errors.Cause to if err, ok := err.(Causer); ok { e2 := err.Cause() if e2 == nil { return err } // otherwise recuse } I think the second change is better.

…

On Sun, Jan 15, 2017 at 3:32 PM, Travis ***@***.***> wrote: I just ran into this, and it was a surprise to me. Here is the code I used: https://play.golang.org/p/RYXqML2VAw Basically, gorilla's securecookie happens to have the same interface, but with different semantics. I did not realize this, and was very confused why it was returning nil. I love this errors package, and it's never failed me. I guess my two cents would be that if an error is not nil, but it's Cause() returns nil, then errors.Cause should return the original error, if that makes sense. I understand if this goes against your original intention though. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#89 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcA8NgepsJanJNTE_Q3CaefBMWB4kJks5rSaFIgaJpZM4KeLZJ> .

[tscholl2]

I agree. That second change is almost exactly what I used in my code right now as a work around. I also think the second way matches semantically with what the docs say (with some liberal interpretation):

Cause returns the underlying cause of the error, if possible.

I guess I'm assuming a non-nil error always has a non-nil cause. But I guess in programming, it often seems like things break for no reason :)

Would you like me to submit a PR with this? I think it might also be good to add a quick sentence in the docs right after the other sentences about the causer interface. Something like: "If Cause() returns nil, the original error is returned". Or maybe a warning about how other packages may have the same cause interface with different semantics?

[chanxuehong]

is this ok?
https://github.com/chanxuehong/errors/blob/master/func.go#L90

[tscholl2]

That looks good to me. If I'm reading it right, a non-nil error always has a non-nil cause, which was my original issue. The only thing I might suggest is adding a sentence to the doc saying this.

[davecheney]

Not necessarily; errors.New does not return an error with a Cause.

…

On 21 Oct 2017, at 01:33, Travis ***@***.***> wrote: That looks good to me. If I'm reading it right, a non-nil error always has a non-nil cause, which was my original issue. The only thing I might suggest is adding a sentence to the doc saying this. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[jaekwon]

Ok, I just ran into this, but what I expected was to be able to do this:

func (err *sdkError) Cause() error {
	if err.cause != nil {
		return err.cause
	}
	return err
}

Sometimes *sdkError has a .cause, sometimes it doesn't -- it's up to the user. And when an error doesn't have another cause to blame, it is the underlying cause. Right? :)

• If you think Cause means ImmediateCause, then you'd expect the underlying error to return nil on Cause.
• If you think Cause means UnderlyingCause, then you'd expect to always return the root/underlying error and never nil or any intermediary errors.
• If you think Cause means BlameCause, then you'd return the immediate cause, or the current error if you couldn't blame another error.

Made a PR (see link) to handle all use-cases (return nil or self as cause).

[puellanivis]

I don’t think a Cause() should ever be expected to return nil, however, I do suppose it is a possible return value from an implementation, and as such, it should be handled gracefully.

[jaekwon]

@davecheney:

I would appreciate it if you can help me understand, on #89, not this
PR, your use case, and why this change is necessary, and could your
requirement be handled without changing the operation of Cause?

I wrote a bit up there (see sdkError example).

We need sdkError because in the SDK, every such error should also have a numeric code for the type of error. When you create an sdkError, you specify a numeric code and any log information for debugging purposes. Sometimes you'll want to attach an immediate cause for creating the sdkError, but other times there will be no immediate cause. So sdkError.Cause() sometimes needs to return nil/itself, and sometimes another error.

https://github.com/cosmos/cosmos-sdk/blob/62f736fbd047194aaee8cb68d2072ca316eec550/errors/errors.go#L155

Of course it would be sufficient to allow errors.Cause() to handle nil return values from causer.Cause(), but it would also be nice to allow the implementor of causer to return itself as the cause, signifying that it is the underlying cause. It would have been the least surprising behavior for me.

[davecheney]

It’s probably not specified explicitly but implementing cause then returning nil is not expected. I don’t know if it will be possible to retrofit this, I’ll have to survey the existing users of this package. I recommend that you return two different errors, one that implants cause if it has a cause, and another they does not implement cause. Alternatively, rather that implementing your own error type that implements cause, use one of the With* methods to wrap your original error, the one that does not implement cause.

…

On 22 Dec 2017, at 13:03, Jae Kwon ***@***.***> wrote: @davecheney: I would appreciate it if you can help me understand, on pkg#89, not this PR, your use case, and why this change is necessary, and could your requirement be handled without changing the operation of Cause? I wrote a bit up here (see sdkError example). We need sdkError because in the SDK, every such error should also have a numeric code for the type of error. When you create an sdkError, you specify a numeric code and any log information for debugging purposes. Sometimes you'll want to attach an immediate cause for creating the sdkError, but other times there will be no immediate cause. So sdkError.Cause() sometimes needs to return nil/itself, and sometimes another error. https://github.com/cosmos/cosmos-sdk/blob/62f736fbd047194aaee8cb68d2072ca316eec550/errors/errors.go#L155 Of course it would be sufficient to allow errors.Cause() to handle nil return values from causer.Cause(), but it would also be nice to allow the implementor of causer to return itself as the cause, signifying that it is the underlying cause. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jaekwon]

I can't use With* to wrap the cause because it needs to be wrapped by sdkError, but I can do the former of having two errors, one that embeds the other and also has a cause field.

Thank you, and looking forward to the results of the survey. LMK if I can help.

[gregwebs]

Can we create a new RootCause function and deprecate the existing Cause function? That will satisfy backwards compatibility. Cause would only be dropped in a breaking release.

[davecheney]

Hi Greg,

What would this new RootCause function do?

[gregwebs]

It would just fix this bug.