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

xerrors: Add a function to test an error's chain against a predicate function #30093

Closed
neild opened this issue Feb 5, 2019 · 6 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Feb 5, 2019

I'm splitting this off from #29934 to allow for more focused discussion.

There are three common patterns used in Go code to make control flow decisions based on an error's value.

  • Comparison to a sentinel: err == X.
  • Conversion to a known type: _, ok := err.(T); ok
  • Testing against a predicate function: f(err).

The error values proposal provides Is and As functions to test an error's chain against a sentinel value or underlying type, respectively. It does, however, not currently provide direct support for the third pattern of predicate functions.

Proposal: Add an IsFunc function to test an error's chain against a predicate function to the golang.org/x/xerrors and errors packages.

// IsFunc calls f for each error in err's chain. IsFunc immediately returns true if f
// returns true. Otherwise, it returns false.
func IsFunc(err error, f func(error) bool) bool { ... }

IsFunc can be used directly with existing predicate functions like os.IsNotExist:

if errors.IsFunc(err, os.IsNotExist) { ... }

Why not update os.IsNotExist et al. to unwrap errors?

  • No good way to write code that works with current and future versions of Go: You can't rely on os.IsNotExist unwrapping so you need to do it yourself to support pre-1.13 Go.

  • This puts the onus on the user to determine if any given predicate function supports unwrapping, and as of what version.

  • Passing a wrapped error to a predicate which doesn't unwrap is a silent runtime bug. The error can't be detected at compile time, and it isn't even a runtime panic; you just silently take the wrong branch.

If we were starting entirely anew, then it might make sense to put the unwrapping in the predicate functions. Doing so at this time seems difficult to accomplish without encouraging the creation of many subtle bugs.

@jimmyfrasche
Copy link
Member

I also played with a version that returns the error that matches the predicate or nil, but

  • it's almost never needed. In situations where this comes up you mostly just care whether there is an error that matches the predicate.
  • when it's not needed, you have to tack a != nil on the end
  • when it is needed, you still need to do whatever assertion/testing again to recover the desired error so at that point it makes more sense to write your own helper that does the tests and returns the correct kind of error

The example predicate I used in the previous thread:

func temporary(err error) bool {
  t, ok := err.(interface{ Temporary() bool })
  return ok && t.Temporary()
}

There's talk in the original thread of updating As to let you assign to interfaces. With that you could do

type temp interface { Temporary() bool }
if xerrors.As(err, &temp) && temp.Temporary() { ... }

but note a subtle difference: IsFunc(err, temporary) returns true if any error in the chain both matches that interface and returns true but the As example only takes the error path if the first error in the chain that matches the interface returns true. Which is correct depends on the application and the potential composition of the error chain, so it's hard to say whether that subtlety is a feature or a bug. This would apply to xerrors.IsFunc(err, os.IsTimeout).

For os.IsExist et al., many of those predicates has a corresponding sentinel error like os.ErrExist that is among the errors that cause the predicate to return true. With appropriate Is methods on *os.PathError et al., those methods would allow you to write xerrors.Is(err, os.ErrExist). That would work nicely and consistently for code that does not need compatibility but otherwise has the same drawbacks as updating the predicates themselves.

@neild
Copy link
Contributor Author

neild commented Feb 5, 2019

With appropriate Is methods on *os.PathError et al., those methods would allow you to write xerrors.Is(err, os.ErrExist).

I think this is absolutely the cleanest API to expose to a user, and we should consider doing it. It does, as you say, have the same drawback of potentially becoming a silent bug in older Go versions. On the other hand, if you always write errors.Is(err, os.ErrExist) (not xerrors.Is), then the failure is at compilation time.

@dsnet
Copy link
Member

dsnet commented Feb 5, 2019

If a predicate function eventually adds support for unwrapping for the user, wouldn't this result in a O(n^2) runtime?

Suppose you had errors E3 (outer-most), E2, and E1 (inner-most). None of which match the predicate function. Then calling:

xerrors.IsFunc(E3, MyFunc)

would result in a call tree like:

call xerrors.IsFunc(E3, MyFunc)
├── handle E3
│   └── call MyFunc(E3)
│       ├── handle E3
│       ├── handle E2
│       └── handle E1
├── handle E2
│   └── call MyFunc(E2)
│       ├── handle E2
│       └── handle E1
└── handle E1
    └── call MyFunc(E1)
        └── handle E1

The problem is that both IsFunc and the predicate function walks the entire chain. IsFunc doesn't know if the provided predicate function will already walk the chain, and the predicate function doesn't know if it is being called by code that is already walking the chain. As such, both functions duplicate the work and walk the chain.

For sufficiently long error-chains, this can be kinda costly.

@neild
Copy link
Contributor Author

neild commented Feb 5, 2019

If a predicate function eventually adds support for unwrapping for the user, wouldn't this result in a O(n^2) runtime?

O(n log n), I think?

But yes, adding errors.IsFunc implies that predicate functions should never add support for implicit unwrapping. Fortunately this doesn't require any work, since none of them unwrap today.

@JavierZunzunegui
Copy link

I also played with a version that returns the error that matches the predicate or nil, but

I use a version that returns an error

  • it's almost never needed. In situations where this comes up you mostly just care whether there is an error that matches the predicate.
  • HTTP error type: passing the user visible msg+error code
  • Crossing RPC boundaries: some of my errors implement an interface that allows them to be serialized and be recovered by the RPC client
  • Monitoring: my prometheus reports not just error, but the component which first caused the error
  • Structured logging: I used zap and must collect all such errors when I log.
  • when it's not needed, you have to tack a != nil on the end

same as the universal if err := ...; err != nil {...}, it's the go way.

  • when it is needed, you still need to do whatever assertion/testing again to recover the desired error so at that point it makes more sense to write your own helper that does the tests and returns the correct kind of error

Yes, but the helper might as well use the error-returning version. Also Code generation makes helper writing trivial (if it wasn't already).

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2019
@andybons andybons added this to the Unreleased milestone Feb 6, 2019
@neild
Copy link
Contributor Author

neild commented Feb 14, 2019

After discussing this internally, we've decided not to add errors.IsFunc or the equivalent at this time.

We believe that the cleanest and best API to expose to users is one based on sentinel errors. You should write errors.Is(err, os.ErrIsNotExist), not errors.IsFunc(err, os.IsNotExist). Adding IsFunc would encourage packages to use the latter. We will update the os package to permit the former.

We will not update os.IsNotExist to unwrap errors, for the reasons detailed in the original message above: Having this function behave differently in Go 1.12 and Go 1.13 is too hazardous.

Nothing, of course, prevents third-party packages from adding IsFunc. We can revisit the decision to add it in the future if it turns out to be broadly useful.

@neild neild closed this as completed Feb 14, 2019
@golang golang locked and limited conversation to collaborators Feb 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants