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.

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

nolintlint: fix false positive. #2013

Merged
merged 2 commits into from
May 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()
}
}