search

go-errors / errors

errors with stacktraces for go
https://godoc.org/github.com/go-errors/errors
MIT License
921 stars 93 forks source link

Is() is not safe as replacement to errors.Is #25

Closed titouanfreville closed 4 years ago

titouanfreville commented 4 years ago

Hello, While working around go errors, I had an issue with the Is function. It does not use the default errors.Is and only compare for strict equality between base errors witch is the default comportment of original Is but not its full implementation. Basically, we are using a custom error type witch reimplement Is and I expected it to be called on goerrors.Is but it is not the cased. It also mean that current goerrors.Is will not have expected behaviour on wrapped errors.

// // Is unwraps its first argument sequentially looking for an error that matches the // second. It reports whether it finds a match. It should be used in preference to // simple equality checks: // // if errors.Is(err, os.ErrExist) // // is preferable to // // if err == os.ErrExist // // because the former will succeed if err wraps os.ErrExist. (errors/errors.go)

ConradIrwin commented 4 years ago

Thanks for the report!

This module was written before the new go error changes, but it seems like a good idea to integrate with the decisions they chose.

It's not technically backward compatible to start using .Is() instead of ==, but I think it's what most users of this library would expect, so it's a change I'm in favor of making.

Would you like to send a pull request to fix this?

P.S. Out of curiosity, what leads to you still using this library in go > 1.14? I haven't thought hard about it, and at @superhuman we diverged from this library a while ago, so I'm curious if this is still useful now that the standard library has better wrapping/unwrapping and stack-trace support.

titouanfreville commented 4 years ago

Hello, I just opened a PR to fix it. :)

We did not found the support for stack-trace under standard library so we switched to this lib. I did not found any indication on standard library being able to log the caller trace without a panic and we use those stacks in all errors wright now. Though it does have an impact on perf, it still helps a lot :)

Though if standard library know allow stack-trace, i'll take a new look on it.

ConradIrwin commented 4 years ago

Ah, it looks like they didn't ship that part of the proposal, my mistake! https://github.com/golang/go/issues/29934#issuecomment-489682919 (it would be great if they did :D — https://go.googlesource.com/proposal/+/master/design/29934-error-values.md#formatting)

titouanfreville commented 4 years ago

Interesting. I still found this in official documentation today: https://godoc.org/github.com/pkg/errors#hdr-Retrieving_the_stack_trace_of_an_error_or_wrapper Will have a look

ConradIrwin commented 4 years ago

FWIW That's the sneakily named github.com/pkg/errors, which is not the same as the builtin errors module.

titouanfreville commented 4 years ago

👍 I'll stay with go-errors for now then :)