golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.13k stars 17.69k forks source link

errors: add ErrUnsupported #41198

Closed ianlancetaylor closed 1 year ago

ianlancetaylor commented 4 years ago

UPDATE: This proposal has shifted from the original description to the one described in comments below.

Go has developed a pattern in which certain interfaces permit their implementing types to provide optional methods. Those optional methods are used if available, and otherwise a generic mechanism is used.

For example:

The io/fs proposal (#41190) proposes various other optional methods, such as ReadFile, where there is again a generic implementation if the method is not defined.

The use of WriterTo and ReaderFrom by io.Copy is awkward, because in some cases whether the implementation is available or not is only known at run time. For example, this happens for os.(*File).ReadFrom, which uses the copy_file_range system call which is only available in certain cases (see the error handling in https://golang.org/src/internal/poll/copy_file_range_linux.go). When os.(*File).ReadFrom is called, but copy_file_range is not available, the ReadFrom method falls back to a generic form of io.Copy. This loses the buffer used by io.CopyBuffer, leading to release notes like https://golang.org/doc/go1.15#os, leading in turn to awkward code and, for people who don't read the release notes, occasional surprising performance loss.

The use of optional methods in the io/fs proposal seems likely to lead to awkwardness with fs middleware, which must provide optional methods to support higher performance, but must then fall back to generic implementations with the underlying fs does not provide the method.

For any given method, it is of course possible to add a result parameter indicating whether the method is supported. However, this doesn't help existing methods. And in any case there is already a result parameter we can use: the error result.

I propose that we add a new value errors.ErrUnimplemented whose purpose is for an optional method to indicate that although the method exists at compile time, it turns out to not be available at run time. This will provide a standard well-understood mechanism for optional methods to indicate that they are not available. Callers will explicitly check for the error and, if found, fall back to the generic syntax.

In normal use this error will not be returned to the program. That will only happen if the program calls one of these methods directly, which is not the common case.

I propose that the implementation be simply the equivalent of

var ErrUnimplemented = errors.New("unimplemented operation")

Adding this error is a simple change. The only goal is to provide a common agreed upon way for methods to indicate whether they are not available at run time.

Changing ReadFrom and WriteTo and similar methods to return ErrUnimplemented in some cases will be a deeper change, as that could cause some programs that currently work to fail unexpectedly. I think the overall effect on the ecosystem would be beneficial, in avoiding problems like the one with io.CopyBuffer, but I can see reasonable counter arguments.

darkfeline commented 4 years ago

This seems to overlap with the existing way of testing whether a method/interface is supported:

foo, ok := bar.(Foo) // Does bar support Foo?

Will we end up in situations where callers need to do two checks for whether a method is supported (the type assertion and an ErrUnimplemented check)? I know we can forbid that by convention, but it seems like a really easy trap to fall into.

ianlancetaylor commented 4 years ago

This new error is intended to be used with type assertions. Type assertions test availablity at compile time, and the error tests availablity at run time. Something like io.copyBuffer would change to look like this:

    if wt, ok := src.(WriterTo); ok {
        written, err := wt.WriteTo(dst)
        if err != errors.ErrUnimplemented {
            return written, err
        }
        // WriteTo existed but was not actually implemented, carry on as though it did not exist.
    }

In other words, yes, for these cases you are expected to write two tests.

darkfeline commented 4 years ago

I see, this is about the implementation determining whether the method can be provided at runtime time, rather than the caller determining whether the "dynamic type" of a value implements the method at runtime. I see the value for this for existing code, but new code can use an interface type with a different implementation determined at runtime.

type Foo interface {}
type Bar interface {}
func NewFoo() Foo {
  if runtimeCheck() {
    return specialFoo{} // supports .(Bar)
  } else {
    return basicFoo{}
  }
}

Does this proposal include recommendations for the wider community on whether to use ErrUnimplemented or the above for new code?

josharian commented 4 years ago

Can we spell it without the Err prefix, since it is in the errors package?

tooolbox commented 4 years ago

The use of optional methods in the io/fs proposal seems likely to lead to awkwardness with fs middleware, which must provide optional methods to support higher performance, but must then fall back to generic implementations with the underlying fs does not provide the method.

Would it suffice for io/fs to define an ErrNotImplemented sentinel? It's not clear to me that it's a positive change to broadly sanction the concept of "methods that don't actually work at runtime".

narqo commented 4 years ago

(A minor suggestion) I feel Unimplemented is hard to read. Can I suggest

errros.NotImplemented
mvdan commented 4 years ago

The use of optional methods in the io/fs proposal seems likely to lead to awkwardness with fs middleware, which must provide optional methods to support higher performance, but must then fall back to generic implementations with the underlying fs does not provide the method.

Has any research gone into whether this can be solved at compile time, not needing to add extra run time checks which Go programs would need to add? I know it's a hard problem to solve, but I assume it's worth a try before falling back to run time checks.

rsc commented 4 years ago

An early version of the FS interface draft had such an error, which is maybe part of what inspired this proposal. I removed it from the FS interface draft before publishing, because in almost all cases I could think of, it was better for a method to either (1) report a definitive failure, or (2) fall back to code that does implement the behavior.

The fact that io.Copy checks for two optional methods complicates a lot and may well have been a mistake. I would rather not make general conclusions about the awkwardness of io.Copy.

Let's look instead at fs.ReadFile, which you raised as an example. Suppose I have an fs wrapper type that adds a prefix to all the underlying calls:

type Subdir struct {
    prefix string
    fsys fs.FS
}

If that type wants to make the ReadFile method on fs available, it can already do:

func (s *Subdir) ReadFile(file string) ([]byte, error) {
    file = path.Join(s.prefix, file)
    if fsys, ok := s.fsys.(fs.ReadFileFS); ok {
        return fsys.ReadFile(file)
    }
    return fs.ReadFile(fsys, file)
}

With this proposal, the code could instead do:

func (s *Subdir) ReadFile(file string) ([]byte, error) {
    file = path.Join(s.prefix, file)
    if fsys, ok := s.fsys.(fs.ReadFileFS); ok {
        return fsys.ReadFile(file)
    }
    return errors.ErrUnimplemented
}

The last line is the only one that changed.

Compared to the first version, being able to write the second version is only slightly less work for the wrapper author, but far more work for the call sites. Now every time a method like this gets called, the caller must check for ErrUnimplemented and do something else to retry the operation a different way. That is, ErrUnimplemented is a new kind of error for Go programs, a "it didn't quite fail, you have to do more work!" error.

And it's not the case that you only have to worry about this if you've tested for an optional interface right before the call. Suppose you have code that takes a value of the concrete type *Subdir as an argument. You can see from godoc etc that there's a ReadFile method, 100% guaranteed. But now every time you call ReadFile you have to check for ErrUnimplemented.

The pattern of "handle the call one way or another" seems much better for more code than the pattern of "refuse to handle the call". It preserves the property that when an error happens, it's a real failure and not something that needs retrying.

In that sense, ErrUnimplemented is a bit like EINTR. I'm wary of introducing that as a new pattern.

rsc commented 4 years ago

On a much more minor point, given that package errors today exports four symbols, none of which has type error, I don't believe that "this is an error" is implied by errors.. (For example, errors.Unwrap is not an error about unwrapping.)

If we are at some point to add one or more standard error values to package errors, the names should probably continue the convention of using an Err prefix. That will be avoid readers needing to memory which symbols in package errors are and are not errors.

rsc commented 4 years ago

For the record, although it's mostly unrelated to this discussion, io.CopyBuffer was a mistake and should not have been added. It introduced new API for a performance optimization that could have been achieved without the new API.

The goal was to reuse a buffer across multiple Copy operations, as in:

buf := make([]byte, size)
for _, op := range ops {
    io.CopyBuffer(op.dst, op.src, buf)
}

But this could instead be done using:

w := bufio.NewWriterSize(nil, size)
for _, op := range ops {
    w.Reset(op.dst)
    io.Copy(w, op.src)
    w.Flush()
}

There was no need to add CopyBuffer to get a buffer reused across Copy operations. Nothing to be done about it now, but given that the entire API was a mistake I am not too worried about the leakiness of the abstraction.

Credit to @bcmills for helping me understand this.

rhysh commented 4 years ago

The result of an optional interface check is always the same for a particular value. Code can branch based off of that and know that the value won't suddenly implement or un-implement the optional method. (Consider an http.Handler that uses http.Flusher several times per response.)

What are the rules for methods that return ErrUnimplemented? If a method call on a value returns it, does that method need to return it on every subsequent call? If a method call on a value doesn't return it (maybe does a successful operation, maybe returns a different error), is the method allowed to return it on a future call?

If there were a way to construct types with methods at runtime (possibly by trimming the method set of an existing type), with static answers for "does it support this optional feature", would that address the need?

tooolbox commented 4 years ago

Has any research gone into whether this can be solved at compile time, not needing to add extra run time checks which Go programs would need to add? I know it's a hard problem to solve, but I assume it's worth a try before falling back to run time checks.

This is where my mind goes on this subject. It seems like the conclusion has been made that this isn't possible, but I think the space is worth exploring.

If there were a way to construct types with methods at runtime (possibly by trimming the method set of an existing type), with static answers for "does it support this optional feature", would that address the need?

I suppose if you had interface A with method set X, and wanted to wrap it with B with method set Y (superset of X) you could re-wrap it with A afterwards to ensure that the final result had only the method set of X. Then at compile time you're assured to not call methods of B which are not actually supported by the underlying A.

tv42 commented 4 years ago

Can we please name it ErrNotImplemented? We have os.ErrNotExist not os.ErrNonexistent,

What are the rules for methods that return ErrUnimplemented? If a method call on a value returns it, does that method need to return it on every subsequent call?

Example where guaranteeing that would be tough: A filesystem that delegates to other filesystems based on a prefix of the path. Any FS-level optional interface that takes path names can lead to different implementations. Forcing the multiplexer to cope with "one answer must stick", either way, could get messy.

tv42 commented 4 years ago

@rsc

The pattern of "handle the call one way or another" seems much better for more code than the pattern of "refuse to handle the call". It preserves the property that when an error happens, it's a real failure and not something that needs retrying.

That's fine for cases where the optional interface is an optimization that can be safely ignored. That's not always true, though. Consider cp --reflink=always on a file tree larger than my free disk; if I can't copy-on-write it, I want to abort.

jimmyfrasche commented 4 years ago

Another pattern could be a HasX() bool method that returns true if the wrapped object actually implements method X.

If there's no HasX method but there is a method X, you assume that X is supported.

This is a bit heavy but not much more than a sentinel error check.

It can be ignored when it doesn't matter and old code will continue to function. If it matters, new code can query it and make a decision about how to proceed. This also let's multiple methods on multiple objects be queried before anything happens.

It also has the nice property that you can see in the documentation that an optimization may or may not be available.

jimmyfrasche commented 4 years ago

I wrote out a simple example for io.StringWriter even though it's probably overkill for something that simple.

It's a bit wordy to have an optional interface for an optional interface but seems to work fine and is easy enough to use.

https://play.golang.org/p/WCnA9tCk189

tv42 commented 4 years ago

@jimmyfrasche

Another pattern could be a HasX() bool method that returns true if the wrapped object actually implements method X.

An FS delegating to other FS'es based on path prefix can't answer that yes-or-no without knowing the arguments to the call (the path to delegate based on).

jimmyfrasche commented 4 years ago

Would it work if the HasX methods for the FS took the path as an argument?

tv42 commented 4 years ago

Maybe, but you're still left with a TOCTOU race. Consider the delegation mapping changing on the fly.

tooolbox commented 4 years ago

@tv42 sounds like you're arguing in favor of ErrNotImplemented, correct?

tv42 commented 4 years ago

@tooolbox I'm trying to discover requirements and make sure people realize their consequences. So far, I haven't seen anything else cover everything. That's not quite the same as having fixed my take on a winner, new ideas welcome!

bcmills commented 4 years ago

See previously #39436, but that is proposed for operations that are not supported at all, not as an indication to fall back to an alternate implementation.

rsc commented 4 years ago

@tv42

That's fine for cases where the optional interface is an optimization that can be safely ignored. That's not always true, though. Consider cp --reflink=always on a file tree larger than my free disk; if I can't copy-on-write it, I want to abort.

You lost me a bit here.

If the method in question is defined to do exactly X, it should definitely not do almost-X instead. I think that's true in general, with or without ErrUnimplemented.

Translating to your example (I hope!), suppose there's a Copy method and an optional CopyReflink method, and CopyReflink is considered a reasonable optimized implementation of Copy, but Copy is not a valid implementation of CopyReflink.

Then I would expect that func Copy might look for a CopyReflink method, use it if possible, and otherwise fall back to Copy. But of course a CopyReflink implementation would never settle for calling Copy instead. In this case, I think you'd write:

func Copy(x Copier, src, dst string) error {
    if x, ok := x.(CopyReflinker); ok {
        if err := x.CopyReflink(src, dst); err == nil {
            return nil
        }
    }
    return x.Copy(src, dst)
}

As written, if x.CopyReflink fails for any reason, Copy falls back to plain x.Copy. In this case, that seems fine: if it fails, presumably no state has changed so doing x.Copy is OK.

But regardless of whether I got the above right, having ErrUnimplemented available doesn't seem to help any. If x.CopyReflink returns ErrUnimplemented, then we agree that the code would fall back to x.Copy. But what if it returns a different error, like "cannot reflink across file systems"? Shouldn't that fall back to x.Copy too? And what if it returns "permission denied"? Maybe that shouldn't fall back to x.Copy, but presumably x.Copy will get the same answer.

In this case there are other errors that should be treated the same way as ErrUnimplemented, but not all. So testing for ErrUnimplemented introduces a special path that is either too special or not special enough.

tv42 commented 4 years ago

@rsc You've successfully debated against my point wrt cp --reflink=always. I think part of the confusion comes from GNU making the various modes of cp --reflink= all be called cp. CopyReflink makes sense, and behaves as cp --reflink=always.

I think this sort of "try an optimization" codepaths should only fall back to the unoptimized case on specific errors explicitly talking about the optimization (here, e.g. EXDEV), not just any error. And in that world, ErrUnimplemented is one of those specific errors. I will happily admit this is more of a philosophical stance than something I can vigorously defend. I don't like programs "hitting the same failure multiple times". I don't like seeing e.g. multiple consecutive attempts to open the same file, with the same error, just because the code tries all possible alternatives on errors that aren't specific to the alternate codepath.

ianlancetaylor commented 4 years ago

Consider a file system operation like os.Link, which creates a new hard link to an existing file. Not all file systems support hard links. A middleware file system might want to provide the Link method. But it might turn out that the target file system does not provide the Link method. There is no fallback operation for the middleware file system: if hard links aren't supported, there is no substitute. What error should the middleware Link method return if the target file system does not define Link?

Of course we can define a particular error result for Link that means "hard links not supported," but it seems like a moderately general concept. (I suppose one could also make an argument for returning syscall.EMLINK, which means "file already has maximum number of hard links", but that doesn't seem entirely satisfactory.)

bcmills commented 4 years ago

@ianlancetaylor, the Link case is exactly why I proposed os.ErrNotSupported (#39436). (One of my motivating examples there was file locking, which is similar: either the filesystem supports locking or it doesn't, and no semantically-equivalent fallback is possible.)

I still prefer the ErrNotSupported naming over ErrUnimplemented. To me, “unimplemented” suggests that the functionality can and should exist and the author just hasn't gotten around to it, while “not supported” does not carry any implication about whether the unsupported operation could, in principle, become supported.

tv42 commented 4 years ago

FWIW, link(2) can return "EPERM The filesystem containing oldpath and newpath does not support the creation of hard links." (but EPERM has multiple meanings, as is typical for errno). The greater point applies, and a more semantically obvious error is nicer -- but any FS implementation that actually does syscalls can return EPERM for Link anyway. Probably easiest to see in action on your UEFI boot FAT partition.

@bcmills I think you typoed ErrNotSupporting when you meant ErrNotSupported

ianlancetaylor commented 4 years ago

@bcmills I suppose the question is whether we should have an os-specific error or whether we should have a more general error.

(There have been several comments about the package and the name of the error, but I'd rather first decide whether we want a general error at all, and then bikeshed about the name.)

ianlancetaylor commented 4 years ago

To clarify, when I say that we can bikeshed over the package, I mean between choices like io and errors. These days the os package does not normally provide errors that are used by other packages.

darkfeline commented 4 years ago

What is the intended semantic meaning of ErrUnimplemented in this proposal? Is the caller always expected to fall back to an alternative implementation? If so, https://github.com/golang/go/issues/41198#issuecomment-686618566 applies. If not, then is there a need for a standard ErrUnimplemented versus defining a package-specific error type?

ianlancetaylor commented 4 years ago

The intended meaning is that the method is not implemented. Whether there is a fall back is going to depend on the method; as @rsc suggests, it will generally not be appropriate for a method with a reasonable fallback to return this error. It will only be a good choice for a result if there is no reasonable fallback.

Sure, we could use a package-specific error type. But we have io.EOF instead of a set of package-specific error types because it expresses a general concept. The question here is whether the concept expressed by this error is sufficiently general that we should give it a name.

bcmills commented 4 years ago

@ianlancetaylor I think it is important that the corresponding errors in the syscall, net/http, x/net/websocket, and x/net/webdav packages report equivalence with the new error via errors.Is. Beyond that I don't particularly care whether the error is in the os package or some other package, except to the extent that many users have already learned to look in the os package for general errors.

(The equivalent syscall errors, as identified in #39436, are EWINDOWS, EPLAN9, ENOSYS, ENOTSUP, and EOPNOTSUPP.)

rsc commented 4 years ago

For me this issue comes down to what the error would be used for. I think that's what Bryan was trying to get at yesterday by drawing a distinction between "not implemented" and "not supported".

To make this concrete, and since the original comment above mentioned WriteString, consider an io.Writer wrapper w wrapping an underlying io.Writer u. Suppose this proposal is adopted to produce the error ErrTBD. And suppose w.WriteString is implemented as follows:

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

Is that a valid implementation of WriteString? Is it a valid use of ErrTBD? As I understood "not implemented", the answer to both of these was "yes", hence my reply above making an analogy to EINTR. I think "yes" has serious problems.

Ian's most recent 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.

All that said, it is important that this error not be used as a singleton like io.EOF. An operation that fails should still follow the standard advice to include information about the requested operation in the returned error. Same way that code should never directly return os.ErrPermission.

rsc commented 4 years ago

Given all the rationale in my previous comment, I would be fine with adding:

package errors

// ErrUnsupported indicates that a request operation cannot be performed,
// because it is unsupported.
// Functions and methods should not return this error but should instead
// return an error including appropriate context that satisfies
//     errors.Is(err, errors.ErrUnsupported)
// either by directly wrapping ErrUnsupported or by implementing an Is method.
var ErrUnsupported = New("unsupported operation")

Is this what you had in mind?

ianlancetaylor commented 4 years ago

SGTM. Thanks.

darkfeline commented 4 years ago

I still don't feel like I understand what the meaning of "unsupported" or "unimplemented" is. If I implemented a database which does not support storing blobs bigger than 1 MB, would I return ErrUnsupported when attempting to store a 2MB blob? If I implemented a wrapper around an audio device file (say, some specific external microphone device), would I return ErrUnsupported if the audio device file were missing? If the audio device file appears later, would I then not return ErrUnsupported for future calls?

The question here is whether the concept expressed by this error is sufficiently general that we should give it a name.

This is what I'm worried about. I'm not convinced that the semantics of "unsupported" or "unimplemented" is well defined generally.

rsc commented 4 years ago

If I implemented a database which does not support storing blobs bigger than 1 MB, would I return ErrUnsupported when attempting to store a 2MB blob?

Sure, if you want to.

If I implemented a wrapper around an audio device file (say, some specific external microphone device), would I return ErrUnsupported if the audio device file were missing?

Probably not. Sounds more like os.ErrNotExist.

In both cases, you need not use these. You are never required to use one, unless you are implementing an interface that requires it.

If it is meaningful for users to distinguish the specific "not supported" error, then instead of defining your own, you could use this one instead, same as os.ErrPermission, os.ErrNotExist, and so on. For example, see the http.Pusher interface doc. It defines specific conditions under which Push returns http.ErrNotSupported. If errors.ErrUnsupported existed, the Pusher interface could have chosen to use it instead of defining a new error.

darkfeline commented 4 years ago

If it is meaningful for users to distinguish the specific "not supported" error, then instead of defining your own, you could use this one instead

In that case, would it make sense to add an errors.ErrPermission? After all, permission errors are very common, I would argue much more common than unsupported errors. If we can justify adding errors.ErrUnsupported to save users from defining their own error type for a "common use case" (hypothetically; I'm not convinced it is a common/general use case), then surely we could also justify adding errors.ErrPermission as it's a more common use case. We should also add an errors.ErrTemporary, since temporary errors are common. "Not exist" errors are also common. "Invalid argument" errors are also common.

rsc commented 4 years ago

@darkfeline, except ErrTemporary those errors are all defined in os and you are welcome to use them. Temporary turns out to be a very confusing concept in error handling and is beyond the scope of this issue.

darkfeline commented 4 years ago

This proposal is about adding ErrUnsupported or ErrUninplemented to the errors package, not the os package. I was questioning the justification that errors should contain a common error value for general reuse, because following that reasoning we could add many more such values to the errors package. I am not objecting to package specific errors existing such as those in the os package (e.g., https://github.com/golang/go/issues/39436 for adding os.ErrUnsupported).

ianlancetaylor commented 4 years ago

Perhaps if we were starting over we would move ErrPermission out of the os package, as the general concept of lacking permission does apply to other domains. But it's not out of place in the os package, as many operations in that package do indeed return errors that are errors.Is(err, os.ErrPermission). We could discuss copying ErrPermission elsewhere, but that is clearly a different proposal, not this one.

The goal of this proposed error, whatever its name, is really not related to the os package at all. It is "this interface method exists at compile time but, for whatever reason, cannot be invoked at run time." While we could put it in the os package, it's not an obvious place for it. And the existence of os.ErrPermission is not an argument for putting this error in the os package.

But maybe I'm misunderstanding you. If this error shouldn't be in the errors package, then where should it be?

darkfeline commented 4 years ago

If we were to add a generic ErrUnsupported, I agree it should be in the errors package. However, I'm not convinced of the premise, that we should add a generic ErrUnsupported.

My understanding is that the entire value provided by a generic ErrUnsupported is that it enables a package author to write

package foo

type Fooer interface {
 // Foo does a thing.  If the implementation does not support it, return errors.ErrUnsupported.
 Foo() error
}

instead of

package foo

import errors

type Fooer interface {
 // Foo does a thing.  If the implementation does not support it, return ErrUnsupported.
 Foo() error
}

var ErrUnsupported = errors.New("unsupported")

To me, that does not justify adding a new feature to the stdlib.

rsc commented 4 years ago

@darkfeline, it's always going to be fine to define your own if you don't want to use the common one. But that probably shouldn't preclude having a common one.

rsc commented 4 years ago

Based on the discussion above, the proposal detailed in https://github.com/golang/go/issues/41198#issuecomment-694577588 seems like a likely accept.

dsnet commented 4 years ago

Much of the discussion have been regarding the difference between "not supported" and "not implemented" and I'm still not entirely clear what the difference exactly is. If we add this sentinel error, I'm fairly sure a large number of Go users will not understand the difference as well.

When I first saw this proposal, my understanding was closer to "not implemented", but upon reading the discussion, it seems that this is something more like "not supported". If this were to be added and I had not read this discussion, I'm fairly sure I would have used this error as if it were "not implemented".

Code like:

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

is exactly like something I would have written using ErrUnsupported in place of ErrTBD.

To be honest, I'm not sure what the utility of this sentinel error is. The proposed documentation on this says:

// Functions and methods should not return this error but should instead // return an error including appropriate context that satisfies // errors.Is(err, errors.ErrUnsupported)

Support users do errors.Is(err, errors.ErrUnsupported), what are the things that we're expecting users to do with this? If they use that to switch to some fallback implementation, how is that functionally different than "not implemented"?

darkfeline commented 4 years ago

@rsc Regarding "But that probably shouldn't preclude having a common one.", I think we should be justifying why we should add this, and not why we shouldn't add this. Adopting this proposal adds almost no value at the cost of permanently expanding the stdlib API.

But let me also address the "why not" (although your comment https://github.com/golang/go/issues/41198#issuecomment-694577557 sounds like a good argument against). As is, ErrUnsupported doesn't have a clear semantic meaning. Compare this with io.EOF, which does have a clear semantic meaning, as defined by the io.Writer and io.Reader interfaces. A caller of these interfaces knows exactly how to interpret a returned io.EOF.

This is not true for ErrUnsupported. Two different packages may have two disparate interpretations for ErrUnsupported, which would be confusing to a developer using both packages. The Go standard library should avoid adding things that promote inconsistency in the Go ecosystem. It may also hinder interoperation of said packages:

func myFunc() error { // callers need to be able to distinguish the missing hardware case from either foo or bar
  if err := foo.Func(); err != nil {  // returns ErrUnsupported for both missing software and hardware
    return err
  }
  return bar.Func() // returns ErrUnsupported for missing hardware and unimplemented RPC method
}

Of course, myFunc can itself attempt to disambiguate the hardware ErrUnsupported case from both foo and bar, but it would be better if foo and bar defined their own error values. The Go standard library should not be encouraging the Go ecosystem to do the wrong thing and overload a common error value with different meanings.

bcmills commented 4 years ago

@dsnet, my main use case for an error of this form is to skip tests of functionality that is not present on the current platform.

Some examples from the cmd/go tests:

The difference between that and "unimplemented" is that no semantically-equivalent fallback is possible. (If a fallback implementation is possible, than the function being called should just do that in the first place.)

dsnet commented 4 years ago

my main use case for an error of this form is to skip tests of functionality that is not present on the current platform.

Thanks for clarifying. That's an reasonable use-case, but I doubt that's what most people are thinking of when they see this.

The difference between that and "unimplemented" is that no semantically-equivalent fallback is possible

I'm going to make the case that most people will not understand this, leading to this error being widely misused and therefore the meaning of it being increasingly muddled to the point where the documentation mismatches what is done in practice.

(If a fallback implementation is possible, than the function being called should just do that in the first place.)

I'm not a fan of this recommendation since there is often information that the caller has where they can do a better job at doing a fallback than what the function can do locally.

jimmyfrasche commented 4 years ago

Clearer documentation that defines when this should and should not be used would certainly be helpful should this be accepted.

ianlancetaylor commented 4 years ago
// ErrUnsupported indicates that a requested operation cannot be performed,
// because it is unsupported. This error indicates that there is no alternative
// way to perform the operation. For example, a call to os.Link when using a
// file system that does not support hard links.
// Functions and methods should not return this error but should instead
// return an error including appropriate context that satisfies
//     errors.Is(err, errors.ErrUnsupported)
// either by directly wrapping ErrUnsupported or by implementing an Is method.