lib/errors: unify multi-errors to fix printing, update error dependencies
Created by: bobheadxi
The issue
Someone should formulate a plan to deprecate use of
lib/errors.Appendand uselib/errors.Combineinstead. The issue here is that they’re not semantically equivalent: the former uses a bag of errors and the later has a primary and secondary. I think we need to tag each of the former with a banner error and use the bag as the secondary error. This might take some amount of manual vetting, there’s a number of references that exist.
Seemed simple enough...
Tangent
I initially started with a spike to explore removing go-multierror entirely. This revealed a few things:
- If anyone expects cockroach's
CombineErrorsto allow them to introspect on the underlying error viaIs,Unwrap,Asand friends, well you can't! https://github.com/cockroachdb/errors/issues/62 The underlying type,SecondaryError, completely obscures the second error, except in certain printing situations.- This means that my (and maybe others') assumption that
CombineErrorsis likeAppendbut for 2 errors instead of vararg errors is not true, the two are fundamentally different - You can use
Wrap, but it doesn't allow you to wrap error A within error B, so deceptivelyCombineErrorsseems to be the solution when it actually probably isn't -
A test reveals that
CombineErrorsdoesn't show the second error in.Error(),%s, and possibly more so the issue raised aboutgo-multierroractually applies to all our methods of error combination
- This means that my (and maybe others') assumption that
- I attempted to rewrite our own multi error type, but it turns out
Unwrapdoesn't really work the way I think it does with Cockroach errors. For example:
reference := New("foo")
unwrapped := Unwrap(err)
assert.NotNil(t, err) // okay, so there's an underlying error to a raw "New" error...?
assert.True(t, Is(unwrapped, reference)) // fails! unwrapped error no longer asserts against a sentinel reference error
assert.True(t, Is(reference, unwrapped)) // well this works but semantically makes no sense because the second argument is supposed to be the reference
New comes with a stack layer that gets unwrapped on Unwrap, which makes using New errors as sentinel errors dangerous - if you unwrap an error too far the sentinel error just doesn't work anymore! So we can't depend on the behaviour of Unwrap to build a lightweight multierror layer on top of cockroach errors, and now I'm also unsure if I can depend on Unwrap to do anything
edit so it turns out I was doing it wrong, see https://github.com/sourcegraph/sourcegraph/pull/31466
Attempted solution
(this PR)
Attempts to bandaid-resolve the issue raised today at backend-crew by patching go-multierror to fix the main issue of "printing doesn't show all errors in multierrors". See: https://github.com/hashicorp/go-multierror/compare/master...sourcegraph:master
Migrates CombineErrors to proxy to go-multierror instead (see below)
Removes unnecessary ErrorListFunc re-export
The better solution?
https://github.com/sourcegraph/sourcegraph/pull/31466
Test plan
Unit tests