Design flaw: Error is a struct, but error is an interface

[xeger]

Because Error is a struct, your library heavily encourages passing *Error around. However, a nil *Error can be assigned to an error and the interface value will test as non-nil, leading to "no error" being false detected as an error condition, when a *Error has thoughtlessly been coerced to an error.

Making this problem worse is the fact that most golang libraries still use the built-in error interface, and in a given block, I often call a mixture of other peoples' libraries and my own code (that is using Error extensively). Because of the err := convention that pervades Go, if I'm not very careful, I can accidentally change the type identity of an *Error variable to error by := adding code.

See the example code below for an illustration of the issue. In older versions of go-errors this panics half the time because I try to call .Error() on a nil *Error, and in newer versions it prints <nil> half the time -- which is possibly even more confusing.

I know that none of this is your fault, but it seems to me that these issues all stem from Error being a struct; if it were an interface instead, this whole class of bugs would be eliminated.

Thoughts?

package main

import "fmt"
import "math/rand"
import "strconv"
import "time"
import "github.com/go-errors/errors"

func internal() *errors.Error {
    if rand.Intn(2) > 0 {
      return errors.New("That's the way the cookie crumbles")
    } else {
      return nil
    }
}

func main() {
    rand.Seed(time.Now().UnixNano())

    // Image this code was written on Wednesday
    i, err := strconv.Atoi("10")
    if err != nil {
    fmt.Println("Oh noes", err.Error())
    return
    }

    // Imagine this code was written on Monday
    err = internal()
    if err != nil {
    fmt.Println("Oh noes", err.Error())
    return
    }

    fmt.Println("i =", i)
}

[ghost]

Ah, looks like someone already got a head start on this change :) https://github.com/go-errors/errors/compare/master...belua:master

[ConradIrwin]

Hey @jim-slattery-rs, I think the comment here: https://github.com/go-errors/errors/pull/4#issuecomment-138383457 is still accurate. You should continue returning error interfaces, even though you know the concrete type.

Just because you know what concrete type the error might be, that doesn't mean you should embed the assumption in your code. As "effective go" says, you should always return the error interface, not a concrete sub-class (all the functions in net return well-known error types, but they all are documented as being error); your logging framework can then deal with type-casting back if you need any of the extra feature from *errors.Error.

[ghost]

Thanks, I'm glad to read about @adrienkohlbecker's experience in that discussion https://github.com/go-errors/errors/pull/4#issuecomment-138384579.

I think many of us are aware of the Effective Go suggestion to return a plain error but decide anyway to return a more specific error-compatible type. I actually haven't seen any downside besides the issue with nil pointers. And even that problem should go away once an interface type is used.

[xeger]

If I understand @ConradIrwin's advice, the intended usage of this package is:

1. one's own APIs should generally return error
2. error-handling sites can use errors.New() or perform a type assertion in order to recover details
3. the callees (things that return error) are responsible for never returning a nil *Error

That gets rid of the unintentional type-hijacking I experienced, but still leaves the callees responsible for never returning a nil *Error by accident.

Might it be useful to provide a func Cast(error) error which returns nil if the input is a nil-error or a nil-pointer, and otherwise returns a *Error produced using Wrap()? It would make the pattern easier to conform to, especially for funcs that do "pass-through" error handling.