You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
Non-reflexive behavior if Is delegates 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 Is treats “target is an OopsError” as a match, or if it compares two wrappers by comparing their underlying causes. This makes errors.Is(err1, err2) true for two distinct OopsError instances that merely share the same cause, which is surprising and can break branching/testing logic that assumes errors.Is matches a sentinel/type (not another runtime wrapper instance).
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).
Hi! Thanks for the library — I’m running into inconsistent/unsafe
errors.Isbehavior aroundOopsError.Is, and it seems related to the trade-offs discussed in #78 and #87.Context
OopsErrorcontains non-comparable fields (maps/slices). This means the stdliberrors.Iscannot rely onerr == targetfor identity checks, so it will fall back toOopsError.Is.Depending on how
OopsError.Isis implemented, we run into at least one of these issues:OopsErrorvalues via==(seeOopsError.Is()panics when comparing non-comparable types #78).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.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)
Expected
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).