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: export Is and/or As in an interface #45568

Closed
perrito666 opened this issue Apr 14, 2021 · 12 comments
Closed

proposal: errors: export Is and/or As in an interface #45568

perrito666 opened this issue Apr 14, 2021 · 12 comments

Comments

@perrito666
Copy link

Currently the definition for the signature of a type implementing the interface for Is and As lives in a one-liner inside Is and As respectively.

Is: https://golang.org/src/errors/wrap.go?s=1170:1201#L49

if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) {
	return true
}

As: https://golang.org/src/errors/wrap.go?s=1170:1201#L91

if x, ok := err.(interface{ As(interface{}) bool }); ok && x.As(target) {
	return true
}

I propose these two be exported as proper interfaces instead for clarity and to ease use of tools like goimpl and akin.

type SupportsIs interface{
    Is(error) bool
}

type SupportsAs interface{
    As(interface{})bool
}

This would also clarify the above snippets (this is just an opinion, not a critique of the code quality)

Is would read "err supports Is"

if x, ok := err.(SupportsIs); ok && x.Is(target) {
	return true
}

As would read "err supports As"

if x, ok := err.(SupportsAs); ok && x.As(target) {
	return true
}
@gopherbot gopherbot added this to the Proposal milestone Apr 14, 2021
@smasher164
Copy link
Member

Dup of #39539. The justification provided was

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

@natefinch
Copy link
Contributor

We can bikeshed the names, but I think it is pretty clear that code which relies on a specific definition of an interface should export that interface if it is not defined elsewhere.

@natefinch
Copy link
Contributor

natefinch commented Apr 14, 2021

For me, the most compelling argument for an interface defined in this package is one of documentation. It's much easier to find on an exported interface than buried in the docs of Is().

But also, the argument for "you can just write a test for it" falls flat for me. I could just write a test for lots of things the compiler could do for me. But why?

I guess I don't see why you wouldn't want it here? It would be more clear and discoverable. It would give one canonical definition of the interface. It gives you a place to anchor the docs. I understand that keeping an API small is desirable, but this interface already exists in the API of this package... It's just invisible. That's bad.

Edit: One more thing that I think is important - by defining the interface in the package, we give it a name. Without a name, it's really hard to have a conversation about this interface, or reference it. Right now, you can't tell someone "oh, you need to implement errors.Iser to do that...." you have to say "oh you have to add a method to your type called Is(error)bool to do that ...."

The json.Marshaler example is perfect. How do you do custom json marshalling? Implement json.Marshaler. I can google for json.Marshaler. I can't google (easily) for interface{Is(error)bool}.

@smasher164

This comment has been minimized.

@ianlancetaylor
Copy link
Member

Has anything changed since #39539 was closed?

@neild
Copy link
Contributor

neild commented Apr 14, 2021

The two compelling arguments against adding exported interfaces containing Is and As are (as I see it):

  1. We don't have good names for these interfaces. Iser is ugly, Wrapper or Unwrapper are confusing (and arguably not idiomatic), SupportsIs and Unwrappable are unwieldy (and arguably not idiomatic). Whatever we pick, someone will be unhappy.
  2. We don't have a compelling use case. I don't believe I've seen an example of code which should type assert to them rather than using errors.Is or errors.As.

I think that a prerequisite for reconsidering whether we should add exported definitions of these interfaces is either unambiguously good names for the interfaces or a compelling use case.

@urandom2
Copy link
Contributor

urandom2 commented Apr 14, 2021

well, the proposed alternative, #40356, has been largely ignored, so that may be contributing to the ongoing suffering.

@neild
Copy link
Contributor

neild commented Apr 14, 2021

I would recommend testing that an error produces the correct results when used with errors.Is or errors.As over a type assertion. A type assertion can only verify that the error type implements the expected method; a test will verify that the method is implemented correctly.

@perrito666
Copy link
Author

I can test my implementation of errors.Is and its behavior but that is not what I am trying to achieve here, the errors package does establish a contract where the presence of an <type>.Is(error)bool method in anything that implements error will cause "special" behavior when invoking errors.Is so an interface should exist to support said contract.

Regarding the name: the fact that we cannot quickly think a name should not stop us, as this error handling method reaches further adoption (I, for instance, am using this more and more) we are talking about it and honestly it feels like trying to talk about Prince when he had a symbol for a name. "the thing that happens when you define an Is or As method in your error" makes it difficult to talk about it, document it, reference it and search for it.

Let us leave this issue open for a few days and see if we can get more voices for and against it, I suspect the result will be different from last time as it has gained a bit more adoption.

@smasher164
Copy link
Member

Wasn't there a spec change that ensured interface types are just method sets now? I would think that the argument in #39539 that naming it would weaken the abstraction doesn't apply anymore.

@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

Closing as duplicate of #39539. (Still too soon.)
Will make sure #40356 gets added to the active set.

@rsc rsc closed this as completed Apr 21, 2021
@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

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

@golang golang locked and limited conversation to collaborators Apr 21, 2022
@rsc rsc moved this to Declined in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants