From 09a00c1ab1cecbdf8f32e5cbcefe3dc211092159 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 18 Mar 2021 12:12:50 -1000 Subject: [PATCH] internal/lsp: fix support for SourceFixAll code actions Some of the refactoring changed the way that we label code action kinds, and we need to add quickfix and fixall kinds for each diagnostic type. Support a per-kind suggested fix, and fix a small issue in setting the analyzer for a fixall code action. Fixes golang/go#45111 Change-Id: I6bb32c9aa7427b690f42910672d3139579e84478 Reviewed-on: https://go-review.googlesource.com/c/tools/+/303209 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Heschi Kreinick --- gopls/internal/regtest/expectation.go | 5 +-- gopls/internal/regtest/misc/fix_test.go | 19 +++++++++++ gopls/internal/regtest/misc/imports_test.go | 4 +-- gopls/internal/regtest/wrappers.go | 4 +-- internal/lsp/cache/analysis.go | 1 + internal/lsp/cache/errors.go | 26 +++++++++----- internal/lsp/cache/mod.go | 8 ++--- internal/lsp/cache/mod_tidy.go | 5 +-- internal/lsp/code_action.go | 38 +++++++++++---------- internal/lsp/fake/editor.go | 5 ++- internal/lsp/mod/diagnostics.go | 2 +- internal/lsp/source/diagnostics.go | 7 ++-- internal/lsp/source/fix.go | 7 ++-- internal/lsp/source/options.go | 22 +++++++++--- internal/lsp/source/view.go | 2 +- 15 files changed, 103 insertions(+), 52 deletions(-) diff --git a/gopls/internal/regtest/expectation.go b/gopls/internal/regtest/expectation.go index 02e00dd3f3..f86bcb6dff 100644 --- a/gopls/internal/regtest/expectation.go +++ b/gopls/internal/regtest/expectation.go @@ -582,8 +582,9 @@ func NoDiagnosticWithMessage(name, msg string) DiagnosticExpectation { return DiagnosticExpectation{path: name, message: msg, present: false} } -// GoSum asserts that a "go.sum is out of sync" diagnostic for the given module -// (as formatted in a go.mod file, e.g. "example.com v1.0.0") is present. +// GoSumDiagnostic asserts that a "go.sum is out of sync" diagnostic for the +// given module (as formatted in a go.mod file, e.g. "example.com v1.0.0") is +// present. func (e *Env) GoSumDiagnostic(name, module string) Expectation { e.T.Helper() // In 1.16, go.sum diagnostics should appear on the relevant module. Earlier diff --git a/gopls/internal/regtest/misc/fix_test.go b/gopls/internal/regtest/misc/fix_test.go index 395bfd8096..9225a83295 100644 --- a/gopls/internal/regtest/misc/fix_test.go +++ b/gopls/internal/regtest/misc/fix_test.go @@ -81,6 +81,25 @@ func Foo() error { env.DiagnosticAtRegexpWithMessage("main.go", `return`, "wrong number of return values"), ReadDiagnostics("main.go", &d), )) + codeActions := env.CodeAction("main.go", d.Diagnostics) + if len(codeActions) != 2 { + t.Fatalf("expected 2 code actions, got %v", len(codeActions)) + } + var foundQuickFix, foundFixAll bool + for _, a := range codeActions { + if a.Kind == protocol.QuickFix { + foundQuickFix = true + } + if a.Kind == protocol.SourceFixAll { + foundFixAll = true + } + } + if !foundQuickFix { + t.Fatalf("expected quickfix code action, got none") + } + if !foundFixAll { + t.Fatalf("expected fixall code action, got none") + } env.ApplyQuickFixes("main.go", d.Diagnostics) env.Await(EmptyDiagnostics("main.go")) }) diff --git a/gopls/internal/regtest/misc/imports_test.go b/gopls/internal/regtest/misc/imports_test.go index 9a95208cb5..2a666c4e61 100644 --- a/gopls/internal/regtest/misc/imports_test.go +++ b/gopls/internal/regtest/misc/imports_test.go @@ -74,7 +74,7 @@ func main() { Run(t, "", func(t *testing.T, env *Env) { env.CreateBuffer("main.go", vim1) env.OrganizeImports("main.go") - actions := env.CodeAction("main.go") + actions := env.CodeAction("main.go", nil) if len(actions) > 0 { got := env.Editor.BufferText("main.go") t.Errorf("unexpected actions %#v", actions) @@ -107,7 +107,7 @@ func main() { Run(t, "", func(t *testing.T, env *Env) { env.CreateBuffer("main.go", vim2) env.OrganizeImports("main.go") - actions := env.CodeAction("main.go") + actions := env.CodeAction("main.go", nil) if len(actions) > 0 { t.Errorf("unexpected actions %#v", actions) } diff --git a/gopls/internal/regtest/wrappers.go b/gopls/internal/regtest/wrappers.go index 4aafdf20e9..8281dab1e1 100644 --- a/gopls/internal/regtest/wrappers.go +++ b/gopls/internal/regtest/wrappers.go @@ -388,9 +388,9 @@ func (e *Env) AcceptCompletion(path string, pos fake.Pos, item protocol.Completi // CodeAction calls testDocument/codeAction for the given path, and calls // t.Fatal if there are errors. -func (e *Env) CodeAction(path string) []protocol.CodeAction { +func (e *Env) CodeAction(path string, diagnostics []protocol.Diagnostic) []protocol.CodeAction { e.T.Helper() - actions, err := e.Editor.CodeAction(e.Ctx, path, nil) + actions, err := e.Editor.CodeAction(e.Ctx, path, nil, diagnostics) if err != nil { e.T.Fatal(err) } diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index 19675af11a..702513e789 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -423,6 +423,7 @@ func (s *snapshot) DiagnosePackage(ctx context.Context, spkg source.Package) (ma clone := *diag clone.SuggestedFixes = eaDiag.SuggestedFixes clone.Tags = eaDiag.Tags + clone.Analyzer = eaDiag.Analyzer diag = &clone } } diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index f18d5edb1e..ace6947fa9 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -163,7 +163,7 @@ func goGetQuickFixes(snapshot *snapshot, uri span.URI, pkg string) ([]source.Sug if err != nil { return nil, err } - return []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, nil + return []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, nil } func analysisDiagnosticDiagnostics(ctx context.Context, snapshot *snapshot, pkg *pkg, a *analysis.Analyzer, e *analysis.Diagnostic) ([]*source.Diagnostic, error) { @@ -184,7 +184,11 @@ func analysisDiagnosticDiagnostics(ctx context.Context, snapshot *snapshot, pkg if err != nil { return nil, err } - fixes, err := suggestedAnalysisFixes(snapshot, pkg, e) + kinds := srcAnalyzer.ActionKind + if len(srcAnalyzer.ActionKind) == 0 { + kinds = append(kinds, protocol.QuickFix) + } + fixes, err := suggestedAnalysisFixes(snapshot, pkg, e, kinds) if err != nil { return nil, err } @@ -197,7 +201,9 @@ func analysisDiagnosticDiagnostics(ctx context.Context, snapshot *snapshot, pkg if err != nil { return nil, err } - fixes = append(fixes, source.SuggestedFixFromCommand(cmd)) + for _, kind := range kinds { + fixes = append(fixes, source.SuggestedFixFromCommand(cmd, kind)) + } } related, err := relatedInformation(snapshot, pkg, e) if err != nil { @@ -242,7 +248,7 @@ func typesCodeHref(snapshot *snapshot, code typesinternal.ErrorCode) string { return fmt.Sprintf("https://%s/golang.org/x/tools/internal/typesinternal#%s", target, code.String()) } -func suggestedAnalysisFixes(snapshot *snapshot, pkg *pkg, diag *analysis.Diagnostic) ([]source.SuggestedFix, error) { +func suggestedAnalysisFixes(snapshot *snapshot, pkg *pkg, diag *analysis.Diagnostic, kinds []protocol.CodeActionKind) ([]source.SuggestedFix, error) { var fixes []source.SuggestedFix for _, fix := range diag.SuggestedFixes { edits := make(map[span.URI][]protocol.TextEdit) @@ -260,10 +266,14 @@ func suggestedAnalysisFixes(snapshot *snapshot, pkg *pkg, diag *analysis.Diagnos NewText: string(e.NewText), }) } - fixes = append(fixes, source.SuggestedFix{ - Title: fix.Message, - Edits: edits, - }) + for _, kind := range kinds { + fixes = append(fixes, source.SuggestedFix{ + Title: fix.Message, + Edits: edits, + ActionKind: kind, + }) + } + } return fixes, nil } diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 2e1917af08..a915d052c0 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -358,7 +358,7 @@ func (s *snapshot) goCommandDiagnostic(pm *source.ParsedModule, spn span.Span, g Source: source.ListError, Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". See https://github.com/golang/go/issues/39164 for more detail on this issue.`, - SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, + SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, }, nil case strings.Contains(goCmdError, "updates to go.sum needed"), strings.Contains(goCmdError, "missing go.sum entry"): @@ -385,8 +385,8 @@ See https://github.com/golang/go/issues/39164 for more detail on this issue.`, Source: source.ListError, Message: msg, SuggestedFixes: []source.SuggestedFix{ - source.SuggestedFixFromCommand(tidyCmd), - source.SuggestedFixFromCommand(updateCmd), + source.SuggestedFixFromCommand(tidyCmd, protocol.QuickFix), + source.SuggestedFixFromCommand(updateCmd, protocol.QuickFix), }, }, nil case strings.Contains(goCmdError, "disabled by GOPROXY=off") && innermost != nil: @@ -405,7 +405,7 @@ See https://github.com/golang/go/issues/39164 for more detail on this issue.`, Severity: protocol.SeverityError, Message: fmt.Sprintf("%v@%v has not been downloaded", innermost.Path, innermost.Version), Source: source.ListError, - SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, + SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, }, nil default: return &source.Diagnostic{ diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index 6891a39314..0fb9ff671a 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -341,7 +341,7 @@ func unusedDiagnostic(m *protocol.ColumnMapper, req *modfile.Require, onlyDiagno Severity: protocol.SeverityWarning, Source: source.ModTidyError, Message: fmt.Sprintf("%s is not used in this module", req.Mod.Path), - SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, + SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, }, nil } @@ -383,6 +383,7 @@ func directnessDiagnostic(m *protocol.ColumnMapper, req *modfile.Require, comput Edits: map[span.URI][]protocol.TextEdit{ m.URI: edits, }, + ActionKind: protocol.QuickFix, }}, }, nil } @@ -413,7 +414,7 @@ func missingModuleDiagnostic(snapshot source.Snapshot, pm *source.ParsedModule, Severity: protocol.SeverityError, Source: source.ModTidyError, Message: fmt.Sprintf("%s is not in your go.mod file", req.Mod.Path), - SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, + SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, }, nil } diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index da6cf97200..7a389b5878 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -147,11 +147,14 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara if len(d.SuggestedFixes) == 0 { continue } - kind := protocol.QuickFix - if d.Analyzer != nil && d.Analyzer.ActionKind != "" { - kind = d.Analyzer.ActionKind + var isFix bool + for _, fix := range d.SuggestedFixes { + if fix.ActionKind == protocol.QuickFix || fix.ActionKind == protocol.SourceFixAll { + isFix = true + break + } } - if kind == protocol.QuickFix || kind == protocol.SourceFixAll { + if isFix { fixDiags = append(fixDiags, d) } else { nonFixDiags = append(nonFixDiags, d) @@ -356,25 +359,13 @@ func codeActionsMatchingDiagnostics(ctx context.Context, snapshot source.Snapsho func codeActionsForDiagnostic(ctx context.Context, snapshot source.Snapshot, sd *source.Diagnostic, pd *protocol.Diagnostic) ([]protocol.CodeAction, error) { var actions []protocol.CodeAction for _, fix := range sd.SuggestedFixes { - action := protocol.CodeAction{ - Title: fix.Title, - Kind: protocol.QuickFix, - Edit: protocol.WorkspaceEdit{}, - Command: fix.Command, - } - if pd != nil { - action.Diagnostics = []protocol.Diagnostic{*pd} - } - if sd.Analyzer != nil && sd.Analyzer.ActionKind != "" { - action.Kind = sd.Analyzer.ActionKind - } - + var changes []protocol.TextDocumentEdit for uri, edits := range fix.Edits { fh, err := snapshot.GetVersionedFile(ctx, uri) if err != nil { return nil, err } - action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, protocol.TextDocumentEdit{ + changes = append(changes, protocol.TextDocumentEdit{ TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{ Version: fh.Version(), TextDocumentIdentifier: protocol.TextDocumentIdentifier{ @@ -384,6 +375,17 @@ func codeActionsForDiagnostic(ctx context.Context, snapshot source.Snapshot, sd Edits: edits, }) } + action := protocol.CodeAction{ + Title: fix.Title, + Kind: fix.ActionKind, + Edit: protocol.WorkspaceEdit{ + DocumentChanges: changes, + }, + Command: fix.Command, + } + if pd != nil { + action.Diagnostics = []protocol.Diagnostic{*pd} + } actions = append(actions, action) } return actions, nil diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 725c5961a8..9678fb1f18 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -1039,7 +1039,7 @@ func (e *Editor) References(ctx context.Context, path string, pos Pos) ([]protoc } // CodeAction executes a codeAction request on the server. -func (e *Editor) CodeAction(ctx context.Context, path string, rng *protocol.Range) ([]protocol.CodeAction, error) { +func (e *Editor) CodeAction(ctx context.Context, path string, rng *protocol.Range, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) { if e.Server == nil { return nil, nil } @@ -1051,6 +1051,9 @@ func (e *Editor) CodeAction(ctx context.Context, path string, rng *protocol.Rang } params := &protocol.CodeActionParams{ TextDocument: e.textDocumentIdentifier(path), + Context: protocol.CodeActionContext{ + Diagnostics: diagnostics, + }, } if rng != nil { params.Range = *rng diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index 0aa8cafa51..6495aeb9b8 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -81,7 +81,7 @@ func DiagnosticsForMod(ctx context.Context, snapshot source.Snapshot, fh source. Severity: protocol.SeverityInformation, Source: source.UpgradeNotification, Message: fmt.Sprintf("%v can be upgraded", req.Mod.Path), - SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd)}, + SuggestedFixes: []source.SuggestedFix{source.SuggestedFixFromCommand(cmd, protocol.QuickFix)}, }) } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 1948381d5a..58154ca1c8 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -12,9 +12,10 @@ import ( ) type SuggestedFix struct { - Title string - Edits map[span.URI][]protocol.TextEdit - Command *protocol.Command + Title string + Edits map[span.URI][]protocol.TextEdit + Command *protocol.Command + ActionKind protocol.CodeActionKind } type RelatedInformation struct { diff --git a/internal/lsp/source/fix.go b/internal/lsp/source/fix.go index 391835593d..6a012396cc 100644 --- a/internal/lsp/source/fix.go +++ b/internal/lsp/source/fix.go @@ -42,10 +42,11 @@ var suggestedFixes = map[string]SuggestedFixFunc{ ExtractFunction: extractFunction, } -func SuggestedFixFromCommand(cmd protocol.Command) SuggestedFix { +func SuggestedFixFromCommand(cmd protocol.Command, kind protocol.CodeActionKind) SuggestedFix { return SuggestedFix{ - Title: cmd.Title, - Command: &cmd, + Title: cmd.Title, + Command: &cmd, + ActionKind: kind, } } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 0f8dda1e40..41557f4508 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -1080,7 +1080,7 @@ func typeErrorAnalyzers() map[string]*Analyzer { return map[string]*Analyzer{ fillreturns.Analyzer.Name: { Analyzer: fillreturns.Analyzer, - ActionKind: protocol.SourceFixAll, + ActionKind: []protocol.CodeActionKind{protocol.SourceFixAll, protocol.QuickFix}, Enabled: true, }, nonewvars.Analyzer.Name: { @@ -1105,7 +1105,7 @@ func convenienceAnalyzers() map[string]*Analyzer { Analyzer: fillstruct.Analyzer, Fix: FillStruct, Enabled: true, - ActionKind: protocol.RefactorRewrite, + ActionKind: []protocol.CodeActionKind{protocol.RefactorRewrite}, }, } } @@ -1150,9 +1150,21 @@ func defaultAnalyzers() map[string]*Analyzer { unusedwrite.Analyzer.Name: {Analyzer: unusedwrite.Analyzer, Enabled: false}, // gofmt -s suite: - simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, Enabled: true, ActionKind: protocol.SourceFixAll}, - simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, Enabled: true, ActionKind: protocol.SourceFixAll}, - simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, Enabled: true, ActionKind: protocol.SourceFixAll}, + simplifycompositelit.Analyzer.Name: { + Analyzer: simplifycompositelit.Analyzer, + Enabled: true, + ActionKind: []protocol.CodeActionKind{protocol.SourceFixAll, protocol.QuickFix}, + }, + simplifyrange.Analyzer.Name: { + Analyzer: simplifyrange.Analyzer, + Enabled: true, + ActionKind: []protocol.CodeActionKind{protocol.SourceFixAll, protocol.QuickFix}, + }, + simplifyslice.Analyzer.Name: { + Analyzer: simplifyslice.Analyzer, + Enabled: true, + ActionKind: []protocol.CodeActionKind{protocol.SourceFixAll, protocol.QuickFix}, + }, } } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 9292a84ec4..412866c0dc 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -531,7 +531,7 @@ type Analyzer struct { // ActionKind is the kind of code action this analyzer produces. If // unspecified the type defaults to quickfix. - ActionKind protocol.CodeActionKind + ActionKind []protocol.CodeActionKind } func (a Analyzer) IsEnabled(view View) bool {