From 11e8f6b853047b22d03ff674d2ae00e2bb406571 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 9 Mar 2021 12:23:42 -0500 Subject: [PATCH] internal/lsp: refactor codeAction As much as possible, try to unify the codeAction code paths. We always run analysis now. And rather than assuming certain categories of analyzers will generate certain kinds of code actions, mark them explicitly and use that information to filter the actions afterward. Change-Id: I8154cd67aa8b59b2a6c8aa9c3ea811de2e190db4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/300170 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- gopls/internal/regtest/watch/watch_test.go | 1 + internal/lsp/code_action.go | 286 ++++++++------------- internal/lsp/diagnostics.go | 2 +- internal/lsp/source/diagnostics.go | 45 ++-- internal/lsp/source/options.go | 20 +- internal/lsp/source/view.go | 6 +- 6 files changed, 141 insertions(+), 219 deletions(-) diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go index 436c09153a..ae43beaf0c 100644 --- a/gopls/internal/regtest/watch/watch_test.go +++ b/gopls/internal/regtest/watch/watch_test.go @@ -370,6 +370,7 @@ func _() { // Tests golang/go#38498. Delete a file and then force a reload. // Assert that we no longer try to load the file. func TestDeleteFiles(t *testing.T) { + testenv.NeedsGo1Point(t, 13) // Poor overlay support causes problems on 1.12. const pkg = ` -- go.mod -- module mod.com diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 69ee199505..9b76f3ff4d 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -69,7 +69,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara if err != nil { return nil, err } - quickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, diags) + quickFixes, err := codeActionsMatchingDiagnostics(ctx, snapshot, diagnostics, diags) if err != nil { return nil, err } @@ -128,75 +128,68 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara if err != nil { return nil, err } - if (wanted[protocol.QuickFix] || wanted[protocol.SourceFixAll]) && len(diagnostics) > 0 { - analysisDiags, err := source.Analyze(ctx, snapshot, pkg) + + pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) + if err != nil { + return nil, err + } + analysisDiags, err := source.Analyze(ctx, snapshot, pkg, true) + if err != nil { + return nil, err + } + fileDiags := append(pkgDiagnostics[uri], analysisDiags[uri]...) + modURI := snapshot.GoModForFile(fh.URI()) + if modURI != "" { + modFH, err := snapshot.GetVersionedFile(ctx, modURI) if err != nil { return nil, err } - - if wanted[protocol.QuickFix] { - pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) - if err != nil { - return nil, err - } - quickFixDiags := append(pkgDiagnostics[uri], analysisDiags[uri]...) - modURI := snapshot.GoModForFile(fh.URI()) - if modURI != "" { - modFH, err := snapshot.GetVersionedFile(ctx, modURI) - if err != nil { - return nil, err - } - modDiags, err := mod.DiagnosticsForMod(ctx, snapshot, modFH) - if err != nil && !source.IsNonFatalGoModError(err) { - // Not a fatal error. - event.Error(ctx, "module suggested fixes failed", err, tag.Directory.Of(snapshot.View().Folder())) - } - quickFixDiags = append(quickFixDiags, modDiags...) - } - quickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, quickFixDiags) - if err != nil { - return nil, err - } - codeActions = append(codeActions, quickFixes...) - + modDiags, err := mod.DiagnosticsForMod(ctx, snapshot, modFH) + if err != nil && !source.IsNonFatalGoModError(err) { + // Not a fatal error. + event.Error(ctx, "module suggested fixes failed", err, tag.Directory.Of(snapshot.View().Folder())) } - if wanted[protocol.SourceFixAll] { - var fixAllEdits []protocol.TextDocumentEdit - for _, ad := range analysisDiags[uri] { - if ad.Analyzer == nil || !ad.Analyzer.HighConfidence { - continue - } - for _, fix := range ad.SuggestedFixes { - edits := fix.Edits[fh.URI()] - if len(edits) == 0 { - continue - } - fixAllEdits = append(fixAllEdits, documentChanges(fh, edits)...) - } + fileDiags = append(fileDiags, modDiags...) + } - } - if len(fixAllEdits) != 0 { - codeActions = append(codeActions, protocol.CodeAction{ - Title: "Simplifications", - Kind: protocol.SourceFixAll, - Edit: protocol.WorkspaceEdit{ - DocumentChanges: fixAllEdits, - }, - }) - } + // Split diagnostics into fixes, which must match incoming diagnostics, + // and non-fixes, which must match the requested range. Build actions + // for all of them. + var fixDiags, nonFixDiags []*source.Diagnostic + for _, d := range fileDiags { + if len(d.SuggestedFixes) == 0 { + continue + } + kind := protocol.QuickFix + if d.Analyzer != nil && d.Analyzer.ActionKind != "" { + kind = d.Analyzer.ActionKind + } + if kind == protocol.QuickFix || kind == protocol.SourceFixAll { + fixDiags = append(fixDiags, d) + } else { + nonFixDiags = append(nonFixDiags, d) } } - if ctx.Err() != nil { - return nil, ctx.Err() + + fixActions, err := codeActionsMatchingDiagnostics(ctx, snapshot, diagnostics, fixDiags) + if err != nil { + return nil, err } - // Add any suggestions that do not necessarily fix any diagnostics. - if wanted[protocol.RefactorRewrite] { - fixes, err := convenienceFixes(ctx, snapshot, pkg, uri, params.Range) + codeActions = append(codeActions, fixActions...) + + for _, nonfix := range nonFixDiags { + // For now, only show diagnostics for matching lines. Maybe we should + // alter this behavior in the future, depending on the user experience. + if !protocol.Intersect(nonfix.Range, params.Range) { + continue + } + actions, err := codeActionsForDiagnostic(ctx, snapshot, nonfix, nil) if err != nil { return nil, err } - codeActions = append(codeActions, fixes...) + codeActions = append(codeActions, actions...) } + if wanted[protocol.RefactorExtract] { fixes, err := extractionFixes(ctx, snapshot, pkg, uri, params.Range) if err != nil { @@ -217,7 +210,14 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara // Unsupported file kind for a code action. return nil, nil } - return codeActions, nil + + var filtered []protocol.CodeAction + for _, action := range codeActions { + if wanted[action.Kind] { + filtered = append(filtered, action) + } + } + return filtered, nil } func (s *Server) getSupportedCodeActions() []protocol.CodeActionKind { @@ -278,94 +278,6 @@ func importDiagnostics(fix *imports.ImportFix, diagnostics []protocol.Diagnostic return results } -// diagnosticToAnalyzer return the analyzer associated with a given diagnostic. -// It assumes that the diagnostic's source will be the name of the analyzer. -// If this changes, this approach will need to be reworked. -func diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer *source.Analyzer) { - // Make sure that the analyzer we found is enabled. - defer func() { - if analyzer != nil && !analyzer.IsEnabled(snapshot.View()) { - analyzer = nil - } - }() - if a, ok := snapshot.View().Options().DefaultAnalyzers[src]; ok { - return a - } - if a, ok := snapshot.View().Options().StaticcheckAnalyzers[src]; ok { - return a - } - if a, ok := snapshot.View().Options().ConvenienceAnalyzers[src]; ok { - return a - } - return nil -} - -func convenienceFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) { - var analyzers []*source.Analyzer - for _, a := range snapshot.View().Options().ConvenienceAnalyzers { - if !a.IsEnabled(snapshot.View()) { - continue - } - if a.Fix == "" { - event.Error(ctx, "convenienceFixes", fmt.Errorf("no suggested fixes for convenience analyzer %s", a.Analyzer.Name)) - continue - } - analyzers = append(analyzers, a) - } - diagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers) - if err != nil { - return nil, err - } - var codeActions []protocol.CodeAction - for _, d := range diagnostics { - // For now, only show diagnostics for matching lines. Maybe we should - // alter this behavior in the future, depending on the user experience. - if d.URI != uri { - continue - } - - if !protocol.Intersect(d.Range, rng) { - continue - } - action, err := diagnosticToCommandCodeAction(ctx, snapshot, d, nil, protocol.RefactorRewrite) - if err != nil { - return nil, err - } - codeActions = append(codeActions, *action) - } - return codeActions, nil -} - -func diagnosticToCommandCodeAction(ctx context.Context, snapshot source.Snapshot, sd *source.Diagnostic, pd *protocol.Diagnostic, kind protocol.CodeActionKind) (*protocol.CodeAction, error) { - // The fix depends on the category of the analyzer. The diagnostic may be - // nil, so use the error's category. - analyzer := diagnosticToAnalyzer(snapshot, string(sd.Source), sd.Message) - if analyzer == nil { - return nil, fmt.Errorf("no convenience analyzer for source %s", sd.Source) - } - if analyzer.Fix == "" { - return nil, fmt.Errorf("no fix for convenience analyzer %s", analyzer.Analyzer.Name) - } - cmd, err := command.NewApplyFixCommand(sd.Message, command.ApplyFixArgs{ - URI: protocol.URIFromSpanURI(sd.URI), - Range: sd.Range, - Fix: analyzer.Fix, - }) - if err != nil { - return nil, err - } - var diagnostics []protocol.Diagnostic - if pd != nil { - diagnostics = append(diagnostics, *pd) - } - return &protocol.CodeAction{ - Title: sd.Message, - Kind: kind, - Diagnostics: diagnostics, - Command: &cmd, - }, nil -} - func extractionFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) { if rng.Start == rng.End { return nil, nil @@ -431,47 +343,63 @@ func documentChanges(fh source.VersionedFileHandle, edits []protocol.TextEdit) [ } } -func quickFixesForDiagnostics(ctx context.Context, snapshot source.Snapshot, pdiags []protocol.Diagnostic, sdiags []*source.Diagnostic) ([]protocol.CodeAction, error) { - var quickFixes []protocol.CodeAction - for _, e := range sdiags { +func codeActionsMatchingDiagnostics(ctx context.Context, snapshot source.Snapshot, pdiags []protocol.Diagnostic, sdiags []*source.Diagnostic) ([]protocol.CodeAction, error) { + var actions []protocol.CodeAction + for _, sd := range sdiags { var diag *protocol.Diagnostic - for _, d := range pdiags { - if sameDiagnostic(d, e) { - diag = &d + for _, pd := range pdiags { + if sameDiagnostic(pd, sd) { + diag = &pd break } } if diag == nil { continue } - for _, fix := range e.SuggestedFixes { - action := protocol.CodeAction{ - Title: fix.Title, - Kind: protocol.QuickFix, - Diagnostics: []protocol.Diagnostic{*diag}, - Edit: protocol.WorkspaceEdit{}, - Command: fix.Command, - } - - 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{ - TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{ - Version: fh.Version(), - TextDocumentIdentifier: protocol.TextDocumentIdentifier{ - URI: protocol.URIFromSpanURI(uri), - }, - }, - Edits: edits, - }) - } - quickFixes = append(quickFixes, action) + diagActions, err := codeActionsForDiagnostic(ctx, snapshot, sd, diag) + if err != nil { + return nil, err } + actions = append(actions, diagActions...) + } - return quickFixes, nil + return actions, nil +} + +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 + } + + 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{ + TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{ + Version: fh.Version(), + TextDocumentIdentifier: protocol.TextDocumentIdentifier{ + URI: protocol.URIFromSpanURI(uri), + }, + }, + Edits: edits, + }) + } + actions = append(actions, action) + } + return actions, nil } func sameDiagnostic(pd protocol.Diagnostic, sd *source.Diagnostic) bool { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 036877163b..6b75febbbc 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -265,7 +265,7 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg s.storeDiagnostics(snapshot, cgf.URI, typeCheckSource, pkgDiagnostics[cgf.URI]) } if includeAnalysis && !pkg.HasListOrParseErrors() { - reports, err := source.Analyze(ctx, snapshot, pkg) + reports, err := source.Analyze(ctx, snapshot, pkg, false) if err != nil { event.Error(ctx, "warning: analyzing package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID())) return diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index e1a74160d7..1948381d5a 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -23,39 +23,19 @@ type RelatedInformation struct { Message string } -func Analyze(ctx context.Context, snapshot Snapshot, pkg Package) (map[span.URI][]*Diagnostic, error) { +func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, includeConvenience bool) (map[span.URI][]*Diagnostic, error) { // Exit early if the context has been canceled. This also protects us // from a race on Options, see golang/go#36699. if ctx.Err() != nil { return nil, ctx.Err() } - // If we don't have any list or parse errors, run analyses. - analyzers := pickAnalyzers(snapshot, pkg.HasTypeErrors()) - analysisDiagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers) - if err != nil { - return nil, err - } - reports := map[span.URI][]*Diagnostic{} - // Report diagnostics and errors from root analyzers. - for _, diag := range analysisDiagnostics { - // If the diagnostic comes from a "convenience" analyzer, it is not - // meant to provide diagnostics, but rather only suggested fixes. - // Skip these types of errors in diagnostics; we will use their - // suggested fixes when providing code actions. - if isConvenienceAnalyzer(string(diag.Source)) { - continue - } - reports[diag.URI] = append(reports[diag.URI], diag) + categories := []map[string]*Analyzer{} + if includeConvenience { + categories = append(categories, snapshot.View().Options().ConvenienceAnalyzers) } - return reports, nil -} - -func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) []*Analyzer { - // Always run convenience analyzers. - categories := []map[string]*Analyzer{snapshot.View().Options().ConvenienceAnalyzers} // If we had type errors, don't run any other analyzers. - if !hadTypeErrors { + if !pkg.HasTypeErrors() { categories = append(categories, snapshot.View().Options().DefaultAnalyzers, snapshot.View().Options().StaticcheckAnalyzers) } var analyzers []*Analyzer @@ -64,7 +44,18 @@ func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) []*Analyzer { analyzers = append(analyzers, a) } } - return analyzers + + analysisDiagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers) + if err != nil { + return nil, err + } + + reports := map[span.URI][]*Diagnostic{} + // Report diagnostics and errors from root analyzers. + for _, diag := range analysisDiagnostics { + reports[diag.URI] = append(reports[diag.URI], diag) + } + return reports, nil } func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (VersionedFileIdentity, []*Diagnostic, error) { @@ -82,7 +73,7 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers } fileDiags := diagnostics[fh.URI()] if !pkg.HasListOrParseErrors() { - analysisDiags, err := Analyze(ctx, snapshot, pkg) + analysisDiags, err := Analyze(ctx, snapshot, pkg, false) if err != nil { return VersionedFileIdentity{}, nil, err } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index d36d12949f..0f8dda1e40 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -97,6 +97,7 @@ func DefaultOptions() *Options { }, Mod: { protocol.SourceOrganizeImports: true, + protocol.QuickFix: true, }, Sum: {}, }, @@ -1078,9 +1079,9 @@ func EnabledAnalyzers(snapshot Snapshot) (analyzers []*Analyzer) { func typeErrorAnalyzers() map[string]*Analyzer { return map[string]*Analyzer{ fillreturns.Analyzer.Name: { - Analyzer: fillreturns.Analyzer, - HighConfidence: true, - Enabled: true, + Analyzer: fillreturns.Analyzer, + ActionKind: protocol.SourceFixAll, + Enabled: true, }, nonewvars.Analyzer.Name: { Analyzer: nonewvars.Analyzer, @@ -1101,9 +1102,10 @@ func typeErrorAnalyzers() map[string]*Analyzer { func convenienceAnalyzers() map[string]*Analyzer { return map[string]*Analyzer{ fillstruct.Analyzer.Name: { - Analyzer: fillstruct.Analyzer, - Fix: FillStruct, - Enabled: true, + Analyzer: fillstruct.Analyzer, + Fix: FillStruct, + Enabled: true, + ActionKind: protocol.RefactorRewrite, }, } } @@ -1148,9 +1150,9 @@ func defaultAnalyzers() map[string]*Analyzer { unusedwrite.Analyzer.Name: {Analyzer: unusedwrite.Analyzer, Enabled: false}, // gofmt -s suite: - simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, Enabled: true, HighConfidence: true}, - simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, Enabled: true, HighConfidence: true}, - simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, Enabled: true, HighConfidence: true}, + 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}, } } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 57712b6d3d..9bc5eca3b9 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -520,9 +520,9 @@ type Analyzer struct { // the analyzer's suggested fixes through a Command, not a TextEdit. Fix string - // If this is true, then we can apply the suggested fixes - // as part of a source.FixAll codeaction. - HighConfidence bool + // ActionKind is the kind of code action this analyzer produces. If + // unspecified the type defaults to quickfix. + ActionKind protocol.CodeActionKind } func (a Analyzer) IsEnabled(view View) bool {