From 3b322a7ad304b0a768a2e8be9b319384f245c4fe Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 16 Mar 2023 17:14:24 +0100 Subject: [PATCH 1/3] fix combination of --fix and --path-prefix This combination used to fail because the fixer was executed after the path prefixer. Then the file paths that it processed didn't match the current working directory, leading to: ERRO Failed to fix issues in file test/fix_sample/main.go: failed to get file bytes for test/fix_sample/main.go: can't read file test/fix_sample/main.go: open test/fix_sample/main.go: no such file or directory Making the fixer a normal processor and moving it before the path prefixer avoids this problem. --- pkg/commands/run.go | 9 +-------- pkg/fsutils/linecache.go | 2 ++ pkg/lint/runner.go | 3 +++ pkg/result/processors/fixer.go | 14 +++++++++++--- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/pkg/commands/run.go b/pkg/commands/run.go index a9971f5e02b2..753abbd52462 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -23,7 +23,6 @@ import ( "github.com/golangci/golangci-lint/pkg/packages" "github.com/golangci/golangci-lint/pkg/printers" "github.com/golangci/golangci-lint/pkg/result" - "github.com/golangci/golangci-lint/pkg/result/processors" ) const defaultFileMode = 0644 @@ -365,13 +364,7 @@ func (e *Executor) runAnalysis(ctx context.Context, args []string) ([]result.Iss return nil, err } - issues, err := runner.Run(ctx, lintersToRun, lintCtx) - if err != nil { - return nil, err - } - - fixer := processors.NewFixer(e.cfg, e.log, e.fileCache) - return fixer.Process(issues), nil + return runner.Run(ctx, lintersToRun, lintCtx) } func (e *Executor) setOutputToDevNull() (savedStdout, savedStderr *os.File) { diff --git a/pkg/fsutils/linecache.go b/pkg/fsutils/linecache.go index 2e92264846ef..64b411dfd72f 100644 --- a/pkg/fsutils/linecache.go +++ b/pkg/fsutils/linecache.go @@ -19,6 +19,8 @@ func NewLineCache(fc *FileCache) *LineCache { } } +func (lc *LineCache) GetFileCache() *FileCache { return lc.fileCache } + // GetLine returns the index1-th (1-based index) line from the file on filePath func (lc *LineCache) GetLine(filePath string, index1 int) (string, error) { if index1 == 0 { // some linters, e.g. gosec can do it: it really means first line diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 1d527711c0b6..0a3a281e791a 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -98,6 +98,9 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint processors.NewSourceCode(lineCache, log.Child(logutils.DebugKeySourceCode)), processors.NewPathShortener(), getSeverityRulesProcessor(&cfg.Severity, log, files), + // The fixer still needs to see paths for the issues that are relative to the current directory. + processors.NewFixer(cfg, log, lineCache.GetFileCache()), + // Now we can modify the issues for output. processors.NewPathPrefixer(cfg.Output.PathPrefix), processors.NewSortResults(cfg), }, diff --git a/pkg/result/processors/fixer.go b/pkg/result/processors/fixer.go index 3a31a33e40b5..4f79e8bb6771 100644 --- a/pkg/result/processors/fixer.go +++ b/pkg/result/processors/fixer.go @@ -36,9 +36,9 @@ func (f Fixer) printStat() { f.sw.PrintStages() } -func (f Fixer) Process(issues []result.Issue) []result.Issue { +func (f Fixer) Process(issues []result.Issue) ([]result.Issue, error) { if !f.cfg.Issues.NeedFix { - return issues + return issues, nil } outIssues := make([]result.Issue, 0, len(issues)) @@ -67,9 +67,17 @@ func (f Fixer) Process(issues []result.Issue) []result.Issue { } f.printStat() - return outIssues + return outIssues, nil } +func (f Fixer) Name() string { + return "fixer" +} + +func (f Fixer) Finish() {} + +var _ Processor = Fixer{} + func (f Fixer) fixIssuesInFile(filePath string, issues []result.Issue) error { // TODO: don't read the whole file into memory: read line by line; // can't just use bufio.scanner: it has a line length limit From 067cc99fdfffeae9163183db293c0dd30e48e3ec Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 17 Mar 2023 12:17:54 +0100 Subject: [PATCH 2/3] test: check that fixer works with --path-prefix The combination with --path gets tested for the first test case. This is arbitrary. It could also be tested for all of them, but then each test would have to run twice (with and without). --- test/fix_test.go | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/test/fix_test.go b/test/fix_test.go index bccf620f5989..f6868080717c 100644 --- a/test/fix_test.go +++ b/test/fix_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "sort" "testing" "github.com/stretchr/testify/require" @@ -35,25 +36,42 @@ func TestFix(t *testing.T) { sources := findSources(t, tmpDir, "in", "*.go") - for _, input := range sources { + // The combination with --path gets tested for the first test case. + // This is arbitrary. It could also be tested for all of them, + // but then each test would have to run twice (with and without). + // To make this determinstic, the sources get sorted by name. + sort.Strings(sources) + + for i, input := range sources { + withPathPrefix := i == 0 input := input t.Run(filepath.Base(input), func(t *testing.T) { t.Parallel() rc := testshared.ParseTestDirectives(t, input) if rc == nil { + if withPathPrefix { + t.Errorf("The testcase %s should not get skipped, it's used for testing --path.", input) + return + } t.Logf("Skipped: %s", input) return } + args := []string{ + "--disable-all", + "--print-issued-lines=false", + "--print-linter-name=false", + "--out-format=line-number", + "--fix", + } + if withPathPrefix { + t.Log("Testing with --path-prefix.") + // This must not break fixing... + args = append(args, "--path-prefix=foobar/") + } testshared.NewRunnerBuilder(t). - WithArgs( - "--disable-all", - "--print-issued-lines=false", - "--print-linter-name=false", - "--out-format=line-number", - "--fix", - ). + WithArgs(args...). WithRunContext(rc). WithTargetPath(input). Runner(). From c5a1743fd23dacd6ae8f33682d04af831b8ad4a1 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Mon, 20 Mar 2023 21:21:55 +0100 Subject: [PATCH 3/3] review: tests, filecache, and minor changes --- pkg/commands/run.go | 2 +- pkg/fsutils/linecache.go | 2 - pkg/lint/runner.go | 10 +++-- pkg/result/processors/fixer.go | 4 +- test/fix_test.go | 76 ++++++++++++++++++++++------------ 5 files changed, 60 insertions(+), 34 deletions(-) diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 753abbd52462..20106b44f624 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -359,7 +359,7 @@ func (e *Executor) runAnalysis(ctx context.Context, args []string) ([]result.Iss lintCtx.Log = e.log.Child(logutils.DebugKeyLintersContext) runner, err := lint.NewRunner(e.cfg, e.log.Child(logutils.DebugKeyRunner), - e.goenv, e.EnabledLintersSet, e.lineCache, e.DBManager, lintCtx.Packages) + e.goenv, e.EnabledLintersSet, e.lineCache, e.fileCache, e.DBManager, lintCtx.Packages) if err != nil { return nil, err } diff --git a/pkg/fsutils/linecache.go b/pkg/fsutils/linecache.go index 64b411dfd72f..2e92264846ef 100644 --- a/pkg/fsutils/linecache.go +++ b/pkg/fsutils/linecache.go @@ -19,8 +19,6 @@ func NewLineCache(fc *FileCache) *LineCache { } } -func (lc *LineCache) GetFileCache() *FileCache { return lc.fileCache } - // GetLine returns the index1-th (1-based index) line from the file on filePath func (lc *LineCache) GetLine(filePath string, index1 int) (string, error) { if index1 == 0 { // some linters, e.g. gosec can do it: it really means first line diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 0a3a281e791a..b11804388e04 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -27,8 +27,10 @@ type Runner struct { Log logutils.Log } -func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lintersdb.EnabledSet, - lineCache *fsutils.LineCache, dbManager *lintersdb.Manager, pkgs []*gopackages.Package) (*Runner, error) { +func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, + es *lintersdb.EnabledSet, + lineCache *fsutils.LineCache, fileCache *fsutils.FileCache, + dbManager *lintersdb.Manager, pkgs []*gopackages.Package) (*Runner, error) { // Beware that some processors need to add the path prefix when working with paths // because they get invoked before the path prefixer (exclude and severity rules) // or process other paths (skip files). @@ -98,8 +100,10 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint processors.NewSourceCode(lineCache, log.Child(logutils.DebugKeySourceCode)), processors.NewPathShortener(), getSeverityRulesProcessor(&cfg.Severity, log, files), + // The fixer still needs to see paths for the issues that are relative to the current directory. - processors.NewFixer(cfg, log, lineCache.GetFileCache()), + processors.NewFixer(cfg, log, fileCache), + // Now we can modify the issues for output. processors.NewPathPrefixer(cfg.Output.PathPrefix), processors.NewSortResults(cfg), diff --git a/pkg/result/processors/fixer.go b/pkg/result/processors/fixer.go index 4f79e8bb6771..a79a846288ee 100644 --- a/pkg/result/processors/fixer.go +++ b/pkg/result/processors/fixer.go @@ -16,6 +16,8 @@ import ( "github.com/golangci/golangci-lint/pkg/timeutils" ) +var _ Processor = Fixer{} + type Fixer struct { cfg *config.Config log logutils.Log @@ -76,8 +78,6 @@ func (f Fixer) Name() string { func (f Fixer) Finish() {} -var _ Processor = Fixer{} - func (f Fixer) fixIssuesInFile(filePath string, issues []result.Issue) error { // TODO: don't read the whole file into memory: read line by line; // can't just use bufio.scanner: it has a line length limit diff --git a/test/fix_test.go b/test/fix_test.go index f6868080717c..4f35f3309d00 100644 --- a/test/fix_test.go +++ b/test/fix_test.go @@ -4,7 +4,6 @@ import ( "os" "os/exec" "path/filepath" - "sort" "testing" "github.com/stretchr/testify/require" @@ -15,7 +14,9 @@ import ( // value: "1" const envKeepTempFiles = "GL_KEEP_TEMP_FILES" -func TestFix(t *testing.T) { +func setupTestFix(t *testing.T) []string { + t.Helper() + testshared.SkipOnWindows(t) tmpDir := filepath.Join(testdataDir, "fix.tmp") @@ -34,44 +35,67 @@ func TestFix(t *testing.T) { testshared.InstallGolangciLint(t) - sources := findSources(t, tmpDir, "in", "*.go") + return findSources(t, tmpDir, "in", "*.go") +} - // The combination with --path gets tested for the first test case. - // This is arbitrary. It could also be tested for all of them, - // but then each test would have to run twice (with and without). - // To make this determinstic, the sources get sorted by name. - sort.Strings(sources) +func TestFix(t *testing.T) { + sources := setupTestFix(t) - for i, input := range sources { - withPathPrefix := i == 0 + for _, input := range sources { input := input t.Run(filepath.Base(input), func(t *testing.T) { t.Parallel() rc := testshared.ParseTestDirectives(t, input) if rc == nil { - if withPathPrefix { - t.Errorf("The testcase %s should not get skipped, it's used for testing --path.", input) - return - } t.Logf("Skipped: %s", input) return } - args := []string{ - "--disable-all", - "--print-issued-lines=false", - "--print-linter-name=false", - "--out-format=line-number", - "--fix", - } - if withPathPrefix { - t.Log("Testing with --path-prefix.") - // This must not break fixing... - args = append(args, "--path-prefix=foobar/") + testshared.NewRunnerBuilder(t). + WithArgs("--disable-all", + "--print-issued-lines=false", + "--print-linter-name=false", + "--out-format=line-number", + "--fix"). + WithRunContext(rc). + WithTargetPath(input). + Runner(). + Run(). + ExpectExitCode(rc.ExitCode) + + output, err := os.ReadFile(input) + require.NoError(t, err) + + expectedOutput, err := os.ReadFile(filepath.Join(testdataDir, "fix", "out", filepath.Base(input))) + require.NoError(t, err) + + require.Equal(t, string(expectedOutput), string(output)) + }) + } +} + +func TestFix_pathPrefix(t *testing.T) { + sources := setupTestFix(t) + + for _, input := range sources { + input := input + t.Run(filepath.Base(input), func(t *testing.T) { + t.Parallel() + + rc := testshared.ParseTestDirectives(t, input) + if rc == nil { + t.Logf("Skipped: %s", input) + return } + testshared.NewRunnerBuilder(t). - WithArgs(args...). + WithArgs("--disable-all", + "--print-issued-lines=false", + "--print-linter-name=false", + "--out-format=line-number", + "--fix", + "--path-prefix=foobar/"). WithRunContext(rc). WithTargetPath(input). Runner().