Strictness with parametrized errors

[titpetric]

Often, errors require parameters, e.g.:

return nil, fmt.Errorf("unknown storage provider '%s'", providerName)

These aren't errors that need a statically/globally defined type, nor is there an error available which we can wrap. Both errors.New and fmt.Errorf report errors with err113, but there isn't a clear way to resolve them. Should the error be reported, if we're not passing an error typed parameter for %w?

[titpetric]

As a subquestion, can we somehow disable this particular dynamic error check, while keeping the errors.Is check active? The fatih/errwrap package already handles wrapping errors, with no false positives like the above example.

[onokonem]

but there isn't a clear way to resolve them

Actually, it is:

var ErrProviderUnknown = errors.New("unknown storage provider")

func someFunc(providerName string) error {
    return fmt.Errorf("%q: %w", providerName, ErrProviderUnknown)
}

[titpetric]

Not to be contrary, but not all returned errors should be wrapped. What you gave as an example is a sentinel error much like io.EOF which, as much as it's expected to be a constant - it's mutable.

Case 1: Issuing errors.New within your code, passing it a string const value is an often used best practice for pkg/errors, as errors.New will attach a stack trace at the point where errors.New is invoked.

In your example, that stack trace will also be unusable as it would be recorded at ErrProviderUnknown declaration.

Case 2: A common implementation for const custom errors Is() is as follows:

func (err constError) Is(target error) bool {
    ts := target.Error()
    es := string(err)
    return ts == es || strings.HasPrefix(ts, es+": ")
}

So, the cleaner version of providing errors is:

const (
    ErrUnknownStorageProvider = "unknown storage provider: %s ..."
)

Usage: return fmt.Errorf(ErrUnknownStorageProvider, providerName) (or errors.New if no parameters in accordance with case 1.).

But more to the point, creating a dynamic error, like done in example here, with or without parameters, isn't discouraged in Go 1.13+, nor should it be discouraged in the err113 check. What you essentially did is deprecate errors.New outside of global context, and any non-wrapped error (e.g. original error) which would be function scoped, without a variable declaration.

Please consider some exceptions here. Using errors.Is is a welcome addition, and using %w instead of %s is also welcome, but other than that I think you're overly strict in terms of how errors are created, and are making some wrong assumptions here.

[onokonem]

In your example, that stack trace will also be unusable as it would be recorded at ErrProviderUnknown declaration.

are you sure? AFAIK the stacktrace will be recorded at fmt.Errorf() point

A common implementation for const custom errors Is() is as follows

This is what I'm fighting against :)

There was no standard way to analyse an error prior to 1.13. So people were doing strange and dangerous things like analyse the error messages. This must be stopped!

With a method I've described above any package can:

1. return as much customised error as the author like
2. provide a bunch of base errors to be used to analyse an actual error returned
3. any returned error can be analysed to be attributed with particular type/kind of error comparing to one of the base errors

p3 is not possible with an error just created at the return point, is it?

this is why I think we should never create new errors itself nowadays but use wrapping everywhere.

[titpetric]

Standard library doesn't record stack traces at all. Only pkg/errors does. The main reason behind that is that the standard library is more performance oriented, and there's a penalty if stack traces would be recorded for every error. That's why with pkg/errors, a newly returned error must go either over errors.New, Errorf, or Wrap, to add the stack trace on the leaf function. At some point, the stdlib may add similar functionality, with similar constraints - stack traces could be effectively free if they can be recorded at compile time, but that's guesswork for a future go release.

Not every error is meant to be handled, some are meant as an user guide. Unfortunately that means that the error is basically returned as a string, possibly including non-standard formatting, newlines and other formatting directives like ansi terminal colors.

Having a base error (e.g. your 2. point) is actually working against this intent. There's also a little more than 500 examples of fmt.Errorf usage in the standard library, which would be reported as errors. fmt.Errorf search in golang/go. An especially painful example are unit tests, where usually the expected and returned values are printed if they don't match. Like:

return fmt.Errorf("different number of files: %d != %d", len(p.files), len(q.files))

Of course, just excluding packages that import testing also isn't enough of an approach, as there are often examples of local packages to help with testing that then return errors in the same way. Not to mention that the same rules should apply for code in tests, as anywhere else.

I think as far as fmt.Errorf goes, the behaviour should be as close as possible to fatih/errwrap, meaning no error should be reported if no error parameters are passed to fmt.Errorf. I don't have a strong opinion on errors.New, but enforcing error values there instead of string literals should be fine. The pkg/errors package is a special case as it won't have fmt.Errorf but errors.Errorf, which should get handled separately (I wouldn't ignore best practices for pkg/errors in regards to %w)

[onokonem]

I'm a little bit confused, sorry

what should we do with this ticket?

I've created this linter to push the strategy I believe in: every error returned must be analyzable with errors.Is() (or even errors.As(), but it's completely different story).

in case of dynamic errors errors.Is() become useless, doesn't it?

so this does not make sense for me to have these checks applied separately.

does it make sense to you?

[titpetric]

With tests and cli apps, the error path is never checked with anything else than != nil, before landing in log.Fatal, t.Fatal/Error; not all errors need to be sentinel errors. It would make more sense to me to forbid err.Err() calls producing a string (assuming somebody matches it), forbid passing it to print functions with %s (only %+v is valid?), only allow %w on Errorf. What you describe is only valid for importable packages where errors are meant to be handled, not just logged. As you see, there are edge cases. So yes, ideally, the checks would be applied separately, or would at least be partially configurable. Gosec which detects os/exec calls has a particular comment to remove single errors, but as Errorf usage may be more widely used (the drone ci file I linked has 5+ examples in a single file), the preffered way would be to ignore such errors in the complete file, or fully disable the check for Errorf. There’s no reason to ever disable errors.Is() checks.

…

On Sat, 16 May 2020 at 00:22, Daniel Podolsky ***@***.***> wrote: I'm a little bit confused, sorry what should we do with this ticket? I've created this linter to push the strategy I believe in: every error returned must be analyzable with errors.Is() (or even errors.As(), but it's completely different story). in case of dynamic errors errors.Is() become useless, doesn't it? so this does not make sense for me to have these checks applied separately. does it make sense to you? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABY7EF2FSZTY2VSMLCDC2DRRW6DRANCNFSM4M5YBSPQ> .

[titpetric]

Just to add my current work-around, using .golangci.yml:

issues:
  exclude-rules:
    - path: _test\.go
      linters:
        - goerr113
    - linters:
        - goerr113
      text: "do not define dynamic errors"

I could probably do without excluding _test.go for my use cases, and I'd have to do some work figuring out how to include goerr113 in a specific path. Having a specific error code would be nicer to exclude, for example:

issues:
  exclude:
    - GE002

[onokonem]

Just to add my current work-around, using .golangci.yml:

definitely looks like a solution

can we close this issue now?

[ewohltman]

I'm not such a fan of using package global variables because not only should we avoid using global variables, there is another golangci-lint linter that flags the usage of global variables.

Perhaps we could lead towards implementing the error interface instead. For example:

const errProviderUnknownMessage = "unknown storage provider"

// ErrProviderUnknown is an unknown provider error.
type ErrProviderUnknown struct {}

// Error satisfies the error interface for ErrProviderUnknown.
func (err *ErrProviderUnknown) Error() string {
    return errProviderUnknownMessage
}

func someFunc(providerName string) error {
    return fmt.Errorf("%s: %w", providerName, &ErrProviderUnknown{})
}

[onokonem]

I'm not such a fan of using package global variables

package-level errors are not variables, they are semi-constant

I do not see any reason to protect them, but, in case we need the really const errors we can go this way:

package main

import (
	"errors"
	"fmt"
)

type ConstError string

func (e ConstError) Error() string {
	return string(e)
}

const ErrSomethig ConstError = "some const error"

func main() {
	err := fmt.Errorf("wrapped: %w", ErrSomethig)

	fmt.Println(ErrSomethig)
	fmt.Println(err)
	fmt.Println(errors.Is(err, ErrSomethig))
}

[ewohltman]

type ConstError string

func (e ConstError) Error() string {
	return string(e)
}

const ErrSomethig ConstError = "some const error"

I like this even better than the empty struct!

[titpetric]

@onokonem i'm fine closing this, i'm even better if you'd be willing to add error codes which we could match against if needed (e.g. GE001 for errors.Is checks, GE002 for Errorf...). After all, I'm currently matching against text from the error, and having a consistent error code would help with that.