From 11d62312e8dc1f7ef39125c8e1c5dc7cb3f93546 Mon Sep 17 00:00:00 2001 From: "Andrew S. Brown" Date: Sun, 27 Dec 2020 11:37:51 -0800 Subject: [PATCH] Revert "Update nolintlint to fix nolint formatting and remove unused nolint statements (#1573)" This reverts commit aeb98303293570ba682ea933a4e9501a11a3aa99. --- pkg/golinters/nolintlint.go | 1 - pkg/golinters/nolintlint/nolintlint.go | 72 ++------- pkg/golinters/nolintlint/nolintlint_test.go | 154 +++----------------- pkg/result/issue.go | 2 +- pkg/result/processors/nolint.go | 12 +- test/testdata/fix/in/nolintlint.go | 13 -- test/testdata/fix/out/nolintlint.go | 13 -- test/testdata/nolintlint.go | 11 +- 8 files changed, 38 insertions(+), 240 deletions(-) delete mode 100644 test/testdata/fix/in/nolintlint.go delete mode 100644 test/testdata/fix/out/nolintlint.go diff --git a/pkg/golinters/nolintlint.go b/pkg/golinters/nolintlint.go index 889cff864c89..d98b50cf8a0c 100644 --- a/pkg/golinters/nolintlint.go +++ b/pkg/golinters/nolintlint.go @@ -72,7 +72,6 @@ func NewNoLintLint() *goanalysis.Linter { Pos: i.Position(), ExpectNoLint: expectNoLint, ExpectedNoLintLinter: expectedNolintLinter, - Replacement: i.Replacement(), } res = append(res, goanalysis.NewIssue(issue, pass)) } diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index ee2d5c13d89a..d20ac30f0951 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -8,21 +8,16 @@ import ( "regexp" "strings" "unicode" - - "github.com/golangci/golangci-lint/pkg/result" ) type BaseIssue struct { fullDirective string directiveWithOptionalLeadingSpace string position token.Position - replacement *result.Replacement } -func (b BaseIssue) Position() token.Position { return b.position } - -func (b BaseIssue) Replacement() *result.Replacement { - return b.replacement +func (b BaseIssue) Position() token.Position { + return b.position } type ExtraLeadingSpace struct { @@ -90,7 +85,7 @@ type UnusedCandidate struct { func (i UnusedCandidate) Details() string { details := fmt.Sprintf("directive `%s` is unused", i.fullDirective) if i.ExpectedLinter != "" { - details += fmt.Sprintf(" for linter %q", i.ExpectedLinter) + details += fmt.Sprintf(" for linter %s", i.ExpectedLinter) } return details } @@ -105,7 +100,6 @@ type Issue interface { Details() string Position() token.Position String() string - Replacement() *result.Replacement } type Needs uint @@ -121,7 +115,7 @@ const ( var commentPattern = regexp.MustCompile(`^//\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`) // matches a complete nolint directive -var fullDirectivePattern = regexp.MustCompile(`^//\s*nolint(?::(\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*))?\s*(//.*)?\s*\n?$`) +var fullDirectivePattern = regexp.MustCompile(`^//\s*nolint(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\s*(//.*)?\s*\n?$`) type Linter struct { excludes []string // lists individual linters that don't require explanations @@ -170,34 +164,19 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1]) } - pos := fset.Position(comment.Pos()) - end := fset.Position(comment.End()) - base := BaseIssue{ fullDirective: comment.Text, directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace, - position: pos, + position: fset.Position(comment.Pos()), } // check for, report and eliminate leading spaces so we can check for other issues - if len(leadingSpace) > 0 { - machineReadableReplacement := &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: pos.Column - 1, - Length: len(leadingSpace) + 2, - NewString: "//", - }, - } + if len(leadingSpace) > 1 { + issues = append(issues, ExtraLeadingSpace{BaseIssue: base}) + } - if (l.needs & NeedsMachineOnly) != 0 { - issue := NotMachine{BaseIssue: base} - issue.BaseIssue.replacement = machineReadableReplacement - issues = append(issues, issue) - } else if len(leadingSpace) > 1 { - issue := ExtraLeadingSpace{BaseIssue: base} - issue.BaseIssue.replacement = machineReadableReplacement - issues = append(issues, issue) - } + if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 { + issues = append(issues, NotMachine{BaseIssue: base}) } fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) @@ -209,7 +188,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { lintersText, explanation := fullMatches[1], fullMatches[2] var linters []string if len(lintersText) > 0 { - lls := strings.Split(lintersText, ",") + lls := strings.Split(lintersText[1:], ",") linters = make([]string, 0, len(lls)) for _, ll := range lls { ll = strings.TrimSpace(ll) @@ -227,34 +206,11 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { // when detecting unused directives, we send all the directives through and filter them out in the nolint processor if (l.needs & NeedsUnused) != 0 { - removeNolintCompletely := &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: pos.Column - 1, - Length: end.Column - pos.Column, - NewString: "", - }, - } - if len(linters) == 0 { - issue := UnusedCandidate{BaseIssue: base} - issue.replacement = removeNolintCompletely - issues = append(issues, issue) + issues = append(issues, UnusedCandidate{BaseIssue: base}) } else { - for i, linter := range linters { - issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter} - replacement := removeNolintCompletely - if len(linters) > 1 { - otherLinters := append(append([]string(nil), linters[0:i]...), linters[i+1:]...) - replacement = &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:"), - Length: len(lintersText) - 1, - NewString: strings.Join(otherLinters, ","), - }, - } - } - issue.replacement = replacement - issues = append(issues, issue) + for _, linter := range linters { + issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}) } } } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index c2abf409ebe0..b4bf4fbac546 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -7,22 +7,16 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/golangci/golangci-lint/pkg/result" ) //nolint:funlen func TestNoLintLint(t *testing.T) { - type issueWithReplacement struct { - issue string - replacement *result.Replacement - } testCases := []struct { desc string needs Needs excludes []string contents string - expected []issueWithReplacement + expected []string }{ { desc: "when no explanation is provided", @@ -39,11 +33,11 @@ func foo() { good() //nolint // this is ok other() //nolintother }`, - expected: []issueWithReplacement{ - {"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", nil}, - {"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", nil}, - {"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", nil}, - {"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", nil}, + expected: []string{ + "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", + "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", + "directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", + "directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", }, }, { @@ -56,8 +50,8 @@ package bar //nolint // this is ok //nolint:dupl func foo() {}`, - expected: []issueWithReplacement{ - {"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil}, + expected: []string{ + "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", }, }, { @@ -82,9 +76,9 @@ func foo() { bad() //nolint bad() // nolint // because }`, - expected: []issueWithReplacement{ - {"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", nil}, - {"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", nil}, + expected: []string{ + "directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", + "directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", }, }, { @@ -97,17 +91,8 @@ func foo() { bad() // nolint good() //nolint }`, - expected: []issueWithReplacement{ - { - "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 8, - Length: 3, - NewString: "//", - }, - }, - }, + expected: []string{ + "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9", }, }, { @@ -119,17 +104,8 @@ func foo() { bad() // nolint good() // nolint }`, - expected: []issueWithReplacement{ - { - "directive `// nolint` should not have more than one leading space at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 8, - Length: 4, - NewString: "//", - }, - }, - }, + expected: []string{ + "directive `// nolint` should not have more than one leading space at testing.go:5:9", }, }, { @@ -143,8 +119,8 @@ func foo() { good() // nolint: linter1,linter2 good() // nolint: linter1, linter2 }`, - expected: []issueWithReplacement{ - {"directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9", nil}, //nolint:lll // this is a string + expected: []string{ + "directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9", //nolint:lll // this is a string }, }, { @@ -157,92 +133,6 @@ func foo() { // something else }`, }, - { - desc: "needs unused without specific linter generates replacement", - needs: NeedsUnused, - contents: ` -package bar - -func foo() { - bad() //nolint -}`, - expected: []issueWithReplacement{ - { - "directive `//nolint` is unused at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 8, - Length: 8, - NewString: "", - }, - }, - }, - }, - }, - { - desc: "needs unused with one specific linter generates replacement", - needs: NeedsUnused, - contents: ` -package bar - -func foo() { - bad() //nolint:somelinter -}`, - expected: []issueWithReplacement{ - { - "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 8, - Length: 19, - NewString: "", - }, - }, - }, - }, - }, - { - desc: "needs unused with multiple specific linter generates replacement for each linter", - needs: NeedsUnused, - contents: ` -package bar - -func foo() { - bad() //nolint:linter1,linter2,linter3 -}`, - expected: []issueWithReplacement{ - { - "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter1\" at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 17, - Length: 22, - NewString: "linter2,linter3", - }, - }, - }, - { - "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter2\" at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 17, - Length: 22, - NewString: "linter1,linter3", - }, - }, - }, - { - "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter3\" at testing.go:5:9", - &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: 17, - Length: 22, - NewString: "linter1,linter2", - }, - }, - }, - }, - }, } for _, test := range testCases { @@ -259,16 +149,12 @@ func foo() { actualIssues, err := linter.Run(fset, expr) require.NoError(t, err) - actualIssuesWithReplacements := make([]issueWithReplacement, 0, len(actualIssues)) + actualIssueStrs := make([]string, 0, len(actualIssues)) for _, i := range actualIssues { - actualIssuesWithReplacements = append(actualIssuesWithReplacements, issueWithReplacement{ - issue: i.String(), - replacement: i.Replacement(), - }) + actualIssueStrs = append(actualIssueStrs, i.String()) } - assert.ElementsMatch(t, test.expected, actualIssuesWithReplacements, - "expected %s \nbut got %s", test.expected, actualIssuesWithReplacements) + assert.ElementsMatch(t, test.expected, actualIssueStrs, "expected %s \nbut got %s", test.expected, actualIssues) }) } } diff --git a/pkg/result/issue.go b/pkg/result/issue.go index 707a2b17cd95..eafdbc4a958a 100644 --- a/pkg/result/issue.go +++ b/pkg/result/issue.go @@ -14,7 +14,7 @@ type Range struct { type Replacement struct { NeedOnlyDelete bool // need to delete all lines of the issue without replacement with new lines - NewLines []string // if NeedDelete is false it's the replacement lines + NewLines []string // is NeedDelete is false it's the replacement lines Inline *InlineFix } diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index e0fde921ac2c..9b292eda32ff 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -21,8 +21,7 @@ type ignoredRange struct { linters []string matchedIssueFromLinter map[string]bool result.Range - col int - originalRange *ignoredRange // pre-expanded range (used to match nolintlint issues) + col int } func (i *ignoredRange) doesMatch(issue *result.Issue) bool { @@ -164,11 +163,7 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) { for _, ir := range fd.ignoredRanges { if ir.doesMatch(i) { - nolintDebugf("found ignored range for issue %v: %v", i, ir) ir.matchedIssueFromLinter[i.FromLinter] = true - if ir.originalRange != nil { - ir.originalRange.matchedIssueFromLinter[i.FromLinter] = true - } return false, nil } } @@ -204,14 +199,9 @@ func (e *rangeExpander) Visit(node ast.Node) ast.Visitor { } expandedRange := *foundRange - // store the original unexpanded range for matching nolintlint issues - if expandedRange.originalRange == nil { - expandedRange.originalRange = foundRange - } if expandedRange.To < nodeEndLine { expandedRange.To = nodeEndLine } - nolintDebugf("found range is %v for node %#v [%d;%d], expanded range is %v", *foundRange, node, nodeStartLine, nodeEndLine, expandedRange) e.expandedRanges = append(e.expandedRanges, expandedRange) diff --git a/test/testdata/fix/in/nolintlint.go b/test/testdata/fix/in/nolintlint.go deleted file mode 100644 index 3187989320bf..000000000000 --- a/test/testdata/fix/in/nolintlint.go +++ /dev/null @@ -1,13 +0,0 @@ -//args: -Enolintlint -Elll -//config: linters-settings.nolintlint.allow-leading-space=false -package p - -func nolintlint() { - run() // nolint:bob // leading space should be dropped - run() // nolint:bob // leading spaces should be dropped - // note that the next lines will retain trailing whitespace when fixed - run() //nolint // nolint should be dropped - run() //nolint:lll // nolint should be dropped - run() //nolint:alice,lll,bob // enabled linter should be dropped - run() //nolint:alice,lll,bob,nolintlint // enabled linter should not be dropped when nolintlint is nolinted -} diff --git a/test/testdata/fix/out/nolintlint.go b/test/testdata/fix/out/nolintlint.go deleted file mode 100644 index 01b5333e8360..000000000000 --- a/test/testdata/fix/out/nolintlint.go +++ /dev/null @@ -1,13 +0,0 @@ -//args: -Enolintlint -Elll -//config: linters-settings.nolintlint.allow-leading-space=false -package p - -func nolintlint() { - run() //nolint:bob // leading space should be dropped - run() //nolint:bob // leading spaces should be dropped - // note that the next lines will retain trailing whitespace when fixed - run() - run() - run() //nolint:alice,bob // enabled linter should be dropped - run() //nolint:alice,lll,bob,nolintlint // enabled linter should not be dropped when nolintlint is nolinted -} diff --git a/test/testdata/nolintlint.go b/test/testdata/nolintlint.go index 66785e228ea8..2294acc7e0e1 100644 --- a/test/testdata/nolintlint.go +++ b/test/testdata/nolintlint.go @@ -1,7 +1,7 @@ -//args: -Enolintlint -Emisspell +//args: -Enolintlint //config: linters-settings.nolintlint.require-explanation=true //config: linters-settings.nolintlint.require-specific=true -//config: linters-settings.nolintlint.allow-leading-space=false +//config: linters-settings.nolintlint.allowing-leading-space=false package testdata import "fmt" @@ -10,11 +10,4 @@ func Foo() { fmt.Println("not specific") //nolint // ERROR "directive `.*` should mention specific linter such as `//nolint:my-linter`" fmt.Println("not machine readable") // nolint // ERROR "directive `.*` should be written as `//nolint`" fmt.Println("extra spaces") // nolint:deadcode // because // ERROR "directive `.*` should not have more than one leading space" - - // test expanded range - //nolint:misspell // deliberate misspelling to trigger nolintlint - func() { - mispell := true - fmt.Println(mispell) - }() }