I just want to note that there is often no need to check the result of Close of a file opened for reading. If all the data has been read, the program behavior won't change if Close returns an error.

While that may be true for os.File, io.Closer does not document this, and many implementations do not maintain this invariant. If this viewpoint is prevailing, I think it worth noting in the godoc and pursuing my Go2 suggestion.

For instance, cloud.google.com/go/storage.Writer.Write notes:

// Since writes happen asynchronously, Write may return a nil
// error even though the write failed (or will fail). Always
// use the error returned from Writer.Close to determine if
// the upload was successful.

As such, storage.Writer.Close is the only way to detect errors, and will change program behaviour, something that is confirmed by its godoc:

// Close completes the write operation and flushes any buffered data.

Side note, this is usually a thorn for me because most linters require that io.Closer errors be explicitly handled. It alone is not a reason to make this change, but suggests that the community supports the direction.

I just want to note that there is often no need to check the result of Close of a file opened for reading. If all the data has been read, the program behavior won't change if Close returns an error.

This is true but makes the linters that check for this like errcheck very noisy as they'll tag the harmless invocations too of which there may now be many.

It may still be useful to log the error as there shouldn't be any error. i.e it may be the result of some insidious bug somewhere else.

So I'd personally be in favour of something like this to make defer error checking less verbose.

And yes you could define such a function in your own code and use it throughout but it does feel like something every Go programmer encounters and as such deserves a spot in the stdlib.

And yes you could define such a function in your own code and use it throughout but it does feel like something every Go programmer encounters and as such deserves a spot in the stdlib.

I also think it's highly opinionated, in terms of writing stuff to metric of log dashboards in case an err is non-nil. I realise that this will all still be possible with this proposal, but since it's not a one-size-fits-all, I don't think it should be part of the stdlib.

just my 2 cents.

People do write code that does

if err := f.Close(); err != nil {
    return err
}

but I am not sure I would want to encourage that behavior. Really such code should be wrapping the error with extra context.

And to be clear,

defer f.Close()

is clearly OK for readers.

For writers you need to check the result, but often if the result is a failure you want to react to the fact that you didn't actually write the thing you thought you wrote. Hiding the possible error inside a helper (which makes it look like the error is "taken care of") only compounds the problem.

It's just unclear how much the library routine would help versus encourage bad code.

There are two problems I have with the current state of things. First, it is non-trivial to extract an error from a defer: (as if err != nil { was not bad enough, now we need two more lines for the defer)

defer func() {
    if err := f.Close(); err != nil {
        log.Println(err)
    }
}()

Second, after you extract it, if you want to interact with the error (and not just log it as above), you must use a named return and decide which error is more important:

func WriteAndClose(w io.WriteCloser, a ...interface{}) (err error) {
    defer func() {
        if err0 := w.Close(); err == nil  && err0 != nil{
            err = err0
        }
    }()
    _, err := fmt.Fprint(w, a...)
    return err
}

I also think it's highly opinionated, in terms of writing stuff to metric of log dashboards in case an err is non-nil. I realise that this will all still be possible with this proposal, but since it's not a one-size-fits-all, I don't think it should be part of the stdlib.

I completely agree, and somewhat regret posing this as a proposal, since what I want to resolve is the root problem and there are a bunch of ways to do this. Unfortunately, github issues are not a good place to brainstorm.

@carnott-snap

First, it is non-trivial to extract an error from a defer ...
Second, ... you must use a named return

These are both reasons not to use defers for code that might fail and influence error handling. So rather than try to fix these problems by adding defer-use-helpers, don't use defer here.

These are both reasons not to use defers for code that might fail and influence error handling. So rather than try to fix these problems by adding defer-use-helpers, don't use defer here.

Not using defer will make the code look even worse, violate line of sight style, and gets exponentially more complex with each additional error)

func WriteAndClose(w io.WriteCloser, a ...interface{}) error {
    _, err0 := fmt.Fprint(w, a...)
    /* 20 lines of other work, none of which can early return */
    err10 := w.Close()
    if err0 != nil {
        return err0
    }
    /* 9 other errN != nil  checks*/
    return err10
}

but I am not sure I would want to encourage that behavior. Really such code should be wrapping the error with extra context.

2c2
<     return err
---
>     return fmt.Errorf("closing a file: %w", err)

is clearly OK for readers.

This is demonstrably incorrect. Say I have a reader that, like the gcloud sdk, holds open resources until closure, and the call to io.Closer.Close fails, leaving the resources dangling, and causing a memory leak. (This is based upon real xp.) One has no way of knowing what is causing the problem.

For writers you need to check the result, but often if the result is a failure you want to react to the fact that you didn't actually write the thing you thought you wrote. Hiding the possible error inside a helper (which makes it look like the error is "taken care of") only compounds the problem.

errors.Close, as proposed, does not necessarily hide the error, but I agree that if something else errors, it could be masked. IMO, this is an argument to allow something like fmt.Errorf("doing a thing: %w and %w", err0, err1) or type Multi []error that makes multi errors trivial to use. Many libraries that parallelise work are force do do this already, and I am happy if adding Close is prerequisite on something like Multi in the stdlib.

It's just unclear how much the library routine would help versus encourage bad code.

I am more than open to alternatives, this was the best bad option I could come up with.

@carnott-snap

Say I have a reader that, like the gcloud sdk, holds open resources until closure, and the call to io.Closer.Close fails, leaving the resources dangling, and causing a memory leak. (This is based upon real xp.)

Such an implementation of Close is incorrect, full-stop. The Close method of an io.Closer must not fail to release underlying resources, because it is explicitly not safe for the caller of such a method to retry the call on failure (see #25390).

The next sentence states that Specific implementations may document their own behavior., so that is patently false. If we need to change this wording, I am game, but the use case remains valid.

Also, the idea of needing to respond to an error is orthogonal to calling Close again. I could have an interface where, if err == ErrsSentinel then you need to call Foo.Cleanup.

@carnott-snap, if an implementation requires behavior that is not safe (or not supported) for io.Closer implementations in general, then that implementation cannot be used as an io.Closer in general. (It fails the Liskov substitution principle.)

In that case, the implementation should take care not to implement the io.Closer interface so that it is not accidentally used as one. So it's a defect in the implementation either way.

Maybe we use the term undefined [behaviour] differently, but based upon the fact that io.Closer states that one can define this behaviour, it does not seem apocryphal or "unsafe". If your opinion is how the docs should be read, imo, it needs to be rewritten. (And use the term "unsafe" and "disallowed").

However, the concept of calling Close is orthogonal to responding to the error. The docs say nothing about the error being meaningless or ignorable. And if this is the case, the signature should be interface{ Close() }.

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

No change in consensus, so declined.
— rsc for the proposal review group

@rsc: since there is a need described in this ticket and you consider my proposed solution unacceptable, can you offer alternatives? Currently there is no ergonomic way to interacted with a deferred error (or io.Closer).

I don't think there is a single solution to the general issue.

Then can we drive towards multiple solutions for the major issue types? I understand that closing this proposal may be your way of signaling this this one idea is not accepted, but it also suggests that there is no need for any fix.

Closing this issue should only suggest that this specific proposal is not accepted.

The issue tracker isn't a good place to brainstorm a solution for a problem. It doesn't handle discussions well. I recommend that you use a forum such as golang-nuts.