Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

Support Go 1.13 error chains in Cause #215

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Conversation

jayschwa
Copy link
Contributor

Imagine module A imports module B and both use pkg/errors. A uses
errors.Cause to inspect wrapped errors returned from B. As-is, B
cannot migrate from errors.Wrap to fmt.Errorf("%w", err) because
that would break errors.Cause calls in A. With this change merged,
errors.Cause becomes forwards-compatible with Go 1.13 error chains.
Module B will be free to switch to fmt.Errorf("%w", err) and that will
not break module A (so long as the top-level project pulls in the newer
version of pkg/errors).

cause_go113.go Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably switch this to a type switch now:

switch v := err.(type) {
case causer:
	err = v.Cause()
case unwrapper:
	cause := errors.Unwrap(err)
	if cause == nil {
		return err
	}
	err = cause
default:
	return err
}

"testing"
)

func TestErrorChainCompat(t *testing.T) {
func TestCauseErrorChainCompat(t *testing.T) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also recommend also testing a WithMessage(fmt.Errorf("wrap2: %w", WithMessage(err, "wrap1"))), "wrap3")

cause_go113.go Outdated
// }
//
// If the error does not implement the Cause interface, the Cause
// function will fallback to the standard library's errors.Unwrap

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s be more explicitly clear about the contract we are building: will we return the original error of any arbitrary ordering of Cause and Unwrap? The text presented here is potentially ambiguous… hm, I think the original was somewhat ambiguous as well.

We should be clear that this function will keep working recursively until it finds an error that is both not a causer and would return errors.Unwrap(err) == nil.

Copy link
Contributor Author

@jayschwa jayschwa Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an error has both Cause() and Unwrap(), I think Cause() should take precedence since that matches the original behavior of errors.Cause.

Copy link

@puellanivis puellanivis Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s not what I am saying. If we have a causer wrapped by a fmt.Errorf wrapped error, which wraps a causer, what is the behavior?

The code as you have written would unwrap all layers (which I think it should), but we should document that it will “recursively” try unwrapping all layers until it gets an unwrapped error.

@aperezg aperezg added this to the 0.9 milestone Dec 13, 2019
@aperezg
Copy link
Member

aperezg commented Dec 13, 2019

@jayschwa If I'm not wrong you're agree with the changes propose for @puellanivis so, could you do this changes, to try unblock this PR, thanks :)

Imagine module A imports module B and both use `pkg/errors`. A uses
`errors.Cause` to inspect wrapped errors returned from B. As-is, B
cannot migrate from `errors.Wrap` to `fmt.Errorf("%w", err)` because
that would break `errors.Cause` calls in A. With this change merged,
`errors.Cause` becomes forwards-compatible with Go 1.13 error chains.
Module B will be free to switch to `fmt.Errorf("%w", err)` and that will
not break module A (so long as the top-level project pulls in the newer
version of `pkg/errors`).
@jayschwa
Copy link
Contributor Author

jayschwa commented Jan 6, 2020

Pushed some changes:

  • Rebased off master branch.
  • Tried to refine function documentation based on feedback.
  • Added alternating stdlib and pkg/errors layers to test case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants