Sorting result.Issues implementation (golangci/golangci-lint#1217) (#1218)

pkg/commands/run.go

@@ -80,6 +80,7 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager, is
 	fs.BoolVar(&oc.PrintIssuedLine, "print-issued-lines", true, wh("Print lines of code with issue"))
 	fs.BoolVar(&oc.PrintLinterName, "print-linter-name", true, wh("Print linter name in issue line"))
 	fs.BoolVar(&oc.UniqByLine, "uniq-by-line", true, wh("Make issues output unique by line"))
+	fs.BoolVar(&oc.SortResults, "sort-results", false, wh("Sort linter results"))
 	fs.BoolVar(&oc.PrintWelcomeMessage, "print-welcome", false, wh("Print welcome message"))
 	fs.StringVar(&oc.PathPrefix, "path-prefix", "", wh("Path prefix to add to output"))
 	hideFlag("print-welcome") // no longer used

pkg/config/config.go

@@ -527,6 +527,7 @@ type Config struct {
 		PrintIssuedLine     bool   `mapstructure:"print-issued-lines"`
 		PrintLinterName     bool   `mapstructure:"print-linter-name"`
 		UniqByLine          bool   `mapstructure:"uniq-by-line"`
+		SortResults         bool   `mapstructure:"sort-results"`
 		PrintWelcomeMessage bool   `mapstructure:"print-welcome"`
 		PathPrefix          string `mapstructure:"path-prefix"`
 	}

pkg/lint/runner.go

@@ -80,6 +80,7 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint
 			processors.NewPathShortener(),
 			getSeverityRulesProcessor(&cfg.Severity, log, lineCache),
 			processors.NewPathPrefixer(cfg.Output.PathPrefix),
+			processors.NewSortResults(cfg),
 		},
 		Log: log,
 	}, nil

pkg/result/processors/sort_results.go

@@ -0,0 +1,173 @@
+package processors
+
+import (
+	"sort"
+	"strings"
+
+	"github.com/golangci/golangci-lint/pkg/config"
+	"github.com/golangci/golangci-lint/pkg/result"
+)
+
+// Base propose of this functionality to sort results (issues)
+// produced by various linters by analyzing code. We achieving this
+// by sorting results.Issues using processor step, and chain based
+// rules that can compare different properties of the Issues struct.
+
+var _ Processor = (*SortResults)(nil)
+
+type SortResults struct {
+	cmp comparator
+	cfg *config.Config
+}
+
+func NewSortResults(cfg *config.Config) *SortResults {
+	// For sorting we are comparing (in next order): file names, line numbers,
+	// position, and finally - giving up.
+	return &SortResults{
+		cmp: ByName{
+			next: ByLine{
+				next: ByColumn{},
+			},
+		},
+		cfg: cfg,
+	}
+}
+
+// Process is performing sorting of the result issues.
+func (sr SortResults) Process(issues []result.Issue) ([]result.Issue, error) {
+	if !sr.cfg.Output.SortResults {
+		return issues, nil
+	}
+
+	sort.Slice(issues, func(i, j int) bool {
+		return sr.cmp.Compare(&issues[i], &issues[j]) == Less
+	})
+
+	return issues, nil
+}
+
+func (sr SortResults) Name() string { return "sort_results" }
+func (sr SortResults) Finish()      {}
+
+type compareResult int
+
+const (
+	Less compareResult = iota - 1
+	Equal
+	Greater
+	None
+)
+
+func (c compareResult) isNeutral() bool {
+	// return true if compare result is incomparable or equal.
+	return c == None || c == Equal
+}
+
+//nolint:exhaustive
+func (c compareResult) String() string {
+	switch c {
+	case Less:
+		return "Less"
+	case Equal:
+		return "Equal"
+	case Greater:
+		return "Greater"
+	}
+
+	return "None"
+}
+
+// comparator describe how to implement compare for two "issues" lexicographically
+type comparator interface {
+	Compare(a, b *result.Issue) compareResult
+	Next() comparator
+}
+
+var (
+	_ comparator = (*ByName)(nil)
+	_ comparator = (*ByLine)(nil)
+	_ comparator = (*ByColumn)(nil)
+)
+
+type ByName struct{ next comparator }
+
+//nolint:golint
+func (cmp ByName) Next() comparator { return cmp.next }
+
+//nolint:golint
+func (cmp ByName) Compare(a, b *result.Issue) compareResult {
+	var res compareResult
+
+	if res = compareResult(strings.Compare(a.FilePath(), b.FilePath())); !res.isNeutral() {
+		return res
+	}
+
+	if next := cmp.Next(); next != nil {
+		return next.Compare(a, b)
+	}
+
+	return res
+}
+
+type ByLine struct{ next comparator }
+
+//nolint:golint
+func (cmp ByLine) Next() comparator { return cmp.next }
+
+//nolint:golint
+func (cmp ByLine) Compare(a, b *result.Issue) compareResult {
+	var res compareResult
+
+	if res = numericCompare(a.Line(), b.Line()); !res.isNeutral() {
+		return res
+	}
+
+	if next := cmp.Next(); next != nil {
+		return next.Compare(a, b)
+	}
+
+	return res
+}
+
+type ByColumn struct{ next comparator }
+
+//nolint:golint
+func (cmp ByColumn) Next() comparator { return cmp.next }
+
+//nolint:golint
+func (cmp ByColumn) Compare(a, b *result.Issue) compareResult {
+	var res compareResult
+
+	if res = numericCompare(a.Column(), b.Column()); !res.isNeutral() {
+		return res
+	}
+
+	if next := cmp.Next(); next != nil {
+		return next.Compare(a, b)
+	}
+
+	return res
+}
+
+func numericCompare(a, b int) compareResult {
+	var (
+		isValuesInvalid  = a < 0 || b < 0
+		isZeroValuesBoth = a == 0 && b == 0
+		isEqual          = a == b
+		isZeroValueInA   = b > 0 && a == 0
+		isZeroValueInB   = a > 0 && b == 0
+	)
+
+	switch {
+	case isZeroValuesBoth || isEqual:
+		return Equal
+	case isValuesInvalid || isZeroValueInA || isZeroValueInB:
+		return None
+	case a > b:
+		return Greater
+	case a < b:
+		return Less
+	}
+
+	return Equal
+}

pkg/result/processors/sort_results_test.go

@@ -0,0 +1,182 @@
+package processors
+
+import (
+	"fmt"
+	"go/token"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/golangci/golangci-lint/pkg/config"
+	"github.com/golangci/golangci-lint/pkg/result"
+)
+
+var issues = []result.Issue{
+	{
+		Pos: token.Position{
+			Filename: "file_windows.go",
+			Column:   80,
+			Line:     10,
+		},
+	},
+	{
+		Pos: token.Position{
+			Filename: "file_linux.go",
+			Column:   70,
+			Line:     11,
+		},
+	},
+	{
+		Pos: token.Position{
+			Filename: "file_darwin.go",
+			Line:     12,
+		},
+	},
+	{
+		Pos: token.Position{
+			Filename: "file_darwin.go",
+			Column:   60,
+			Line:     10,
+		},
+	},
+}
+
+type compareTestCase struct {
+	a, b     result.Issue
+	expected compareResult
+}
+
+func testCompareValues(t *testing.T, cmp comparator, name string, tests []compareTestCase) {
+	t.Parallel()
+
+	for i := 0; i < len(tests); i++ {
+		test := tests[i]
+		t.Run(fmt.Sprintf("%s(%d)", name, i), func(t *testing.T) {
+			res := cmp.Compare(&test.a, &test.b)
+			assert.Equal(t, test.expected.String(), res.String())
+		})
+	}
+}
+
+func TestCompareByLine(t *testing.T) {
+	testCompareValues(t, ByLine{}, "Compare By Line", []compareTestCase{
+		{issues[0], issues[1], Less},    // 10 vs 11
+		{issues[0], issues[0], Equal},   // 10 vs 10
+		{issues[3], issues[3], Equal},   // 10 vs 10
+		{issues[0], issues[3], Equal},   // 10 vs 10
+		{issues[3], issues[2], Less},    // 10 vs 12
+		{issues[1], issues[1], Equal},   // 11 vs 11
+		{issues[1], issues[0], Greater}, // 11 vs 10
+		{issues[1], issues[2], Less},    // 11 vs 12
+		{issues[2], issues[3], Greater}, // 12 vs 10
+		{issues[2], issues[1], Greater}, // 12 vs 11
+		{issues[2], issues[2], Equal},   // 12 vs 12
+	})
+}
+
+func TestCompareByName(t *testing.T) { //nolint:dupl
+	testCompareValues(t, ByName{}, "Compare By Name", []compareTestCase{
+		{issues[0], issues[1], Greater}, // file_windows.go vs file_linux.go
+		{issues[1], issues[2], Greater}, // file_linux.go vs file_darwin.go
+		{issues[2], issues[3], Equal},   // file_darwin.go vs file_darwin.go
+		{issues[1], issues[1], Equal},   // file_linux.go vs file_linux.go
+		{issues[1], issues[0], Less},    // file_linux.go vs file_windows.go
+		{issues[3], issues[2], Equal},   // file_darwin.go vs file_darwin.go
+		{issues[2], issues[1], Less},    // file_darwin.go vs file_linux.go
+		{issues[0], issues[0], Equal},   // file_windows.go vs file_windows.go
+		{issues[2], issues[2], Equal},   // file_darwin.go vs file_darwin.go
+		{issues[3], issues[3], Equal},   // file_darwin.go vs file_darwin.go
+	})
+}
+
+func TestCompareByColumn(t *testing.T) { //nolint:dupl
+	testCompareValues(t, ByColumn{}, "Compare By Column", []compareTestCase{
+		{issues[0], issues[1], Greater}, // 80 vs 70
+		{issues[1], issues[2], None},    // 70 vs zero value
+		{issues[3], issues[3], Equal},   // 60 vs 60
+		{issues[2], issues[3], None},    // zero value vs 60
+		{issues[2], issues[1], None},    // zero value vs 70
+		{issues[1], issues[0], Less},    // 70 vs 80
+		{issues[1], issues[1], Equal},   // 70 vs 70
+		{issues[3], issues[2], None},    // vs zero value
+		{issues[2], issues[2], Equal},   // zero value vs zero value
+		{issues[1], issues[1], Equal},   // 70 vs 70
+	})
+}
+
+func TestCompareNested(t *testing.T) {
+	var cmp = ByName{
+		next: ByLine{
+			next: ByColumn{},
+		},
+	}
+
+	testCompareValues(t, cmp, "Nested Comparing", []compareTestCase{
+		{issues[1], issues[0], Less},    // file_linux.go vs file_windows.go
+		{issues[2], issues[1], Less},    // file_darwin.go vs file_linux.go
+		{issues[0], issues[1], Greater}, // file_windows.go vs file_linux.go
+		{issues[1], issues[2], Greater}, // file_linux.go vs file_darwin.go
+		{issues[3], issues[2], Less},    // file_darwin.go vs file_darwin.go, 10 vs 12
+		{issues[0], issues[0], Equal},   // file_windows.go vs file_windows.go
+		{issues[2], issues[3], Greater}, // file_darwin.go vs file_darwin.go, 12 vs 10
+		{issues[1], issues[1], Equal},   // file_linux.go vs file_linux.go
+		{issues[2], issues[2], Equal},   // file_darwin.go vs file_darwin.go
+		{issues[3], issues[3], Equal},   // file_darwin.go vs file_darwin.go
+	})
+}
+
+func TestNumericCompare(t *testing.T) {
+	var tests = []struct {
+		a, b     int
+		expected compareResult
+	}{
+		{0, 0, Equal},
+		{0, 1, None},
+		{1, 0, None},
+		{1, -1, None},
+		{-1, 1, None},
+		{1, 1, Equal},
+		{1, 2, Less},
+		{2, 1, Greater},
+	}
+
+	t.Parallel()
+
+	for i := 0; i < len(tests); i++ {
+		test := tests[i]
+		t.Run(fmt.Sprintf("%s(%d)", "Numeric Compare", i), func(t *testing.T) {
+			res := numericCompare(test.a, test.b)
+			assert.Equal(t, test.expected.String(), res.String())
+		})
+	}
+}
+
+func TestNoSorting(t *testing.T) {
+	var tests = make([]result.Issue, len(issues))
+	copy(tests, issues)
+
+	var sr = NewSortResults(&config.Config{})
+
+	results, err := sr.Process(tests)
+	assert.Equal(t, tests, results)
+	assert.Nil(t, err, nil)
+}
+
+func TestSorting(t *testing.T) {
+	var tests = make([]result.Issue, len(issues))
+	copy(tests, issues)
+
+	var expected = make([]result.Issue, len(issues))
+	expected[0] = issues[3]
+	expected[1] = issues[2]
+	expected[2] = issues[1]
+	expected[3] = issues[0]
+
+	var cfg = config.Config{}
+	cfg.Output.SortResults = true
+	var sr = NewSortResults(&cfg)
+
+	results, err := sr.Process(tests)
+	assert.Equal(t, results, expected)
+	assert.Nil(t, err, nil)
+}

test/run_test.go

@@ -126,6 +126,41 @@ func TestLineDirectiveProcessedFilesLiteLoading(t *testing.T) {
 	r.ExpectExitCode(exitcodes.IssuesFound).ExpectOutputEq(output + "\n")
 }
 
+func TestSortedResults(t *testing.T) {
+	var testCases = []struct {
+		opt  string
+		want string
+	}{
+		{
+			"--sort-results=false",
+			strings.Join([]string{
+				"testdata/sort_results/main.go:12:5: `db` is unused (deadcode)",
+				"testdata/sort_results/main.go:15:13: Error return value of `returnError` is not checked (errcheck)",
+				"testdata/sort_results/main.go:8:6: func `returnError` is unused (unused)",
+			}, "\n"),
+		},
+		{
+			"--sort-results=true",
+			strings.Join([]string{
+				"testdata/sort_results/main.go:8:6: func `returnError` is unused (unused)",
+				"testdata/sort_results/main.go:12:5: `db` is unused (deadcode)",
+				"testdata/sort_results/main.go:15:13: Error return value of `returnError` is not checked (errcheck)",
+			}, "\n"),
+		},
+	}
+
+	dir := getTestDataDir("sort_results")
+
+	t.Parallel()
+	for i := range testCases {
+		test := testCases[i]
+		t.Run(test.opt, func(t *testing.T) {
+			r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", test.opt, dir)
+			r.ExpectExitCode(exitcodes.IssuesFound).ExpectOutputEq(test.want + "\n")
+		})
+	}
+}
+
 func TestLineDirectiveProcessedFilesFullLoading(t *testing.T) {
 	r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config",
 		"--exclude-use-default=false", "-Egolint,govet", getTestDataDir("quicktemplate"))

test/testdata/sort_results/main.go

@@ -0,0 +1,16 @@
+package sortresults
+
+import (
+	"database/sql"
+	"errors"
+)
+
+func returnError() error {
+	return errors.New("sss")
+}
+
+var db *sql.DB
+
+func _() {
+	returnError()
+}