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

proposal: annotate which error values should never be wrapped #40827

Closed
dpifke opened this issue Aug 16, 2020 · 11 comments
Closed

proposal: annotate which error values should never be wrapped #40827

dpifke opened this issue Aug 16, 2020 · 11 comments
Labels
Projects
Milestone

Comments

@dpifke
Copy link
Contributor

@dpifke dpifke commented Aug 16, 2020

Following up on #39155, I propose there be a consistent method to annotate which error values can never be wrapped, such as io.EOF. This would be the first step towards being able to automate detection when illegal wrapping occurs, e.g. causing fmt.Errorf("some message: %w", io.EOF) to panic, or go vet to emit a warning.

A naive way to determine this would likely be to grep for err == in the standard library. (Per the above-linked issue, the use of == instead of errors.Is() is a feature, not a bug.) This list should be linked from the errors package documentation, and there should be an indication that an error is "not wrappable" in the error value (e.g. io.EOF) and function (e.g. io.Reader.Read) documentation.

The aim is to detect and prevent incorrect (but reasonable-looking) code such as:

func (r *MyReader) Read(b []byte) (int, error) {
    n, err := r.underlying.Read(b)
    if err != nil {
        err = fmt.Errorf("reading %q: %w", r.Filename, err)
    }
    return n, err
}
@dpifke
Copy link
Contributor Author

@dpifke dpifke commented Aug 16, 2020

In addition to documenting which errors should never be wrapped, it would be nice for third-party package authors to know when == is preferable over errors.Is. The current documentation says the opposite, which is contravened by usage in the standard library.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 18, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 18, 2020

I'm not sure what you mean by annotate here. Do you mean something other than documentation?

@dpifke
Copy link
Contributor Author

@dpifke dpifke commented Aug 18, 2020

I was intentionally being somewhat vague, as I don't have a strong opinion as to whether that particular bikeshed should be painted red or blue.

The goal would be something visible in documentation, as well as consumable by static analysis tools.

As a straw man proposal, I think something as simple as an additional method in errors would get us part of the way there:

func NewNotWrappable(text string) error { return New(text) }

This would show up in godoc without changing any method signatures as e.g.

var EOF = errors.NewNotWrappable("EOF")

...and I think would be at least partially usable for static analysis.

If it returned error but the value also had an additional method (e.g. NotWrappable() bool), it might also be detectable by e.g. fmt.Errorf().

@dpifke
Copy link
Contributor Author

@dpifke dpifke commented Aug 18, 2020

If errors exposed a CanWrap(err error) bool method, the incorrect code from the summary could be rewritten as:

func (r *MyReader) Read(b []byte) (int, error) {
    n, err := r.underlying.Read(b)
    if err != nil && errors.CanWrap(err) {
        err = fmt.Errorf("reading %q: %w", r.Filename, err)
    }
    return n, err
}

(possibly omitting the nil check as well, since nil is not wrappable)

@mjgarton
Copy link
Contributor

@mjgarton mjgarton commented Aug 18, 2020

I have code (not involved in implementing io.Reader that returns wrapped io.EOF that works perfectly fine as designed.

I don't think the issue is that io.EOF shouldn't be wrapped ever, it's that it shouldn't be wrapped when implementing io.Reader

In other words, the constraint here is not a property of io.EOF but a property of io.Reader so asking something purely about io.EOF can't give a useful answer.

(For clarity, I'm not necessarily disagreeing with anything here, but making an observation and commenting that there's nothing wrong with wrapping io.EOF in general)

@rsc rsc moved this from Incoming to Active in Proposals Sep 23, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 23, 2020

I think it's just io.EOF.
Are there others?

@rsc
Copy link
Contributor

@rsc rsc commented Sep 30, 2020

It sounds like it is really just io.EOF.
That is a special case that is unfortunate but is now with us and not going anywhere.
Assuming there are no others, it seems like we should not introduce the general concept.
We could add a comment to the EOF docs mentioning that EOF should not be wrapped.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 30, 2020

Change https://golang.org/cl/258524 mentions this issue: io: make clear that EOF should not be wrapped

@rsc
Copy link
Contributor

@rsc rsc commented Oct 1, 2020

Having submitted https://golang.org/cl/258524, is there anything left to propose here?

gopherbot pushed a commit that referenced this issue Oct 1, 2020
For #40827.

Change-Id: Ifd108421abd8d0988dd7b985e4f9e2bd5356964a
Reviewed-on: https://go-review.googlesource.com/c/go/+/258524
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2020

With the io.EOF doc updated, it seems like there is nothing left to do here. This seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Oct 14, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Oct 21, 2020

No change in consensus, so declined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
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.