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

proposal: errors: expose Iser and Aser #39539

Closed
carnott-snap opened this issue Jun 11, 2020 · 13 comments
Closed

proposal: errors: expose Iser and Aser #39539

carnott-snap opened this issue Jun 11, 2020 · 13 comments
Labels
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jun 11, 2020

The interfaces for errors.Is and errors.As seem stable and complete. Unfortunately, it requires reading through the godoc and use of a custom type to ensure that your custom error type implements interface{ As(interface{}) bool } or interface{ Is(error) bool }.

I propose exposing the following symbols:

package errors

// Aser is the interface implemented by types that can be converted into other types.
//
// See As for full usage.
type Aser interface{ As(interface{}) bool }

// Iser is the interface implemented by types that are comparable to other errors.
//
// See Is for full usage.
type Iser interface{ Is(error) bool }

And fixing up errors.Is and errors.As to document and use them:

--- a/src/errors/wrap.go
+++ b/src/errors/wrap.go
@@ -27,7 +27,7 @@
 // repeatedly calling Unwrap.
 //
 // An error is considered to match a target if it is equal to that target or if
-// it implements a method Is(error) bool such that Is(target) returns true.
+// it implements Iser such that Is(target) returns true.
 //
 // An error type might provide an Is method so it can be treated as equivalent
 // to an existing error. For example, if MyError defines
@@ -46,7 +46,7 @@
 		if isComparable && err == target {
 			return true
 		}
-		if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) {
+		if x, ok := err.(Iser); ok && x.Is(target) {
 			return true
 		}
 		// TODO: consider supporting target.Is(err). This would allow
@@ -65,9 +65,9 @@
 // repeatedly calling Unwrap.
 //
 // An error matches target if the error's concrete value is assignable to the value
-// pointed to by target, or if the error has a method As(interface{}) bool such that
-// As(target) returns true. In the latter case, the As method is responsible for
-// setting target.
+// pointed to by target, or if the error implements Aser such that As(target)
+// returns true. In the latter case, the As method is responsible for setting
+// target.
 //
 // An error type might provide an As method so it can be treated as if it were a
 // a different error type.
@@ -92,7 +92,7 @@
 			val.Elem().Set(reflectlite.ValueOf(err))
 			return true
 		}
-		if x, ok := err.(interface{ As(interface{}) bool }); ok && x.As(target) {
+		if x, ok := err.(Aser); ok && x.As(target) {
 			return true
 		}
 		err = Unwrap(err)

This allows error implementers to ensure that the interfaces are implemented correctly, while also cleaning up errors documentation and internal logic:

var _, _, _ = errors.Iser(myError{}), errors.Aser(myError{}), error(myError{})

That being said, the names Iser and Aser sound like fantasy characters and are not strictly required, since they do not exist today and errors.Is and errors.As work fine. Furthermore, I cannot think of a reason to pass, accept, or typecast these interfaces, so their usefulness is limited. That being said, json.Marshaler suffers from similar issues, yet it exists.

@gopherbot gopherbot added this to the Proposal milestone Jun 11, 2020
@gopherbot gopherbot added the Proposal label Jun 11, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 12, 2020

Any package could define those interfaces itself, of course. I'm not sure how compelling it is to define them in the standard library. I would not expect many packages to use them.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 12, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 12, 2020

While I agree that any package can define these symbols, exporting them from errors makes for a formal contract that is verifiable at compile time. For me, this is one of the main benefits of using a compiled language.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

Simply implementing the interfaces seems like not enough if you are checking correctness. If you write a test, you'll check the whole end-to-end implementation, including the method signatures.

One thing we could do is update the cmd/vet check to check the signature of an Is or As method on a type implementing error, like we do in the 'stdmethods' check for things like WriteByte.

Both of those would seem better than exporting Iser/Aser.

@rsc rsc moved this from Incoming to Active in Proposals Jun 24, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 24, 2020

I missed this before, but xerrors exports Wrapper; was this omitted for similar reasons? Seems like we should add it, if we are doing the other two.

While I agree such end-to-end tests are better, var _ xxx.Interfacer = Type{} checks are usually sufficient, easier, and more concise. I dislike the concept of having these magical interfaces exist only in the docs, and internal implementation logic, because typos happen. Plus the standard library stance seems unclear, few if any Xxxer interfaces seem required, and we are opposed to this here, but in other packages it is fine, say json.Marshaler.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 8, 2020

I'm pretty sure we didn't copy Wrapper over because all code should use errors.Unwrap. There's little need for it once you have Unwrap. We could make the same vet check check an Unwrap method on an error implementation too.

It sounds like people are generally in favor of the vet check instead of exposing interfaces? Do I have that right?

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jul 8, 2020

While I dislike that these interfaces only exist in documentation, the vet check would serve the same purpose as var _ ... with lower user impact. To confirm, types that implement error that contain methods with the given name must meet the following interface:

  • Is: interface{ Is(error) bool }
  • As: interface{ As(interface{}) bool }
  • Unwrap: interface{ Unwrap() error }

Are we concerned with collisions, since these words are not use case specific, like json.Marshaler, interface{ MarshalJSON() ([]byte, error) }? Maybe it would have been good to have named these interfaces XxxError.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2020

Are we concerned with collisions,

Not really, because the check would only happen if you also implement Error() string. At that point we know it's an error. The method being named IsError wouldn't clarify much.

Based on the discussion above, it sounds like this is a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Jul 16, 2020
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jul 16, 2020

Can this be exported for documentation purposes. Like how json.Unmarshaler is exported even though almost no one will use it directly. Hunting for the method signature in the errors.Is doc comments isn't very nice and having it exported will make it easier to point people to

@as
Copy link
Contributor

@as as commented Jul 16, 2020

I think exporting a named interface weakens the abstraction, as the name of the interface matters in some cases unlike its anonymous counterpart (there was an issue about this, but I can't find it).

Also, errors.Iser and errors.Aser are not great names, but I can't think of anything better, since errors.As and errors.Is are already used in that package.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

No change in consensus, so declined.

@rsc rsc moved this from Likely Decline to Declined in Proposals Jul 22, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jul 22, 2020

Would you like me to submit a different proposal for the vet check, or does it make more sense to transmute this one?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 22, 2020

@carnott-snap I think a different proposal would be better. Thanks.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 6, 2020

@rsc: closing this one as well, did the process change, or were these two just missed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.