Skip to content

Commit

Permalink
nolintlint: allow to fix //nolint lines (#1583)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ashanbrown
ashanbrown committed Mar 13, 2021
1 parent d28c666 commit e1a734e
Show file tree
Hide file tree
Showing 10 changed files with 330 additions and 117 deletions.
1 change: 1 addition & 0 deletions pkg/golinters/nolintlint.go
View file
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
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

0 comments on commit e1a734e

Please sign in to comment.