internal/lsp: stop caching diagnostics on the package

Now that we are using the memoize package to cache analysis results, we
can use that cache for suggested fixes.

Change-Id: I42905a6fe575f49d38979d53d58ea8ec59210ae0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/203278
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This commit is contained in:
Rebecca Stambler 2019-10-24 15:44:41 -04:00
parent e7a0da15e1
commit 3d91e92cde
18 changed files with 122 additions and 117 deletions

View File

@ -123,7 +123,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id packageID, mode source.P
}
h := s.view.session.cache.store.Bind(buildActionKey(a, cph), func(ctx context.Context) interface{} {
data := &actionData{}
data.diagnostics, data.result, data.err = ah.exec(ctx, s.view.session.cache.fset)
data.diagnostics, data.result, data.err = run(ctx, s.view.session.cache.fset, ah)
return data
})
ah.handle = h
@ -141,6 +141,15 @@ func (act *actionHandle) analyze(ctx context.Context) ([]*source.Error, interfac
return data.diagnostics, data.result, data.err
}
func (act *actionHandle) cached() ([]*source.Error, interface{}, error) {
v := act.handle.Cached()
if v == nil {
return nil, nil, errors.Errorf("no analyses for %s", act.pkg.ID())
}
data := v.(*actionData)
return data.diagnostics, data.result, data.err
}
func buildActionKey(a *analysis.Analyzer, cph *checkPackageHandle) string {
return hashContents([]byte(fmt.Sprintf("%p %s", a, string(cph.key))))
}
@ -177,7 +186,7 @@ func execAll(ctx context.Context, fset *token.FileSet, actions []*actionHandle)
return diagnostics, results, g.Wait()
}
func (act *actionHandle) exec(ctx context.Context, fset *token.FileSet) ([]*source.Error, interface{}, error) {
func run(ctx context.Context, fset *token.FileSet, act *actionHandle) ([]*source.Error, interface{}, error) {
// Analyze dependencies.
_, depResults, err := execAll(ctx, fset, act.deps)
if err != nil {
@ -207,14 +216,22 @@ func (act *actionHandle) exec(ctx context.Context, fset *token.FileSet) ([]*sour
// Run the analysis.
pass := &analysis.Pass{
Analyzer: act.analyzer,
Fset: fset,
Files: act.pkg.GetSyntax(ctx),
Pkg: act.pkg.GetTypes(),
TypesInfo: act.pkg.GetTypesInfo(),
TypesSizes: act.pkg.GetTypesSizes(),
ResultOf: inputs,
Report: func(d analysis.Diagnostic) { diagnostics = append(diagnostics, &d) },
Analyzer: act.analyzer,
Fset: fset,
Files: act.pkg.GetSyntax(),
Pkg: act.pkg.GetTypes(),
TypesInfo: act.pkg.GetTypesInfo(),
TypesSizes: act.pkg.GetTypesSizes(),
ResultOf: inputs,
Report: func(d analysis.Diagnostic) {
// Prefix the diagnostic category with the analyzer's name.
if d.Category == "" {
d.Category = act.analyzer.Name
} else {
d.Category = act.analyzer.Name + "." + d.Category
}
diagnostics = append(diagnostics, &d)
},
ImportObjectFact: act.importObjectFact,
ExportObjectFact: act.exportObjectFact,
ImportPackageFact: act.importPackageFact,

View File

@ -202,11 +202,11 @@ func (cph *checkPackageHandle) MissingDependencies() []string {
return md
}
func (cph *checkPackageHandle) Cached(ctx context.Context) (source.Package, error) {
return cph.cached(ctx)
func (cph *checkPackageHandle) Cached() (source.Package, error) {
return cph.cached()
}
func (cph *checkPackageHandle) cached(ctx context.Context) (*pkg, error) {
func (cph *checkPackageHandle) cached() (*pkg, error) {
v := cph.handle.Cached()
if v == nil {
return nil, errors.Errorf("no cached type information for %s", cph.m.pkgPath)

View File

@ -167,7 +167,7 @@ func typeErrorRange(ctx context.Context, fset *token.FileSet, pkg *pkg, pos toke
if err != nil {
return spn, nil // ignore errors
}
file, m, _, err := ph.Cached(ctx)
file, m, _, err := ph.Cached()
if err != nil {
return spn, nil
}
@ -200,7 +200,7 @@ func scannerErrorRange(ctx context.Context, fset *token.FileSet, pkg *pkg, posn
if err != nil {
return span.Span{}, err
}
file, _, _, err := ph.Cached(ctx)
file, _, _, err := ph.Cached()
if err != nil {
return span.Span{}, err
}
@ -228,7 +228,7 @@ func spanToRange(ctx context.Context, pkg *pkg, spn span.Span) (protocol.Range,
if err != nil {
return protocol.Range{}, err
}
_, m, _, err := ph.Cached(ctx)
_, m, _, err := ph.Cached()
if err != nil {
return protocol.Range{}, err
}

View File

@ -17,7 +17,7 @@ func (v *view) CheckPackageHandles(ctx context.Context, f source.File) (source.S
// Get the snapshot that will be used for type-checking.
s := v.getSnapshot()
cphs, err := s.checkPackageHandles(ctx, f)
cphs, err := s.CheckPackageHandles(ctx, f)
if err != nil {
return nil, nil, err
}
@ -27,7 +27,7 @@ func (v *view) CheckPackageHandles(ctx context.Context, f source.File) (source.S
return s, cphs, nil
}
func (s *snapshot) checkPackageHandles(ctx context.Context, f source.File) ([]source.CheckPackageHandle, error) {
func (s *snapshot) CheckPackageHandles(ctx context.Context, f source.File) ([]source.CheckPackageHandle, error) {
ctx = telemetry.File.With(ctx, f.URI())
fh := s.Handle(ctx, f)
@ -65,6 +65,9 @@ func (s *snapshot) checkPackageHandles(ctx context.Context, f source.File) ([]so
}
cphs = results
}
if len(cphs) == 0 {
return nil, errors.Errorf("no CheckPackageHandles for %s", f.URI())
}
return cphs, nil
}
@ -106,7 +109,7 @@ func (v *view) GetActiveReverseDeps(ctx context.Context, f source.File) (results
if _, ok := seen[f.URI()]; ok {
continue
}
cphs, err := s.checkPackageHandles(ctx, f)
cphs, err := s.CheckPackageHandles(ctx, f)
if err != nil {
continue
}

View File

@ -81,7 +81,7 @@ func (h *parseGoHandle) Parse(ctx context.Context) (*ast.File, *protocol.ColumnM
return data.ast, data.mapper, data.parseError, data.err
}
func (h *parseGoHandle) Cached(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error, error) {
func (h *parseGoHandle) Cached() (*ast.File, *protocol.ColumnMapper, error, error) {
v := h.handle.Cached()
if v == nil {
return nil, nil, nil, errors.Errorf("no cached AST for %s", h.file.Identity().URI)

View File

@ -8,9 +8,7 @@ import (
"context"
"go/ast"
"go/types"
"sync"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/span"
@ -32,9 +30,6 @@ type pkg struct {
types *types.Package
typesInfo *types.Info
typesSizes types.Sizes
diagMu sync.Mutex
diagnostics map[*analysis.Analyzer][]source.Diagnostic
}
// Declare explicit types for package paths and IDs to ensure that we never use
@ -68,10 +63,10 @@ func (p *pkg) File(uri span.URI) (source.ParseGoHandle, error) {
return nil, errors.Errorf("no ParseGoHandle for %s", uri)
}
func (p *pkg) GetSyntax(ctx context.Context) []*ast.File {
func (p *pkg) GetSyntax() []*ast.File {
var syntax []*ast.File
for _, ph := range p.files {
file, _, _, err := ph.Cached(ctx)
file, _, _, err := ph.Cached()
if err == nil {
syntax = append(syntax, file)
}
@ -107,34 +102,27 @@ func (p *pkg) GetImport(ctx context.Context, pkgPath string) (source.Package, er
return nil, errors.Errorf("no imported package for %s", pkgPath)
}
func (p *pkg) SetDiagnostics(a *analysis.Analyzer, diags []source.Diagnostic) {
p.diagMu.Lock()
defer p.diagMu.Unlock()
if p.diagnostics == nil {
p.diagnostics = make(map[*analysis.Analyzer][]source.Diagnostic)
}
p.diagnostics[a] = diags
}
func (p *pkg) FindDiagnostic(pdiag protocol.Diagnostic) (*source.Diagnostic, error) {
p.diagMu.Lock()
defer p.diagMu.Unlock()
for a, diagnostics := range p.diagnostics {
if a.Name != pdiag.Source {
continue
func (s *snapshot) FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*source.Error, error) {
acts := s.getActionHandles(packageID(id), source.ParseFull)
for _, act := range acts {
errors, _, err := act.analyze(ctx)
if err != nil {
return nil, err
}
for _, d := range diagnostics {
if d.Message != pdiag.Message {
for _, err := range errors {
if err.Category != diag.Source {
continue
}
if protocol.CompareRange(d.Range, pdiag.Range) != 0 {
if err.Message != diag.Message {
continue
}
return &d, nil
if protocol.CompareRange(err.Range, diag.Range) != 0 {
continue
}
return err, nil
}
}
return nil, errors.Errorf("no matching diagnostic for %v", pdiag)
return nil, errors.Errorf("no matching diagnostic for %v", diag)
}
func (p *pkg) FindFile(ctx context.Context, uri span.URI) (source.ParseGoHandle, source.Package, error) {

View File

@ -109,6 +109,19 @@ func (s *snapshot) getPackage(id packageID, m source.ParseMode) *checkPackageHan
return s.packages[key]
}
func (s *snapshot) getActionHandles(id packageID, m source.ParseMode) []*actionHandle {
s.mu.Lock()
defer s.mu.Unlock()
var acts []*actionHandle
for k, v := range s.actions {
if k.pkg.id == id && k.pkg.mode == m {
acts = append(acts, v)
}
}
return acts
}
func (s *snapshot) getAction(id packageID, m source.ParseMode, a *analysis.Analyzer) *actionHandle {
s.mu.Lock()
defer s.mu.Unlock()

View File

@ -27,7 +27,8 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
return nil, err
}
fh := view.Snapshot().Handle(ctx, f)
snapshot := view.Snapshot()
fh := snapshot.Handle(ctx, f)
// Determine the supported actions for this file kind.
fileKind := fh.Identity().Kind
@ -75,7 +76,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
}
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, f, diagnostics)
qf, err := quickFixes(ctx, snapshot, f, diagnostics)
if err != nil {
log.Error(ctx, "quick fixes failed", err, telemetry.File.Of(uri))
}
@ -205,9 +206,9 @@ func importDiagnostics(fix *imports.ImportFix, diagnostics []protocol.Diagnostic
return results
}
func quickFixes(ctx context.Context, view source.View, f source.File, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) {
func quickFixes(ctx context.Context, s source.Snapshot, f source.File, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) {
var codeActions []protocol.CodeAction
_, cphs, err := view.CheckPackageHandles(ctx, f)
cphs, err := s.CheckPackageHandles(ctx, f)
if err != nil {
return nil, err
}
@ -217,16 +218,12 @@ func quickFixes(ctx context.Context, view source.View, f source.File, diagnostic
if err != nil {
return nil, err
}
pkg, err := cph.Cached(ctx)
if err != nil {
return nil, err
}
for _, diag := range diagnostics {
sdiag, err := pkg.FindDiagnostic(diag)
srcErr, err := s.FindAnalysisError(ctx, cph.ID(), diag)
if err != nil {
continue
}
for _, fix := range sdiag.SuggestedFixes {
for _, fix := range srcErr.SuggestedFixes {
edits := make(map[string][]protocol.TextEdit)
for uri, e := range fix.Edits {
edits[protocol.NewURI(uri)] = e

View File

@ -416,7 +416,7 @@ func Completion(ctx context.Context, view View, f File, pos protocol.Position, o
if err != nil {
return nil, nil, err
}
file, m, _, err := ph.Cached(ctx)
file, m, _, err := ph.Cached()
if err != nil {
return nil, nil, err
}

View File

@ -129,7 +129,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
if err != nil {
return CompletionItem{}, err
}
file, _, _, err := ph.Cached(c.ctx)
file, _, _, err := ph.Cached()
if err != nil {
return CompletionItem{}, err
}

View File

@ -171,22 +171,27 @@ func analyses(ctx context.Context, snapshot Snapshot, cph CheckPackageHandle, di
return err
}
// For caching diagnostics.
// TODO(https://golang.org/issue/32443): Cache diagnostics on the snapshot.
pkg, err := cph.Check(ctx)
if err != nil {
return err
}
// Report diagnostics and errors from root analyzers.
var sdiags []Diagnostic
for a, diags := range diagnostics {
for _, diag := range diags {
sdiag := toDiagnostic(diag, a.Name)
addReport(snapshot.View(), reports, sdiag)
sdiags = append(sdiags, sdiag)
for _, diags := range diagnostics {
for _, e := range diags {
// 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(e.SuggestedFixes) {
tags = append(tags, protocol.Unnecessary)
}
addReport(snapshot.View(), reports, Diagnostic{
URI: e.URI,
Range: e.Range,
Message: e.Message,
Source: e.Category,
Severity: protocol.SeverityWarning,
Tags: tags,
SuggestedFixes: e.SuggestedFixes,
Related: e.Related,
})
}
pkg.SetDiagnostics(a, sdiags)
}
return nil
}
@ -219,29 +224,6 @@ func singleDiagnostic(uri span.URI, format string, a ...interface{}) map[span.UR
}
}
func toDiagnostic(e *Error, category string) Diagnostic {
// 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(e.SuggestedFixes) {
tags = append(tags, protocol.Unnecessary)
}
if e.Category != "" {
category += "." + e.Category
}
return Diagnostic{
URI: e.URI,
Range: e.Range,
Message: e.Message,
Source: category,
Severity: protocol.SeverityWarning,
Tags: tags,
SuggestedFixes: e.SuggestedFixes,
Related: e.Related,
}
}
// onlyDeletions returns true if all of the suggested fixes are deletions.
func onlyDeletions(fixes []SuggestedFix) bool {
for _, fix := range fixes {

View File

@ -64,7 +64,7 @@ func Identifier(ctx context.Context, view View, f File, pos protocol.Position) (
if err != nil {
return nil, err
}
file, m, _, err := ph.Cached(ctx)
file, m, _, err := ph.Cached()
if err != nil {
return nil, err
}
@ -248,7 +248,7 @@ func objToNode(ctx context.Context, pkg Package, obj types.Object) (ast.Decl, er
if err != nil {
return nil, err
}
declAST, _, _, err := ph.Cached(ctx)
declAST, _, _, err := ph.Cached()
if declAST == nil {
return nil, err
}
@ -313,12 +313,12 @@ func importSpec(ctx context.Context, snapshot Snapshot, pkg Package, file *ast.F
if err != nil {
return nil, err
}
if importedPkg.GetSyntax(ctx) == nil {
if importedPkg.GetSyntax() == nil {
return nil, errors.Errorf("no syntax for for %q", importPath)
}
// Heuristic: Jump to the longest (most "interesting") file of the package.
var dest *ast.File
for _, f := range importedPkg.GetSyntax(ctx) {
for _, f := range importedPkg.GetSyntax() {
if dest == nil || f.End()-f.Pos() > dest.End()-dest.Pos() {
dest = f
}

View File

@ -180,7 +180,7 @@ func (i *IdentifierInfo) getPkgName(ctx context.Context) (*IdentifierInfo, error
if err != nil {
return nil, err
}
file, _, _, err := ph.Cached(ctx)
file, _, _, err := ph.Cached()
if err != nil {
return nil, err
}

View File

@ -117,7 +117,7 @@ func (r *renamer) checkInPackageBlock(from types.Object) {
}
// Check for conflicts between package block and all file blocks.
for _, f := range pkg.GetSyntax(r.ctx) {
for _, f := range pkg.GetSyntax() {
fileScope := pkg.GetTypesInfo().Scopes[f]
b, prev := fileScope.LookupParent(r.to, token.NoPos)
if b == fileScope {
@ -331,7 +331,7 @@ func forEachLexicalRef(ctx context.Context, pkg Package, obj types.Object, fn fu
return true
}
for _, f := range pkg.GetSyntax(ctx) {
for _, f := range pkg.GetSyntax() {
ast.Inspect(f, visit)
if len(stack) != 0 {
panic(stack)
@ -808,7 +808,7 @@ func (r *renamer) satisfy() map[satisfy.Constraint]bool {
r.from, r.to, pkg.PkgPath())
return nil
}
f.Find(pkg.GetTypesInfo(), pkg.GetSyntax(r.ctx))
f.Find(pkg.GetTypesInfo(), pkg.GetSyntax())
}
r.satisfyConstraints = f.Result
}
@ -841,7 +841,7 @@ func someUse(info *types.Info, obj types.Object) *ast.Ident {
//
func pathEnclosingInterval(ctx context.Context, fset *token.FileSet, pkg Package, start, end token.Pos) (resPkg Package, path []ast.Node, exact bool) {
var pkgs = []Package{pkg}
for _, f := range pkg.GetSyntax(ctx) {
for _, f := range pkg.GetSyntax() {
for _, imp := range f.Imports {
if imp == nil {
continue
@ -858,7 +858,7 @@ func pathEnclosingInterval(ctx context.Context, fset *token.FileSet, pkg Package
}
}
for _, p := range pkgs {
for _, f := range p.GetSyntax(ctx) {
for _, f := range p.GetSyntax() {
if f.Pos() == token.NoPos {
// This can happen if the parser saw
// too many errors and bailed out.

View File

@ -47,7 +47,7 @@ func SignatureHelp(ctx context.Context, view View, f File, pos protocol.Position
if err != nil {
return nil, err
}
file, m, _, err := ph.Cached(ctx)
file, m, _, err := ph.Cached()
if err != nil {
return nil, err
}

View File

@ -34,7 +34,7 @@ func DocumentSymbols(ctx context.Context, view View, f File) ([]protocol.Documen
if err != nil {
return nil, err
}
file, m, _, err := ph.Cached(ctx)
file, m, _, err := ph.Cached()
if err != nil {
return nil, err
}

View File

@ -181,7 +181,7 @@ func posToMapper(ctx context.Context, pkg Package, pos token.Pos) (*protocol.Col
if err != nil {
return nil, err
}
_, m, _, err := ph.Cached(ctx)
_, m, _, err := ph.Cached()
return m, err
}

View File

@ -73,7 +73,7 @@ type ParseGoHandle interface {
Parse(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error, error)
// Cached returns the AST for this handle, if it has already been stored.
Cached(ctx context.Context) (*ast.File, *protocol.ColumnMapper, error, error)
Cached() (*ast.File, *protocol.ColumnMapper, error, error)
}
// ParseMode controls the content of the AST produced when parsing a source file.
@ -109,7 +109,7 @@ type CheckPackageHandle interface {
Check(ctx context.Context) (Package, error)
// Cached returns the Package for the CheckPackageHandle if it has already been stored.
Cached(ctx context.Context) (Package, error)
Cached() (Package, error)
// MissingDependencies reports any unresolved imports.
MissingDependencies() []string
@ -263,6 +263,14 @@ type Snapshot interface {
// Analyze runs the analyses for the given package at this snapshot.
Analyze(ctx context.Context, id string, analyzers []*analysis.Analyzer) (map[*analysis.Analyzer][]*Error, error)
// FindAnalysisError returns the analysis error represented by the diagnostic.
// This is used to get the SuggestedFixes associated with that error.
FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*Error, error)
// CheckPackageHandles returns the CheckPackageHandles for the packages
// that this file belongs to.
CheckPackageHandles(ctx context.Context, f File) ([]CheckPackageHandle, error)
}
// File represents a source file of any type.
@ -278,16 +286,13 @@ type Package interface {
PkgPath() string
Files() []ParseGoHandle
File(uri span.URI) (ParseGoHandle, error)
GetSyntax(context.Context) []*ast.File
GetSyntax() []*ast.File
GetErrors() []*Error
GetTypes() *types.Package
GetTypesInfo() *types.Info
GetTypesSizes() types.Sizes
IsIllTyped() bool
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)
@ -303,7 +308,7 @@ type Error struct {
Range protocol.Range
Kind ErrorKind
Message string
Category string
Category string // only used by analysis errors so far
SuggestedFixes []SuggestedFix
Related []RelatedInformation
}