serrors isn't treating by value or by reference errors consistently

[jiceatscion]

serrors.New returns a pointer. all the wrapper constructors return values.
Is() expects a value. It will fail to recognize a pointer as a basicError. However, if given a value, it will panic for any error that happens to be a wrapper around basicErrors, as it will try to compare these objects which are not comparable.

WithCtx() expects only a value and will fail to downcast a basicError ptr, resulting in always wrapping instead of merging ctx.
A number of test cases have been crafted to expect that un-planned behaviour. For example private/mgmtapi/cppki/api/api_test.go expect a wrapped error while serror() was designed to produce the same error in merged form. (i.e. fixing serrors) will break those tests.

Either we fix serrors and the tests or, may be simpler, we remove that accidentally unused merging mechanism from serrors.

In either case we should decide if we want serrrors to handle values, or references, or both and implement whatever we chose consistently. I suspect we really want references since that's what we always use (otherwise Is() would be panicking).

[oncilla]

However, if given a value, it will panic for any error that happens to be a wrapper around basicErrors, as it will try to compare these objects which are not comparable.

Do you have a minimal reproducer? I think that would be easier to understand.

WithCtx() expects only a value and will fail to downcast a basicError ptr, resulting in always wrapping instead of merging ctx.
A number of test cases have been crafted to expect that un-planned behaviour. For example private/mgmtapi/cppki/api/api_test.go expect a wrapped error while serror() was designed to produce the same error in merged form. (i.e. fixing serrors) will break those tests.

IMO, WithCtx is anyway not all that useful. It could be dropped and replaced with regular error wrapping.

For example private/mgmtapi/cppki/api/api_test.go expect a wrapped error while serror() was designed to produce the same error in merged form. (i.e. fixing serrors) will break those tests.

Link to the offending line would be useful here.

[jiceatscion]

One simple way to see what's happening: modify serrors.New() to return a value. Then run make test.

One other sample of what's broken. This code (from pkg/slayers/path/scion/raw_test.go)

expected := serrors.New("NumHops too large", "NumHops", 65, "Maximum", scion.MaxHops)
err := s.DecodeFromBytes(b)
assert.Equal(t, expected.Error(), err.Error())

Does what's expected, but replacing the last line with:
assert.ErrorIs(t, expected, err)

Doesn't. The errors are deemed different.

[jiceatscion]

After much back-and-forth navigating the constraints. I settled on removing the context merging feature. I took advantage of this to simplify the code a bit and introduce new constructor names that, I think, reflect more honestly what happens. The deprecated functions are still there to avoid mixing this change with mass refactoring. The withCtx() function now always wraps the original error as the cause of a new made-up error. The very few tests that could detect the difference have been adjusted. Our rules for Is(err) treating pointers types differently can still cause some surprises but I cannot prove that we don't need it. In passing, Is() had a flaw. It was possible to make it panic.