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.

I did a quick update for the linter to use Go's analyzer which has some great features. This linter no longer has to figure out paths to files, tests, filenames, lines etc. It's also super easy to maintain tests since you just add diagnostics to your test package and write a oneliner test using analysistest. I had one issue with testing test files as you can see which resulted in an upstream issue here. The test code generated will have global variables which will make the test not pass.

The main reason however was to address golangci-lint#1393 and make it easier to keep golangci-lint in sync with this linter.

After merging this it will be super easy to just add this analyzer to golangci-lint like this.

Please let me know if you think this is a good idea or if you want any changes to this approach!

It is possible to initialize test variables by TestMain. However global variables are the only way to share data with TestMain and tests. Can we ignore global variables in _test packages or _test.go files in general?

I have a couple of calls to regexp.MustCompile which, in my opinion, make sense to be stored in a global value because they ensure that the regex is valid at compile time. Would it make sense to add an exception for cases like that? So to whitelist a couple of functions that work like regexp.MustCompile from the standard library?

An example can be found here: https://github.com/mastertinner/latest/blob/master/internal/app/latest/brew/upgrade.go#L13

Error variables are variables starting with 'err' or 'Err' which were made by either errors.New or fmt.Errorf.

There still is leeway for improvement such as checking if the node implements the error interface. This would allow us to whitelist custom error variables.

Fixes #2

x.go:11::warning: ErrHandlerAlreadyExists is a global variable (gochecknoglobals)

It's a common practice to create global error variables such as the one above. It might be wise to not warn about them. What do you think?

I have an app having the following layout:

├── cmd
│   └── myprj
│       └── main.go       # package main
└── pkg
    └── myprj
        ├── featureA.go   # package myprj
        └── featureB.go   # package myprj

The file main.go looks like the following:

[...]

//nolint:gochecknoglobals
var (
	url     = flag.String("url", "", "Host and port")
	id      = flag.String("id", "", "Identifier")
	token   = flag.String("token", "", "Authentication Token")
	table   = flag.String("table", "", "Table name")
	key     = flag.String("key", "", "Key name")
	record  = flag.String("record", "", "Record name")
	symbol  = flag.String("symbol", "", "Symbol ID")
	topic   = flag.String("topic", "", "Topic ID")
	start   = flag.String("start", "", "Start time")
	stop    = flag.String("stop", "", "Start time")
	limit   = flag.Uint("limit", 0, "Max")
	columns = flag.String("columns", "", "Column names")
    [...]
    // more flags
)

func main() {
	flag.Parse()
	log.SetLevel(log.DebugLevel)
	log.Infof("config: -url     = %v", *url)
	log.Infof("config: -id      = %v", *id)
	log.Infof("config: -token   = %v", *token)
	log.Infof("config: -table   = %v", *table)
	log.Infof("config: -key     = %v", *key)
	log.Infof("config: -record  = %v", *record)
	log.Infof("config: -symbol  = %v", *symbol)
	log.Infof("config: -topic   = %v", *topic)
	log.Infof("config: -start   = %v", *start)
	log.Infof("config: -stop    = %v", *stop)
	log.Infof("config: -limit   = %v", *limit)
	log.Infof("config: -columns = %v", *columns)
    [...]
    // more flags

    [...]

    // processing: calling functions in the package myprj
}

I want to remove the line //nolint:gochecknoglobals.

I could copy the global variables witin the main() body, but the number of lines becomes too high.

However, I think in such cases, global variable is acceptable:

1. the package containing the function main() is composed of one signgle Go file.
2. the signgle Go file contains very few functions (in such cases, the stuff is in the pkg folder).

There is no issue with global variable in a tiny main.go file like that.

Many projects populate a "Version" variable at build-time using ldflags. For instance, a Makefile might set a Makefile variable to the latest Git tag and rev, and pass it to a "Version" variable through -X.

Doing so makes it easy for binaries to include version info (e.g. for a CLI program to print the version number when passed the -v flag). This is a good use-case for a global variable and shouldn't be considered an error, as long as there's only one per project.

Right now I have to do the following with golangci-lint:

// Package assets contains templates.
package assets

import "embed"

var (
        //go:embed templates
        assets embed.FS //nolint: gochecknoglobals // no other possibility
)

// Get gets the templates in templates folder as embed.FS.
func Get() embed.FS {
        return assets
}

Global variables prefixed with the new go 1.16 feature //go:embed should be ingored (like it already does for regular expressions).

The corresponding golangci-lint issue: https://github.com/golangci/golangci-lint/issues/1727

Current best practice for setting the version of a Go executable requires a global variable. This change adds a narrow exception for variables name Version exactly. It also adds tests to verify that and updates to README to explicitly state the permitted exceptions.

package 4d63.com/gochecknoglobals/checknoglobals: unrecognized import path "4d63.com/gochecknoglobals/checknoglobals": reading https://4d63.com/gochecknoglobals/checknoglobals?go-get=1: 404 Not Found

Default on...

pixley@M-C02DQ6AMML85> golangci-lint --version
golangci-lint has version 1.49.0 built from cc2d97f3 on 2022-08-24T10:24:37Z
pixley@M-C02DQ6AMML85> uname -a
Darwin M-C02DQ6AMML85 21.6.0 Darwin Kernel Version 21.6.0: Wed Aug 10 14:25:27 PDT 2022; root:xnu-8020.141.5~2/RELEASE_X86_64 x86_64

checks _test.go files. I wish I could turn this off but I don't see any yaml config for this.

So... two errors, one may not be yours. 1) golangci-lint should not check _test.go files by default and 2) I wish there were a yaml option for me to explicitly turn it off.

Hello,

First, thank you for providing this linter! I am using it as part of golangci-lint. I am now working on a project that have some regex.Must in global variables and have noticed that these are excluded from linter when used as global variables.

I assume that is because they are in this case compile-time errors if something goes wrong.

Now I also need to do same thing with templates for policy documents which are part of HTTP request body. Template documents are defined as consts and in global variable I am doing following:

	adminPolicyTemplate        = template.Must(template.New("AdminPolicy").Parse(AdminPolicy))
	readerPolicyTemplate       = template.Must(template.New("ReaderPolicy").Parse(ReaderPolicy))

which if anything is wrong with AdminPolicy template document or ReaderPolicy template document are compiler-ime errors, same as for regex. Therefore I think it would be nice to have same exception for template.Must as we have for regex.Must.

Only treat global error variables as such if they have an "err" or "Err" prefix and implement the error interface.

Implements https://github.com/leighmcculloch/gochecknoglobals/issues/5.

Using the guidance here: https://pkg.go.dev/cmd/go/internal/generate

This tool should ignore files which contain the ^// Code generated .* DO NOT EDIT\.$ regex match.

This can be a flag or default, but it should allow for ignoring those files.

Using this code:

var namePatterns = []map[string]string{
	{`[^\pL-\s']`: ""}, // cut off everything except [alpha, hyphen, whitespace, apostrophe]
	{`\s{2,}`: " "},    // trim more than two whitespaces to one
	{`-{2,}`: "-"},     // trim more than two hyphens to one
	{`'{2,}`: "'"},     // trim more than two apostrophes to one
	{`( )*-( )*`: "-"}, // trim enclosing whitespaces around hyphen
}

var nameRegex = regexp.MustCompile(`[\p{L}]([\p{L}|[:space:]\-']*[\p{L}])*`)

func onlyOne(s string) string {
	for _, v := range namePatterns {
		for f, r := range v {
			s = regexp.MustCompile(f).ReplaceAllLiteralString(s, r)
		}
	}

	return s
}

namePatterns is reported as a global variable which is correct, but what is the best pattern here?

Should I move it in my onlyOne func?

Isn't it slower doing so?