dev: new processor order (#5322)

Author: ldez

Diff for: pkg/fsutils/fsutils.go

@@ -34,13 +34,13 @@ func Getwd() (string, error) {
 			return
 		}
 
-		evaledWd, err := EvalSymlinks(cachedWd)
+		evaluatedWd, err := EvalSymlinks(cachedWd)
 		if err != nil {
 			cachedWd, cachedWdError = "", fmt.Errorf("can't eval symlinks on wd %s: %w", cachedWd, err)
 			return
 		}
 
-		cachedWd = evaledWd
+		cachedWd = evaluatedWd
 	})
 
 	return cachedWd, cachedWdError
@@ -83,8 +83,8 @@ func ShortestRelPath(path, wd string) (string, error) {
 	path = evaledPath
 
 	// make path absolute and then relative to be able to fix this case:
-	// we are in /test dir, we want to normalize ../test, and have file file.go in this dir;
-	// it must have normalized path file.go, not ../test/file.go,
+	// we are in `/test` dir, we want to normalize `../test`, and have file `file.go` in this dir;
+	// it must have normalized path `file.go`, not `../test/file.go`,
 	var absPath string
 	if filepath.IsAbs(path) {
 		absPath = path

Diff for: pkg/fsutils/fsutils_test.go

@@ -0,0 +1,42 @@
+package fsutils
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestShortestRelPath(t *testing.T) {
+	testCases := []struct {
+		desc     string
+		path     string
+		wd       string
+		expected string
+	}{
+		{
+			desc:     "based on parent path",
+			path:     "fsutils_test.go",
+			wd:       filepath.Join("..", "fsutils"),
+			expected: "fsutils_test.go",
+		},
+		{
+			desc:     "based on current working directory",
+			path:     "fsutils_test.go",
+			wd:       "",
+			expected: "fsutils_test.go",
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+
+			rel, err := ShortestRelPath("fsutils_test.go", filepath.Join("..", "fsutils"))
+			require.NoError(t, err)
+
+			assert.Equal(t, test.expected, rel)
+		})
+	}
+}

Diff for: pkg/lint/runner.go

@@ -79,10 +79,12 @@ func NewRunner(log logutils.Log, cfg *config.Config, args []string, goenv *gouti
 			// Must be after FilenameUnadjuster.
 			processors.NewInvalidIssue(log.Child(logutils.DebugKeyInvalidIssue)),
 
-			// Must be before diff, nolint and exclude autogenerated processor at least.
+			// Must be before Diff, SkipFiles, SkipDirs, ExcludeRules processors at least.
 			processors.NewPathPrettifier(log),
+
+			// must be after PathPrettifier.
 			skipFilesProcessor,
-			skipDirsProcessor, // must be after path prettifier
+			skipDirsProcessor,
 
 			processors.NewAutogeneratedExclude(cfg.Issues.ExcludeGenerated),
 
@@ -94,20 +96,21 @@ func NewRunner(log logutils.Log, cfg *config.Config, args []string, goenv *gouti
 
 			processors.NewNolint(log.Child(logutils.DebugKeyNolint), dbManager, enabledLinters),
 
-			processors.NewUniqByLine(cfg),
 			processors.NewDiff(&cfg.Issues),
+
+			// The fixer still needs to see paths for the issues that are relative to the current directory.
+			processors.NewFixer(cfg, log, fileCache, metaFormatter),
+
+			// Must be after the Fixer.
+			processors.NewUniqByLine(cfg),
 			processors.NewMaxPerFileFromLinter(cfg),
 			processors.NewMaxSameIssues(cfg.Issues.MaxSameIssues, log.Child(logutils.DebugKeyMaxSameIssues), cfg),
 			processors.NewMaxFromLinter(cfg.Issues.MaxIssuesPerLinter, log.Child(logutils.DebugKeyMaxFromLinter), cfg),
 
+			// Now we can modify the issues for output.
 			processors.NewSourceCode(lineCache, log.Child(logutils.DebugKeySourceCode)),
 			processors.NewPathShortener(),
 			processors.NewSeverity(log.Child(logutils.DebugKeySeverityRules), files, &cfg.Severity),
-
-			// The fixer still needs to see paths for the issues that are relative to the current directory.
-			processors.NewFixer(cfg, log, fileCache, metaFormatter),
-
-			// Now we can modify the issues for output.
 			processors.NewPathPrefixer(cfg.Output.PathPrefix),
 			processors.NewSortResults(cfg),
 		},

Diff for: pkg/result/processors/autogenerated_exclude.go

@@ -39,6 +39,10 @@ type fileSummary struct {
 	generated bool
 }
 
+// AutogeneratedExclude filters generated files.
+//   - mode "lax": see `isGeneratedFileLax` documentation.
+//   - mode "strict": see `isGeneratedFileStrict` documentation.
+//   - mode "disable": skips this processor.
 type AutogeneratedExclude struct {
 	debugf logutils.DebugFunc
 

Diff for: pkg/result/processors/cgo.go

@@ -10,10 +10,11 @@ import (
 
 var _ Processor = (*Cgo)(nil)
 
-// Cgo some linters (e.g. gosec, deadcode) return incorrect filepaths for cgo issues,
-// also cgo files have strange issues looking like false positives.
+// Cgo filters cgo artifacts.
 //
-// Require absolute filepath.
+// Some linters (e.g. gosec, etc.) return incorrect file paths for cgo files.
+//
+// Require absolute file path.
 type Cgo struct {
 	goCacheDir string
 }

Diff for: pkg/result/processors/diff.go

@@ -18,6 +18,12 @@ const envGolangciDiffProcessorPatch = "GOLANGCI_DIFF_PROCESSOR_PATCH"
 
 var _ Processor = (*Diff)(nil)
 
+// Diff filters issues based on options `new`, `new-from-rev`, etc.
+//
+// Uses `git`.
+// The paths inside the patch are relative to the path where git is run (the same location where golangci-lint is run).
+//
+// Warning: it doesn't use `path-prefix` option.
 type Diff struct {
 	onlyNew       bool
 	fromRev       string

Diff for: pkg/result/processors/exclude.go

@@ -11,6 +11,7 @@ import (
 
 var _ Processor = (*Exclude)(nil)
 
+// Exclude filters reports only based on regular expressions applied to the report text.
 type Exclude struct {
 	name string
 

Diff for: pkg/result/processors/exclude_rules.go

@@ -15,6 +15,13 @@ type excludeRule struct {
 	baseRule
 }
 
+// ExcludeRules filters reports based on multiple criteria:
+//   - linter names (string)
+//   - file path (regular expressions)
+//   - text (regular expressions)
+//   - code source (regular expressions)
+//
+// It uses the shortest relative paths and `path-prefix` option.
 type ExcludeRules struct {
 	name string
 

Diff for: pkg/result/processors/filename_unadjuster.go

@@ -22,11 +22,13 @@ type adjustMap struct {
 	m map[string]posMapper
 }
 
-// FilenameUnadjuster is needed because a lot of linters use `fset.Position(f.Pos())` to get filename.
-// And they return adjusted filename (e.g.` *.qtpl`) for an issue.
+// FilenameUnadjuster fixes filename based on adjusted and unadjusted position (related to line directives and cgo).
+//
+// A lot of linters use `fset.Position(f.Pos())` to get filename,
+// and they return adjusted filename (e.g.` *.qtpl`) for an issue.
 // We need restore real `.go` filename to properly output it, parse it, etc.
 //
-// Require absolute filepath.
+// Require absolute file path.
 type FilenameUnadjuster struct {
 	m                   map[string]posMapper // map from adjusted filename to position mapper: adjusted -> unadjusted position
 	log                 logutils.Log

Diff for: pkg/result/processors/fixer.go

@@ -31,6 +31,8 @@ var _ Processor = (*Fixer)(nil)
 
 const filePerm = 0644
 
+// Fixer fixes reports if possible.
+// The reports that are not fixed are passed to the next processor.
 type Fixer struct {
 	cfg       *config.Config
 	log       logutils.Log

Diff for: pkg/result/processors/identifier_marker.go

@@ -13,6 +13,9 @@ type replacePattern struct {
 	repl string
 }
 
+// IdentifierMarker modifies report text.
+// It must be before [Exclude] and [ExcludeRules]:
+// users configure exclusions based on the modified text.
 type IdentifierMarker struct {
 	patterns map[string][]replacePattern
 }

Diff for: pkg/result/processors/invalid_issue.go

@@ -9,6 +9,9 @@ import (
 
 var _ Processor = (*InvalidIssue)(nil)
 
+// InvalidIssue filters invalid reports.
+//   - non-go files (except `go.mod`)
+//   - reports without file path
 type InvalidIssue struct {
 	log logutils.Log
 }

Diff for: pkg/result/processors/max_from_linter.go

@@ -8,6 +8,7 @@ import (
 
 var _ Processor = (*MaxFromLinter)(nil)
 
+// MaxFromLinter limits the number of reports from the same linter.
 type MaxFromLinter struct {
 	linterCounter map[string]int
 	limit         int
@@ -34,11 +35,6 @@ func (p *MaxFromLinter) Process(issues []result.Issue) ([]result.Issue, error) {
 	}
 
 	return filterIssuesUnsafe(issues, func(issue *result.Issue) bool {
-		if issue.SuggestedFixes != nil && p.cfg.Issues.NeedFix {
-			// we need to fix all issues at once => we need to return all of them
-			return true
-		}
-
 		p.linterCounter[issue.FromLinter]++ // always inc for stat
 
 		return p.linterCounter[issue.FromLinter] <= p.limit

Diff for: pkg/result/processors/max_per_file_from_linter.go

@@ -7,6 +7,7 @@ import (
 
 var _ Processor = (*MaxPerFileFromLinter)(nil)
 
+// MaxPerFileFromLinter limits the number of reports by file and by linter.
 type MaxPerFileFromLinter struct {
 	fileLinterCounter          fileLinterCounter
 	maxPerFileFromLinterConfig map[string]int

Diff for: pkg/result/processors/max_same_issues.go

@@ -10,6 +10,7 @@ import (
 
 var _ Processor = (*MaxSameIssues)(nil)
 
+// MaxSameIssues limits the number of reports with the same text.
 type MaxSameIssues struct {
 	textCounter map[string]int
 	limit       int
@@ -36,12 +37,8 @@ func (p *MaxSameIssues) Process(issues []result.Issue) ([]result.Issue, error) {
 	}
 
 	return filterIssuesUnsafe(issues, func(issue *result.Issue) bool {
-		if issue.SuggestedFixes != nil && p.cfg.Issues.NeedFix {
-			// we need to fix all issues at once => we need to return all of them
-			return true
-		}
-
 		p.textCounter[issue.Text]++ // always inc for stat
+
 		return p.textCounter[issue.Text] <= p.limit
 	}), nil
 }

Diff for: pkg/result/processors/nolint.go

@@ -64,6 +64,7 @@ type fileData struct {
 	ignoredRanges []ignoredRange
 }
 
+// Nolint filters and sorts reports related to `nolint` directives.
 type Nolint struct {
 	fileCache      map[string]*fileData
 	dbManager      *lintersdb.Manager

Diff for: pkg/result/processors/path_prefixer.go

@@ -7,7 +7,8 @@ import (
 
 var _ Processor = (*PathPrefixer)(nil)
 
-// PathPrefixer adds a customizable prefix to every output path
+// PathPrefixer adds a customizable path prefix to report file paths for user facing.
+// It uses the shortest relative paths and `path-prefix` option.
 type PathPrefixer struct {
 	prefix string
 }
@@ -24,11 +25,14 @@ func (*PathPrefixer) Name() string {
 
 // Process adds the prefix to each path
 func (p *PathPrefixer) Process(issues []result.Issue) ([]result.Issue, error) {
-	if p.prefix != "" {
-		for i := range issues {
-			issues[i].Pos.Filename = fsutils.WithPathPrefix(p.prefix, issues[i].Pos.Filename)
-		}
+	if p.prefix == "" {
+		return issues, nil
 	}
+
+	for i := range issues {
+		issues[i].Pos.Filename = fsutils.WithPathPrefix(p.prefix, issues[i].Pos.Filename)
+	}
+
 	return issues, nil
 }
 

Diff for: pkg/result/processors/path_prettifier.go

@@ -10,6 +10,7 @@ import (
 
 var _ Processor = (*PathPrettifier)(nil)
 
+// PathPrettifier modifies report file path with the shortest relative path.
 type PathPrettifier struct {
 	log logutils.Log
 }

Diff for: pkg/result/processors/path_shortener.go

@@ -10,6 +10,8 @@ import (
 
 var _ Processor = (*PathShortener)(nil)
 
+// PathShortener modifies text of the reports to reduce file path inside the text.
+// It uses the rooted path name corresponding to the current directory (`wd`).
 type PathShortener struct {
 	wd string
 }

Diff for: pkg/result/processors/severity.go

@@ -19,6 +19,10 @@ type severityRule struct {
 	severity string
 }
 
+// Severity modifies report severity.
+// It uses the same `baseRule` structure as [ExcludeRules] processor.
+//
+// Warning: it doesn't use `path-prefix` option.
 type Severity struct {
 	name string
 

Diff for: pkg/result/processors/skip_dirs.go

@@ -21,6 +21,8 @@ var StdExcludeDirRegexps = []string{
 	normalizePathRegex("builtin"),
 }
 
+// SkipDirs filters reports based on directory names.
+// It uses the shortest relative paths and `path-prefix` option.
 type skipStat struct {
 	pattern string
 	count   int

Diff for: pkg/result/processors/skip_files.go

@@ -10,6 +10,9 @@ import (
 
 var _ Processor = (*SkipFiles)(nil)
 
+// SkipFiles filters reports based on filename.
+//
+// It uses the shortest relative paths and `path-prefix` option.
 type SkipFiles struct {
 	patterns   []*regexp.Regexp
 	pathPrefix string

Diff for: pkg/result/processors/sort_results.go

@@ -10,11 +10,6 @@ import (
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
-// Base propose of this functionality to sort results (issues)
-// produced by various linters by analyzing code. We're achieving this
-// by sorting results.Issues using processor step, and chain based
-// rules that can compare different properties of the Issues struct.
-
 const (
 	orderNameFile     = "file"
 	orderNameLinter   = "linter"
@@ -31,6 +26,10 @@ var _ Processor = (*SortResults)(nil)
 
 type issueComparator func(a, b *result.Issue) int
 
+// SortResults sorts reports based on criteria:
+//   - file names, line numbers, positions
+//   - linter names
+//   - severity names
 type SortResults struct {
 	cmps map[string][]issueComparator
 

Diff for: pkg/result/processors/source_code.go

@@ -8,6 +8,13 @@ import (
 
 var _ Processor = (*SourceCode)(nil)
 
+// SourceCode modifies displayed information based on [result.Issue.GetLineRange()].
+//
+// This is used:
+//   - to display the "UnderLinePointer".
+//   - in some rare cases to display multiple lines instead of one (ex: `dupl`)
+//
+// It requires to use [fsutils.LineCache] ([fsutils.FileCache]) to get the file information before the fixes.
 type SourceCode struct {
 	lineCache *fsutils.LineCache
 	log       logutils.Log