Fix uninitialized error detection

[jeanspector-google]

This should address issue #35

[ConradIrwin]

@dsoprea this is technically a breaking change, though I think the impact is likely to be low. As someone who uses the library a lot I'm curious to hear your opinion on whether we should make a change like this?

[elliottmina]

@ConradIrwin FWIW, this issue through me for a loop. I'm switching a project over from vanilla errors, and got stuck chasing down nil != nil. I hoped to find this exact issue, which of course I did.

It's a little awkward to work around, but not a lot of work. I think the bigger issue is cognitive. The module does not work as I would expect, especially when I crack open the Wrap function and see:

    if e == nil {
        return nil
    }

I would call this a gotcha.

ty for sharing the module!

[ConradIrwin]

Hey! Sorry for the slow reply here!

I may be misunderstanding the use-case here, but I expect people who are using go-errors to wrap errors returned by a function, and in go it is idiomatic to have functions that return an error to return an interface type error (rather than a concrete error type).

If you have a function that returns an interface type error then in order for if err == nil to work, you must return an untyped nil, instead of a typed nil; so I wouldn't expect people to be passing a typed nil to Wrap.

Is there a use-case I'm not thinking about that causes this to break, or is it legacy code that is returning untyped nil (in which case probably the right fix is to update the function that's returning a typed nil error).

[stevenpelley]

Sorry to dig up an old issue but I hit this and would like to further it. There's been more discussion here vs the open issue so I'm choosing to post here. (edit to tag @ConradIrwin. No urgency, just making sure this reaches you)

in go it is idiomatic to have functions that return an error to return an interface type error (rather than a concrete error type). I agree this is the heart of the issue. The problem is that go-errors itself fails to follow this idiom. I cannot pass functions creating errors to Join and expect the correct nil filtering. Example:

err1 := f1() // might be nil
err2 := f2() // might be nil
return errors.Join(
  errors.WrapPrefix(err1, "f1: ", 0),
  errors.WrapPrefix(err2, "f2: ", 0))

I want this to return a nil error if err1 and err2 are both nil. Instead, I get a Join error with a slice of 2 entries, both nil *Error. This is as described in the issue and this pull request. The golang faq also describes this: https://go.dev/doc/faq#nil_error

What I mean to emphasize is: this issue breaks the internal consistency of the go-errors library. You cannot use the various functions with each other and achieve the expected result. If go-errors is going to return *Error instead of error then it aught to check for nil *Error everywhere it currently checks against nil (e.g., Wrap, WrapPrefix) or passes a value to "baseErrors" that filters nil (e.g., Join). This way one gets the desired behavior so long as you stick strictly to go-errors function calls.

[stevenpelley]

I discovered https://github.com/go-errors/errors/pull/33 after the fact and see that changing functions to return error instead of *Error has already been tried and broke things

[scr-oath]

Well… can we make new functions WrapError or the like that return error so that when nil it's not *Error(nil)?