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.