From 6e7c317610eb0b78db2d1ce0debec0614f3b1bb7 Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Mon, 13 Jul 2020 00:35:08 +0300 Subject: [PATCH] Sorting result.Issues implementation (golangci/golangci-lint#1217) (#1218) --- pkg/commands/run.go | 1 + pkg/config/config.go | 1 + pkg/lint/runner.go | 1 + pkg/result/processors/sort_results.go | 173 ++++++++++++++++++++ pkg/result/processors/sort_results_test.go | 182 +++++++++++++++++++++ test/run_test.go | 35 ++++ test/testdata/sort_results/main.go | 16 ++ 7 files changed, 409 insertions(+) create mode 100644 pkg/result/processors/sort_results.go create mode 100644 pkg/result/processors/sort_results_test.go create mode 100644 test/testdata/sort_results/main.go diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 519d4eb48ca7..92f0b175f0df 100644 --- a/pkg/commands/run.go +++ b/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 diff --git a/pkg/config/config.go b/pkg/config/config.go index b7704f739934..69178580d2b7 100644 --- a/pkg/config/config.go +++ b/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"` } diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 14baecc06c01..084912226b63 100644 --- a/pkg/lint/runner.go +++ b/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 diff --git a/pkg/result/processors/sort_results.go b/pkg/result/processors/sort_results.go new file mode 100644 index 000000000000..e726c3adfe05 --- /dev/null +++ b/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 +} diff --git a/pkg/result/processors/sort_results_test.go b/pkg/result/processors/sort_results_test.go new file mode 100644 index 000000000000..f1d05da4c28d --- /dev/null +++ b/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) +} diff --git a/test/run_test.go b/test/run_test.go index 739d1dea3068..c9bd7a801b0e 100644 --- a/test/run_test.go +++ b/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")) diff --git a/test/testdata/sort_results/main.go b/test/testdata/sort_results/main.go new file mode 100644 index 000000000000..2d7c3d38869f --- /dev/null +++ b/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() +}