unexportedglobal: Linter to require '_' prefix on globals

[abhinav]

This adds a unexportedglobal to golangci-lint.
unexportedglobal is a linter that requires that unexported global variables and constants are prefixed with '_'.

package foo

// Bad
var pool = sync.Pool{ /* ... */ }

// Good
var _pool = sync.Pool{ /* ... */ }

The idea is to eliminate the risk of conflict between names of local variables and names of global variables.

The linter is inspired by the Prefix Unexported Globals with _ guidance in Uber's Go Style Guide.

The linter is not enabled by default.
I have also not added it to the 'style' preset because not everyone may agree with this change.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

• The CLA must be signed

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic(), log.fatal(), os.exit(), or similar.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
• WithURL() must contain the URL of the repository.

Recommendations

• The linter should not use SSA. (SSA can be a problem with generics)
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

[ldez]

The linter is not enabled by default.

This sentence is partially wrong because all the linters are enabled when using enable-all.

I have seen different behavior with the handle of a new linter:

• if a linter doesn't have the "right" default behavior they just ignore it.
• some people just follow the rules because "linters say the truth".

This is why for me the default behavior of a linter (and not default linters) is really important.

I have also not added it to the 'style' preset because not everyone may agree with this change.

I should be a part of the style preset because it's a linter about style, nothing else is required to be in this preset.

---

I personally dislike global variables: it's a source of confusion, and bugs, and can produce side effects. IMO only a few cases really need global variables.
It's the same thing for init function, especially inside a library.

I'm also not a fan of prefixing things: it feels like a rule from another epoch or language.

The fact that the rule comes from a company is just an authority argument, so it's not an argument that interests me.

As you can understand, for now, I disagree with this rule and this linter, but I'm not alone on this project, I will wait for feedback. Also, I need to think more about this rule.

So I'm not making any decision at this time.

[abhinav]

Thanks for looking over this, @ldez!

I should be a part of the style preset because it's a linter about style, nothing else is required to be in this preset.

Makes sense. I'll add it to the preset.

---

I personally dislike global variables

Same! But are legitimate cases where you need them.
Buffer pools, default values, static collections (e.g., list of known subcommands), etc.
Valid uses are rare, but present.

I'm also not a fan of prefixing things: it feels like a rule from another epoch or language.

Fair. The prefixing is specifically to separate the namespaces -- similar to how exported variables are already a different namespace (because they start with uppercase letters).

The fact that the rule comes from a company is just an authority argument, so it's not an argument that interests me.

It was not my intent to use the company's practices as an authority, only as an example.
The argument in favor of the pattern is made in the linked style guide, and in the README for the linter, but in short:

The idea is to prevent mistakes where you accidentally use or modify the global variable when you intended to use the local variable.

By prefixing globals with _, we separate the namespaces (because idiomatic Go code will never use a _ prefixed local variable), and avoid these conflicts while still being able to use short and concise names for them. As global variables are meant to be rare anyway, this makes the choice to refer to the global quite explicit.

This also has a nice effect on readability while reviewing code on a website (GitHub, Phabricator, whatever): when you see a _-prefixed variable, you can assume it's a global and don't need to scan upwards in the function to find where that variable was declared.

[ldez]

Same! But are legitimate cases where you need them.
Buffer pools, default values, static collections (e.g., list of known subcommands), etc.
Valid uses are rare, but present.

Your examples are the illustration of the problem because they are not valid use cases for me:

• buffer pools could be fields
• default values should be constants or handled in a constructor
• static collections could be functions

Another concrete illustration: you removed a global variable and an init function inside your linter. In the context of your linter, and mainly because your linter is a library, those elements are not needed and should be forbidden. We forbid those kinds of elements because, for example, the init will be called even if your linter is not used (note it's the same thing for global variables). If there is a panic, the panic will block golangci-lint (we added this rule because the problem already happened) and the users will not be able to fix that.

Valid use cases are, for example, analyzers that share their reports with other analyzers (like inspect.Analyzer) because the instance of the analyzer is used as a key inside pass.ResultOf.
But an analyzer/linter inside golangci-lint, because its reports are not shared, doesn't need to be a global variable.

The discussion is a bit off-topic but it's interesting to talk about that 😸.

[Antonboom]

JFYI: some of Uber style guide rules were covered by ruleguard – https://github.com/quasilyte/uber-rules
Usually such simple linters do not require be a separate Go module.

Also gochecknoglobals (included in golangci-lint) already handles _ prefix.

[ldez]

I don't want to be responsible for a new convention about global variable naming.

This can be implemented as a plugin or as ruleguard rule.

So we will decline the PR.

thank you anyway, the discussion was fun.

[abhinav]

Reasonable. Thanks for reviewing.

Thanks for the rule-guard tip, @Antonboom.
(Small correction: gochecknoglobals allows global variables named _, not variables named _foo.)

[Antonboom]

@abhinav I respect your work and don't want to belittle it.
But maybe it will be better to do configuration contribution in gochecknoglobals? 🤔

[abhinav]

But maybe it will be better to do configurable contribution in gochecknoglobals? 🤔

No offense taken. Yep, an option in gochecknoglobals was indeed what I was considering next. 😄