feat: add reassign linter

[chokoswitch]

reassign is a linter to check when global variables in other packages are reassigned, for example io.EOF = nil
https://github.com/curioswitch/go-reassign

[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.

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description about 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 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.

Recommendations

• The linter should not use SSA. (currently, SSA does not support 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.

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

[chokoswitch]

Hi @ldez - it's been more than a week so just checking in, what sort of schedule can we expect on this sort of code review? Happy to iterate on the code as reviews come in. Thanks!

[ldez]

I have some doubts about this linter because by default it only detected io.EOF reassignment.

[chokoswitch]

@ldez The default is to detect io.EOF and any variable prefixed with Err. I think this provides the least chance of false positives and can catch most patterns of reassigning errors which is common use of package variables, but do you have an idea for a better default?

[ldez]

Does your linter only detect reassignment of global variables?

For you, in which cases the reassignment is a problem? and why do you think there is a problem?

[chokoswitch]

Yes the job of the linter is to detect reassignment of global variables. The background for the linter is here

https://github.com/curioswitch/go-reassign#background

Some people prefer constant errors pattern to errors.New out of principle because of the potential of reassignment causing problems, so a linter helps make them both robust.

[ldez]

Yes the job of the linter is to detect reassignment of global variables.

so the description of your linter must be explicit on that.

I think you have to improve the default detection and maybe you have to provide a slice instead of one string for the pattern.

Also, I'm not sure about regexp, why did you use it instead of simple string?

[chokoswitch]

so the description of your linter must be explicit on that.

Do you mean PR description? It says "public variables in other packages" which I think is fairly explicit

I think you have to improve the default detection and maybe you have to provide a slice instead of one string for the pattern.

Could you describe what sort of improvement you're thinking of? Should the pattern accept any variable to detect them all?

I can change from string to slice

Also, I'm not sure about regexp, why did you use it instead of simple string?

It's to match against unknown variable names, often defined by a user. When matching against something arbitrary like that, I think regex flexibility can be needed

[ldez]

It says "public variables in other packages" which I think is fairly explicit

"public variables" or "top-level variables" are not really explicit.
"public" is not really a Go vocabulary, we use mainly "exposed".
"top-level" is just related to a position inside something.
"global variables" is a common and clear programming concept.

Could you describe what sort of improvement you're thinking of? Should the pattern accept any variable to detect them all?

I already said that the current default is a bit weak.
In which you will use this linter? what will be your rule?
I will not provide the list of the global vars of the std lib.

I think regex flexibility can be needed

Regexps are CPU intensives, so you have to a clear need to use them.
regexps are powerful but they must be used carefully.

The weak default and the use of regex, give me the impression that you don't really know the concrete use cases of your linter, that's why I ask you questions.

[chokoswitch]

The weak default and the use of regex, give me the impression that you don't really know the concrete use cases of your linter, that's why I ask you questions.

Did you read the background I linked? I'm not sure how much more to add about the concrete use case which is to safely provide exposed var ErrFoo = errors.New() type of pattern.

It can also be used outside the case of errors to detect more broadly, in which case the regex would likely be set more permissive but that may increase chance of false positives.

[chokoswitch]

Because golangci has its own suppression mechanisms, perhaps the pattern isn't as useful when invoked via golangci (it'd still be needed for CLI invocation in the binary itself). Would it be better to instead default to checking all reassignments without the pattern option, assuming that false positives could be fixed or suppressed with golangci?

[ldez]

Would it be better to instead default to checking all reassignments without the pattern option, assuming that false positives could be fixed or suppressed with golangci?

It's not a good idea: generating false positives is not a good thing at all.

As your linter is not specific to error as global variables, I expected something different as default but I understand that you are focused on error, so we will go with the current default.

Can you add a slice for the patterns?

[chokoswitch]

Thanks @ldez! I've changed the config to a slice

[ldez]

There is something that is unexpected for me.

tree

.
├── config
│   └── config.go
├── .golangci.yml
├── go.mod
└── main.go

config/config.go

package config

import "net/http"

var DefaultClient = &http.Client{}

main.go

package main

import (
	"io"
	"net/http"

	"github.com/golangci/sandbox/config"
)

var DefaultClient = &http.Client{}

func reassignPattern() {
	io.EOF = nil

	config.DefaultClient = nil // <- fail
	DefaultClient = nil        // <- fail

	http.DefaultClient = nil
	http.DefaultTransport = nil
}

.golangci.yml

linters:
  disable-all: true
  enable:
    - reassign
    - typecheck

linters-settings:
  reassign:
    patterns:
      - "DefaultClient"
      - "DefaultTransport"

[chokoswitch]

Thanks @ldez sorry for the trouble. I had forgotten to rename the yaml field name and had a bug with multiple slashes. Added tests for latter upstream and yaml here

[ldez]

I think that the reassignment should be handled even inside the same package.

examples

package a

import (
	"errors"
)

var MyError = errors.New("My error)

func reassignPattern() {
	MyError = nil

}

package a

import (
	"net/http"
)

var DefaultClient = &http.Client{}

func reassignPattern() {
	DefaultClient = nil
}

It can be seen as an improvement.

---

Also, I think the fact that the patterns are only based on the name instead of the package and the name may produce false positives.

[ldez]

LGTM

[chokoswitch]

@ldez Thanks for all the help on this! 🙏

I think that the reassignment should be handled even inside the same package.

Yeah this is something that came to mind. I didn't implement it yet mostly because within-package, the author has control of the code so can worry less, having their variables modified outside the package from unknown codebase can cause undefined behavior though. But I think it could be an improvement, possibly opt-in