Open ccampo133 opened 2 years ago
By obtaining an *Error
, you can do useful stuff like retrieve the WrappedErrors()
. Also, check the method ErrorOrNil()
, which seems to have been designed exactly to avoid the problem you encountered.
See https://golang.org/doc/faq#nil_error
still, i don’t get the need to return a pointer, just return a struct.
@bfreis if Wait
returned error
instead of *Error
, you could still retrieve an *Error
with a type assertion or using errors.As
. For example:
err := g.Wait().(*multierror.Error)
errs := err.WrappedErrors()
...
or
var e *multierror.Error
if errors.As(g.Wait(), &e) {
// err is a *multierror.Error, and e is set to the error's value
errs := e.WrappedErrors()
...
}
From that FAQ link above (thanks @stephanwesten), the Go maintainers recommend:
It's a good idea for functions that return errors always to use the
error
type in their signature (as we did above) rather than a concrete type such as*MyError
, to help guarantee the error is created correctly. As an example,os.Open
returns anerror
even though, if not nil, it's always of concrete type*os.PathError
.
Seems like that would be a good pattern to follow here as well. It would certainly eliminate this sort of gotcha, which in my opinion, is quite an ugly wart of Go itself.
It's just a bug in the library:
https://github.com/hashicorp/go-multierror/blob/master/group.go#L33
func (g *Group) Wait() *Error {
The signature should end with error
instead. Send a small patch.
Cheers
PR https://github.com/hashicorp/go-multierror/pull/58 addresses this issue by changing to error
instead of *Error
where necessary. Thanks :)
I think an additional issue here is that Append()
will return a non-nil error even when all arguments are nil.
e.g.,
Append(nil, nil) != nil
It should really return
return Append(&Error{}, newErrs...).ErrorOrNil()
edit:
and then Append
still returns an *Error
, and so it will have a non-nil value if assigned to an error
, because the value will have an underlying type.
Append()
really should return a error
instead of *Error
, but I can see that being a big backwards compatibility break
In this case, Group
should probably keep the member variable as a *Error
, and Wait() error
should call g.err.ErrorOrNil()
upon return
PR https://github.com/hashicorp/go-multierror/pull/58 addresses this issue by changing to error instead of *Error where necessary. Thanks :)
Not to sound like a total jerk, but I think it's customary to put a thanks @purpleidea or similar in the commit log when someone gives you the answer for your patch =D
Seems this repository is dead anyway, considering even this tiny PR has sat nearly a year without as much as a glance.
Also, not to be a total jerk @purpleidea, but did you really provide the answer? I mentioned the provided solution in the description of this issue when I opened it ;).
My question - is there a good reason why Group.Wait returns *Error instead of just error, which is more idiomatic? Returning error would eliminate this weirdness seen here.
It's not uncommon to see code like this:
However, there is a subtle bug here. The
main
function will always panic, despite no error being returned by any of the Goroutines managed by the groupg
. This is due to the fact that Go treats interfaces as "fat pointers". See:A "fix" is to add the following nil check to the
Run
method above:This is certainly not intuitive, and relies on type-inference to solve the problem. I've added a test to my fork of this repo to clearly demonstrate the issue:
My question - is there a good reason why
Group.Wait
returns*Error
instead of justerror
, which is more idiomatic? Returningerror
would eliminate this weirdness seen here.