From 5cbde84900e2ca5dd30b220000384420c04f65eb Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 16:36:48 +0100 Subject: [PATCH 1/4] fix: filter invalid issues before other processors --- pkg/lint/runner.go | 3 + pkg/logutils/logutils.go | 1 + .../processors/autogenerated_exclude.go | 13 --- pkg/result/processors/invalid_issue.go | 50 ++++++++ pkg/result/processors/invalid_issue_test.go | 109 ++++++++++++++++++ pkg/result/processors/nolint.go | 20 ++-- 6 files changed, 170 insertions(+), 26 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 92045581b79a..b1d604c964d0 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 a5461817e6e7..e4bb98109db1 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 9f4092425f29..b5944cd1d0a7 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/invalid_issue.go b/pkg/result/processors/invalid_issue.go new file mode 100644 index 000000000000..74af435938ee --- /dev/null +++ b/pkg/result/processors/invalid_issue.go @@ -0,0 +1,50 @@ +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() == "" { + p.log.Warnf("no file path for 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 000000000000..a4bb38523f99 --- /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 48be85b6ad3e..df8e81495908 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) { From 7fd17e367a7cbfea0636e0b8fefac1b118d2024d Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 16:45:28 +0100 Subject: [PATCH 2/4] fix: ignore issue without file path from contextcheck --- pkg/result/processors/invalid_issue.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/result/processors/invalid_issue.go b/pkg/result/processors/invalid_issue.go index 74af435938ee..c2a349dbb4cb 100644 --- a/pkg/result/processors/invalid_issue.go +++ b/pkg/result/processors/invalid_issue.go @@ -33,7 +33,11 @@ func (p InvalidIssue) shouldPassIssue(issue *result.Issue) (bool, error) { } if issue.FilePath() == "" { - p.log.Warnf("no file path for issue: probably a bug inside the linter %q: %#v", issue.FromLinter, issue) + // contextcheck has a known bug https://github.com/kkHAIKE/contextcheck/issues/21 + if issue.FromLinter != "contextcheck" { + p.log.Warnf("no file path for issue: probably a bug inside the linter %q: %#v", issue.FromLinter, issue) + } + return false, nil } From 1a0f6c0ef79bc73c6aabf99c8364dd8db09db88b Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 16:47:20 +0100 Subject: [PATCH 3/4] chore: improve message --- pkg/result/processors/invalid_issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/result/processors/invalid_issue.go b/pkg/result/processors/invalid_issue.go index c2a349dbb4cb..a8cfef892087 100644 --- a/pkg/result/processors/invalid_issue.go +++ b/pkg/result/processors/invalid_issue.go @@ -35,7 +35,7 @@ func (p InvalidIssue) shouldPassIssue(issue *result.Issue) (bool, error) { 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 issue: probably a bug inside the linter %q: %#v", issue.FromLinter, issue) + p.log.Warnf("no file path for the issue: probably a bug inside the linter %q: %#v", issue.FromLinter, issue) } return false, nil From 8df111266de5c96cceeff8eec89b906b68fc9302 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 16:50:46 +0100 Subject: [PATCH 4/4] tests: the cases are now handle by another processor --- .../processors/autogenerated_exclude_test.go | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/pkg/result/processors/autogenerated_exclude_test.go b/pkg/result/processors/autogenerated_exclude_test.go index c3291e4e3c2d..26cead18892a 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,