Clarifiaction on internal custom errors

[tombh]

First, thank you so much for this great linter. It's already brought me so much sanity, like a lost desert wanderer coming across an oasis 🏝️!

It seems that this doesn't, and indeed shouldn't, generate a lint message?

package main

import (
	"os"
	"github.com/pkg/errors"
)

var CustomError = errors.New("👷")

func main() {
	do()
}

func do() error {
	_, err := os.Open("/doesnt/exist")
	if err != nil {
		return CustomError
	}
	return nil
}

But my intuition from the proposition of this project is that it should. And it seems like that opinion is, or at least was, shared by others, judging by discussions in #3 and #6?

What's the current official position and reasoning? And are there any recommendations for helping ensure that return CustomError is wrapped?

[tomarrell]

Thanks a lot for the question, and I'm glad you find the linter useful!

You've come across quite a difficult question when it comes to whether or not we should report on this.

Generally, my reasoning was that if you define a sentinel error in this manner, you usually use it quite intentionally. This already gives you a rather well-defined set of places where this error may be used. Forcing extra wrapping on this struck me as being more annoying to users than adding much value.

However, I'm not opposed to adding the functionality. It may potentially be suitable behind a flag if it's something that you think would bring you value.

Happy to hear your thoughts.

[tombh]

That seems reasonable, and the main thing is that it's just good to know what the intended behaviour is. Thank you.

So for whatever it's worth I'll share my situation. I've inherited a large codebase that's never had any linting and I don't have access to the old developers. It's been a real challenge getting things done. But thanks to this linter I've already seen a huge improvement in debugging 🙇 However the code is still littered with these codebase-native, package-instantiated errors. Things like ErrNoRows! So perhaps despite best intentions, they've just become impractical, you know, needing to add trails of log.Debug(1/2/3) everywhere to find the actual source of the problem.

So my, albeit biased and perhaps naive, opinion would be this: by default all errors should be instantiated (or wrapped) at the occurrence site. And that would be my definition of the goal of this linter. Of course a flag would be nice to whitelist sentinel errors, and I'd argue that it be off by default 😏

Anyway, I can think of one workaround. I could put all these custom errors in their own package so that the linter would see them as "external". The superficial downside to that is that it's a bit unintuitive to define package features outside of itself. And also the loss of namespacing. But the bigger downside is that it still doesn't enforce the desired behaviour, namely that package-internal errors can be instantiated away from the occurrence site.