go: proposal: change standard library to check for io.EOF using errors.Is

The downside is API confusion. Once we split the ecosystem, some clients of Read check with err == io.EOF and some check with errors.Is(err, io.EOF). We’re never going to get rid of all of the people using err == io.EOF. So a robust implementation of Read can never return a wrapped io.EOF. But we might get rid of most of the err == io.EOF clients, at which point people might be tempted to use a wrapped io.EOF. And in the code they test with, it might work. But it will break some err == io.EOF clients they don’t even know about, but who depend on their Read method.

It’s better if the stdlib keeps using err == io.EOF, because people who try to use a wrapped io.EOF will likely fail immediately, and they will know not to do that.

Why do you want to wrap io.EOF anyway? I don’t see why context would matter for that error value.

I don’t understand the association. io.EOF is a sentinel value, it’s defined in the various io interfaces that it must be returned on end of file. There is no need to use errors.Is because by definition io.EOF must not be wrapped.

I believe that io.EOF is the exception here, so retitling.

The issue I’m hitting is definitely with io.EOF but I haven’t done a full scan. io.ErrUnexpectedEOF seems to have similar checks, although usually it is generated by stdlib itself (but doesn’t need to be).

The standard library says EOF is the error returned by Read when no more input is available.

The semantic question in here is what does “is the error” mean after go1.13 . Is an error that is wrapped still the same error? From the method names (and design objectives) added in go1.13 it would strongly suggest that it is.

I think we agree that that can’t change without breaking the Go 1 guarantee.

All the Read() methods in stdlib that generate io.EOF directly should continue to do that. Old code that used this and does io.EOF check with == will continue to work.

As I described, after this would ship, and a 3rd party library decides to wrap an IO error, they could break another 3rd party library that upgrades to use them. But 3rd party libraries are not under any go1 backward compatibility guarantees. Nobody can be sure they don’t decide to not implement io.EOF at all. Semver can be used in these modules to signal changes.

It will definitely take time for all people to always use errors.Is when dealing with errors. My point is that the quicker we take some steps for transition, the sooner the messy period will be over. As this doesn’t break anyone, it could be just considered following a best practice like the errors package suggests.

The question is whether there are places in the standard library that use == io.EOF even though they are checking errors that are not returned from a Read method.

My issue definitely is that there is no way to add typed information to errors that arise through io processing (at least without forking a significant portion of stdlib). If there are other cases they are less likely to actually solve the issues.

The standard library says EOF is the error returned by Read when no more input is available.. I think we agree that that can’t change without breaking the Go 1 guarantee. Since io.Copy is using Read, it seems OK that io.Copy checks using == io.EOF rather than errors.Is. The question is whether there are places in the standard library that use == io.EOF even though they are checking errors that are not returned from a Read method.

It might be possible to write a vet-check (or any of the gajillion third-party static analysis tools) that tries to find code that returns a wrapped io.EOF from a Read([]byte) (int, error) method. I agree with others that wrapping io.EOF is not incorrect in general, so shouldn’t panic. And I also agree with others that replacing err == io.EOF with errors.Is(err, io.EOF) is counterproductive. But I don’t see the harm in a vet-check, if it’s selective enough to only look at Read-methods.

by definition io.EOF must not be wrapped.

The io.Reader documentation doesn’t mention wrapping, and I don’t think the error wrapping documentation calls out that some errors should never be wrapped. If this proposal is rejected, I propose updating the documentation to make this clear.

Is there a downside to using errors.Is(err, io.EOF) instead of err == io.EOF?

I was bitten by this, when implementing a Reader which wraps another Reader, and includes additional diagnostic information when passing through any errors. In order to make my Reader usable with archive/zip, I had to special-case io.EOF, and not return any diagnostics in that case.

I think the stdlib should use errors.Is() when checking error values, and I’d be happy to contribute patches to that effect. (Presumably the reviewers’ preference would be to split these up by package, e.g. one for archive/zip, one for compress/bzip2, etc? Advice welcome.)

It’s a valid concern that third-party packages may also be doing equality checks instead of errors.Is(), but this doesn’t seem like a good reason the stdlib shouldn’t follow best practice; in my case, I’m not using any third-party packages so it’s just the stdlib holding me back. If I’m understanding this argument correctly, it seems a little like a chicken-and-egg situation, in that if wrapped io.EOF worked in the stdlib, package authors would be encouraged to follow suit.

I also think go vet should warn about checking equality instead of using errors.Is, but that is perhaps a separate issue.

I tend to think of io.EOF as a special case; a sentinel value indicating expected behavior, not an error.

This is only true if the “file” is not infinite. It is the user/developer that has the expectation that there will ever be an end-of-file, not the io.Reader interface. I see it as an expected error rather than a special case, hence why the concrete value is so ubiquitous.

Unless we change all code to use errors.Is on io.EOF instead of performing a direct == comparison (which we cannot feasibly do), I believe having the standard library use errors.Is is more harmful than good. It provides the illusion that wrapping io.EOF in Read is permissible, when it is not. Some users will depend on that behavior and think that its okay to return wrapped io.EOF errors since it works with the standard library, but be surprised that it doesn’t work with some other library that uses io.Reader, despite that library being fully compliant with the current specified behavior.

I’m curious to understand how adjusting the docs about io.EOF would break compatibility between two 3rd-party codebases in a way that’s not already possible today. Furthermore, if the error checking paradigm from Go 1.13 is fully embraced, shouldn’t tooling such as linters or maybe even gofix also prefer errors.Is over equality?

It’s not an issue of whether tooling can migrate existing code. It’s a question of whether correct code today will continue to function after the documented change. Most code today only do an err == io.EOF check. Changing the documentation makes it such that this correct code is now incorrect by redefinition.

I’d encourage you raise that as a separate proposal.

Done, thanks!

https://github.com/golang/go/issues/40827

Which I realise is kind of the point you’re making, but it’s also my point — when you operate at this level — interposition your reader on top of another — you have to maintain the io.EOF invariant

Wouldn’t it be easier if we didn’t have to maintain that invariant? Saying “always use errors.Is, don’t use err ==” is easily understood, and could be automated using tools like go vet. It’s about a 200-300 line patch to fix it everywhere io.EOF is checked in the standard library.

Maintaining a list of “unwrappable” errors in documentation and/or tooling seems really hard.

I’m offering to do the work to make the standard library easier to use, by replacing err == io.EOF with errors.Is(err, io.EOF). If that work is unwelcome, I’m willing to spend an equivalent amount of effort elsewhere to prevent the next person from falling into this trap. Other suggestions as to better places to direct that effort are welcome.

It’s better if the stdlib keeps using err == io.EOF, because people who try to use a wrapped io.EOF will likely fail immediately, and they will know not to do that.

Would you support a patch which causes fmt.Errorf to panic if the "%w" argument == io.EOF? This would have saved me a few hours of debugging today.