polyfloyd / go-errorlint

A source code linter that can be used to find code that will cause problems with Go's error wrapping scheme
MIT License
249 stars 19 forks source link

disable comparison linter for `Is(target error) bool` methods #11

Closed kolyshkin closed 2 years ago

kolyshkin commented 3 years ago

I wrote some code with a custom error type:

var ErrInvalidConfig = errors.New("invalid configuration")

// invalidConfig type is needed to make any error returned from Validator
// to be ErrInvalidConfig.
type invalidConfig struct {
    err error
}

func (e *invalidConfig) Is(target error) bool {
    return target == ErrInvalidConfig
}

...

Basically, the idea is to make errors.Is(err, ErrInvalidConfig) to return true for every error which is wrapped in invalidConfig type.

Now, the comparison linter complains:

validator.go:42:9: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
    return target == ErrInvalidConfig
           ^

Which is obviously incorrect because

  1. The argument of Is method, i.e. target, is not supposed to be unwrapped.
  2. Using errors.Is in the above example will result in infinite recursion.

There's a similar code in goland standard library (src/syscall/syscall_unix.go):

func (e Errno) Is(target error) bool {
          switch target {
          case oserror.ErrPermission:
                  return e == EACCES || e == EPERM
          case oserror.ErrExist:
                  return e == EEXIST || e == ENOTEMPTY
          case oserror.ErrNotExist:
                  return e == ENOENT
          }
          return false
  }

Note that target is compared with oserror.* directly.

polyfloyd commented 3 years ago

Yup, you should be able to make that comparison.

I think the best way to resolve this is to check whether the expression is in the scope of a function that adheres tot the Is signature. A linter exception is also possible, but I'd rather not make a roundtrip to the caveat section of the readme needed for something so common.

Unfortunately, I am very short on time and energy. If you need this to be resolved soon I recommend submitting a PR or adding a linter exception for now.

kolyshkin commented 3 years ago

or adding a linter exception for now

That is what I did for now. Might try to properly fix this in go-errorlint later...