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: discourage cyclic error types #34957

Closed
smasher164 opened this issue Oct 17, 2019 · 7 comments
Closed

errors: discourage cyclic error types #34957

smasher164 opened this issue Oct 17, 2019 · 7 comments

Comments

@smasher164
Copy link
Member

@smasher164 smasher164 commented Oct 17, 2019

It is possible to create a cycle of errors such that errors.Is and errors.As never terminate. A minimal example is shown below: (https://play.golang.org/p/gC4Ws7zV8eo)

package main

import (
    "errors"
    "io"
)

type cyclicerror struct {
    err error
}

func (e *cyclicerror) Error() string { return "error" }
func (e *cyclicerror) Unwrap() error { return e.err }

func main() {
    a := new(cyclicerror)
    a.err = a
    errors.Is(a, io.EOF)
}

That said, is it reasonable to explicitly discourage packages from returning errors that can potentially form an Unwrap cycle, even through a recursive error type? Do I import a package on good faith after an extensive audit that it won't introduce a cycle, or should errors.Is and errors.As perform cycle detection?

@smasher164 smasher164 added this to the Backlog milestone Oct 17, 2019
@tv42
Copy link

@tv42 tv42 commented Oct 18, 2019

How is this different from

func (e *cyclicerror) Error() string { for {}; return "not gonna happen" }
@smasher164
Copy link
Member Author

@smasher164 smasher164 commented Oct 18, 2019

They are functionally the same, but this is caused by some chain of Unwrap calls that forms a cycle. Although I don't think cycles will happen through intentional API design, I worry they may unintentionally happen from recursive error types.

errors.Is and errors.As could do the extra work of detecting cycles and return false / nil in those circumstances. Otherwise, I think it is worth documenting that errors.Is and errors.As don't check for cycles.

@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented Oct 18, 2019

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 18, 2019

My opinion is that errors.Is and errors.As do not need to check for cycles. My reasoning is that for any type that implements error and also includes a subfield of type error, the Error method will ordinarily print the value of error. Therefore, if there is a cycle, the Error method will not return. So we will only run into trouble for the unusual case of a type that implements error and has an Unwrap method but for which the Error method does not print the error being wrapped. I think that case is sufficiently unusual that it is not necessary to add code to errors.Is and errors.As. It is sufficient to document this somewhere.

@jba
Copy link
Contributor

@jba jba commented Oct 18, 2019

I agree with Ian. The usual convention in linked-list based APIs, not just in simplicity-forward languages like Go but also in languages like Java that have different priorities, is to write the simple loop and let the user worry about cycles. (Search for indexOf on http://developer.classpath.org/doc/java/util/LinkedList-source.html.) Common Lisp provides a list-length function which will work correctly in the presence of cycles, where the generic length function will not. But that is presumably because circular lists are useful, and Common Lisp had no fear about being a big language.

In this case, cycles are both rare and indicate a bug. So I don't think it's the standard library's job to find them.

@neild
Copy link
Contributor

@neild neild commented Oct 18, 2019

I agree with Ian and Jonathan. In addition, I don't see any way to accidentally produce a cyclic error.

We could add explicit documentation that errors.Is and errors.As don't perform cycle detection, but personally I think the default expectation for APIs operating on linked lists is no special handling of cycles.

@smasher164
Copy link
Member Author

@smasher164 smasher164 commented Oct 19, 2019

I agree that cycle detection should not be the responsibility of the errors package. On further thought, it makes sense to see how these functions are used in the wild before preemptively documenting its behavior. As such, I will close the issue.

@smasher164 smasher164 closed this Oct 19, 2019
@golang golang locked and limited conversation to collaborators Oct 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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