Go: errors: discourage cyclic error types

[smasher164]

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?

[tv42]

How is this different from

func (e *cyclicerror) Error() string { for {}; return "not gonna happen" }

[smasher164]

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]

/cc @neild @jba @mpvl

[ianlancetaylor]

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]

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]

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]

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.