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 <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go
index 436c091..ae43bea 100644
--- a/gopls/internal/regtest/watch/watch_test.go
+++ b/gopls/internal/regtest/watch/watch_test.go
@@ -370,6 +370,7 @@
// 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 69ee199..9b76f3f 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -69,7 +69,7 @@
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 @@
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 @@
// 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 @@
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 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 0368771..6b75feb 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -265,7 +265,7 @@
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 e1a7416..1948381 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -23,39 +23,19 @@
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 @@
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 @@
}
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 d36d129..0f8dda1 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -97,6 +97,7 @@
},
Mod: {
protocol.SourceOrganizeImports: true,
+ protocol.QuickFix: true,
},
Sum: {},
},
@@ -1078,9 +1079,9 @@
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 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 @@
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 57712b6..9bc5eca 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -520,9 +520,9 @@
// 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 {
