Wrap() doc does not indicate that *Error argument is returned unchanged

[stevenpelley]

Wrap() does one of four things depending on its input "e":

1. nil -> returns nil
2. *Error -> returns it
3. error -> wraps in *Error with new stack trace
4. default -> wraps fmt.Errorf("%v", e) in *Error with new stack trace

(3) and (4) are clearly stated in the doc. I think it's fair to say that (1) is implicit.

(2) is not documented, and this has implications for Wrap's use. In many cases I want it to do (2), but in some cases I want to additionally wrap with a new stack trace:

• When propagating an error where I want to ensure it has a stack trace (e.g., the incoming error was returned from a library) I will call wrap. If the incoming error happens to already be *Error (perhaps the library itself uses go-errors) it will just be returned. Great.
• When an error passes between goroutines I want to additionally Wrap it to produce a "return trace" spanning all goroutines. As an error is taken from a channel it will be wrapped. Using Wrap() does not produce the desired error. I believe errors.Errorf does provide this.

All of this applies to WrapPrefix() as well.

I recommend changing the doc from:

Wrap makes an Error from the given value. If that value is already an error then it will be used directly, if not, it will be passed to fmt.Errorf("%v"). The skip parameter indicates how far up the stack to start the stacktrace. 0 is from the current call, 1 from its caller, etc.

to:

Wrap makes an Error from the given value. If that value is already an error then it will be used directly, if not, it will be passed to fmt.Errorf("%v"). If that value is already an Error it will not be wrapped and instead will be directly returned. To explicitly wrap an Error use Errorf. The skip parameter indicates how far up the stack to start the stacktrace. 0 is from the current call, 1 from its caller, etc.

I'm happy to write the PR but I wanted to discuss first.

[ConradIrwin]

That makes sense! I also saw a recent article on return traces, and they seem like a nice solution.

The doc changes make sense to me, and if you think there needs to be more support for that, I'm interested in hearing what you think it looks like

[stevenpelley]

I'll submit a pull request when I get a chance.

We must have seen the same HN article on return traces. I like the concept as a goal of error handling, at least for errors that may be handled as internal errors and need enough information to debug, but I think a framework to do it is a little too magical.