Don't allow BasicLit as error, suggest to make as const

[bombsimon]

This PR extends #12 which will only allow global error variables prefixed with Err if they're assigned from fmt.Errorf or errors.New.

This branch is based on whitelisted-selectors which makes the diff towards master looks bigger than it is. When(/if) #12 is merged I will rebase this branch to visualise the diff better.

This PR is a result of #12 (comment) where I got asked to limit the first PR to only add support for regexp.MustCompile and not touch error handling. I still think this PR is valid since there's a lot of missed errors not caught and since go 1.14 we shouldn't use custom error types if possible to avoid.

---

Only allow global error variables if the're assigned from fmt.Errorf and errors.New. Relates to #5

I know this is probably very opinionated but I'll just leave it here to marinate and use for discussion.

---

Don't allow global vars, even if they're prefixed with err or Err, to be assigned to a *ast.BasicLit since these can be converted to const instead.

[leighmcculloch]

Thanks for opening this @bombsimon. I always appreciate the thoughtfulness you give to this topic.

I'm very much on the fence about this one. While I don't see frequently, it is valid to make a custom error type, then define several variables that have values using that error type. I think the question for me is, if someone has named the variable with an err or Err prefix there is a good chance it is an error sentinel value, no matter the type. What problem are we solving by restricting it further?

[bombsimon]

This has not been an issue to me at all since it's allowing more than I need. I've very rarely seen custom errors (in a global scope) after error wrapping game along in Go 1.13. If you implement your own error it would usually be to add dynamic data and in that scenario a global error makes little sense to me.

As suggested in #5 it would be nice to just ensure that the type implements the error interface but this is not easy. On the other hand the only positive false I can think of would be something describing error management, f.ex. var ErrorsAllowed = 2 which should be a const.

With that in mind, would it be a better solution to just ensure that the value is not a *ast.BasicLit? This means that any strings or ints added (potentially by accident) in a global var could be converted to a const?

If not, this is not something I feel very strong about so feel free to close this PR if you don't want to restrict the usage of global variables as long as they have the correct prefix. To avoid confusion it might be a good idea to close #5 as well if you decide on that.

[leighmcculloch]

it would be nice to just ensure that the type implements the error interface but this is not easy

Agreed, but I don't know how we do that. I don't think we can get that from the AST, we'd been the compiler to resolve the types? Which would probably increase the cost of using this tool. I think it's a trade off.

With that in mind, would it be a better solution to just ensure that the value is not a *ast.BasicLit? This means that any strings or ints added (potentially by accident) in a global var could be converted to a const?

I really like this idea. Independent of errors this ensures as you say that things that can be const are const. I'd need to check, but is a value that is a struct not a ast.BasicLit?

[bombsimon]

As far as I know only built in types are *ast.BasicLit. At least strucs and errors are not: https://play.golang.org/p/OfhLbp6-jCG

[bombsimon]

Rebased on master and switched focus, this PR now only targets to disallow *ast.BasicLit for errors, suggesting to make them const.

[leighmcculloch]

Something I'm noticing here is from the users perspective, knowing little about how we got here, that myVar = 1 and ErrIsNotErr = 1 look awfully alike and yet result in different errors.

I think the suggestion "should be a const" probably applies to any basic literal, not just errors. So instead of introducing it specifically as a feature for errors, we introduce it as a feature that applies to any var, as a suggestion. When a var is detected in this fashion, we don't bother checking the other exclusions, except the go:embed one, because any others can be changed to a const.

What do you think?

[bombsimon]

Oh, of course! This is in fact already how it works and why I pass both the variable and it's value when doing the check now. We first search for *ast.BasicLit and then we check the name.

The analyzer test package doesn't require the directive to match exactly, only a subset have to match, that's why the tests passed even though it didn't have the full error message.

I updated the test cases for myVar, to make this clearer, everything that's assigned to a *ast.BasicLit will see "should be a const".

[leighmcculloch]

@bombsimon I believe that #32 is going to address this issue as well. I'm going to merge it, at which point it should prevent basic literals, unless those basic literals are wrapped in some other type that is in fact an error. Do you agree?

[bombsimon]

Super nice with that PR! It will ensure nothing but errors will be assigned to the global variable but it won't give a hint about what can be converted to a const instead. But maybe that's not a part of this linter's responsibility. Feel free to close this PR in favor of #32!

[leighmcculloch]

Ahh yes that's a really good point. I think that's worthwhile because I don't think we want this linter to cause people to remove globals that could be const. I'll look at merging this after #32.