Skip to content

Commit

Permalink
nolintlint: fix false positive. (#2013)
Browse files Browse the repository at this point in the history
  • Loading branch information
ldez committed May 31, 2021
1 parent 4c27b33 commit 6172338
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 9 deletions.
22 changes: 17 additions & 5 deletions pkg/golinters/nolintlint/nolintlint.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// nolintlint provides a linter to ensure that all //nolint directives are followed by explanations
// Package nolintlint provides a linter to ensure that all //nolint directives are followed by explanations
package nolintlint

import (
Expand All @@ -19,10 +19,12 @@ type BaseIssue struct {
replacement *result.Replacement
}

//nolint:gocritic // TODO must be change in the future.
func (b BaseIssue) Position() token.Position {
return b.position
}

//nolint:gocritic // TODO must be change in the future.
func (b BaseIssue) Replacement() *result.Replacement {
return b.replacement
}
Expand All @@ -31,64 +33,75 @@ type ExtraLeadingSpace struct {
BaseIssue
}

//nolint:gocritic // TODO must be change in the future.
func (i ExtraLeadingSpace) Details() string {
return fmt.Sprintf("directive `%s` should not have more than one leading space", i.fullDirective)
}

//nolint:gocritic // TODO must be change in the future.
func (i ExtraLeadingSpace) String() string { return toString(i) }

type NotMachine struct {
BaseIssue
}

//nolint:gocritic // TODO must be change in the future.
func (i NotMachine) Details() string {
expected := i.fullDirective[:2] + strings.TrimLeftFunc(i.fullDirective[2:], unicode.IsSpace)
return fmt.Sprintf("directive `%s` should be written without leading space as `%s`",
i.fullDirective, expected)
}

//nolint:gocritic // TODO must be change in the future.
func (i NotMachine) String() string { return toString(i) }

type NotSpecific struct {
BaseIssue
}

//nolint:gocritic // TODO must be change in the future.
func (i NotSpecific) Details() string {
return fmt.Sprintf("directive `%s` should mention specific linter such as `%s:my-linter`",
i.fullDirective, i.directiveWithOptionalLeadingSpace)
}

//nolint:gocritic // TODO must be change in the future.
func (i NotSpecific) String() string { return toString(i) }

type ParseError struct {
BaseIssue
}

//nolint:gocritic // TODO must be change in the future.
func (i ParseError) Details() string {
return fmt.Sprintf("directive `%s` should match `%s[:<comma-separated-linters>] [// <explanation>]`",
i.fullDirective,
i.directiveWithOptionalLeadingSpace)
}

//nolint:gocritic // TODO must be change in the future.
func (i ParseError) String() string { return toString(i) }

type NoExplanation struct {
BaseIssue
fullDirectiveWithoutExplanation string
}

//nolint:gocritic // TODO must be change in the future.
func (i NoExplanation) Details() string {
return fmt.Sprintf("directive `%s` should provide explanation such as `%s // this is why`",
i.fullDirective, i.fullDirectiveWithoutExplanation)
}

//nolint:gocritic // TODO must be change in the future.
func (i NoExplanation) String() string { return toString(i) }

type UnusedCandidate struct {
BaseIssue
ExpectedLinter string
}

//nolint:gocritic // TODO must be change in the future.
func (i UnusedCandidate) Details() string {
details := fmt.Sprintf("directive `%s` is unused", i.fullDirective)
if i.ExpectedLinter != "" {
Expand All @@ -97,6 +110,7 @@ func (i UnusedCandidate) Details() string {
return details
}

//nolint:gocritic // TODO must be change in the future.
func (i UnusedCandidate) String() string { return toString(i) }

func toString(i Issue) string {
Expand Down Expand Up @@ -126,8 +140,7 @@ var commentPattern = regexp.MustCompile(`^//\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-
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
needs Needs // indicates which linter checks to perform
needs Needs // indicates which linter checks to perform
excludeByLinter map[string]bool
}

Expand All @@ -147,6 +160,7 @@ func NewLinter(needs Needs, excludes []string) (*Linter, error) {
var leadingSpacePattern = regexp.MustCompile(`^//(\s*)`)
var trailingBlankExplanation = regexp.MustCompile(`\s*(//\s*)?$`)

//nolint:funlen,gocyclo
func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
var issues []Issue

Expand Down Expand Up @@ -214,7 +228,6 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {

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))
Expand All @@ -227,7 +240,6 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
trimmedLinterName := strings.TrimSpace(ll)
if trimmedLinterName != "" {
linters = append(linters, trimmedLinterName)
linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd})
}
rangeStart = rangeEnd
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/result/processors/nolint.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"go/ast"
"go/parser"
"go/token"
"regexp"
"sort"
"strings"

Expand Down Expand Up @@ -46,7 +47,7 @@ func (i *ignoredRange) doesMatch(issue *result.Issue) bool {

// handle possible unused nolint directives
// nolintlint generates potential issues for every nolint directive and they are filtered out here
if issue.FromLinter == golinters.NolintlintName && issue.ExpectNoLint {
if issue.FromLinter == golinters.NolintlintName && issue.ExpectNoLint {
if issue.ExpectedNoLintLinter != "" {
return i.matchedIssueFromLinter[issue.ExpectedNoLintLinter]
}
Expand Down Expand Up @@ -146,7 +147,6 @@ func (p *Nolint) buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, fil

func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
nolintDebugf("got issue: %v", *i)

if i.FromLinter == golinters.NolintlintName && i.ExpectNoLint && i.ExpectedNoLintLinter != "" {
// don't expect disabled linters to cover their nolint statements
nolintDebugf("enabled linters: %v", p.enabledLinters)
Expand Down Expand Up @@ -234,7 +234,7 @@ func (p *Nolint) extractFileCommentsInlineRanges(fset *token.FileSet, comments .

func (p *Nolint) extractInlineRangeFromComment(text string, g ast.Node, fset *token.FileSet) *ignoredRange {
text = strings.TrimLeft(text, "/ ")
if !strings.HasPrefix(text, "nolint") {
if ok, _ := regexp.MatchString(`^nolint( |:|$)`, text); !ok {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion test/testdata_etc/unused_exported/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ import (

func main() {
lib.PublicFunc()
}
}

0 comments on commit 6172338

Please sign in to comment.