search

tomarrell / wrapcheck

A Go linter to check that errors from external packages are wrapped
https://blog.tomarrell.com/post/introducing_wrapcheck_linter_for_go
MIT License
302 stars 27 forks source link

Why should I wrap variable errors created with errors.New? #3

Closed arvenil closed 3 years ago

arvenil commented 3 years ago 
import (
    "github.com/pkg/errors"
)

var (
    errInvalidDuration = errors.New("invalid duration")
)
config/config.go:152:10: error returned from external package is unwrapped (wrapcheck)
                return errInvalidDuration

Why should I wrap this error? It is a variable defined in the same package and it already has stack trace.

benjaminbartels commented 3 years ago

@arvenil Why did you decide to close this? I still see this behavior and I agree with you. I don't think I should have to wrap an error that is explicitly defined in the same package.

arvenil commented 3 years ago

@benjaminbartels because I disagree with myself :) The problem is that error declared at package level can be returned at different locations in the code, e.g. you can have 3 functions and all of them returning the same error. Even if you attach a stacktrace to it (like github.com/pkg/errors does) then it will lead you to the declaration of the error, not where it was returned. So in the end you need to always wrap it to understand WHERE the error was returned.

tombh commented 1 year ago

I'm wondering what the recommended way is to guard against these internally defined untraceable errors? I see there was some discussion in https://github.com/tomarrell/wrapcheck/issues/6 as well.

My intuition is that if you are using this linter then the behaviour described by OP is not a bug, but indeed intended and desired behaviour. Defining a package-global error such as:

var errInvalidDuration = errors.New("invalid duration")

is indeed useful for being able to share and thus match specific errors, but of course it loses its provenance. Therefore by its very design it will be used in more than one place and, due to its lack of stacktrace and identical message, it will be difficult to trace its provenance. Or in other words, what's the point in creating a custom error if it's only used in one traceable place?

I'm sure I must be misunderstanding something?

Edit: I only realised afterwards that OP was the same person to close the issue! So I don't see a lint warning for this case, but I would like to. So maybe I've setup the linter wrong?