chore: Replace errors.As with generic As and AsInterface#63047
Conversation
| require.Panics(t, func() { | ||
| var check payloadLessPtrError | ||
| require.True(t, As(err, &check)) | ||
| }) |
There was a problem hiding this comment.
These panics become type errors, so we can just remove them instead of checking for panics.
camdencheek
left a comment
There was a problem hiding this comment.
Very nice, thank you!
cc @bobheadxi just because I think you probably have the most context around lib/errors
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/cockroachdb/errors" //nolint:depguard // needed for implementation of multiError.As |
There was a problem hiding this comment.
Why do we need the upstream As, instead of your new As?
There was a problem hiding this comment.
We can constrain our usage of errors.As with the new interface, but we shouldn't change the implementation of interface{ As() } to be specific to this library because multierrors created by this library can be passed to code that doesn't use this library. Otherwise, 3rd-party code that calls, e.g., var netErr net.DNSError ; errors.As(err, &netErr) on an error from this library will no longer work correctly.
There was a problem hiding this comment.
we shouldn't change the implementation of interface{ As() } to be specific to this library
Ah, this clicks for me 😁 Could we add a brief docstring to this effect where we use As?
There was a problem hiding this comment.
We could probably also use the stdlib errors.As() here. Cockroach shouldn't be adding any special behavior to that one
There was a problem hiding this comment.
I've tried to make this change semantically a no-op as much as possible. It's potentially a bit riskier to move between libraries (ideally they should have the same semantics but...), we can make that change in a follow-up PR.
There was a problem hiding this comment.
Could we add a brief docstring to this effect where we use As?
Added here: https://github.com/sourcegraph/sourcegraph/pull/63047/commits/bbf7a9f38d86a061b97eb95f4cd5de59f706fe59
Splits the signature of
errors.Asinto two more specialized functionswhich catch more errors at compile-time using generics.
Test plan
Covered by existing tests, added some new tests.
Changelog