Add support to whitelist selector expressions

[bombsimon]

This will allow a pre defined list of selector expressions X and Sel
idents to mark as whitelisted. By doing this we could allow a specific
type or function from a specific package such as regexp.MustCompile.

---

I had a discussion today regarding how to handle things such as regexp.MustCompile() or pre defined errors as a global variable which this linter doesn't really support. I know you can ignore errors by using specific variable names but I also saw that there were some TODO makers to skip fake errors. I also found #6 which addresses the same issue I have and that you were open to PRs.

I guess I could and should've asked in the ticket but I thought I had a go at an implementation to handle this. Since you're already open to match the name of the variable, would you be open to define a list of selector expression value that could be whitelisted?

As seen in the PR I created a 2D array which holds an X ident and a Sel ident which will be matched. I added three defaults to test this; errors New, fmt Errorf and regexp MustCompile. If this is a feasible solution this could even be configurable to allow any expressions, like http Client I use in the test. Regarding errors specific I guess a check if it implements the Error() interface would be better but I still see a use case for a feature like this.

I added a new test file to show the behaviour and also updated the existing tests. Please let me know what you think! If this is not the way you want to go I totally understand and will close this PR.

[codecov-io]

Codecov Report

Merging #12 into master will increase coverage by 11.87%.
The diff coverage is 94.73%.

@@             Coverage Diff             @@
##           master      #12       +/-   ##
===========================================
+ Coverage   59.15%   71.02%   +11.87%     
===========================================
  Files           2        2               
  Lines          71      107       +36     
===========================================
+ Hits           42       76       +34     
- Misses         27       28        +1     
- Partials        2        3        +1

Impacted Files	Coverage Δ
check_no_globals.go	92.68% <94.73%> (+1.37%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c3491d...0133dfa. Read the comment docs.

[leighmcculloch]

Thanks this is great! Also thanks for updating and adding sufficient tests. One small request inline.

[bombsimon]

I tried to look at your other code to find what style or likings you've got. Feel free to nit on formatting, newlines, naming and whatnot! Also tell me if you wan't me to squash this into a single commit!

[leonklingele]

Just follow the Go convention of prefixing global error variables with "Err" or "err". IMO there's no need to whitelist error variables using a different naming scheme.

What's the benefit of whitelisting regexp.MustCompile given that the regex won't be checked at runtime?

EDIT:
Don't get me wrong. I really like the idea of a whitelist but I think it shouldn't include any of those defaults.

[bombsimon]

Yeah I'm all for following the naming convention for errors and only allow names staring with Err or err, although it might be a good idea to check both things. This example isn't really an error and shouldn't be an allowed global variable:

var ErrorsAllowed = 1

Regarding regexp.MustCompile it's not so much for the compilation or error checking rather than it's not really efficient to re-compile the regexp each time it's used. Consider calling this function 10000 times per minute:

func F(in []byte) [][]byte {
    re := regexp.MustCompile(`^\d+$`)

    return re.FindAll(in, -1)
}

versus

var re = regexp.MustCompile(`^\d+$`)

func F(in []byte) [][]byte {
    return re.FindAll(in, -1)
}

I noticed now that one of the main reasons to add support for this according to the issue was to check for compilation errors but the reason I made this PR was to avoid re-compiling the regexp more than once.

[leonklingele]

Why need to call regexp.MustCompile multiple times? Just call it once and store its result:

func NewServer() *Server {
	re := regexp.MustCompile(`^\d+$`)
	return &Server{
		re: re,
	}
}

// Then use and inject (*Server).re anywhere you like

[bombsimon]

How about code with functions requiring regexp that is not attached to a struct? In my case I don't always have a Server (or similar). It might be a library with helper functions that don't need a struct.

And even if I did, I don't think I like the idea of adding fields to a struct just to be able to make them "global". The same way I don't like to implement methods on a struct if the struct itself is never used.

But it boils down to a matter of opinion I guess. You could always argue to refactor your code to avoid the need for global variables I guess.

[leonklingele]

Attaching the *regexp.Regexp to a struct was done to illustrate that there's always a way to somehow store it without having to call regexp.MustCompile again.
If you don't have a struct available, simply pass *regexp.Regexp just like you pass a context.Context.

[akarl]

This seems to boil down only to opinions and tastes. Compiled regexps as constants or not.

I personally would like to have compiled regexps as constants. But not everyone agrees.

Could we make the linter configurable. Perhaps providing a list of allowed prefixes or types?

[bombsimon]

Yeah I got that it was an example, that’s why I ended my previous post the way I did. I also tried to show other examples which might be harder to refactor for. It wouldn't be much of a helping function if you were to provide the regexp executed yourself.

I’ll leave the PR open and let a maintainer chose if they want to close the PR.

Thanks for your input!

[cnmcavoy]

@leighmcculloch are you going to merge this PR in or did you decide not to support this use case? If you didn't decide to support this behavior, closing this PR would be nice, because its very unclear what state this is in, as an observer 1yr later.

[leonklingele]

I still don't like the idea of whitelisting any function calls. errors.New and fmt.Errorf is already whitelisted, just prefix the variable with err or Err — stick to the recommended Go coding style. Having a global *regexp.Regexp produced by regexp. MustCompile is bad style as well. See my previous comments: #12 (comment), #12 (comment) & #12 (comment).

[cnmcavoy]

Regular expressions are often used in the same was as constants are in code, so declaring them at the var level in one package, then referencing the var in other packages is a fairly common code pattern. The documentation for MustCompile mentions global variables explicitly:

// MustCompile is like Compile but panics if the expression cannot be parsed.
// It simplifies safe initialization of global variables holding compiled regular
// expressions.

I would be concerned about suggesting code with regexp.MustCompile to be rewritten as struct field members. It sounds like it might encourage Singletons structs, and other worse code-readability antipatterns, whereas var members are very readable and easy to find.

---

Obviously that is all just my opinion. If there isn't support for this, then it would be more clear to close this PR, rather than leave it open and confuse others in the future.

[leighmcculloch]

Thanks for reviving this PR. I apologize it dropped through the cracks. I think there's a lot of value including regular expression compilation, even though they are not constants, many Go developers treat them like constants including myself.

The errors I'm unclear about, and I think it'd be great if we can make errX and ErrX only pass if the right-hand side is errors.New or fmt.Errorf as suggested above too, but I think that is worth tackling and thinking about independently of shipping the regexp win.

Could we reduce this PR with just the regexp.MustCompile function in the list?

[bombsimon]

Sure thing, I can split this into two separate PRs, probably later today or tomorrow.

[leighmcculloch]

🎉 Looks fantastic.

[leighmcculloch]

Thank you for renaming whitelist to allow. 💯

[leighmcculloch]

👏 Thanks @bombsimon!

[leighmcculloch]

Closed #6