Skip to content

Commit

Permalink
fix: filter invalid issues before other processors (#4552)
Browse files Browse the repository at this point in the history
  • Loading branch information
ldez committed Mar 20, 2024
1 parent 921d535 commit cd890db
Show file tree
Hide file tree
Showing 7 changed files with 174 additions and 59 deletions.
3 changes: 3 additions & 0 deletions pkg/lint/runner.go
Expand Up @@ -68,6 +68,9 @@ func NewRunner(log logutils.Log, cfg *config.Config, args []string, goenv *gouti
// Must go after Cgo.
processors.NewFilenameUnadjuster(lintCtx.Packages, log.Child(logutils.DebugKeyFilenameUnadjuster)),

// Must go after FilenameUnadjuster.
processors.NewInvalidIssue(log.Child(logutils.DebugKeyInvalidIssue)),

// Must be before diff, nolint and exclude autogenerated processor at least.
processors.NewPathPrettifier(),
skipFilesProcessor,
Expand Down
1 change: 1 addition & 0 deletions pkg/logutils/logutils.go
Expand Up @@ -25,6 +25,7 @@ const (
DebugKeyExcludeRules = "exclude_rules"
DebugKeyExec = "exec"
DebugKeyFilenameUnadjuster = "filename_unadjuster"
DebugKeyInvalidIssue = "invalid_issue"
DebugKeyForbidigo = "forbidigo"
DebugKeyGoEnv = "goenv"
DebugKeyLinter = "linter"
Expand Down
13 changes: 0 additions & 13 deletions pkg/result/processors/autogenerated_exclude.go
@@ -1,7 +1,6 @@
package processors

import (
"errors"
"fmt"
"go/parser"
"go/token"
Expand Down Expand Up @@ -59,18 +58,6 @@ func (p *AutogeneratedExclude) shouldPassIssue(issue *result.Issue) (bool, error
return true, nil
}

if filepath.Base(issue.FilePath()) == "go.mod" {
return true, nil
}

if issue.FilePath() == "" {
return false, errors.New("no file path for issue")
}

if !isGoFile(issue.FilePath()) {
return false, nil
}

// The file is already known.
fs := p.fileSummaryCache[issue.FilePath()]
if fs != nil {
Expand Down
33 changes: 0 additions & 33 deletions pkg/result/processors/autogenerated_exclude_test.go
Expand Up @@ -166,28 +166,6 @@ func Test_shouldPassIssue(t *testing.T) {
},
assert: assert.True,
},
{
desc: "go.mod",
strict: false,
issue: &result.Issue{
FromLinter: "example",
Pos: token.Position{
Filename: filepath.FromSlash("/a/b/c/go.mod"),
},
},
assert: assert.True,
},
{
desc: "non Go file",
strict: false,
issue: &result.Issue{
FromLinter: "example",
Pos: token.Position{
Filename: filepath.FromSlash("/a/b/c/test.txt"),
},
},
assert: assert.False,
},
{
desc: "lax ",
strict: false,
Expand Down Expand Up @@ -239,17 +217,6 @@ func Test_shouldPassIssue_error(t *testing.T) {
issue *result.Issue
expected string
}{
{
desc: "missing Filename",
strict: false,
issue: &result.Issue{
FromLinter: "example",
Pos: token.Position{
Filename: "",
},
},
expected: "no file path for issue",
},
{
desc: "non-existing file (lax)",
strict: false,
Expand Down
54 changes: 54 additions & 0 deletions pkg/result/processors/invalid_issue.go
@@ -0,0 +1,54 @@
package processors

import (
"path/filepath"

"github.com/golangci/golangci-lint/pkg/logutils"
"github.com/golangci/golangci-lint/pkg/result"
)

var _ Processor = InvalidIssue{}

type InvalidIssue struct {
log logutils.Log
}

func NewInvalidIssue(log logutils.Log) *InvalidIssue {
return &InvalidIssue{log: log}
}

func (p InvalidIssue) Process(issues []result.Issue) ([]result.Issue, error) {
return filterIssuesErr(issues, p.shouldPassIssue)
}

func (p InvalidIssue) Name() string {
return "invalid_issue"
}

func (p InvalidIssue) Finish() {}

func (p InvalidIssue) shouldPassIssue(issue *result.Issue) (bool, error) {
if issue.FromLinter == "typecheck" {
return true, nil
}

if issue.FilePath() == "" {
// contextcheck has a known bug https://github.com/kkHAIKE/contextcheck/issues/21
if issue.FromLinter != "contextcheck" {
p.log.Warnf("no file path for the issue: probably a bug inside the linter %q: %#v", issue.FromLinter, issue)
}

return false, nil
}

if filepath.Base(issue.FilePath()) == "go.mod" {
return true, nil
}

if !isGoFile(issue.FilePath()) {
p.log.Infof("issue related to file %s is skipped", issue.FilePath())
return false, nil
}

return true, nil
}
109 changes: 109 additions & 0 deletions pkg/result/processors/invalid_issue_test.go
@@ -0,0 +1,109 @@
package processors

import (
"go/token"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/golangci/golangci-lint/pkg/logutils"
"github.com/golangci/golangci-lint/pkg/result"
)

func TestInvalidIssue_Process(t *testing.T) {
logger := logutils.NewStderrLog(logutils.DebugKeyInvalidIssue)
logger.SetLevel(logutils.LogLevelDebug)

p := NewInvalidIssue(logger)

testCases := []struct {
desc string
issues []result.Issue
expected []result.Issue
}{
{
desc: "typecheck",
issues: []result.Issue{
{FromLinter: "typecheck"},
},
expected: []result.Issue{
{FromLinter: "typecheck"},
},
},
{
desc: "Go file",
issues: []result.Issue{
{
FromLinter: "example",
Pos: token.Position{
Filename: "test.go",
},
},
},
expected: []result.Issue{
{
FromLinter: "example",
Pos: token.Position{
Filename: "test.go",
},
},
},
},
{
desc: "go.mod",
issues: []result.Issue{
{
FromLinter: "example",
Pos: token.Position{
Filename: "go.mod",
},
},
},
expected: []result.Issue{
{
FromLinter: "example",
Pos: token.Position{
Filename: "go.mod",
},
},
},
},
{
desc: "non Go file",
issues: []result.Issue{
{
FromLinter: "example",
Pos: token.Position{
Filename: "test.txt",
},
},
},
expected: []result.Issue{},
},
{
desc: "no filename",
issues: []result.Issue{
{
FromLinter: "example",
Pos: token.Position{
Filename: "",
},
},
},
expected: []result.Issue{},
},
}

for _, test := range testCases {
test := test
t.Run(test.desc, func(t *testing.T) {
t.Parallel()

after, err := p.Process(test.issues)
require.NoError(t, err)

assert.Equal(t, test.expected, after)
})
}
}
20 changes: 7 additions & 13 deletions pkg/result/processors/nolint.go
@@ -1,7 +1,6 @@
package processors

import (
"errors"
"go/ast"
"go/parser"
"go/token"
Expand Down Expand Up @@ -99,19 +98,15 @@ func (p *Nolint) Process(issues []result.Issue) ([]result.Issue, error) {
return filterIssuesErr(issues, p.shouldPassIssue)
}

func (p *Nolint) getOrCreateFileData(issue *result.Issue) (*fileData, error) {
func (p *Nolint) getOrCreateFileData(issue *result.Issue) *fileData {
fd := p.cache[issue.FilePath()]
if fd != nil {
return fd, nil
return fd
}

fd = &fileData{}
p.cache[issue.FilePath()] = fd

if issue.FilePath() == "" {
return nil, errors.New("no file path for issue")
}

// TODO: migrate this parsing to go/analysis facts
// or cache them somehow per file.

Expand All @@ -120,12 +115,14 @@ func (p *Nolint) getOrCreateFileData(issue *result.Issue) (*fileData, error) {
f, err := parser.ParseFile(fset, issue.FilePath(), nil, parser.ParseComments)
if err != nil {
// Don't report error because it's already must be reporter by typecheck or go/analysis.
return fd, nil
return fd
}

fd.ignoredRanges = p.buildIgnoredRangesForFile(f, fset, issue.FilePath())

nolintDebugf("file %s: built nolint ranges are %+v", issue.FilePath(), fd.ignoredRanges)
return fd, nil

return fd
}

func (p *Nolint) buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, filePath string) []ignoredRange {
Expand Down Expand Up @@ -161,10 +158,7 @@ func (p *Nolint) shouldPassIssue(issue *result.Issue) (bool, error) {
nolintDebugf("checking that lint issue was used for %s: %v", issue.ExpectedNoLintLinter, issue)
}

fd, err := p.getOrCreateFileData(issue)
if err != nil {
return false, err
}
fd := p.getOrCreateFileData(issue)

for _, ir := range fd.ignoredRanges {
if ir.doesMatch(issue) {
Expand Down

0 comments on commit cd890db

Please sign in to comment.