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

x/xerrors: retrieving original "wrapped" error should not depend on a verb #31432

Closed
mfridman opened this issue Apr 12, 2019 · 4 comments
Closed

x/xerrors: retrieving original "wrapped" error should not depend on a verb #31432

mfridman opened this issue Apr 12, 2019 · 4 comments

Comments

@mfridman
Copy link

@mfridman mfridman commented Apr 12, 2019

What version of Go are you using (go version)?

$ go version
go version devel +964fe4b8 Thu Apr 4 00:26:24 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

N/A. Working off master.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOHOSTARCH="amd64"
GOHOSTOS="darwin"

Currently using golang.org/x/xerrors v0.0.0-20190410155217-1f06c39b4373

Suppose I have a function that internally calls bufio and somewhere an error occurs. I'd like to return the error with context:

pkg/error:

// err here is bufio.ErrBufferFull
if err != nil {
	return errors.Wrap(err, "some context here")
}

golang.org/x/xerrors:

presumably the above would be equivalent to:

// err here is bufio.ErrBufferFull
if err != nil {
	return xerrors.Errorf("some context here: %w", err) <- wrapped
        // or..
	return xerrors.Errorf("some context here: %v", err) <- unwrapped
}

So, how does one "unwrap" the original error, in this contrived example the error is bufio.ErrBufferFull ?

pkg/error:

if errors.Cause(err) == bufio.ErrBufferFull {
	// true
}

golang.org/x/xerrors:

if xerrors.Is(err, bufio.ErrBufferFull) {
        // true if %w was used at the site of the original error
	// false if %v or %s 
}

By allowing users the option of selecting a verb to "wrap" an error will lead to unintended consequences up the call chain.

Would love to see a helper function with the signature:

(err error, format string, args ...interface{}) error

  1. being explicit would prevent API misuse
  2. too verbose, i think all wrapped errors will start life as `errors.Errorf("%w: ",err)
  3. xerrors.Errorf() is a bit too magical
@bcmills bcmills changed the title xerrors: retrieving original "wrapped" error should not depend on a verb x/xerrors: retrieving original "wrapped" error should not depend on a verb Apr 12, 2019
@gopherbot gopherbot added this to the Unreleased milestone Apr 12, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Apr 12, 2019

@jba
Copy link
Contributor

@jba jba commented Apr 13, 2019

If I understand you right, you feel %w is too magical and would prefer an explicit Wrapf function instead. I also think you are saying that users may accidentally use one instead of the other because they are similar, and that would have "unintended consequences up the call chain."

We've discussed %w vs. Wrapf quite a bit over on #29934. See Russ Cox's comment in particular. We've decided to keep %w for now.

@mfridman
Copy link
Author

@mfridman mfridman commented Apr 13, 2019

Ah thanks, missed the train on that one.

This is more of a user report after trying to port a few of our internal pkgs from pkg/errors to golang.org/x/xerrors.

I think there can be a middle ground where both %w is available through errors.Errorf and have a helper function from the std lib that explicitly forces users to pass error as an argument, which explicitly wraps the error.

The explicitness of Wrapf prevents ambiguity whether an error is wrapped or not, while still having the option to prevent wrapping with Opaque or Errorf with %v or %s.

From a code review perspective, it's immediately obvious (with an explicit function) an error is being wrapped and annotated vs. spotting the %w verb.

stylistically consistent. It helps if the code looks similar throughout the code base.

Anyways, just some feedback. Feel free to close issue. @jba

@agnivade
Copy link
Contributor

@agnivade agnivade commented Apr 15, 2019

Thanks for the feedback @mfridman. Please feel free to add your comments to the proposal thread (#29934).

@agnivade agnivade closed this Apr 15, 2019
@golang golang locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
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.