-
-
Notifications
You must be signed in to change notification settings - Fork 34
Description
Hi! Thanks for the library — I’m running into inconsistent/unsafe errors.Is behavior around OopsError.Is, and it seems related to the trade-offs discussed in #78 and #87.
Context
OopsError contains non-comparable fields (maps/slices). This means the stdlib errors.Is cannot rely on err == target for identity checks, so it will fall back to OopsError.Is.
Depending on how OopsError.Is is implemented, we run into at least one of these issues:
- Panic when comparing two
OopsErrorvalues via==(seeOopsError.Is()panics when comparing non-comparable types #78). - Non-reflexive behavior if
Isdelegates directly to the wrapped cause only, e.g.return errors.Is(o.err, target)(see errors.Is() behavior with oops errors #87):errors.Is(oopsErr, oopsErr)becomes false. - False positives if
Istreats “target is anOopsError” as a match, or if it compares two wrappers by comparing their underlying causes. This makeserrors.Is(err1, err2)true for two distinctOopsErrorinstances that merely share the same cause, which is surprising and can break branching/testing logic that assumeserrors.Ismatches a sentinel/type (not another runtime wrapper instance).
Reproduction (test)
package oops_test
import (
"errors"
"io"
"testing"
"github.com/samber/oops"
)
func TestErrorsIsSemanticsWithOopsErrorTargets(t *testing.T) {
t.Run("no_panic_when_target_is_non_comparable_oops_error", func(t *testing.T) {
base := oops.New("base")
wrapped := oops.With("k", "v").Wrap(base)
defer func() {
if r := recover(); r != nil {
t.Fatalf("errors.Is panicked: %v", r)
}
}()
_ = errors.Is(wrapped, base)
})
t.Run("oops_error_is_reflexive", func(t *testing.T) {
err := oops.New("boom")
if !errors.Is(err, err) {
t.Fatalf("expected errors.Is(err, err) to be true")
}
})
t.Run("different_oops_errors_should_not_match_each_other_even_if_same_cause", func(t *testing.T) {
err1 := oops.Wrap(io.EOF)
err2 := oops.With("ctx", "x").Wrap(io.EOF)
if errors.Is(err1, err2) || errors.Is(err2, err1) {
t.Fatalf("expected errors.Is to be false between distinct oops errors; got true")
}
})
}Expected
- No panic.
- errors.Is(err, err) is always true (reflexive), including for OopsError.
- Two distinct OopsError instances should not match each other via errors.Is unless they are the same logical instance (identity), or unless users explicitly compare against the underlying sentinel/type (e.g. io.EOF).
Suggestion
When target is OopsError / *OopsError, Is should perform an identity check (not “same type” and not “same cause”). Because OopsError is non-comparable, an internal comparable identifier (e.g. a generated ID created on New/Errorf/Wrap/Wrapf) could be used for identity checks. For non-OopsError targets, Is can continue delegating to errors.Is(o.err, target).