nolintlint: allow to fix //nolint lines

[ashanbrown]

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

[ashanbrown]

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

[ashanbrown]

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.

[ashanbrown]

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

[ashanbrown]

@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]

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]

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

[ldez]

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

[ashanbrown]

@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]

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

[ldez]

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.