Skip to content

Commit

Permalink
Revert "Update nolintlint to fix nolint formatting and remove unused …
Browse files Browse the repository at this point in the history 
…nolint statements (#1573)" (#1584)

This reverts commit aeb9830.

There are some cases that nolinter fixer wasn't handling properly or expectedly (#1579, #1580, #1581) so we'll fix those in a new attempt.
  • Loading branch information
@ashanbrown
ashanbrown authored Dec 27, 2020
1 parent 85049e5 commit 306816e
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 240 deletions.
1 change: 0 additions & 1 deletion pkg/golinters/nolintlint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
72 changes: 14 additions & 58 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand All @@ -105,7 +100,6 @@ type Issue interface {
Details() string
Position() token.Position
String() string
Replacement() *result.Replacement
}

type Needs uint
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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})
}
}
}
Expand Down
154 changes: 20 additions & 134 deletions pkg/golinters/nolintlint/nolintlint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
},
},
{
Expand All @@ -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",
},
},
{
Expand All @@ -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",
},
},
{
Expand All @@ -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",
},
},
{
Expand All @@ -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",
},
},
{
Expand All @@ -143,8 +119,8 @@ func foo() {
good() // nolint: linter1,linter2
good() // nolint: linter1, linter2
}`,
expected: []issueWithReplacement{
{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
expected: []string{
"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", //nolint:lll // this is a string
},
},
{
Expand All @@ -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 {
Expand All @@ -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)
})
}
}
2 changes: 1 addition & 1 deletion pkg/result/issue.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
Loading

0 comments on commit 306816e

Please sign in to comment.