Go: proposal: annotate which error values should never be wrapped

[dpifke]

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]

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]

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

[dpifke]

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]

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]

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]

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

[rsc]

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]

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

[rsc]

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

[rsc]

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

[rsc]

No change in consensus, so declined.