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.Append
and uselib/errors.Combine
instead. 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
CombineErrors
to allow them to introspect on the underlying error viaIs
,Unwrap
,As
and 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
CombineErrors
is likeAppend
but 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 deceptivelyCombineErrors
seems to be the solution when it actually probably isn't -
A test reveals that
CombineErrors
doesn't show the second error in.Error()
,%s
, and possibly more so the issue raised aboutgo-multierror
actually 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
Unwrap
doesn'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