Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@mfridman
Copy link

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 bcmills added FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 12, 2019
@bcmills
Copy link
Contributor

bcmills commented Apr 12, 2019

CC @neild @mpvl @jba

@jba
Copy link
Contributor

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 commented Apr 13, 2019 •
edited
Loading

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

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

@mfridman mfridman mentioned this issue Apr 16, 2019
Closed
@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.
Assignees
No one assigned
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@agnivade @bcmills @mfridman @gopherbot @jba