Skip to content

Commit

Permalink
fix: nolintlint comment analysis. (#1571)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ldez
ldez authored Dec 22, 2020
1 parent a893212 commit be02979
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 142 deletions.
153 changes: 78 additions & 75 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type NotSpecific struct {
}

func (i NotSpecific) Details() string {
return fmt.Sprintf("directive `%s` should mention specific linter such as `//%s:my-linter`",
return fmt.Sprintf("directive `%s` should mention specific linter such as `%s:my-linter`",
i.fullDirective, i.directiveWithOptionalLeadingSpace)
}

Expand All @@ -58,7 +58,7 @@ type ParseError struct {
}

func (i ParseError) Details() string {
return fmt.Sprintf("directive `%s` should match `//%s[:<comma-separated-linters>] [// <explanation>]`",
return fmt.Sprintf("directive `%s` should match `%s[:<comma-separated-linters>] [// <explanation>]`",
i.fullDirective,
i.directiveWithOptionalLeadingSpace)
}
Expand Down Expand Up @@ -112,8 +112,7 @@ const (
NeedsAll = NeedsMachineOnly | NeedsSpecific | NeedsExplanation
)

// matches lines starting with the nolint directive
var directiveOnlyPattern = regexp.MustCompile(`^\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`)
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?$`)
Expand Down Expand Up @@ -142,98 +141,102 @@ var trailingBlankExplanation = regexp.MustCompile(`\s*(//\s*)?$`)

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 {
text := c.Text()
matches := directiveOnlyPattern.FindStringSubmatch(text)
if len(matches) == 0 {
continue
}
directive := matches[1]
for _, comment := range c.List {
if !commentPattern.MatchString(comment.Text) {
continue
}

// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(c.List[0].Text) // c.Text() doesn't have all leading space
if len(leadingSpaceMatches) == 0 {
continue
}
leadingSpace := leadingSpaceMatches[1]
// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)

directiveWithOptionalLeadingSpace := directive
if len(leadingSpace) > 0 {
directiveWithOptionalLeadingSpace = " " + directive
}
var leadingSpace string
if len(leadingSpaceMatches) > 0 {
leadingSpace = leadingSpaceMatches[1]
}

base := BaseIssue{
fullDirective: c.List[0].Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: fset.Position(c.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 leadingSpace != "" && leadingSpace != " " {
issues = append(issues, ExtraLeadingSpace{
BaseIssue: base,
})
}
base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: fset.Position(comment.Pos()),
}

if (l.needs&NeedsMachineOnly) != 0 && strings.HasPrefix(directiveWithOptionalLeadingSpace, " ") {
issues = append(issues, NotMachine{BaseIssue: base})
}
// check for, report and eliminate leading spaces so we can check for other issues
if len(leadingSpace) > 1 {
issues = append(issues, ExtraLeadingSpace{BaseIssue: base})
}

fullMatches := fullDirectivePattern.FindStringSubmatch(c.List[0].Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
}
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)
}
if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
issues = append(issues, NotMachine{BaseIssue: base})
}
}
if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})

fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
}
}

// 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})
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)
}
}
}
}

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 (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
}
}
if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(c.List[0].Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})

// 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})
}
}
}

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,
})
}
}
}
}
}
}

return issues, nil
}
Loading

0 comments on commit be02979

Please sign in to comment.