From d8ad9c3b1435f51490d80c8782dbe64bacd86242 Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Wed, 10 Jun 2020 14:23:23 -0400 Subject: [PATCH] Allow custom linters to auto-fix This allows custom linters hook into the `--fix` functionality. Custom linters specify the fixes using the Go analysis structures, which allow for arbitrary char offsets for fixes; they get converted into golangci structures, which are line-based. If the conversion is not possible, the fix is dropped on the floor. Signed-off-by: Steve Coffman --- pkg/golinters/goanalysis/linter.go | 6 +- pkg/golinters/goanalysis/runners.go | 111 +++++++--- pkg/golinters/goanalysis/runners_test.go | 245 +++++++++++++++++++++++ pkg/golinters/gocritic.go | 2 +- 4 files changed, 334 insertions(+), 30 deletions(-) create mode 100644 pkg/golinters/goanalysis/runners_test.go diff --git a/pkg/golinters/goanalysis/linter.go b/pkg/golinters/goanalysis/linter.go index ef49e4284aa3..cb6a8e75765a 100644 --- a/pkg/golinters/goanalysis/linter.go +++ b/pkg/golinters/goanalysis/linter.go @@ -51,7 +51,11 @@ type Linter struct { needUseOriginalPackages bool } -func NewLinter(name, desc string, analyzers []*analysis.Analyzer, cfg map[string]map[string]interface{}) *Linter { +func NewLinter( + name, desc string, + analyzers []*analysis.Analyzer, + cfg map[string]map[string]interface{}, +) *Linter { return &Linter{name: name, desc: desc, analyzers: analyzers, cfg: cfg} } diff --git a/pkg/golinters/goanalysis/runners.go b/pkg/golinters/goanalysis/runners.go index 7e4cf902e79c..0522c9102f95 100644 --- a/pkg/golinters/goanalysis/runners.go +++ b/pkg/golinters/goanalysis/runners.go @@ -2,6 +2,7 @@ package goanalysis import ( "fmt" + "go/token" "runtime" "sort" "strings" @@ -34,7 +35,14 @@ func runAnalyzers(cfg runAnalyzersConfig, lintCtx *linter.Context) ([]result.Iss const stagesToPrint = 10 defer sw.PrintTopStages(stagesToPrint) - runner := newRunner(cfg.getName(), log, lintCtx.PkgCache, lintCtx.LoadGuard, cfg.getLoadMode(), sw) + runner := newRunner( + cfg.getName(), + log, + lintCtx.PkgCache, + lintCtx.LoadGuard, + cfg.getLoadMode(), + sw, + ) pkgs := lintCtx.Packages if cfg.useOriginalPackages() { @@ -84,38 +92,70 @@ func runAnalyzers(cfg runAnalyzersConfig, lintCtx *linter.Context) ([]result.Iss return issues, nil } -func buildIssues(diags []Diagnostic, linterNameBuilder func(diag *Diagnostic) string) []result.Issue { +func buildIssues( + diags []Diagnostic, + linterNameBuilder func(diag *Diagnostic) string, +) []result.Issue { var issues []result.Issue for i := range diags { diag := &diags[i] - linterName := linterNameBuilder(diag) + issues = append(issues, buildSingleIssue(diag, linterNameBuilder(diag))) + } + return issues +} - var text string - if diag.Analyzer.Name == linterName { - text = diag.Message - } else { - text = fmt.Sprintf("%s: %s", diag.Analyzer.Name, diag.Message) - } +func buildSingleIssue(diag *Diagnostic, linterName string) result.Issue { + text := generateIssueText(diag, linterName) + issue := result.Issue{ + FromLinter: linterName, + Text: text, + Pos: diag.Position, + Pkg: diag.Pkg, + } + + if len(diag.SuggestedFixes) > 0 { + // Don't really have a better way of picking a best fix right now + chosenFix := diag.SuggestedFixes[0] + + // It could be confusing to return more than one issue per single diagnostic, + // but if we return a subset it might be a partial application of a fix. Don't + // apply a fix unless there is only one for now + if len(chosenFix.TextEdits) == 1 { + edit := chosenFix.TextEdits[0] + + pos := diag.Pkg.Fset.Position(edit.Pos) + end := diag.Pkg.Fset.Position(edit.End) + + newLines := strings.Split(string(edit.NewText), "\n") - issues = append(issues, result.Issue{ - FromLinter: linterName, - Text: text, - Pos: diag.Position, - Pkg: diag.Pkg, - }) - - if len(diag.Related) > 0 { - for _, info := range diag.Related { - issues = append(issues, result.Issue{ - FromLinter: linterName, - Text: fmt.Sprintf("%s(related information): %s", diag.Analyzer.Name, info.Message), - Pos: diag.Pkg.Fset.Position(info.Pos), - Pkg: diag.Pkg, - }) + // This only works if we're only replacing whole lines with brand-new lines + if onlyReplacesWholeLines(pos, end, newLines) { + // both original and new content ends with newline, + // omit to avoid partial line replacement + newLines = newLines[:len(newLines)-1] + + issue.Replacement = &result.Replacement{NewLines: newLines} + issue.LineRange = &result.Range{From: pos.Line, To: end.Line - 1} + + return issue } } } - return issues + + return issue +} + +func onlyReplacesWholeLines(oPos, oEnd token.Position, newLines []string) bool { + return oPos.Column == 1 && oEnd.Column == 1 && + oPos.Line < oEnd.Line && // must be replacing at least one line + newLines[len(newLines)-1] == "" // edit.NewText ended with '\n' +} + +func generateIssueText(diag *Diagnostic, linterName string) string { + if diag.Analyzer.Name == linterName { + return diag.Message + } + return fmt.Sprintf("%s: %s", diag.Analyzer.Name, diag.Message) } func getIssuesCacheKey(analyzers []*analysis.Analyzer) string { @@ -160,7 +200,12 @@ func saveIssuesToCache(allPkgs []*packages.Package, pkgsFromCache map[*packages. atomic.AddInt32(&savedIssuesCount, int32(len(encodedIssues))) if err := lintCtx.PkgCache.Put(pkg, pkgcache.HashModeNeedAllDeps, lintResKey, encodedIssues); err != nil { - lintCtx.Log.Infof("Failed to save package %s issues (%d) to cache: %s", pkg, len(pkgIssues), err) + lintCtx.Log.Infof( + "Failed to save package %s issues (%d) to cache: %s", + pkg, + len(pkgIssues), + err, + ) } else { issuesCacheDebugf("Saved package %s issues (%d) to cache", pkg, len(pkgIssues)) } @@ -178,7 +223,12 @@ func saveIssuesToCache(allPkgs []*packages.Package, pkgsFromCache map[*packages. close(pkgCh) wg.Wait() - issuesCacheDebugf("Saved %d issues from %d packages to cache in %s", savedIssuesCount, len(allPkgs), time.Since(startedAt)) + issuesCacheDebugf( + "Saved %d issues from %d packages to cache in %s", + savedIssuesCount, + len(allPkgs), + time.Since(startedAt), + ) } //nolint:gocritic @@ -206,7 +256,12 @@ func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context, defer wg.Done() for pkg := range pkgCh { var pkgIssues []EncodingIssue - err := lintCtx.PkgCache.Get(pkg, pkgcache.HashModeNeedAllDeps, lintResKey, &pkgIssues) + err := lintCtx.PkgCache.Get( + pkg, + pkgcache.HashModeNeedAllDeps, + lintResKey, + &pkgIssues, + ) cacheRes := pkgToCacheRes[pkg] cacheRes.loadErr = err if err != nil { diff --git a/pkg/golinters/goanalysis/runners_test.go b/pkg/golinters/goanalysis/runners_test.go new file mode 100644 index 000000000000..55d381efdd0d --- /dev/null +++ b/pkg/golinters/goanalysis/runners_test.go @@ -0,0 +1,245 @@ +package goanalysis + +import ( + "go/token" + "reflect" + "testing" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/packages" + + "github.com/golangci/golangci-lint/pkg/result" +) + +const someLinterName = "some-linter" + +func Test_buildIssues(t *testing.T) { + type args struct { + diags []Diagnostic + linterNameBuilder func(diag *Diagnostic) string + } + tests := []struct { + name string + args args + want []result.Issue + }{ + { + name: "No Diagnostics", + args: args{ + diags: []Diagnostic{}, + linterNameBuilder: func(*Diagnostic) string { + return someLinterName + }, + }, + want: []result.Issue(nil), + }, + { + name: "Linter Name is Analyzer Name", + args: args{ + diags: []Diagnostic{ + { + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + }, + Analyzer: &analysis.Analyzer{ + Name: someLinterName, + }, + Position: token.Position{}, + Pkg: nil, + }, + }, + linterNameBuilder: func(*Diagnostic) string { + return someLinterName + }, + }, + want: []result.Issue{ + { + FromLinter: someLinterName, + Text: "failure message", + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := buildIssues(tt.args.diags, tt.args.linterNameBuilder); !reflect.DeepEqual( + got, + tt.want, + ) { + t.Errorf("buildIssues() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_buildSingleIssue(t *testing.T) { //nolint:funlen + type args struct { + diag *Diagnostic + linterName string + } + fakePkg := packages.Package{ + Fset: makeFakeFileSet(), + } + tests := []struct { + name string + args args + wantIssue result.Issue + }{ + { + name: "Linter Name is Analyzer Name", + args: args{ + diag: &Diagnostic{ + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + }, + Analyzer: &analysis.Analyzer{ + Name: someLinterName, + }, + Position: token.Position{}, + Pkg: nil, + }, + + linterName: someLinterName, + }, + wantIssue: result.Issue{ + FromLinter: someLinterName, + Text: "failure message", + }, + }, + { + name: "Linter Name is NOT Analyzer Name", + args: args{ + diag: &Diagnostic{ + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + }, + Analyzer: &analysis.Analyzer{ + Name: "some-analyzer", + }, + Position: token.Position{}, + Pkg: nil, + }, + linterName: someLinterName, + }, + wantIssue: result.Issue{ + FromLinter: someLinterName, + Text: "some-analyzer: failure message", + }, + }, + { + name: "Shows issue when suggested edits exist but has no TextEdits", + args: args{ + diag: &Diagnostic{ + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "fix something", + TextEdits: []analysis.TextEdit{}, + }, + }, + }, + Analyzer: &analysis.Analyzer{Name: "some-analyzer"}, + Position: token.Position{}, + Pkg: nil, + }, + linterName: someLinterName, + }, + wantIssue: result.Issue{ + FromLinter: someLinterName, + Text: "some-analyzer: failure message", + }, + }, + { + name: "Replace Whole Line", + args: args{ + diag: &Diagnostic{ + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "fix something", + TextEdits: []analysis.TextEdit{ + { + Pos: 101, + End: 201, + NewText: []byte("// Some comment to fix\n"), + }, + }, + }, + }, + }, + Analyzer: &analysis.Analyzer{Name: "some-analyzer"}, + Position: token.Position{}, + Pkg: &fakePkg, + }, + linterName: someLinterName, + }, + wantIssue: result.Issue{ + FromLinter: someLinterName, + Text: "some-analyzer: failure message", + LineRange: &result.Range{ + From: 2, + To: 2, + }, + Replacement: &result.Replacement{ + NeedOnlyDelete: false, + NewLines: []string{ + "// Some comment to fix", + }, + }, + Pkg: &fakePkg, + }, + }, + { + name: "Excludes Replacement if TextEdit doesn't modify only whole lines", + args: args{ + diag: &Diagnostic{ + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "fix something", + TextEdits: []analysis.TextEdit{ + { + Pos: 101, + End: 151, + NewText: []byte("// Some comment to fix\n"), + }, + }, + }, + }, + }, + Analyzer: &analysis.Analyzer{Name: "some-analyzer"}, + Position: token.Position{}, + Pkg: &fakePkg, + }, + linterName: someLinterName, + }, + wantIssue: result.Issue{ + FromLinter: someLinterName, + Text: "some-analyzer: failure message", + Pkg: &fakePkg, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if gotIssues := buildSingleIssue(tt.args.diag, tt.args.linterName); !reflect.DeepEqual( + gotIssues, + tt.wantIssue, + ) { + t.Errorf("buildSingleIssue() = %v, want %v", gotIssues, tt.wantIssue) + } + }) + } +} + +func makeFakeFileSet() *token.FileSet { + fSet := token.NewFileSet() + file := fSet.AddFile("fake.go", 1, 1000) + for i := 100; i < 1000; i += 100 { + file.AddLine(i) + } + return fSet +} diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index e98f23445ae9..2ffe92fbfcd3 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -116,7 +116,7 @@ func configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string // but the file parsers (TOML, YAML, JSON) don't create the same representation for raw type. // then we have to convert value types into the expected value types. // Maybe in the future, this kind of conversion will be done in go-critic itself. -//nolint:exhaustive // only 3 types (int, bool, and string) are supported by CheckerParam.Value +//nolint:exhaustive,nolintlint // only 3 types (int, bool, and string) are supported by CheckerParam.Value func normalizeCheckerParamsValue(p interface{}) interface{} { rv := reflect.ValueOf(p) switch rv.Type().Kind() {