serrors: delete error context merging and code cleanup

[jiceatscion]

There were a number of intertwined issues difficult to characterize in isolation.
This proposes something that has a more consistent behavior and interface, with shims to ease transition.

Most issues stemmed from the withCtx() function which (in rare cases) created hybrid objects that required complicated support code and, in some cases could cause Is() to panic.

Removed the feature, replaced WithCtx() with a shim that doesn't create hybrids. Removed all the complicated code that resulted. The code is now more obvious but not shorter. There are now two subclasses of errors:

• basicError: dedicated to errors of the form string+cause - Constructed by Wrap()
• joinedError: dedicated to errors of the form error+cause - Constructed by Join()
  This allows for Is() and Unwrap() to be different and do the right thing in all cases.

Introduced new constructors with obvious semantics to replace those already marked deprecated.

I still plan on editing the calls to deprecated constructors (and common.ErrMsg) out of everywhere, but in a subsequent PR.

Fixes #4486

[jiceatscion]

This change is 

[oncilla]

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 13 files reviewed, 4 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

a discussion (no related file):
note: only high level comments so far. I need to dig into the logic some more and give a more thorough look.

---

---

pkg/private/serrors/errors.go line 75 at r3 (raw file):

// representation.
func (e basicError) MarshalLogObject(enc zapcore.ObjectEncoder) error {
	enc.AddString("msg", e.msg.Error())

I think here you should still check whether the wrapped error implements the zapcore.ObjectMarshaler interface, and use it if so.

Otherwise, this does not play nicely with JSON encoded logging.

---

pkg/private/serrors/errors.go line 245 at r3 (raw file):

}

// Wrap() is deprecated. Use FromErr() instead. This shim has almost the same result.

Use the syntax described in https://go.dev/wiki/Deprecated
This will mark it as deprecated in documentation, and will add squiggly lines in editors.

---

pkg/private/serrors/errors.go line 252 at r3 (raw file):

}

// WrapStr() is deprecated. Use FromStrWithStack() instead.

Ditto

[jiceatscion]

Reviewable status: 2 of 13 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 75 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

I think here you should still check whether the wrapped error implements the zapcore.ObjectMarshaler interface, and use it if so.

Otherwise, this does not play nicely with JSON encoded logging.

Not calling it was deliberate in that I wanted to treat e.msg as a generic error. Specifically, if it happens to be a basicError, and if it happens to have a stack and a cause, I think those ought to be ignored; the ones that are meaningful are e.cause and e.stack. So that leaves us with e.msg.Error() as the only representation we can get. Ok, we can still get a context in the string. Is that what ruins the json representation? If so, I should probably encode it since we can't know what e.msg.Error() looks like (in cases it doesn't implement objectMarshaler, should we check for it).

---

pkg/private/serrors/errors.go line 245 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Use the syntax described in https://go.dev/wiki/Deprecated
This will mark it as deprecated in documentation, and will add squiggly lines in editors.

Done.

---

pkg/private/serrors/errors.go line 252 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Ditto

Done.

[oncilla]

Reviewable status: 2 of 13 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 75 at r3 (raw file):

Previously, jiceatscion wrote…

Not calling it was deliberate in that I wanted to treat e.msg as a generic error. Specifically, if it happens to be a basicError, and if it happens to have a stack and a cause, I think those ought to be ignored; the ones that are meaningful are e.cause and e.stack. So that leaves us with e.msg.Error() as the only representation we can get. Ok, we can still get a context in the string. Is that what ruins the json representation? If so, I should probably encode it since we can't know what e.msg.Error() looks like (in cases it doesn't implement objectMarshaler, should we check for it).

argh. I think I read the code wrong. I thought his was about cause rather than msg. Ignore me 🙇 :

---

pkg/private/serrors/errors.go line 245 at r3 (raw file):

Previously, jiceatscion wrote…

Done.

Don't see it. Did you push your changes?

---

pkg/private/serrors/errors.go line 252 at r3 (raw file):

Previously, jiceatscion wrote…

Done.

Ditto

[jiceatscion]

Reviewable status: 2 of 13 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 245 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Don't see it. Did you push your changes?

Indeed I hadn't. Done now.

---

pkg/private/serrors/errors.go line 252 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Ditto

Done.

[oncilla]

Reviewable status: 2 of 13 files reviewed, 3 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 173 at r4 (raw file):

	}
	if cause != nil {
		// In the odd case where err already has a causes, the new one takes precedence.

nit: I think that branch is unnecessary. You can always assign cause in the variable initialization.

My only guess for having this if condition is that you want to protect against typed nil, but that does not work:
https://go.dev/play/p/SYObtbU3eyk

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

// 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 {

This PR introduced 4 error constructors:

• func FromErr
• func FromMsgWithStack
• func FromStr
• func FromStrWithStack

And deprecates the following:

• Wrap
• WrapStr
• WithCtx

I have two main concerns:

1. IMO it becomes even harder to understand what should be the go to mechanism for wrapping errors. In 99% of the cases, we want to go with WrapStr/FromStrWithStack.

2. The common case becomes unreadable -> our code will be messy to read as we have serrors.FromStrWithStack ~everywhere now.

I think we should take a step back. What is it that we really want to achieve?

1. We want a way to easily wrap errors and attach additional context. The error wrapping and additional context should play nicely with JSON encoding.

2. We want a way to use sentinel errors in response to other errors such that client code can change their behavior. (Here, in most cases we do not even care about additional context, most call sites do not add context)

I think we get ourselves in a bit of a mess because we try to stick both of these goals in the same type.

What if we do the following:

type basicError struct {
    msg string
    cause error
    stack *stack
}

func New(msg string, errCtx ...any) error {}

// Replaces WrapStr
func Wrap(msg string, cause error, errCtx ...any) error {}

type sentinelError struct {
    sentinel error
    cause error
    stack *stack
    // potentially add ctx?
}

// Replaces Wrap
func Sentinel(sentinel error, cause error) error {}

This simplifies the package API to 3 functions:

• New
• Wrap
• Sentinel

it should also cover all the use cases that we have.
WithCtx can be covered by combining Sentinel and Wrap together AFAICT.

[jiceatscion]

Reviewable status: 2 of 13 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 173 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: I think that branch is unnecessary. You can always assign cause in the variable initialization.

My only guess for having this if condition is that you want to protect against typed nil, but that does not work:
https://go.dev/play/p/SYObtbU3eyk

You're right; that's a left-over from a more complicated variant. I'll just assign it unconditionally.

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

This PR introduced 4 error constructors:

• func FromErr
• func FromMsgWithStack
• func FromStr
• func FromStrWithStack

And deprecates the following:

• Wrap
• WrapStr
• WithCtx

I have two main concerns:

1. IMO it becomes even harder to understand what should be the go to mechanism for wrapping errors. In 99% of the cases, we want to go with WrapStr/FromStrWithStack.

2. The common case becomes unreadable -> our code will be messy to read as we have serrors.FromStrWithStack ~everywhere now.

I think we should take a step back. What is it that we really want to achieve?

1. We want a way to easily wrap errors and attach additional context. The error wrapping and additional context should play nicely with JSON encoding.

2. We want a way to use sentinel errors in response to other errors such that client code can change their behavior. (Here, in most cases we do not even care about additional context, most call sites do not add context)

I think we get ourselves in a bit of a mess because we try to stick both of these goals in the same type.

What if we do the following:

type basicError struct {
    msg string
    cause error
    stack *stack
}

func New(msg string, errCtx ...any) error {}

// Replaces WrapStr
func Wrap(msg string, cause error, errCtx ...any) error {}

type sentinelError struct {
    sentinel error
    cause error
    stack *stack
    // potentially add ctx?
}

// Replaces Wrap
func Sentinel(sentinel error, cause error) error {}

This simplifies the package API to 3 functions:

• New
• Wrap
• Sentinel

it should also cover all the use cases that we have.
WithCtx can be covered by combining Sentinel and Wrap together AFAICT.

I don't agreee with this proposal: Sentinel() is confusing. It gives the impression the we're building a sentinel error while we're building FROM a sentinel error. Likewise for the name of the struct, of course. Besides, we really do not need two different structures to construct errors wrapped around sentinel errors. We can obtain the same result by requiring sentinel errors to be of some explicit type, or just not care about the underlying type of the msg field besides it implementing error. What's wrong about that? Costs next to nothing and gives us freedom to refactor the using code incrementally. So, I;m afraid I don't really understand why you want a Sentinel error type. How would its behavior compare with that of basicError?

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, jiceatscion wrote…

I don't agreee with this proposal: Sentinel() is confusing. It gives the impression the we're building a sentinel error while we're building FROM a sentinel error. Likewise for the name of the struct, of course. Besides, we really do not need two different structures to construct errors wrapped around sentinel errors. We can obtain the same result by requiring sentinel errors to be of some explicit type, or just not care about the underlying type of the msg field besides it implementing error. What's wrong about that? Costs next to nothing and gives us freedom to refactor the using code incrementally. So, I;m afraid I don't really understand why you want a Sentinel error type. How would its behavior compare with that of basicError?

Done as per our oob discussion. There are more than 3 new constructors because I insist on enabling the creation of stack-dump-free errors, but no-one needs to be aware of them. The short names are for the stack-full ones. It isn't possible to be rid of Is(). Our error objects aren't comparable. We'd need to replace the context map by a pointer-to-same, and even then, I'm not 100% sure that the default comparison would do what we want (but I can test it in case we want that badly).

[oncilla]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Our error objects aren't comparable.

I fail to see why they would need to be comparable.
As we have discussed OOB, our errors are solely there to wrap or join.
They should not be used for constructing sentinel errors.

[oncilla]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Our error objects aren't comparable.

I fail to see why they would need to be comparable.
As we have discussed OOB, our errors are solely there to wrap or join.
They should not be used for constructing sentinel errors.

In other words:

s1 := errors.New("s1")
s2 := errors.New("s2")

wrapped := serrors.Wrap("wrapped", s1)
joined := serrors.Join(s1, s2)

errors.Is(wrapped, s1) // true
errors.Is(wrapped, s2) // false
errors.Is(wrapped, wrapped) // can be false, because we do not care :shrug:

erors.Is(joined, s1) // true
erors.Is(joined, s2) // true
erors.Is(joined, joined) // can be false, because we do not care :shrug:

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

In other words:

s1 := errors.New("s1")
s2 := errors.New("s2")

wrapped := serrors.Wrap("wrapped", s1)
joined := serrors.Join(s1, s2)

errors.Is(wrapped, s1) // true
errors.Is(wrapped, s2) // false
errors.Is(wrapped, wrapped) // can be false, because we do not care :shrug:

erors.Is(joined, s1) // true
erors.Is(joined, s2) // true
erors.Is(joined, joined) // can be false, because we do not care :shrug:

You might think that our entire code base should be a certain way but that doesn't mean it is. For now we rely on Is() behaving the way it does and I did not sign up to refactor everything so it no longer matters. Not in this PR. Also, I would be careful when stating that something shouldn't matter based on a current usage pattern. It is too easy to create opportunities for inconsistencies and really absurd behaviors that are only revealed when someone later uses the API in a new, yet legitimate way. Aaanyway... I did make one quick check. If we force our errors to be comparable by making the map a pointer, then, error behaves as we want even without the Is() methods. If it matters very much to you that we do not implement Is(), then it can be had. That saves ~30 LoCs.

[oncilla]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, jiceatscion wrote…

You might think that our entire code base should be a certain way but that doesn't mean it is. For now we rely on Is() behaving the way it does and I did not sign up to refactor everything so it no longer matters. Not in this PR. Also, I would be careful when stating that something shouldn't matter based on a current usage pattern. It is too easy to create opportunities for inconsistencies and really absurd behaviors that are only revealed when someone later uses the API in a new, yet legitimate way. Aaanyway... I did make one quick check. If we force our errors to be comparable by making the map a pointer, then, error behaves as we want even without the Is() methods. If it matters very much to you that we do not implement Is(), then it can be had. That saves ~30 LoCs.

I do not care about the lines of code.

I care about the code being correct, and it is not obvious to me that the following snippet does the right thing.

func (e joinedError) Is(err error) bool {
	switch other := err.(type) {
	case joinedError:
		// Two joinedError values. They're not comparable, that's why errors.Is() called this.
		// (That's unlikely given how joinedError is used, but entirely feasible).
		if e.error != nil && other.error != nil && !(reflect.TypeOf(e.error).Comparable() &&
			reflect.TypeOf(other.error).Comparable()) {
			return false
		}
		return e.error == other.error
	}
	// Otherwise only possible equality is if e.cause.Is(err) or e.error.Is(err). Those'll be tested
	// after Unwrap().
	return false
}

I would rather not bother and just let errors.Is deal with it.

We can use a map pointer, or we could return pointer instead of value types in our New,Wrap,Join.

FWIW, we can completely drop the map and just have a pointer to a slice instead. Saves a bit of garbage too.

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

I do not care about the lines of code.

I care about the code being correct, and it is not obvious to me that the following snippet does the right thing.

func (e joinedError) Is(err error) bool {
	switch other := err.(type) {
	case joinedError:
		// Two joinedError values. They're not comparable, that's why errors.Is() called this.
		// (That's unlikely given how joinedError is used, but entirely feasible).
		if e.error != nil && other.error != nil && !(reflect.TypeOf(e.error).Comparable() &&
			reflect.TypeOf(other.error).Comparable()) {
			return false
		}
		return e.error == other.error
	}
	// Otherwise only possible equality is if e.cause.Is(err) or e.error.Is(err). Those'll be tested
	// after Unwrap().
	return false
}

I would rather not bother and just let errors.Is deal with it.

We can use a map pointer, or we could return pointer instead of value types in our New,Wrap,Join.

FWIW, we can completely drop the map and just have a pointer to a slice instead. Saves a bit of garbage too.

I just said I can give you a solution where error.Is() is enough to satisfy all the requirements that we test for, it does remove this code that you have doubts about AND relies only on trusted errors.Is(). Is that still not acceptable? Regarding the means, a slice will not be better than a map. They're both non-comparable.

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, jiceatscion wrote…

I just said I can give you a solution where error.Is() is enough to satisfy all the requirements that we test for, it does remove this code that you have doubts about AND relies only on trusted errors.Is(). Is that still not acceptable? Regarding the means, a slice will not be better than a map. They're both non-comparable.

But yeah, ptr or not, a slice would be enough. That seems to be completely orthogonal to this PR's focus, though, but if you'll review it, I'll do it.

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, jiceatscion wrote…

But yeah, ptr or not, a slice would be enough. That seems to be completely orthogonal to this PR's focus, though, but if you'll review it, I'll do it.

And here it is. I think it is cleaner and simpler than the map. ctxPairs is what we'd turn it into for encodeContext anyway. Might as well skip the map. Is that a keeper?

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

note: only high level comments so far. I need to dig into the logic some more and give a more thorough look.

ack

---

[jiceatscion]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

Previously, jiceatscion wrote…

And here it is. I think it is cleaner and simpler than the map. ctxPairs is what we'd turn it into for encodeContext anyway. Might as well skip the map. Is that a keeper?

done

[oncilla]

Reviewed 2 of 5 files at r1, 6 of 9 files at r2, 1 of 3 files at r3, 4 of 5 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 198 at r4 (raw file):

I just said I can give you a solution where error.Is() is enough to satisfy all the requirements that we test for, it does remove this code that you have doubts about AND relies only on trusted errors.Is(). Is that still not acceptable?

It is acceptable, I did not mean to imply it wasn't. It is actually my preferred way. Sorry, it looks like I did not phrase it clearly enough.

But yeah, ptr or not, a slice would be enough. That seems to be completely orthogonal to this PR's focus, though, but if you'll review it, I'll do it.

Orthogonal, yes. Hence the FWIW. But again, it is the preferable option, especially since we are touching all of this code already anyway.

Is that a keeper?

Indeed.

---

pkg/private/serrors/errors.go line 79 at r8 (raw file):

	}
	for _, pair := range *e.ctx {
		if err := enc.AddReflected(pair.Key, pair.Value); err != nil {

Reminder: include the changes from #4594 here too after rebase.

---

pkg/private/serrors/errors.go line 134 at r8 (raw file):

	// 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) {

I think you need to check both basic error or joined error here.
Otherwise we have two stack traces if we wrap a joined error.

---

pkg/private/serrors/errors.go line 257 at r8 (raw file):

}

// Deprecated: WithCtx should never have existed. Depending on intent, use:

nit: (Feel free to ignore)

To make this look nice in pkg.go.dev, you can use go doc supported formatting: https://tip.golang.org/doc/comment#lists

[jiceatscion]

Reviewable status: 13 of 14 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 79 at r8 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Reminder: include the changes from #4594 here too after rebase.

I wasn't going to miss that one. It conflicted nicely with the map->slice change :-)

---

pkg/private/serrors/errors.go line 134 at r8 (raw file):

Previously, oncilla (Dominik Roos) wrote…

I think you need to check both basic error or joined error here.
Otherwise we have two stack traces if we wrap a joined error.

Yup, I think you're right. I'm considering adding a "stacktracer" implemented by errorInfo, but may be that's overkill.

---

pkg/private/serrors/errors.go line 257 at r8 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: (Feel free to ignore)

To make this look nice in pkg.go.dev, you can use go doc supported formatting: https://tip.golang.org/doc/comment#lists

Done

[oncilla]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 134 at r8 (raw file):

Previously, jiceatscion wrote…

Yup, I think you're right. I'm considering adding a "stacktracer" implemented by errorInfo, but may be that's overkill.

I think it is fine as is for now.

[oncilla]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

[jiceatscion]

Reviewed 2 of 5 files at r1, 6 of 9 files at r2, 1 of 3 files at r3, 4 of 5 files at r7, 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

[jiceatscion]

Took the liberty of bypassing review for the last commit: reformating a comment due to long line.