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

errors: ErrUnsupported should document expected side effects #63942

Open
dsnet opened this issue Nov 3, 2023 · 3 comments
Open

errors: ErrUnsupported should document expected side effects #63942

dsnet opened this issue Nov 3, 2023 · 3 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Nov 3, 2023

I'm struggling to figure out how to correctly use errors.ErrUnsupported.

In the proposal, @rsc gave this example:

func (w) WriteString(s string) (int, error) {
    if u, ok := w.u.(io.StringWriter); ok {
        return u.WriteString(s)
    }
    return 0, ErrTBD
}

where ErrTBD is presumably what became ErrUnsupported.

Let's say hypothetically, we wanted to make io.WriteString make use of errors.ErrUnsupported, what would that look like?

func WriteString(w Writer, s string) (n int, err error) {
	if sw, ok := w.(StringWriter); ok {
		switch n, err := sw.WriteString(s); {
		case err == nil:
			return n, nil
		case !errors.Is(err, errors.ErrUnsupported)):
			return n, err
		}
		// otherwise err is errors.ErrUnsupported, fallback on other logic
	}
	return w.Write([]byte(s))
}

However, the only way this is possibly correct is if returning errors.ErrUnsupported also indicates that the operation had no side-effects and could thus be retried in an alternative way. As of right now, there is no such documented guarantee.

If we do document that errors.ErrUnsupported is side-effect free, then it also means that this is error prune (pun intended) where returning the error up the call stack increases the probability that there are indeed side-effects from other operations.

\cc @ianlancetaylor @bcmills

@bcmills
Copy link
Member

bcmills commented Nov 3, 2023

@dsnet, note that the example you cited above is an example of what not to do. From later in that comment:

Ian's #41198 (comment) makes it sound like the answer is "no, ErrTBD is inappropriate in this case; the method should end with return w.Write([]byte(s))". In that case, I'm much more comfortable with the idea and simply agree with Bryan that "not supported" is a better name for the idea than "not implemented". Too many people will think "not implemented" is OK to use in the WriteString example above - the method is, after all, not implemented by w.u.

@bcmills
Copy link
Member

bcmills commented Nov 3, 2023

That would imply that the smallest-diff “correct” implementation for that example is actually:

func (w someStruct) WriteString(s string) (int, error) {
	if u, ok := w.u.(io.StringWriter); ok {
		return u.WriteString(s)
	}
	return io.WriteString(w.u, s)
}

Or, more concisely:

func (w someStruct) WriteString(s string) (int, error) {
	return io.WriteString(w.u, s)
}

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 6, 2023
@bcmills bcmills added this to the Backlog milestone Nov 6, 2023
@adonovan
Copy link
Member

[@bcmills I assume youmeant io.WriteString(w.u, s), twice.]

[@dsnet] If we do document that errors.ErrUnsupported is side-effect free, then it also means that this is error prune (pun intended) where returning the error up the call stack increases the probability that there are indeed side-effects from other operations.

...or conversely that an I/O function fails with ErrUnsupported not because the immediate operation was unsupported, but because some distant operation bubbled it up--not unlike the problem of recovering from panics that pass through arbitrary code--after having effects on the state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants