go: errors: add ErrUnsupported
UPDATE: This proposal has shifted from the original description to the one described in comments below.
- https://github.com/golang/go/issues/41198#issuecomment-693014681
- https://github.com/golang/go/issues/41198#issuecomment-694577557
- https://github.com/golang/go/issues/41198#issuecomment-694577588
- https://github.com/golang/go/issues/41198#issuecomment-698029567
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:
io.WriteString
checks whether anio.Writer
has aWriteString
method, and either calls it or callsWrite
.io.Copy
checks the source for aWriterTo
method, and then checks the destination for aReaderFrom
method.net/http.(*timeoutWriter).Push
checks for aPush
method, and returnsErrNotSupported
if not found.
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.
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Reactions: 30
- Comments: 99 (71 by maintainers)
Commits related to this issue
- rename ErrNotSupported to ErrUnsupported In the future, this should be replaced by errros.ErrUnsupported, if that ever gets implemented: https://github.com/golang/go/issues/41198 — committed to relab/wrfs by johningve 3 years ago
- rename ErrNotSupported to ErrUnsupported In the future, this should be replaced by errros.ErrUnsupported, if that ever gets implemented: https://github.com/golang/go/issues/41198 — committed to relab/wrfs by johningve 3 years ago
- cmd/go/internal/modfetch: use errors.ErrUnsupported CL 473935 added errors.ErrUnsupported, let's use it. Updates #41198 Change-Id: If6534d19cb31ca979ff00d529bd6bdfc964a616d Reviewed-on: https://go-... — committed to golang/go by tklauser a year ago
- syscall: handle errors.ErrUnsupported in isNotSupported Updates #41198 Change-Id: Ifed913f6088b77abc7a21d2a79168a20799f9d0e Reviewed-on: https://go-review.googlesource.com/c/go/+/475857 Run-TryBot: ... — committed to golang/go by tklauser a year ago
- syscall: let EPLAN9 and EWINDOWS implement errors.ErrUnsupported As suggested by Bryan. This should fix the failing TestIPConnSpecificMethods on plan9 after CL 476217 was submitted. For #41198 Chan... — committed to golang/go by tklauser a year ago
- syscall: let ENOSYS, ENOTSUP and EOPNOTSUPP implement errors.ErrUnsupported As suggested by Bryan, also update (Errno).Is on windows to include the missing oserror cases that are covered on other pla... — committed to golang/go by tklauser a year ago
- syscall: let errors.ErrUnsupported match ERROR_NOT_SUPPORTED and ERROR_CALL_NOT_IMPLEMENTED These error codes are returned on windows in case a particular functions is not supported. Updates #41198 ... — committed to golang/go by tklauser a year ago
- cmd/go/internal/lockedfile/internal/filelock: use errors.ErrUnsupported All platform specific errors are now covered by errors.ErrUnsupported. Updates #41198 Change-Id: Ia9c0cad7c493305835bd5a1f349... — committed to golang/go by tklauser a year ago
- net/http: let ErrNotSupported match errors.ErrUnsupported For #41198 Change-Id: Ibb030e94618a1f594cfd98ddea214ad7a88d2e73 Reviewed-on: https://go-review.googlesource.com/c/go/+/494122 Auto-Submit: I... — committed to golang/go by ianlancetaylor a year ago
- doc/go1.21: mention errors.ErrUnsupported Also mention errors that implement it. For #41198 Change-Id: I4f01b112f53b19e2494b701bb012cb2cb52f8962 Reviewed-on: https://go-review.googlesource.com/c/go... — committed to golang/go by ianlancetaylor a year ago
- doc/go1.21: mention errors.ErrUnsupported Also mention errors that implement it. For #41198 Change-Id: I4f01b112f53b19e2494b701bb012cb2cb52f8962 Reviewed-on: https://go-review.googlesource.com/c/go... — committed to golangFame/go by ianlancetaylor a year 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:
If that type wants to make the ReadFile method on fs available, it can already do:
With this proposal, the code could instead do:
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.
For the record, despite the apparent consensus, I’m -1 on this proposal: I think it’s unnecessary and sets a bad precedent.
A couple of thoughts:
Firstly, by making this a centrally defined error value, there’s an increased risk of confusion when errors are passed up unchanged. For example, if I call
F
which callsG
which returnsErrUnsupported
, then the caller ofF
might think thatF
itself is unsupported, when it’s actuallyG
. I’ve seen this kind of error-conflation issue in the past with sharedErrNotFound
errors, and it can result in rare, hard-to-find bugs. I don’t really see what’s wrong with having domain-specificErrUnsupported
error values targetted to the specific interface that’s returning the error. What is it aboutErrUnsupported
that means that it’s worthy being the only centrally defined error in theerrors
package?Secondly, unlike with type assertions, it’s not possible to find out whether the operation is supported without attempting to actually perform the operation. For example, in this comment @bcmills suggests:
That means that you’ll need to actually create a symlink (perhaps with a bogus name) to see whether symlinks are supported. Symlink creation is a lightweight operation, so it probably doesn’t matter in this case, but I could imagine other situations where there might be more cost involved. That said, I’m not keen on having additional
HasX
methods either, so perhaps this is fine - maybe it should just be a “thing” that when documenting a given interface, we say that calling a method with zero values will do nothing except return an error if the method isn’t supported.In general, I’m -1 on this proposal. Although
ErrUnsupported
is a useful concept, I don’t think that it in a central place is particularly helpful, and it could be harmful in some circumstances.Shh 🤫, rogpepe and I are hoping they forget this proposal was approved because it’s actually a bad idea. Let’s just let it stay quiet before someone writes a CL.
Sorry, the “close” and “unsubscribe” buttons in the GitHub app are very close
Can we spell it without the Err prefix, since it is in the errors package?
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.
@ianlancetaylor, the
Link
case is exactly why I proposedos.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 overErrUnimplemented
. 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.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:
But this could instead be done using:
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.
(A minor suggestion) I feel
Unimplemented
is hard to read. Can I suggestNo change in consensus, so accepted:
(And again, people writing APIs that don’t find this error appropriate are in no way obligated to use or return it.)
@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 withio.EOF
, which does have a clear semantic meaning, as defined by theio.Writer
andio.Reader
interfaces. A caller of these interfaces knows exactly how to interpret a returnedio.EOF
.This is not true for
ErrUnsupported
. Two different packages may have two disparate interpretations forErrUnsupported
, 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:Of course,
myFunc
can itself attempt to disambiguate the hardware ErrUnsupported case from bothfoo
andbar
, but it would be better iffoo
andbar
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.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:In other words, yes, for these cases you are expected to write two tests.
Given all the rationale in my previous comment, I would be fine with adding:
Is this what you had in mind?
Sorry, I might be a little late to the party, but have you considered using a constant instead of a variable (aka. constant errors):
Admittedly, I haven’t read the entire issue, but I couldn’t find any mention of constants.
Also, this doesn’t follow the current pattern for global error variables, so might be out of scope here (although the Go 1 BC promise would not allow to change this later).
Sorry for the noise if this is too late or out of scope for this proposal.
I can see cases where a caller might notice that they get
ErrUnsupported
and then stop attempting a sequences of operations—say I have a bunch of files I want to link, stopping if any returnErrUnsupported
—but I’m unclear what the benefits are of having a central definition forErrUnsupported
. Why not havefs.ErrUnsupported
distinct fromhttp.ErrUnsupported
? Is it just an error hierarchy for the sake of ontological tidiness?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.Writeru
. Suppose this proposal is adopted to produce the errorErrTBD
. And suppose w.WriteString is implemented as follows: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.
Thanks for clarifying. That’s an reasonable use-case, but I doubt that’s what most people are thinking of when they see this.
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.
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.
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 genericErrUnsupported
.My understanding is that the entire value provided by a generic
ErrUnsupported
is that it enables a package author to writeinstead of
To me, that does not justify adding a new feature to the stdlib.
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”.This seems to overlap with the existing way of testing whether a method/interface is supported:
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.
I looked at
x/net/webdav.ErrNotImplemented
. This is an example of the pattern that @rsc described at https://github.com/golang/go/issues/41198#issuecomment-686618566: an optional method is permitted to provide a more efficient operation, and if it returnsErrNotImplemented
then a standardized approach is used instead. This error should not matchErrUnsupported
.x/net/websocket/ErrNotImplemented
is not used, but if it were used it should probably not matchErrUnsupported
.x/net/websocket/ErrNotSupported
is used bywebsocket.Message
. The latter is defined to support marshalingstring
and[]byte
values only. If you try to marshal something else, you getErrNotSupported
. To me this seems different fromerrors.ErrUnsupported
, which is about cases that might sometimes work but, in a particular case, don’t. Thewebsocket.ErrNotSupported
seems more like programmer error. It’s a case that will never work.So I don’t think we need to change anything in these packages. Happy to hear counter-arguments.
I’m not sure if it makes more sense to comment here or on the CL, but to me, the problem is there’s no such thing as “unsupported” in general. There are specific unsupported operations. If I try to HTTP2 push and it fails, that’s different than trying to write to a read only filesystem. So what if instead there were a struct (UnsupportedError?) with Op string and Cause error fields?
Unfortunately in this kind of situation, the program tends to think that it knows exactly what the error is and thus discards the error rather than logging it.
If the result of the method is documented, I still don’t really see why there’s an advantage in having this value in a central place rather than associated more closely with the API that contains the documentation.
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?
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.
(Marking as release-blocker so that we ensure that this is either completed or rolled back before the release.)
It seems like a key aspect of the error is that it’s for generic code which does not accurately model the underlying system or the thing it’s wrapping, and so you have a method call that’s actually invalid. It’s incorrect that the method exists at all on the interface or struct, but there’s just no better option given the desired abstraction, the language, and whatever other constraints.
At first brush it seems like this would most come up in an operating system or filesystem, which is why I am/was in favor of it being in one of those packages. But I suppose there are other areas where there are similar-but-different items, and which therefore don’t perfectly fit into abstractions, and so we get invalid methods on an interface, etc.
If we are going to add this, I feel like it’s worth documenting that point: if you’re returning this error, it’s because the abstraction has failed, and what you have in that interface or struct doesn’t actually quite fulfill it, and it can’t fake fulfillment with a semantically equivalent operation.
To me, that gives me a much better conceptual understanding of when I should return that error, while informing me that I should strive to avoid doing so if at all possible.
(I’m sure there’s a more prosaic or nicer way to put it in the actual docs, while still getting this point across.)
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 addingerrors.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 addingerrors.ErrPermission
as it’s a more common use case. We should also add anerrors.ErrTemporary
, since temporary errors are common. “Not exist” errors are also common. “Invalid argument” errors are also common.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?
This is what I’m worried about. I’m not convinced that the semantics of “unsupported” or “unimplemented” is well defined generally.
@tv42
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:
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.
@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!
We think the the CL above has closed this issue.
Yes, I think I’m ready to call it an addendum to this proposal. Sending a CL.
Technically I think that making
net/http.ErrNotSupported
matcherrors.ErrUnsupported
will require defining either anIs
or anUnwrap
method onnet/http.ProtocolError
. So that is new API, which technically requires a proposal. Anybody think we can skip the proposal here?I agree that
net/http.ErrNotSupported
should matcherrors.ErrUnsupported
. It could unwrap toerrors.ErrNotUnsupported
, or perhaps we should just defineErrNotSupported = errors.ErrUnsupported
.Should http.ErrNotSupported gain an Is method to equal errors.Unsupported?
~I hate to just pop in and bike shed, but there was never a discussion around what should be done next time for stutter like
context.Context
(#42877), and whileerrors.ErrXxx
does follow thevar ErrXxx = errors.New("")
convention, we have never had a sentinel inpackage errors
, would it be better to useerrors.Unsupported
, like we haveio.EOF
?~EDIT: nevermind, https://github.com/golang/go/issues/41198#issuecomment-686620712
I do think that any method that might return this has to document it. So if
F
callsG
which returnsErrUnsupported
, thenF
needs to decide at that point whether it should returnErrUnsupported
, meaning thatF
is not supported, or whether it should take some other action. For cases where it escapes up the stack, the docs are clear that it should always be wrapper so if nothing else theError
text should report which method is unavailable.It’s true that there will be cases where a
HasM
method is appropriate, or where there should be some special way of calling the method that just reports whether it is supported. That’s OK.There was some confusion over the past week about what was the likely accept, but @ianlancetaylor has edited the top comment to make that clearer. I don’t see any change in consensus about accepting the text from https://github.com/golang/go/issues/41198#issuecomment-694577588:
To respond to @darkfeline’s comment above https://github.com/golang/go/issues/41198#issuecomment-697916765, it’s true for every generic error “class” that different packages may have slightly different meanings, but that still doesn’t make them not useful. For example os.ErrPermission, io.ErrUnexpectedEOF, and so on can all have slightly different interpretations based on what the actual operation was. It can still be worth having a general instance.
As a concrete example, http.Push’s ErrNotSupported could declare itself to be Is(ErrUnsupported).
Will leave open for another week but this still seems like a likely accept.
@darkfeline Yes. The meaning of the suggested error has shifted from the original proposal. I’ll edit the initial comment.
That sounds like it contradicts many of the given examples and the original proposal:
Based on the discussion above, the proposal detailed in https://github.com/golang/go/issues/41198#issuecomment-694577588 seems like a likely accept.
@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.
@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.
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 meantErrNotSupported
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.
Does this proposal include recommendations for the wider community on whether to use ErrUnimplemented or the above for new code?