Error() should handle nils gracefully

[hashicorp-cla]

All committers have signed the CLA.

[mitchellh]

Hey! This looks reasonable to me. A couple comments:

• Do you mind adding a test?
• Do you think empty string is correct or should this return something like "" ?

[soniah]

Sure, I'll add a test tomorrow.

Do you think empty string is correct or should this return something like "" ?

I don't know what you mean here - the zero value of a string is ""

[mitchellh]

I mean to ask that if you call Error on a nil *Error is the expected value an empty string or should we be more clear that it is a nil error. I'm unclear in what scenario you're experiencing a nil error that you're calling Error on and I'm just curious what would be better since in many cases (of course not always) its indicative of a bug.

[soniah]

Ok, I understand what you mean. If you look at the doco for the error interface, nil represents no error:

// The error built-in interface type is the conventional interface for
// representing an error condition, with the nil value representing no error.
type error interface {
    Error() string
}

If you look at a use case where I got the bug (in some code I inherited), you can see that having "no error" isn't a problem, and you wouldn't expect anything but "" to be returned by Error():

var merr *multierror.Error
for err := range errc {
    merr = multierror.Append(merr, err)
}
return merr

(errc is a channel of errors from an ETL pipeline, and normally there are no errors in the channel).

My immediate fix was to return merr.ErrorOrNil() (as per your README). But the bigger picture is that you're trying to implement the Maybe Monad (similar to Kotlins Safe Call "Elvis" Operator ?.), but that's unexpected behaviour for Go.

Soon Go will have the try built-in, and all Go error handling will be happiness and light 🌞 🤣

---

Also, as requested, I've added a test for this.

[mitchellh]

Ah, thanks for describing the use case. This is a very common use case we follow at HashiCorp as well.

We're not trying to implement the Maybe monad at all. What we're doing is working around Go's behavior of nil-ness and errors. See the Go FAQ here: https://golang.org/doc/faq#nil_error

The issue (you might super be aware of this I'm just describing it to be extra clear) is that (error)(nil) != (*Error)(nil) so as you said a nil error does indeed mean "no error" but a nil *Error does not in Go's type system, because a nil *Error is actually NOT nil.

See this program as an example: https://play.golang.org/p/wsLo5t5i_2v

As builders of the library we had to either return error directly or add the ErrorOrNil function so we can get a nil error interface type. The reason we did the latter is because by returning Error you can call functions on it (cause its typed). If we returned error then users would have to type switch it every time.

We've had issues and talked about adding another Append function that returns error and call it like AppendInterface or something. But that itself is pretty weird and non-idiomatic and likely very confusing to new users (to this library or Go).

So when we do this pattern at HashiCorp we do something like this:

var merr error
for err := range errc {
    merr = multierror.Append(merr, err).ErrorOrNil()
}

return merr

---

I like to ask for use case cause it helps describe the goal of a suggestion in issue or PR. In this case, I think gracefully handling a nil *Error seems useful but every time this suggestion has come up in the past its masking other issues. In this case, I think that's still the case so I'm inclined to not merge this PR. For your intended use case I think you want to call ErrorOrNil

[soniah]

OK, thanks!