serrors: delete error context merging and code cleanup

pkg/private/common/BUILD.bazel

@@ -8,6 +8,7 @@ go_library(
     ],
     importpath = "github.com/scionproto/scion/pkg/private/common",
     visibility = ["//visibility:public"],
+    deps = ["//pkg/private/serrors:go_default_library"],
 )
 
 go_test(

pkg/private/common/errors.go

@@ -15,10 +15,10 @@
 
 package common
 
+import (
+	"github.com/scionproto/scion/pkg/private/serrors"
+)
+
 // ErrMsg should be used for error string constants. The constant can then be
 // used for Is checking in the calling code.
-type ErrMsg string
-
-func (e ErrMsg) Error() string {
-	return string(e)
-}
+type ErrMsg = serrors.ErrMsg

pkg/private/serrors/errors.go

@@ -27,38 +27,30 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"reflect"
 	"sort"
 	"strings"
 
 	"go.uber.org/zap/zapcore"
 )
 
-type errOrMsg struct {
-	str string
-	err error
-}
+// ErrMsg is a custom error type used instead of strings in many places. It is a good type to use
+// for sentinel errors. There are basicError constructors for both string and error.
+type ErrMsg string
 
-func (m errOrMsg) Error() string {
-	if m.err != nil {
-		return m.err.Error()
-	}
-	return m.str
-}
-
-func (m errOrMsg) addToEncoder(enc zapcore.ObjectEncoder) error {
-	if m.err != nil {
-		if marshaler, ok := m.err.(zapcore.ObjectMarshaler); ok {
-			return enc.AddObject("msg", marshaler)
-		}
-		enc.AddString("msg", m.err.Error())
-		return nil
-	}
-	enc.AddString("msg", m.str)
-	return nil
+// Error implements the Go error interface.
+func (e ErrMsg) Error() string {
+	return string(e)
 }
 
+// basicError is an implementation of error that encapsulates various pieces of information besides
+// a message. The msg field is any kind of error, inluding a basicError. It receives no  particular
+// treatment in that case. Most notably constructing an basicError E, such that E.msg.msg == T does
+// *not* imply that E.Is(T) is true. The intended use of a basicError as msg is to support the
+// extent use of sentinel error created by New(). For that purpose simpler errors such as ErrMsg
+// would be preferable.
 type basicError struct {
-	msg    errOrMsg
+	msg    error
 	fields map[string]interface{}
 	cause  error
 	stack  *stack
@@ -80,9 +72,7 @@ func (e basicError) Error() string {
 // MarshalLogObject implements zapcore.ObjectMarshaler to have a nicer log
 // representation.
 func (e basicError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
-	if err := e.msg.addToEncoder(enc); err != nil {
-		return err
-	}
+	enc.AddString("msg", e.msg.Error())
 	if e.cause != nil {
 		if m, ok := e.cause.(zapcore.ObjectMarshaler); ok {
 			if err := enc.AddObject("cause", m); err != nil {
@@ -108,20 +98,29 @@ func (e basicError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
 func (e basicError) Is(err error) bool {
 	switch other := err.(type) {
 	case basicError:
+		// When error's underlying value isn't a pointer error.Is() calls us because basicError
+		// isn't comparable. This check is loose but about the only real use case is E.Is(E).
+		// We still need to make sure we don't panic if the two msg fields are non-comparable.
+		// (That's unlikely given how basicError is used, but entirely feasible).
+		if e.msg != nil && other.msg != nil && !(reflect.TypeOf(e.msg).Comparable() &&
+			reflect.TypeOf(other.msg).Comparable()) {
+			return false
+		}
 		return e.msg == other.msg
+
+		// No special case if the underlying value is a basicError pointer. For identical pointers,
+		// this method is never called. For different error pointers we want them to be always
+		// different, except for... see below.
+
 	default:
-		if e.msg.err != nil {
-			return e.msg.err == err
-		}
-		return false
+		// If err was created with New(), its underlying value is a pointer, so it matches this
+		// case.
+		return e.msg == err // If true, e is the result of FromMsg(err, ...), so equal.
 	}
 }
 
 func (e basicError) As(as interface{}) bool {
-	if e.msg.err != nil {
-		return errors.As(e.msg.err, as)
-	}
-	return false
+	return errors.As(e.msg, as)
 }
 
 func (e basicError) Unwrap() error {
@@ -159,75 +158,102 @@ func IsTemporary(err error) bool {
 	return errors.As(err, &t) && t.Temporary()
 }
 
-// WithCtx returns an error that is the same as the given error but contains the
-// additional context. The additional context is printed in the Error method.
-// The returned error implements Is and Is(err) returns true.
-// Deprecated: use WrapStr or New instead.
-func WithCtx(err error, errCtx ...interface{}) error {
-	if top, ok := err.(basicError); ok {
-		return basicError{
-			msg:    top.msg,
-			fields: combineFields(top.fields, errCtxToFields(errCtx)),
-			cause:  top.cause,
-			stack:  top.stack,
-		}
-	}
-
-	return basicError{
-		msg:    errOrMsg{err: err},
+// FromErrStackOpt returns an error that associates the given error, with the given cause
+// (an underlying error) unless nil, and the given context. A stack dump is added if requested and
+// cause isn't a basicError. The returned error implements Is. Is(err) returns true. Is(cause)
+// returns true. Any stack dump attached to err (if err is a basicError) is subsequently ignored.
+// The result of err.Error will be part of the result of Error. Most other constructors call
+// this one.
+func FromErrStackOpt(err error, cause error, addStack bool, errCtx ...interface{}) error {
+	r := basicError{
+		msg:    err,
 		fields: errCtxToFields(errCtx),
 	}
-}
-
-// Wrap wraps the cause with the msg error and adds context to the resulting
-// error. The returned error implements Is and Is(msg) and Is(cause) returns
-// true.
-// Deprecated: use WrapStr instead.
-func Wrap(msg, cause error, errCtx ...interface{}) error {
-	return basicError{
-		msg:    errOrMsg{err: msg},
-		cause:  cause,
-		fields: errCtxToFields(errCtx),
+	if cause != nil {
+		// In the odd case where err already has a causes, the new one takes precedence.
+		r.cause = cause
+	}
+	if !addStack {
+		return r
 	}
-}
 
-// WrapStr wraps the cause with an error that has msg in the error message and
-// adds the additional context. The returned error implements Is and Is(cause)
-// returns true.
-func WrapStr(msg string, cause error, errCtx ...interface{}) error {
 	var (
 		existingVal basicError
 		existingPtr *basicError
-		st          *stack
 	)
 
-	// We attach a stacktrace if there is no basic error already.
-	if !errors.As(cause, &existingVal) && !errors.As(cause, &existingPtr) {
-		st = callers()
-	}
-	return basicError{
-		msg:    errOrMsg{str: msg},
-		cause:  cause,
-		fields: errCtxToFields(errCtx),
-		stack:  st,
+	// We attach a stacktrace if there is no basic error cause already. Note that if the innermost
+	// basicError was without a stack trace, then there'll never be one. That's to avoid looking
+	// for it in every level or every constructor. TB revisisted if necessary.
+	if r.cause == nil || !errors.As(cause, &existingVal) && !errors.As(cause, &existingPtr) {
+		r.stack = callers()
 	}
+	return r
+}
+
+// FromMsg returns an error that associates the given error, with the given cause
+// (an underlying error) unless nil, and the given context.
+// The returned error implements Is. Is(err) returns true. Is(cause) returns true if cause is not
+// nil.
+func FromErr(err, cause error, errCtx ...interface{}) error {
+	return FromErrStackOpt(err, cause, false, errCtx...)
+}
+
+// FromMsgWithStack returns an error that associates the given error, with the given cause
+// (an underlying error) unless nil, and the given context. A stack dump is added if cause isn't
+// a basicError. The returned error implements Is. Is(err) returns true. Is(cause) returns true.
+func FromMsgWithStack(err, cause error, errCtx ...interface{}) error {
+	return FromErrStackOpt(err, cause, true, errCtx...)
+}
+
+// FromStr returns an error that associates the given message, with the given cause
+// (an underlying error) unless nil, and the given context.
+// The returned error implements Is and Is(cause) returns true.
+func FromStr(msg string, cause error, errCtx ...interface{}) error {
+	return FromErrStackOpt(ErrMsg(msg), cause, false, errCtx...)
 }
 
-// New creates a new error with the given message and context.
+// FromStrWithStack returns an error that associates the given message, with the given cause
+// (an underlying error) unless nil, and the given context. A stack dump is added if cause isn't a
+// basicError. The returned error implements Is and Is(cause) returns true.
+func FromStrWithStack(msg string, cause error, errCtx ...interface{}) error {
+	return FromErrStackOpt(ErrMsg(msg), cause, true, errCtx...)
+}
+
+// New creates a new error with the given message and context, with a stack dump.
+// It is equivalent to FromMsgWithStack() but returns by reference as is expected of "New()".
+// Avoid using this in performance-critical code: it is the most expensive variant. If used to
+// construct other errors, such as with FromErr(), the embedded stack trace and context serve no
+// purpose. Therefore to make sentinel errors, ErrMsg should be preferred.
 func New(msg string, errCtx ...interface{}) error {
-	if len(errCtx) == 0 {
-		return &basicError{
-			msg:   errOrMsg{str: msg},
-			stack: callers(),
-		}
-	}
 	return &basicError{
-		msg:    errOrMsg{str: msg},
+		msg:    ErrMsg(msg),
 		fields: errCtxToFields(errCtx),
 		stack:  callers(),
 	}
 }
 
+// Deprecated: WithCtx should never have existed. Use FromErr or FromStr to create
+// a new error with the original as the cause. This shim does it for you for the time being.
+// WithCtx used to attempt the merger of the given error into the newly created one with
+// semantically incorrect results. That feature is gone and the results differ only slightly in the
+// formated string output. WithCtx still doesn't add a stack.
+func WithCtx(err error, errCtx ...interface{}) error {
+	return FromErrStackOpt(ErrMsg("error"), err, false, errCtx...)
+}
+
+// Deprecated: Wrap has been renamed FromErr. FromErr and the historical do differ very slightly:
+// any stack dump that might have be attached to err is ignored when logging. Like before, no stack
+// dump is added to the returned error.
+func Wrap(err, cause error, errCtx ...interface{}) error {
+	return FromErrStackOpt(err, cause, false, errCtx...)
+}
+
+// Deprecated: WrapStr has been renamed FromStrWithStack.
+func WrapStr(msg string, cause error, errCtx ...interface{}) error {
+	return FromErrStackOpt(ErrMsg(msg), cause, true, errCtx...)
+}
+
 // List is a slice of errors.
 type List []error
 
@@ -296,17 +322,6 @@ func errCtxToFields(errCtx []interface{}) map[string]interface{} {
 	return fields
 }
 
-func combineFields(a, b map[string]interface{}) map[string]interface{} {
-	fields := make(map[string]interface{}, len(a)+len(b))
-	for k, v := range a {
-		fields[k] = v
-	}
-	for k, v := range b {
-		fields[k] = v
-	}
-	return fields
-}
-
 type ctxPair struct {
 	Key   string
 	Value interface{}

pkg/private/serrors/errors_test.go

@@ -219,6 +219,9 @@ func TestEncoding(t *testing.T) {
 			goldenFileBase: "testdata/wrapped-error",
 		},
 		"error with context": {
+			// WithCtx is deprecated. The shim does it the new way: sets err as the cause of a new
+			// error. The string output isn't exactly the same. Which almost nothing ever notices...
+			// except this test.
 			err:            serrors.WithCtx(serrors.New("simple err"), "someCtx", "someValue"),
 			goldenFileBase: "testdata/error-with-context",
 		},
@@ -353,13 +356,15 @@ func ExampleNew() {
 }
 
 func ExampleWithCtx() {
+	// It is not possible to augment the context of a basicError.
+	// WithCtx turns that into an error with a cause.
 	// ErrBadL4 is an error defined at package scope.
 	var ErrBadL4 = serrors.New("Unsupported L4 protocol")
 	addedCtx := serrors.WithCtx(ErrBadL4, "type", "SCTP")
 
 	fmt.Println(addedCtx)
 	// Output:
-	// Unsupported L4 protocol {type=SCTP}
+	// error {type=SCTP}: Unsupported L4 protocol
 }
 
 func ExampleWrapStr() {
@@ -411,3 +416,15 @@ func sanitizeLog(log []byte) []byte {
 	}
 	return log
 }
+
+func TestUncomparable(t *testing.T) {
+	t.Run("Is", func(t *testing.T) {
+		// We make two wrappers of uncomparable error objects. We could also create custom error
+		// types for the same result, but this is closer to our use cases.
+		errObject := serrors.Wrap(serrors.New("simple err"), nil, "dummy", "context")
+		wrapperA := serrors.Wrap(errObject, nil, "dummy", "context")
+		wrapperB := serrors.Wrap(errObject, nil, "dummy", "context")
+		assert.NotErrorIs(t, wrapperA, wrapperB)
+		// no panic
+	})
+}

pkg/private/serrors/testdata/error-list.log

@@ -1 +1 @@
-{"err":[{"ctx1":"val1","msg":"test err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:227","testing.tRunner gosdk"]},{"msg":"test err2","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:228","testing.tRunner gosdk"]}],"level":"info","msg":"Failed to do thing"}
+{"err":[{"ctx1":"val1","msg":"test err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:230","testing.tRunner gosdk"]},{"msg":"test err2","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:231","testing.tRunner gosdk"]}],"level":"info","msg":"Failed to do thing"}

pkg/private/serrors/testdata/error-with-context.err

@@ -1 +1 @@
-simple err {someCtx=someValue}
+error {someCtx=someValue}: simple err

pkg/private/serrors/testdata/error-with-context.log

@@ -1 +1 @@
-{"err":{"msg":{"msg":"simple err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:222","testing.tRunner gosdk"]},"someCtx":"someValue"},"level":"info","msg":"Failed to do thing"}
+{"err":{"cause":{"msg":"simple err","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:225","testing.tRunner gosdk"]},"msg":"error","someCtx":"someValue"},"level":"info","msg":"Failed to do thing"}

pkg/private/serrors/testdata/goroutine.log

@@ -1 +1 @@
-{"err":{"msg":"msg","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding.func2.1 pkg/private/serrors/errors_test.go:236"]},"level":"info","msg":"Failed to do thing"}
+{"err":{"msg":"msg","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding.func2.1 pkg/private/serrors/errors_test.go:239"]},"level":"info","msg":"Failed to do thing"}

pkg/private/serrors/testdata/wrapped-error.log

@@ -1 +1 @@
-{"err":{"cause":{"msg":"msg cause","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:215","testing.tRunner gosdk"]},"k0":"v0","k1":1,"msg":{"msg":"msg error","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:214","testing.tRunner gosdk"]}},"level":"info","msg":"Failed to do thing"}
+{"err":{"cause":{"msg":"msg cause","stacktrace":["pkg/private/serrors/go_default_test_test.TestEncoding pkg/private/serrors/errors_test.go:215","testing.tRunner gosdk"]},"k0":"v0","k1":1,"msg":"msg error"},"level":"info","msg":"Failed to do thing"}

private/mgmtapi/cppki/api/testdata/certificates-malformed.json

@@ -1,5 +1,5 @@
 {
-    "detail": "[ invalid ISD-AS {value=garbage} {parameter=isd_as} ]",
+    "detail": "[ error {parameter=isd_as}: invalid ISD-AS {value=garbage} ]",
     "status": 400,
     "title": "malformed query parameters",
     "type": "/problems/bad-request"

private/mgmtapi/segments/api/api.go

@@ -50,14 +50,14 @@ func (s *Server) GetSegments(w http.ResponseWriter, r *http.Request, params GetS
 		if ia, err := addr.ParseIA(*params.StartIsdAs); err == nil {
 			q.StartsAt = []addr.IA{ia}
 		} else {
-			errs = append(errs, serrors.WithCtx(err, "parameter", "start_isd_as"))
+			errs = append(errs, serrors.WrapStr("invalid start ISD_AS", err))
 		}
 	}
 	if params.EndIsdAs != nil {
 		if ia, err := addr.ParseIA(*params.EndIsdAs); err == nil {
 			q.EndsAt = []addr.IA{ia}
 		} else {
-			errs = append(errs, serrors.WithCtx(err, "parameter", "end_isd_as"))
+			errs = append(errs, serrors.WrapStr("invalid end ISD_AS", err))
 		}
 	}
 	if err := errs.ToError(); err != nil {

private/mgmtapi/segments/api/testdata/segments-malformed-query.json

@@ -1,5 +1,5 @@
 {
-    "detail": "[ parsing AS part {index=0; parameter=start_isd_as; value=ff001:0:110}: strconv.ParseUint: parsing \"ff001\": value out of range; parsing AS part {index=0; parameter=end_isd_as; value=ff000:0:112}: strconv.ParseUint: parsing \"ff000\": value out of range ]",
+    "detail": "[ invalid start ISD_AS: parsing AS part {index=0; value=ff001:0:110}: strconv.ParseUint: parsing \"ff001\": value out of range; invalid end ISD_AS: parsing AS part {index=0; value=ff000:0:112}: strconv.ParseUint: parsing \"ff000\": value out of range ]",
     "status": 400,
     "title": "malformed query parameters",
     "type": "/problems/bad-request"