Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add reassign linter #3064

Merged
merged 11 commits into from Aug 23, 2022
Merged

feat: add reassign linter #3064

merged 11 commits into from Aug 23, 2022

Conversation

chokoswitch
Copy link
Contributor

@chokoswitch chokoswitch commented Aug 5, 2022

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
Copy link

boring-cyborg bot commented Aug 5, 2022

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2022

CLA assistant check
All committers have signed the CLA.

@ldez
Copy link
Member

ldez commented Aug 5, 2022

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.

@ldez ldez added the linter: new Support new linter label Aug 5, 2022
@chokoswitch
Copy link
Contributor Author

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 ldez added feedback required Requires additional feedback blocked Need's direct action from maintainer and removed feedback required Requires additional feedback labels Aug 20, 2022
@ldez
Copy link
Member

ldez commented Aug 20, 2022

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

@chokoswitch
Copy link
Contributor Author

@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
Copy link
Member

ldez commented Aug 20, 2022

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
Copy link
Contributor Author

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
Copy link
Member

ldez commented Aug 20, 2022

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?

@ldez ldez added feedback required Requires additional feedback and removed blocked Need's direct action from maintainer labels Aug 20, 2022
@chokoswitch
Copy link
Contributor Author

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
Copy link
Member

ldez commented Aug 20, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

ldez commented Aug 20, 2022

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
Copy link
Contributor Author

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

@ldez ldez removed the feedback required Requires additional feedback label Aug 22, 2022
@ldez
Copy link
Member

ldez commented Aug 22, 2022

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"

@ldez ldez added the feedback required Requires additional feedback label Aug 22, 2022
@chokoswitch
Copy link
Contributor Author

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
Copy link
Member

ldez commented Aug 23, 2022

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 ldez removed the feedback required Requires additional feedback label Aug 23, 2022
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez merged commit 58809c3 into golangci:master Aug 23, 2022
@chokoswitch
Copy link
Contributor Author

@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

SeigeC pushed a commit to SeigeC/golangci-lint that referenced this pull request Apr 4, 2023
@ldez ldez added this to the v1.49 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants