cybercyst/go
1
0
Fork 0
mirror of https://github.com/golang/go.git synced 2025-05-05 15:43:04 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

internal/lsp: associate code action diagnostics with suggested fixes

Browse Source 
Instead of relying on the diagnostics cached on the package, use the
diagnostics sent by the code action when computing suggested fixes.

Change-Id: I77f7fd468b34b824c6c5000a51edbe0f8cc6f637
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197097
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This commit is contained in:
Rebecca Stambler 2019-09-24 16:28:59 -04:00
parent c006dc79eb
commit 3af8461759
9 changed files with 122 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
internal/lsp/cache/pkg.go vendored
View File

@ -13,6 +13,7 @@ import (
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/packages"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/span"
errors "golang.org/x/xerrors"
@ -200,15 +201,25 @@ func (pkg *pkg) SetDiagnostics(a *analysis.Analyzer, diags []source.Diagnostic)
pkg.diagnostics[a] = diags
}
func (pkg *pkg) GetDiagnostics() []source.Diagnostic {
pkg.diagMu.Lock()
defer pkg.diagMu.Unlock()
func (p *pkg) FindDiagnostic(pdiag protocol.Diagnostic) (*source.Diagnostic, error) {
p.diagMu.Lock()
defer p.diagMu.Unlock()
var diags []source.Diagnostic
for _, d := range pkg.diagnostics {
diags = append(diags, d...)
for a, diagnostics := range p.diagnostics {
if a.Name != pdiag.Source {
continue
}
for _, d := range diagnostics {
if d.Message != pdiag.Message {
continue
}
if protocol.CompareRange(d.Range, pdiag.Range) != 0 {
continue
}
return &d, nil
}
}
return diags
return nil, errors.Errorf("no matching diagnostic for %v", pdiag)
}
func (p *pkg) FindFile(ctx context.Context, uri span.URI) (source.ParseGoHandle, source.Package, error) {

40
internal/lsp/code_action.go
View File

@ -75,21 +75,21 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
if err != nil {
return nil, err
}
if wanted[protocol.QuickFix] {
if diagnostics := params.Context.Diagnostics; wanted[protocol.QuickFix] && len(diagnostics) > 0 {
// First, add the quick fixes reported by go/analysis.
qf, err := quickFixes(ctx, view, gof)
qf, err := quickFixes(ctx, view, gof, diagnostics)
if err != nil {
log.Error(ctx, "quick fixes failed", err, telemetry.File.Of(uri))
}
codeActions = append(codeActions, qf...)
// If we also have diagnostics for missing imports, we can associate them with quick fixes.
if findImportErrors(params.Context.Diagnostics) {
if findImportErrors(diagnostics) {
// Separate this into a set of codeActions per diagnostic, where
// each action is the addition, removal, or renaming of one import.
for _, importFix := range editsPerFix {
// Get the diagnostics this fix would affect.
if fixDiagnostics := importDiagnostics(importFix.Fix, params.Context.Diagnostics); len(fixDiagnostics) > 0 {
if fixDiagnostics := importDiagnostics(importFix.Fix, diagnostics); len(fixDiagnostics) > 0 {
codeActions = append(codeActions, protocol.CodeAction{
Title: importFixTitle(importFix.Fix),
Kind: protocol.QuickFix,
@ -207,36 +207,36 @@ func importDiagnostics(fix *imports.ImportFix, diagnostics []protocol.Diagnostic
return results
}
func quickFixes(ctx context.Context, view source.View, gof source.GoFile) ([]protocol.CodeAction, error) {
func quickFixes(ctx context.Context, view source.View, gof source.GoFile, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) {
var codeActions []protocol.CodeAction
// TODO: This is technically racy because the diagnostics provided by the code action
// may not be the same as the ones that gopls is aware of.
// We need to figure out some way to solve this problem.
cphs, err := gof.CheckPackageHandles(ctx)
if err != nil {
return nil, err
}
cph := source.NarrowestCheckPackageHandle(cphs)
// We get the package that source.Diagnostics would've used. This is hack.
// TODO(golang/go#32443): The correct solution will be to cache diagnostics per-file per-snapshot.
cph := source.WidestCheckPackageHandle(cphs)
pkg, err := cph.Cached(ctx)
if err != nil {
return nil, err
}
for _, diag := range pkg.GetDiagnostics() {
pdiag, err := toProtocolDiagnostic(ctx, diag)
for _, diag := range diagnostics {
sdiag, err := pkg.FindDiagnostic(diag)
if err != nil {
return nil, err
continue
}
for _, fix := range diag.SuggestedFixes {
for _, fix := range sdiag.SuggestedFixes {
edits := make(map[string][]protocol.TextEdit)
for uri, e := range fix.Edits {
edits[protocol.NewURI(uri)] = e
}
codeActions = append(codeActions, protocol.CodeAction{
Title: fix.Title,
Kind: protocol.QuickFix, // TODO(matloob): Be more accurate about these?
Title: fix.Title,
Kind: protocol.QuickFix,
Diagnostics: []protocol.Diagnostic{diag},
Edit: &protocol.WorkspaceEdit{
Changes: &map[string][]protocol.TextEdit{
protocol.NewURI(diag.URI): fix.Edits,
},
Changes: &edits,
},
Diagnostics: []protocol.Diagnostic{pdiag},
})
}
}

21
internal/lsp/diagnostics.go
View File

@ -74,30 +74,22 @@ func (s *Server) diagnostics(view source.View, uri span.URI) error {
}
func (s *Server) publishDiagnostics(ctx context.Context, uri span.URI, diagnostics []source.Diagnostic) error {
protocolDiagnostics, err := toProtocolDiagnostics(ctx, diagnostics)
if err != nil {
return err
}
s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
Diagnostics: protocolDiagnostics,
Diagnostics: toProtocolDiagnostics(ctx, diagnostics),
URI: protocol.NewURI(uri),
})
return nil
}
func toProtocolDiagnostics(ctx context.Context, diagnostics []source.Diagnostic) ([]protocol.Diagnostic, error) {
func toProtocolDiagnostics(ctx context.Context, diagnostics []source.Diagnostic) []protocol.Diagnostic {
reports := []protocol.Diagnostic{}
for _, diag := range diagnostics {
diagnostic, err := toProtocolDiagnostic(ctx, diag)
if err != nil {
return nil, err
}
reports = append(reports, diagnostic)
reports = append(reports, toProtocolDiagnostic(ctx, diag))
}
return reports, nil
return reports
}
func toProtocolDiagnostic(ctx context.Context, diag source.Diagnostic) (protocol.Diagnostic, error) {
func toProtocolDiagnostic(ctx context.Context, diag source.Diagnostic) protocol.Diagnostic {
var severity protocol.DiagnosticSeverity
switch diag.Severity {
case source.SeverityError:
@ -110,5 +102,6 @@ func toProtocolDiagnostic(ctx context.Context, diag source.Diagnostic) (protocol
Range: diag.Range,
Severity: severity,
Source: diag.Source,
}, nil
Tags: diag.Tags,
}
}

12
internal/lsp/lsp_test.go
View File

@ -331,26 +331,28 @@ func (r *runner) SuggestedFix(t *testing.T, data tests.SuggestedFixes) {
for _, spn := range data {
uri := spn.URI()
filename := uri.Filename()
v := r.server.session.ViewOf(uri)
view := r.server.session.ViewOf(uri)
fixed := string(r.data.Golden("suggestedfix", filename, func() ([]byte, error) {
cmd := exec.Command("suggestedfix", filename) // TODO(matloob): what do we do here?
out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
return out, nil
}))
f, err := getGoFile(r.ctx, v, uri)
f, err := getGoFile(r.ctx, view, uri)
if err != nil {
t.Fatal(err)
}
results, _, err := source.Diagnostics(r.ctx, v, f, nil)
diagnostics, _, err := source.Diagnostics(r.ctx, view, f, nil)
if err != nil {
t.Fatal(err)
}
_ = results
actions, err := r.server.CodeAction(r.ctx, &protocol.CodeActionParams{
TextDocument: protocol.TextDocumentIdentifier{
URI: protocol.NewURI(uri),
},
Context: protocol.CodeActionContext{Only: []protocol.CodeActionKind{protocol.QuickFix}},
Context: protocol.CodeActionContext{
Only: []protocol.CodeActionKind{protocol.QuickFix},
Diagnostics: toProtocolDiagnostics(r.ctx, diagnostics[uri]),
},
})
if err != nil {
if fixed != "" {

11
internal/lsp/source/analysis.go
View File

@ -61,17 +61,18 @@ func analyze(ctx context.Context, v View, cphs []CheckPackageHandle, analyzers [
// package (as different analyzers are applied, either in sequence or
// parallel), and across packages (as dependencies are analyzed).
type Action struct {
once sync.Once
Analyzer *analysis.Analyzer
Pkg Package
Deps []*Action
once sync.Once
Analyzer *analysis.Analyzer
Pkg Package
Deps []*Action
diagnostics []analysis.Diagnostic
pass *analysis.Pass
isroot bool
objectFacts map[objectFactKey]analysis.Fact
packageFacts map[packageFactKey]analysis.Fact
inputs map[*analysis.Analyzer]interface{}
result interface{}
diagnostics []analysis.Diagnostic
err error
duration time.Duration
view View

25
internal/lsp/source/diagnostics.go
View File

@ -26,15 +26,11 @@ type Diagnostic struct {
Message string
Source string
Severity DiagnosticSeverity
Tags []protocol.DiagnosticTag
SuggestedFixes []SuggestedFix
}
type SuggestedFix struct {
Title string
Edits []protocol.TextEdit
}
type DiagnosticSeverity int
const (
@ -238,22 +234,29 @@ func toDiagnostic(ctx context.Context, view View, diag analysis.Diagnostic, cate
if err != nil {
return Diagnostic{}, err
}
ca, err := getCodeActions(ctx, view, pkg, diag)
if err != nil {
return Diagnostic{}, err
}
rng, err := spanToRange(ctx, view, pkg, spn, false)
if err != nil {
return Diagnostic{}, err
}
fixes, err := suggestedFixes(ctx, view, pkg, diag)
if err != nil {
return Diagnostic{}, err
}
// This is a bit of a hack, but clients > 3.15 will be able to grey out unnecessary code.
// If we are deleting code as part of all of our suggested fixes, assume that this is dead code.
// TODO(golang/go/#34508): Return these codes from the diagnostics themselves.
var tags []protocol.DiagnosticTag
if onlyDeletions(fixes) {
tags = append(tags, protocol.Unnecessary)
}
return Diagnostic{
URI: spn.URI(),
Range: rng,
Source: category,
Message: diag.Message,
Severity: SeverityWarning,
SuggestedFixes: ca,
SuggestedFixes: fixes,
Tags: tags,
}, nil
}

31
internal/lsp/source/suggested_fix.go
View File

@ -8,13 +8,19 @@ import (
"golang.org/x/tools/internal/span"
)
func getCodeActions(ctx context.Context, view View, pkg Package, diag analysis.Diagnostic) ([]SuggestedFix, error) {
type SuggestedFix struct {
Title string
Edits map[span.URI][]protocol.TextEdit
}
func suggestedFixes(ctx context.Context, view View, pkg Package, diag analysis.Diagnostic) ([]SuggestedFix, error) {
var fixes []SuggestedFix
for _, fix := range diag.SuggestedFixes {
var edits []protocol.TextEdit
edits := make(map[span.URI][]protocol.TextEdit)
for _, e := range fix.TextEdits {
posn := view.Session().Cache().FileSet().Position(e.Pos)
ph, _, err := pkg.FindFile(ctx, span.FileURI(posn.Filename))
uri := span.FileURI(posn.Filename)
ph, _, err := pkg.FindFile(ctx, uri)
if err != nil {
return nil, err
}
@ -30,7 +36,7 @@ func getCodeActions(ctx context.Context, view View, pkg Package, diag analysis.D
if err != nil {
return nil, err
}
edits = append(edits, protocol.TextEdit{
edits[uri] = append(edits[uri], protocol.TextEdit{
Range: rng,
NewText: string(e.NewText),
})
@ -42,3 +48,20 @@ func getCodeActions(ctx context.Context, view View, pkg Package, diag analysis.D
}
return fixes, nil
}
// onlyDeletions returns true if all of the suggested fixes are deletions.
func onlyDeletions(fixes []SuggestedFix) bool {
for _, fix := range fixes {
for _, edits := range fix.Edits {
for _, edit := range edits {
if edit.NewText != "" {
return false
}
if protocol.ComparePosition(edit.Range.Start, edit.Range.End) == 0 {
return false
}
}
}
}
return true
}

5
internal/lsp/source/view.go
View File

@ -297,8 +297,9 @@ type Package interface {
GetTypesInfo() *types.Info
GetTypesSizes() types.Sizes
IsIllTyped() bool
GetDiagnostics() []Diagnostic
SetDiagnostics(a *analysis.Analyzer, diag []Diagnostic)
SetDiagnostics(*analysis.Analyzer, []Diagnostic)
FindDiagnostic(protocol.Diagnostic) (*Diagnostic, error)
// GetImport returns the CheckPackageHandle for a package imported by this package.
GetImport(ctx context.Context, pkgPath string) (Package, error)

42
internal/lsp/tests/tests.go
View File

@ -295,28 +295,26 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data {
// Collect any data that needs to be used by subsequent tests.
if err := data.Exported.Expect(map[string]interface{}{
"diag": data.collectDiagnostics,
"item": data.collectCompletionItems,
"complete": data.collectCompletions(CompletionDefault),
"unimported": data.collectCompletions(CompletionUnimported),
"deep": data.collectCompletions(CompletionDeep),
"fuzzy": data.collectCompletions(CompletionFuzzy),
"rank": data.collectCompletions(CompletionRank),
"snippet": data.collectCompletionSnippets,
"fold": data.collectFoldingRanges,
"format": data.collectFormats,
"import": data.collectImports,
"godef": data.collectDefinitions,
"typdef": data.collectTypeDefinitions,
"hover": data.collectHoverDefinitions,
"highlight": data.collectHighlights,
"refs": data.collectReferences,
"rename": data.collectRenames,
"prepare": data.collectPrepareRenames,
"symbol": data.collectSymbols,
"signature": data.collectSignatures,
// LSP-only features.
"diag": data.collectDiagnostics,
"item": data.collectCompletionItems,
"complete": data.collectCompletions(CompletionDefault),
"unimported": data.collectCompletions(CompletionUnimported),
"deep": data.collectCompletions(CompletionDeep),
"fuzzy": data.collectCompletions(CompletionFuzzy),
"rank": data.collectCompletions(CompletionRank),
"snippet": data.collectCompletionSnippets,
"fold": data.collectFoldingRanges,
"format": data.collectFormats,
"import": data.collectImports,
"godef": data.collectDefinitions,
"typdef": data.collectTypeDefinitions,
"hover": data.collectHoverDefinitions,
"highlight": data.collectHighlights,
"refs": data.collectReferences,
"rename": data.collectRenames,
"prepare": data.collectPrepareRenames,
"symbol": data.collectSymbols,
"signature": data.collectSignatures,
"link": data.collectLinks,
"suggestedfix": data.collectSuggestedFixes,
}); err != nil {