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.

Sign up for GitHub

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

Jump to bottom

nolintlint: allow to fix //nolint lines #1583

Merged
merged 4 commits into from Mar 13, 2021
Merged

nolintlint: allow to fix //nolint lines #1583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
432b252
Add fix option for nolintlint
ashanbrown Feb 20, 2021
1621d5d
Fix test
ashanbrown Feb 21, 2021
446db3f
Fix expected linter
ashanbrown Feb 27, 2021
d7db174
minor changes
ldez Mar 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/golinters/nolintlint.go
View file
Open in desktop
Expand Up @@ -72,6 +72,7 @@ func NewNoLintLint() *goanalysis.Linter {
Pos: i.Position(),
ExpectNoLint: expectNoLint,
ExpectedNoLintLinter: expectedNolintLinter,
Replacement: i.Replacement(),
}
res = append(res, goanalysis.NewIssue(issue, pass))
}
Expand Down
193 changes: 122 additions & 71 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Open in desktop
Expand Up @@ -8,18 +8,25 @@ 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
}

type ExtraLeadingSpace struct {
BaseIssue
}
Expand Down Expand Up @@ -85,7 +92,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 %s", i.ExpectedLinter)
details += fmt.Sprintf(" for linter %q", i.ExpectedLinter)
}
return details
}
Expand All @@ -100,6 +107,7 @@ type Issue interface {
Details() string
Position() token.Position
String() string
Replacement() *result.Replacement
}

type Needs uint
Expand All @@ -115,7 +123,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 @@ -143,96 +151,139 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
var issues []Issue

for _, node := range nodes {
if file, ok := node.(*ast.File); ok {
for _, c := range file.Comments {
for _, comment := range c.List {
if !commentPattern.MatchString(comment.Text) {
continue
}
file, ok := node.(*ast.File)
if !ok {
continue
}

// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)
for _, c := range file.Comments {
for _, comment := range c.List {
if !commentPattern.MatchString(comment.Text) {
continue
}

var leadingSpace string
if len(leadingSpaceMatches) > 0 {
leadingSpace = leadingSpaceMatches[1]
}
// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)

directiveWithOptionalLeadingSpace := comment.Text
if len(leadingSpace) > 0 {
split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//")
directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
}
var leadingSpace string
if len(leadingSpaceMatches) > 0 {
leadingSpace = leadingSpaceMatches[1]
}

base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: fset.Position(comment.Pos()),
}
directiveWithOptionalLeadingSpace := comment.Text
if len(leadingSpace) > 0 {
split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//")
directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
}

// check for, report and eliminate leading spaces so we can check for other issues
if len(leadingSpace) > 1 {
issues = append(issues, ExtraLeadingSpace{BaseIssue: base})
}
pos := fset.Position(comment.Pos())
end := fset.Position(comment.End())

if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
issues = append(issues, NotMachine{BaseIssue: base})
}
base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: pos,
}

fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
// check for, report and eliminate leading spaces so we can check for other issues
if len(leadingSpace) > 0 {
removeWhitespace := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column + 1,
Length: len(leadingSpace),
NewString: "",
},
}
if (l.needs & NeedsMachineOnly) != 0 {
issue := NotMachine{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issues = append(issues, issue)
} else if len(leadingSpace) > 1 {
issue := ExtraLeadingSpace{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
issues = append(issues, issue)
}
}

lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
if len(lintersText) > 0 {
lls := strings.Split(lintersText[1:], ",")
linters = make([]string, 0, len(lls))
for _, ll := range lls {
ll = strings.TrimSpace(ll)
if ll != "" {
linters = append(linters, ll)
}
fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
}

lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
var linterRange []result.Range
if len(lintersText) > 0 {
lls := strings.Split(lintersText, ",")
linters = make([]string, 0, len(lls))
rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:")
for i, ll := range lls {
rangeEnd := rangeStart + len(ll)
if i < len(lls)-1 {
rangeEnd++ // include trailing comma
}
trimmedLinterName := strings.TrimSpace(ll)
if trimmedLinterName != "" {
linters = append(linters, trimmedLinterName)
linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd})
}
rangeStart = rangeEnd
}
}

if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
}
if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
}
}

// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
if (l.needs & NeedsUnused) != 0 {
if len(linters) == 0 {
issues = append(issues, UnusedCandidate{BaseIssue: base})
} else {
for _, linter := range linters {
issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
}
}
// 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 (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
// otherwise, check if we are excluding all of the mentioned linters
for _, ll := range linters {
if !l.excludeByLinter[ll] { // if a linter does require explanation
needsExplanation = true
break
if len(linters) == 0 {
issue := UnusedCandidate{BaseIssue: base}
issue.replacement = removeNolintCompletely
issues = append(issues, issue)
} else {
for _, linter := range linters {
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
// only offer replacement if there is a single linter
// because of issues around commas and the possibility of all
// linters being removed
if len(linters) == 1 {
issue.replacement = removeNolintCompletely
}
issues = append(issues, issue)
}
}
}

if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})
if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
// otherwise, check if we are excluding all of the mentioned linters
for _, ll := range linters {
if !l.excludeByLinter[ll] { // if a linter does require explanation
needsExplanation = true
break
}
}

if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})
}
}
}
}
Expand Down