From 376db57240db1206c484ac8f7c3257422be08047 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 2 Mar 2021 18:19:32 -0500 Subject: [PATCH] internal/lsp: use pre-existing quick fixes for analysis diagnostics Now that we're generating quick fixes at analysis time, we can use those in code action requests and delete a fair amount of redundancy. The codeAction function is a little cluttered, but I want to get it all in one place before I decide how to split it up. Change-Id: Icd91e2547542cce0a05c18c02a088833f71232a4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/297532 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/errors.go | 1 + internal/lsp/code_action.go | 180 +++++++++-------------------- internal/lsp/diagnostics.go | 2 +- internal/lsp/source/diagnostics.go | 4 +- internal/lsp/source/view.go | 1 + 5 files changed, 60 insertions(+), 128 deletions(-) diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index 072d78fc5d..a876473f66 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -185,6 +185,7 @@ func analysisDiagnosticDiagnostics(ctx context.Context, snapshot *snapshot, pkg Message: e.Message, Related: related, SuggestedFixes: fixes, + Analyzer: srcAnalyzer, } // If the fixes only delete code, assume that the diagnostic is reporting dead code. if onlyDeletions(fixes) { diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 0608740bc8..69ee199505 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -62,14 +62,18 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara switch fh.Kind() { case source.Mod: if diagnostics := params.Context.Diagnostics; len(diagnostics) > 0 { - modQuickFixes, err := moduleQuickFixes(ctx, snapshot, fh, diagnostics) + diags, err := mod.DiagnosticsForMod(ctx, snapshot, fh) if source.IsNonFatalGoModError(err) { return nil, nil } if err != nil { return nil, err } - codeActions = append(codeActions, modQuickFixes...) + quickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, diags) + if err != nil { + return nil, err + } + codeActions = append(codeActions, quickFixes...) } case source.Go: // Don't suggest fixes for generated files, since they are generally @@ -125,40 +129,61 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara return nil, err } if (wanted[protocol.QuickFix] || wanted[protocol.SourceFixAll]) && len(diagnostics) > 0 { - pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) + analysisDiags, err := source.Analyze(ctx, snapshot, pkg) if err != nil { return nil, err } - diagnosticFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, pkgDiagnostics[fh.URI()]) - if err != nil { - return nil, err - } - codeActions = append(codeActions, diagnosticFixes...) - analysisQuickFixes, highConfidenceEdits, err := analysisFixes(ctx, snapshot, pkg, diagnostics) - if err != nil { - return nil, err - } - if wanted[protocol.QuickFix] { - // Add the quick fixes reported by go/analysis. - codeActions = append(codeActions, analysisQuickFixes...) - // If there are any diagnostics relating to the go.mod file, - // add their corresponding quick fixes. - modQuickFixes, err := moduleQuickFixes(ctx, snapshot, fh, diagnostics) - if source.IsNonFatalGoModError(err) { - // Not a fatal error. - event.Error(ctx, "module suggested fixes failed", err, tag.Directory.Of(snapshot.View().Folder())) + if wanted[protocol.QuickFix] { + pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) + if err != nil { + return nil, err } - codeActions = append(codeActions, modQuickFixes...) + 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...) + } - if wanted[protocol.SourceFixAll] && len(highConfidenceEdits) > 0 { - codeActions = append(codeActions, protocol.CodeAction{ - Title: "Simplifications", - Kind: protocol.SourceFixAll, - Edit: protocol.WorkspaceEdit{ - DocumentChanges: highConfidenceEdits, - }, - }) + 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)...) + } + + } + if len(fixAllEdits) != 0 { + codeActions = append(codeActions, protocol.CodeAction{ + Title: "Simplifications", + Kind: protocol.SourceFixAll, + Edit: protocol.WorkspaceEdit{ + DocumentChanges: fixAllEdits, + }, + }) + } } } if ctx.Err() != nil { @@ -253,78 +278,6 @@ func importDiagnostics(fix *imports.ImportFix, diagnostics []protocol.Diagnostic return results } -func analysisFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, []protocol.TextDocumentEdit, error) { - if len(diagnostics) == 0 { - return nil, nil, nil - } - var ( - codeActions []protocol.CodeAction - sourceFixAllEdits []protocol.TextDocumentEdit - ) - for _, diag := range diagnostics { - srcErr, analyzer, ok := findDiagnostic(ctx, snapshot, pkg.ID(), diag) - if !ok { - continue - } - // If the suggested fix for the diagnostic is expected to be separate, - // see if there are any supported commands available. - if analyzer.Fix != "" { - action, err := diagnosticToCommandCodeAction(ctx, snapshot, srcErr, &diag, protocol.QuickFix) - if err != nil { - return nil, nil, err - } - codeActions = append(codeActions, *action) - continue - } - for _, fix := range srcErr.SuggestedFixes { - action := protocol.CodeAction{ - Title: fix.Title, - Kind: protocol.QuickFix, - Diagnostics: []protocol.Diagnostic{diag}, - Edit: protocol.WorkspaceEdit{}, - } - for uri, edits := range fix.Edits { - fh, err := snapshot.GetVersionedFile(ctx, uri) - if err != nil { - return nil, nil, err - } - docChanges := documentChanges(fh, edits) - if analyzer.HighConfidence { - sourceFixAllEdits = append(sourceFixAllEdits, docChanges...) - } - action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, docChanges...) - } - codeActions = append(codeActions, action) - } - } - return codeActions, sourceFixAllEdits, nil -} - -func findDiagnostic(ctx context.Context, snapshot source.Snapshot, pkgID string, diag protocol.Diagnostic) (*source.Diagnostic, source.Analyzer, bool) { - analyzer := diagnosticToAnalyzer(snapshot, diag.Source, diag.Message) - if analyzer == nil { - return nil, source.Analyzer{}, false - } - analysisErrors, err := snapshot.Analyze(ctx, pkgID, []*source.Analyzer{analyzer}) - if err != nil { - return nil, source.Analyzer{}, false - } - for _, err := range analysisErrors { - if err.Message != diag.Message { - continue - } - if protocol.CompareRange(err.Range, diag.Range) != 0 { - continue - } - if string(err.Source) != analyzer.Analyzer.Name { - continue - } - // The error matches. - return err, *analyzer, true - } - return nil, source.Analyzer{}, false -} - // 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. @@ -478,29 +431,6 @@ func documentChanges(fh source.VersionedFileHandle, edits []protocol.TextEdit) [ } } -func moduleQuickFixes(ctx context.Context, snapshot source.Snapshot, fh source.VersionedFileHandle, pdiags []protocol.Diagnostic) ([]protocol.CodeAction, error) { - var modFH source.VersionedFileHandle - switch fh.Kind() { - case source.Mod: - modFH = fh - case source.Go: - modURI := snapshot.GoModForFile(fh.URI()) - if modURI == "" { - return nil, nil - } - var err error - modFH, err = snapshot.GetVersionedFile(ctx, modURI) - if err != nil { - return nil, err - } - } - diags, err := mod.DiagnosticsForMod(ctx, snapshot, modFH) - if err != nil { - return nil, err - } - return quickFixesForDiagnostics(ctx, snapshot, pdiags, diags) -} - 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 { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 0f7ba4a209..036877163b 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, pkgDiagnostics) + reports, err := source.Analyze(ctx, snapshot, pkg) 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 db8abd56d4..e1a74160d7 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -23,7 +23,7 @@ type RelatedInformation struct { Message string } -func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, typeCheckDiagnostics map[span.URI][]*Diagnostic) (map[span.URI][]*Diagnostic, error) { +func Analyze(ctx context.Context, snapshot Snapshot, pkg Package) (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 { @@ -82,7 +82,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, diagnostics) + analysisDiags, err := Analyze(ctx, snapshot, pkg) if err != nil { return VersionedFileIdentity{}, nil, err } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index d36c41bc00..57712b6d3d 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -588,6 +588,7 @@ type Diagnostic struct { // Fields below are used internally to generate quick fixes. They aren't // part of the LSP spec and don't leave the server. SuggestedFixes []SuggestedFix + Analyzer *Analyzer } type DiagnosticSource string