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

errors: ensure the 1.13 method to access the cause is called Cause() not Unwrap() #31778

Closed
knz opened this issue May 1, 2019 · 19 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@knz
Copy link

knz commented May 1, 2019

The rationale in https://go.googlesource.com/proposal/+/master/design/go2draft-error-values-overview.md mentions this:

Unwrap. It is unclear if Unwrap is the right name for the method returning the next error in the error chain. Dave Cheney’s github.com/pkg/errors has popularized Cause for the method name, but it also uses Cause for the function that returns the last error in the chain. At least a few people we talked to did not at first understand the subtle semantic difference between method and function. An early draft of our design used Next, but all our explanations referred to wrapping and unwrapping, so we changed the method name to match.

In summary the rationale was "some people were confused so we're going to change the name".

However this choice creates a more serious technical problem.

Suppose a program upgrades to use Go 1.13 but has some dependency library that relies on github.com/pkg/errors (or another Cause()-compatible error library). Now consider what happens if our program makes some API call through its dependency, and that encounters an error from the standard library -- for example io.EOF or context.DeadlineExceeded. And then the intermediate library dependency wraps the error using the facilities in github.com/pkg/errors which only provides Cause().

How is the program now meant to identify the ultimate cause of its error?

The proposal with Is() and Unwrap() will be unable to "peek through" the wrappers created by the existing library code. This is defective design.

Solution: By changing the design in Go 1.13 to instead make the causer access method remain Cause(), the new 1.13 error package would become/remain drop-in compatible with existing library error wrapper code.

@ianlancetaylor
Copy link
Contributor

CC @jba @neild

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels May 1, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone May 1, 2019
@jba
Copy link
Contributor

jba commented May 1, 2019

It is not a goal of the proposal to be compatible with existing error packages. Indeed, that is impossible, since they contradict each other. The goal is to establish a common set of names and behaviors, and the hope is that existing packages like pkg/errors will adopt them.

How is the program now meant to identify the ultimate cause of its error?

Couldn't you continue to use Cause? Leaving aside the fact that you should never wrap io.EOF, you could write

if pkgerrors.Cause(err) == io.EOF || xerrors.Is(err, io.EOF) { ... }

@knz
Copy link
Author

knz commented May 1, 2019

It is not a goal of the proposal to be compatible with existing error packages.

  1. why?
  2. can you also explain why in the proposal itself

Indeed, that is impossible, since they contradict each other.

why/how?

Couldn't you continue to use Cause? Leaving aside the fact that you should never wrap io.EOF, you could write
if pkgerrors.Cause(err) == io.EOF || xerrors.Is(err, io.EOF) { ... }

No that will fail if an error is wrapped both by xerrors and pkgerrors.

@neild
Copy link
Contributor

neild commented May 1, 2019

why/how?

There exist error implementations where the Cause method returns the root of the chain, as well as ones where Cause returns the next step in the chain. There is no way to reconcile these two definitions.

@knz
Copy link
Author

knz commented May 1, 2019

There exist error implementations where the Cause method returns the root of the chain, as well as ones where Cause returns the next step in the chain. There is no way to reconcile these two definitions.

Can you provide examples. This is not my understanding. If you look at the doc of the related packages you will see the following used consistently:

  • the package-level function accesses the root cause
  • the causer interface method Cause() accesses the next step

What is the evidence you've found that some libraries do it differently? Are you going to report this evidence in the proposal?

@neild
Copy link
Contributor

neild commented May 1, 2019

What is the evidence you've found that some libraries do it differently? Are you going to report this evidence in the proposal?

One package where the Cause method returns the root of the chain is gopkg.in/errgo.v2/errors, which is cited in the draft design for error inspection.

@knz
Copy link
Author

knz commented May 1, 2019

I went and looked at the package you linked, and I am afraid the source code contradicts your statements:

  • the Cause() method does exactly what I said and only accesses one level up -- which is what the top level issue in this thread is requesting for Go 1.13. See the code here:
    https://github.com/go-errgo/errgo/blob/v2.1.0/errors/struct.go#L48

  • the Cause() package-level function accesses just 1 level deep (instead of the root cause) and I would agree it is better to provide Is() like you did, but this point is irrelevant for the issue at hand.

So at this point I still see no evidence of library code in the ecosystem where the Cause() method is contradicting the proposal.

I would kindly request that you either provide some evidence or instead reconsider the Go 2/1.13 proposal for the method as requested at the top of this thread.

@neild
Copy link
Contributor

neild commented May 1, 2019

I went and looked at the package you linked, and I am afraid the source code contradicts your statements:

You are mistaken:
https://play.golang.org/p/1rR97pPojOd

@jba
Copy link
Contributor

jba commented May 1, 2019

It looks to me like the errgo Cause function goes one level:

func Cause(err error) error {
	if err, ok := err.(Causer); ok {
		if cause := err.Cause(); cause != nil {
			return cause
		}
	}
	return err
}

while the pkg/errors one follows the chain:

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

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

@jba
Copy link
Contributor

jba commented May 1, 2019

No that will fail if an error is wrapped both by xerrors and pkgerrors.

You could instead write your own function, looking for both Cause and Unwrap methods.

@knz
Copy link
Author

knz commented May 1, 2019

You are mistaken:
play.golang.org/p/1rR97pPojOd

Thank you for the example, although I believe it is disingenuous: this exemplifies specifically the feature of that package that hides the cause. The goal of that Note API is to provide control for the code that constructs the error on the ability for other downstream code to query that cause. That use case / requirement would exist also for users of xerrors and arguably an xerror-compatible implementation could provide the same feature that prevents a cause from being visible via Unwrap().

So far it does not seem you have provided evidence that in the common case (even a common case for gopkg.in/errgo.v2/errors) the established ecosystem is incompatible with the Go 1.13 proposal, and it does not seem that the conversation so far has provided a reason to keep the current design that outweighs the inconvenience that I have outlined initially.

No that will fail if an error is wrapped both by xerrors and pkgerrors.
You could instead write your own function, looking for both Cause and Unwrap methods.

Yes I am aware I could do this. However, if I and other people find ourselves required to do this often (as I am arguing since the beginning of the thread), this would be strong evidence that the proposed definition of Go's error API is defective. Now (before 1.13 is released) would be an adequate time to study this risk and perhaps make adjustments before the Go user community finds themselves inconvenienced by the defect.

@darkfeline
Copy link
Contributor

As a matter of practicality, golang.org/x/xerrors uses Unwrap(), and there are people using that package now. Which means that whether we end up choosing Unwrap() or Cause() or something else, everyone else is going to have to add backward compatibility,

If we choose Unwrap(), then github.com/pkg/errors and everyone else will have to add support for Unwrap().

If we choose Cause(), then golang.org/x/xerrors and everyone else will have to add support for Cause().

And ditto for any other API that is out there.

I understand that the error values proposal isn't final yet, but I think it would be impractical to change it drastically without a strong reason since people are using it already (including me). Similarly, there was no guarantee that github.com/pkg/errors provided any kind of standard API across the Go ecosystem and their users should have understood that they were committing to an API only implemented by github.com/pkg/errors and any explicitly cooperating packages.

The point of this part of the error values proposal, as I see it, was to commit to a standard API across the Go ecosystem. That it is not the github.com/pkg/errors API is perhaps unfortunate, but at this point in time being noncommittal about the API will cause more harm than benefit.

Addressing @knz's concern directly,

Suppose a program upgrades to use Go 1.13 but has some dependency library that relies on github.com/pkg/errors (or another Cause()-compatible error library). Now consider what happens if our program makes some API call through its dependency, and that encounters an error from the standard library -- for example io.EOF or context.DeadlineExceeded. And then the intermediate library dependency wraps the error using the facilities in github.com/pkg/errors which only provides Cause().

How is the program now meant to identify the ultimate cause of its error?

  • If your program previously used Cause style functions to check errors, it will keep working.
  • If you switched your program to use Unwrap style functions on a library that doesn't support it, well, don't do that.
  • If the library adds support for Unwrap style errors (which is trivial), then you are free to to switch to using Unwrap style functions or keep using Cause style functions.

The error values proposal doesn't change the fact that every package needs to document an explicit contract about how a user will go about unwrapping or checking errors, or the backward compatibility implications thereof. All it does is adds a standard contract that hopefully all packages can agree upon going forward.

@knz
Copy link
Author

knz commented May 6, 2019

As a matter of practicality, golang.org/x/xerrors uses Unwrap(), and there are people using that package now. Which means that whether we end up choosing Unwrap() or Cause() or something else, everyone else is going to have to add backward compatibility

I am really glad that you recognize that some people are going to be inconvenienced.

It would be interesting here, for the sake of clarity and methodology, to estimate the size of the user populations. AFAIK xerrors has been around for less time than the alternatives using Cause().

There is a utility argument to be made about the amount of inconvenience. A solution that causes less inconvenience in aggregate seems (to me) to be the better solution. I am sure that the fact that you happen to be invested in the initial development of xerrors will not bias you against looking at the utility aspect as a whole.

@tv42
Copy link

tv42 commented May 17, 2019

I would expect third party packages like github.com/pkg/errors to change their Cause function to support both their own Cause method and Unwrap. That seems like the way to migrate from github.com/pkg/errors to xerrors to Go1.14.

@knz
Copy link
Author

knz commented May 17, 2019 via email

@josharian
Copy link
Contributor

@knz please be respectful.

@AlekSi
Copy link
Contributor

AlekSi commented May 17, 2019

Was this issue a reason for revert at 3e2c522?

Edit after @jba's answer below.
Found it: #29934 (comment)

@jba
Copy link
Contributor

jba commented May 17, 2019

No.

@jba
Copy link
Contributor

jba commented Jun 5, 2019

Our final decision is to stay with Unwrap.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

9 participants