From cd890db217def69d68cf1862504bb735c0333879 Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Wed, 20 Mar 2024 17:25:22 +0100 Subject: [PATCH] fix: filter invalid issues before other processors (#4552) --- pkg/lint/runner.go | 3 + pkg/logutils/logutils.go | 1 + .../processors/autogenerated_exclude.go | 13 --- .../processors/autogenerated_exclude_test.go | 33 ------ pkg/result/processors/invalid_issue.go | 54 +++++++++ pkg/result/processors/invalid_issue_test.go | 109 ++++++++++++++++++ pkg/result/processors/nolint.go | 20 ++-- 7 files changed, 174 insertions(+), 59 deletions(-) create mode 100644 pkg/result/processors/invalid_issue.go create mode 100644 pkg/result/processors/invalid_issue_test.go diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 92045581..b1d604c9 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -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, diff --git a/pkg/logutils/logutils.go b/pkg/logutils/logutils.go index a5461817..e4bb9810 100644 --- a/pkg/logutils/logutils.go +++ b/pkg/logutils/logutils.go @@ -25,6 +25,7 @@ const ( DebugKeyExcludeRules = "exclude_rules" DebugKeyExec = "exec" DebugKeyFilenameUnadjuster = "filename_unadjuster" + DebugKeyInvalidIssue = "invalid_issue" DebugKeyForbidigo = "forbidigo" DebugKeyGoEnv = "goenv" DebugKeyLinter = "linter" diff --git a/pkg/result/processors/autogenerated_exclude.go b/pkg/result/processors/autogenerated_exclude.go index 9f409242..b5944cd1 100644 --- a/pkg/result/processors/autogenerated_exclude.go +++ b/pkg/result/processors/autogenerated_exclude.go @@ -1,7 +1,6 @@ package processors import ( - "errors" "fmt" "go/parser" "go/token" @@ -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 { diff --git a/pkg/result/processors/autogenerated_exclude_test.go b/pkg/result/processors/autogenerated_exclude_test.go index c3291e4e..26cead18 100644 --- a/pkg/result/processors/autogenerated_exclude_test.go +++ b/pkg/result/processors/autogenerated_exclude_test.go @@ -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, @@ -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, diff --git a/pkg/result/processors/invalid_issue.go b/pkg/result/processors/invalid_issue.go new file mode 100644 index 00000000..a8cfef89 --- /dev/null +++ b/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 +} diff --git a/pkg/result/processors/invalid_issue_test.go b/pkg/result/processors/invalid_issue_test.go new file mode 100644 index 00000000..a4bb3852 --- /dev/null +++ b/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) + }) + } +} diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index 48be85b6..df8e8149 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -1,7 +1,6 @@ package processors import ( - "errors" "go/ast" "go/parser" "go/token" @@ -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. @@ -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 { @@ -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) {