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

nolintlint: allow to fix //nolint lines #1583

Conversation

ashanbrown
Copy link
Contributor

@ashanbrown ashanbrown commented Dec 27, 2020

This PR adds a fixing capability to nolintlint. It doesn't try to fix everything.

What it does (when --fix is enabled):

  • Removes non-specific //nolint directives that are not used.
  • Removes a linter from a linter-dispecific //nolint directive if that linter is not used for that line.
  • Removes extra leading whitespace from //. nolint directives for any violations on the number of spaces. We assume the intent was to have a single leading space unless allow-leading-whitespace is set to false, in which case we assume no leading spaces (i.e. //nolint).

What it does not do:

  • Offer explanations when explanations for nolint are required but not provided. I suppose it could add a placeholder that the user could replace but that doesn't seem really in the spirit of how the fix option is supposed to work (since the user might check in that placeholder).
  • Make non-specific linter directives specific. This could be done but there is a risk that it cannot divine the intent of the user in setting the nolint directive and I'd argue it's best for the user to make this determination on their own.
  • Try to remove linters when multiple linters are specified on a line. This case is not common and creates some trickiness around handling of commas and the case when all linters are removed. It's simpler just to ignore it.

Note that fixes may leave trailing spaces or empty lines behind. Presumably, this would be cleaned up by gofmt in most cases which could also fix these cases. I'm not sure whether it's really possible or necessary to try to remove such spaces. It would be nice to at least remove empty lines if there is a way to identify if a comment is the only thing on a line, but I'm not sure how to do that just given the AST. One possibility might be to give the fixer a hint to clean up trailing whitespace or remove blank lines. That could potentially be useful for other linters as well.

Fixes #1579
Fixes #1580
Fixes #1581
Related to #1573

func nolintlint() {
run() // nolint:bob // leading space should be dropped
run() // nolint:bob // leading spaces should be dropped
fmt.Println() // nolint:bob // leading space should be dropped
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just changed this to something that doesn't trigger other linter errors

@ashanbrown ashanbrown changed the title Allow multiple linters to be removed from nolint statement by nolint … Allow multiple linters to be removed from nolint by nolint fixer Dec 27, 2020
@ashanbrown ashanbrown force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch from 83e14f8 to ba47cb7 Compare December 27, 2020 18:37
@ldez

This comment has been minimized.

@ashanbrown ashanbrown force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch 3 times, most recently from 141b43f to 7e85d7d Compare December 27, 2020 18:51
@ldez ldez added the bug Something isn't working label Dec 27, 2020
@ashanbrown

This comment has been minimized.

@ashanbrown ashanbrown marked this pull request as ready for review December 27, 2020 18:57
if issue.ExpectNoLint {
if issue.ExpectedNoLintLinter != "" {
return i.matchedIssueFromLinter[issue.ExpectedNoLintLinter]
// only allow selective nolinting of nolintlint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been reorganized so that we can do //nolint:nolintlint to disable nolintlint checking, having it behave just like every other linter. The one exception is that a blanket //nolint directive does not disable the nolintlint linter.

@ldez ldez self-requested a review December 27, 2020 19:28
@ldez

This comment has been minimized.

@ashanbrown ashanbrown force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch from 496a54b to dcd5924 Compare December 27, 2020 19:49
@ashanbrown ashanbrown changed the title Allow multiple linters to be removed from nolint by nolint fixer Allow nolintlint to fix linter rules Dec 27, 2020
@ashanbrown ashanbrown force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch from dcd5924 to 7c992af Compare December 27, 2020 19:51
@ashanbrown ashanbrown changed the title Allow nolintlint to fix linter rules Allow nolintlint to fix linter rules (take 2) Dec 27, 2020
@ashanbrown ashanbrown changed the title Allow nolintlint to fix linter rules (take 2) Allow nolintlint to fix //nolint lines (take 2) Dec 27, 2020
@@ -141,19 +146,14 @@ func (p *Nolint) buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, fil

func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
nolintDebugf("got issue: %v", *i)
if i.FromLinter == golinters.NolintlintName {
// always pass nolintlint issues except ones trying find unused nolint directives
if !i.ExpectNoLint {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we are allowing nolint directives to be excluded by //nolint:nolintlint.

@ashanbrown ashanbrown added linter: update version Update version of linter feedback required Requires additional feedback and removed bug Something isn't working labels Dec 28, 2020
@ashanbrown
Copy link
Contributor Author

@ldez I would love it if you could take a look at this re-attempt to add a //nolint fixer. I believe it addresses all the issues you pointed out with the original fixer. Thank you.

@ldez ldez changed the title Allow nolintlint to fix //nolint lines (take 2) Allow nolintlint to fix //nolint lines Jan 6, 2021
@ldez ldez removed the feedback required Requires additional feedback label Jan 6, 2021
@ldez ldez force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch from c133cbc to d0f91d0 Compare January 6, 2021 13:40
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.

I confirm that #1579, #1580, #1581 are fixed.


Trailing comma after the fix

one unused linter (begin) [OK]
// wantedErrors parses expected errors from comments in a file.
//nolint:gocyclo,nakedret
func wantedErrors(file, short string) (errs []wantedError) {
// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret
func wantedErrors(file, short string) (errs []wantedError) {
one unused linter (end) [KO]
// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret,gocyclo
func wantedErrors(file, short string) (errs []wantedError) {
// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret,
func wantedErrors(file, short string) (errs []wantedError) {
two unused linter (begin) [OK]
// wantedErrors parses expected errors from comments in a file.
//nolint:gocyclo,dupl,nakedret
func wantedErrors(file, short string) (errs []wantedError) {
// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret
func wantedErrors(file, short string) (errs []wantedError) {
two unused linter (end) [KO]
// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret,gocyclo,dupl
func wantedErrors(file, short string) (errs []wantedError) {
// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret,
func wantedErrors(file, short string) (errs []wantedError) {

Trailing two dot after the fix

only unused linters [KO]
// wantedErrors parses expected errors from comments in a file.
//nolint:gocyclo,dupl
func wantedErrors(file, short string) (errs []wantedError) {
// wantedErrors parses expected errors from comments in a file.
//nolint:
func wantedErrors(file, short string) (errs []wantedError) {
test/errchk.go:163:1: directive `//nolint:` should match `//nolint:[:<comma-separated-linters>] [// <explanation>]` (nolintlint)
//nolint:
^

The fact to not have a homogeneous behavior depending on the position of the unused linter is not expected for me.

In some cases (ex: only unused linters), the auto-fix produces an error

I think that the trailing commas must handle by this PR.

Also when the auto-fix will produce an empty //nolint:, I think that the fix must not be applied, and Replacement{NeedOnlyDelete: true} can be better here.

@ashanbrown
Copy link
Contributor Author

@ldez Thanks for looking at this more. I'm concerned trying to completely cleanly handle the cases with multiple linters specified with //nolint will make this PR significantly more complicated and I'd prefer to handle that case in a subsequent PR. I think these issues could be fixed up by hand after running fix. The nolintlint linter itself will catch the failing cases, preventing them from being committed. I believe that, due to the possibility of conflicting fixes between different linters, we do not guarantee that "fix" always fixes everything. If leaving the cases you mention as TODO is ok with you, I'd prefer to merge this as is. Note that I did this work during my xmas break and I likely won't have more time to spend on this for a few weeks. That said, if you'd care to try to address those issues before merging, please feel welcome. I'd be happy to review your changes.

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.

@ashanbrown could you clean a bit your PR? I think you made a wrong rebase 😉

@ashanbrown ashanbrown closed this Feb 20, 2021
@ashanbrown ashanbrown force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch from 6723032 to eace6a1 Compare February 20, 2021 22:08
@ashanbrown ashanbrown deleted the asb/allow-multiple-nolint-linters-to-be-removed branch February 20, 2021 22:13
@ashanbrown ashanbrown restored the asb/allow-multiple-nolint-linters-to-be-removed branch February 20, 2021 22:13
@ashanbrown ashanbrown reopened this Feb 20, 2021
@ashanbrown ashanbrown force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch 2 times, most recently from 6723032 to c69dde5 Compare February 20, 2021 22:19
@ashanbrown
Copy link
Contributor Author

@ldez I think I've fixed it up. The one change since last time is to just skip trying to fix unused linter issues for directives with multiple linters specified.

@ashanbrown ashanbrown requested a review from ldez February 27, 2021 14:30
@ashanbrown
Copy link
Contributor Author

@ldez Tests are passing now. I hadn't fixed them correctly after merging the latest code and needed to add another expected_linter field.

ashanbrown and others added 4 commits March 13, 2021 04:13
Repair whitespace issues and remove nolint statements that aren't being triggered
@ldez ldez force-pushed the asb/allow-multiple-nolint-linters-to-be-removed branch from 3c960f1 to d7db174 Compare March 13, 2021 03:50
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.

To clarify the scope of the PR because the scope of the PR has changed since it was open: this PR only fixes the cases where only one linter is defined in the nolint directive and this only one linter is unused.

@ldez ldez changed the title Allow nolintlint to fix //nolint lines nolintlint: allow to fix //nolint lines Mar 13, 2021
@ldez ldez merged commit e1a734e into golangci:master Mar 13, 2021
@ldez ldez added this to the v1.39 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: update version Update version of linter
Projects
None yet
2 participants