fix: filter invalid issues before other processors (golangci#4552)

Author: ldez

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,

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"

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 {

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,

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
+}

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)
+		})
+	}
+}

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) {