diff --git a/go.mod b/go.mod index 975e144b1935..2f6080376f67 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/ashanbrown/forbidigo v1.3.0 github.com/ashanbrown/makezero v1.1.1 github.com/bkielbasa/cyclop v1.2.0 - github.com/blizzy78/varnamelen v0.6.0 + github.com/blizzy78/varnamelen v0.8.0 github.com/bombsimon/wsl/v3 v3.3.0 github.com/breml/bidichk v0.2.2 github.com/breml/errchkjson v0.2.3 diff --git a/go.sum b/go.sum index b0feaff6dc6b..9ef3f2cfab73 100644 --- a/go.sum +++ b/go.sum @@ -98,8 +98,8 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs= github.com/bkielbasa/cyclop v1.2.0 h1:7Jmnh0yL2DjKfw28p86YTd/B4lRGcNuu12sKE35sM7A= github.com/bkielbasa/cyclop v1.2.0/go.mod h1:qOI0yy6A7dYC4Zgsa72Ppm9kONl0RoIlPbzot9mhmeI= -github.com/blizzy78/varnamelen v0.6.0 h1:TOIDk9qRIMspALZKX8x+5hQfAjuvAFogppnxtvuNmBo= -github.com/blizzy78/varnamelen v0.6.0/go.mod h1:zy2Eic4qWqjrxa60jG34cfL0VXcSwzUrIx68eJPb4Q8= +github.com/blizzy78/varnamelen v0.8.0 h1:oqSblyuQvFsW1hbBHh1zfwrKe3kcSj0rnXkKzsQ089M= +github.com/blizzy78/varnamelen v0.8.0/go.mod h1:V9TzQZ4fLJ1DSrjVDfl89H7aMnTvKkApdHeyESmyR7k= github.com/bombsimon/wsl/v3 v3.3.0 h1:Mka/+kRLoQJq7g2rggtgQsjuI/K5Efd87WX96EWFxjM= github.com/bombsimon/wsl/v3 v3.3.0/go.mod h1:st10JtZYLE4D5sC7b8xV4zTKZwAQjCH/Hy2Pm1FNZIc= github.com/breml/bidichk v0.2.2 h1:w7QXnpH0eCBJm55zGCTJveZEkQBt6Fs5zThIdA6qQ9Y= @@ -814,6 +814,7 @@ golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPh golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a/go.mod h1:P+XmwS30IXTQdn5tA2iutPOUgjI07+tq3H3K9MVA1s8= golang.org/x/crypto v0.0.0-20210817164053-32db794688a5/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.0.0-20220214200702-86341886e292/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= diff --git a/pkg/golinters/goanalysis/linter.go b/pkg/golinters/goanalysis/linter.go index ef49e4284aa3..36b755452ff7 100644 --- a/pkg/golinters/goanalysis/linter.go +++ b/pkg/golinters/goanalysis/linter.go @@ -10,6 +10,7 @@ import ( "golang.org/x/tools/go/analysis" "github.com/golangci/golangci-lint/pkg/lint/linter" + libpackages "github.com/golangci/golangci-lint/pkg/packages" "github.com/golangci/golangci-lint/pkg/result" ) @@ -144,6 +145,31 @@ func (lnt *Linter) configure() error { return nil } +func buildIssuesFromErrorsForTypecheckMode(errs []error, lintCtx *linter.Context) ([]result.Issue, error) { + var issues []result.Issue + uniqReportedIssues := map[string]bool{} + for _, err := range errs { + itErr, ok := errors.Cause(err).(*IllTypedError) + if !ok { + return nil, err + } + for _, err := range libpackages.ExtractErrors(itErr.Pkg) { + i, perr := parseError(err) + if perr != nil { // failed to parse + if uniqReportedIssues[err.Msg] { + continue + } + uniqReportedIssues[err.Msg] = true + lintCtx.Log.Errorf("typechecking error: %s", err.Msg) + } else { + i.Pkg = itErr.Pkg // to save to cache later + issues = append(issues, *i) + } + } + } + return issues, nil +} + func (lnt *Linter) preRun(lintCtx *linter.Context) error { if err := analysis.Validate(lnt.analyzers); err != nil { return errors.Wrap(err, "failed to validate analyzers") diff --git a/pkg/golinters/goanalysis/linter_test.go b/pkg/golinters/goanalysis/linter_test.go index 44ded60043c0..a56e0be12997 100644 --- a/pkg/golinters/goanalysis/linter_test.go +++ b/pkg/golinters/goanalysis/linter_test.go @@ -2,9 +2,14 @@ package goanalysis import ( "fmt" + "go/token" + "reflect" "testing" + "github.com/golangci/golangci-lint/pkg/result" + "github.com/stretchr/testify/assert" + "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/packages" ) @@ -46,3 +51,234 @@ func TestParseError(t *testing.T) { assert.Equal(t, "msg", i.Text) } } + +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 "some-linter" + }, + }, + want: []result.Issue(nil), + }, + { + name: "Linter Name is Analyzer Name", + args: args{ + diags: []Diagnostic{ + { + Diagnostic: analysis.Diagnostic{ + Message: "failure message", + }, + Analyzer: &analysis.Analyzer{ + Name: "some-linter", + }, + Position: token.Position{}, + Pkg: nil, + }, + }, + linterNameBuilder: func(*Diagnostic) string { + return "some-linter" + }, + }, + want: []result.Issue{ + { + FromLinter: "some-linter", + 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) { + 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: "some-linter", + }, + Position: token.Position{}, + Pkg: nil, + }, + + linterName: "some-linter", + }, + wantIssue: result.Issue{ + FromLinter: "some-linter", + 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: "some-linter", + }, + wantIssue: result.Issue{ + FromLinter: "some-linter", + 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: "some-linter", + }, + wantIssue: result.Issue{ + FromLinter: "some-linter", + 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: "some-linter", + }, + wantIssue: result.Issue{ + FromLinter: "some-linter", + 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: "some-linter", + }, + wantIssue: result.Issue{ + FromLinter: "some-linter", + 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/goanalysis/runners.go b/pkg/golinters/goanalysis/runners.go index 7e4cf902e79c..8277fd5c9298 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" @@ -88,34 +89,63 @@ func buildIssues(diags []Diagnostic, linterNameBuilder func(diag *Diagnostic) st 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 token.Position, 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 {