From 3eedecdc80844777beb9205d91b2f5fd2d9e842f Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 1 May 2019 22:46:07 -0400 Subject: [PATCH] internal/lsp: propagate diagnostics for reverse dependencies Prior to this change, if a package was rendered invalid by a change in one of its dependencies, diagnostics would not be propagated until the user typed in one of the package's files. Now, these updated diagnostics are sent along with the diagnostics for the dependency. Fixes golang/go#29817 Change-Id: I4761de31c4bdee820e024005f6112b3b3d2e1da6 Reviewed-on: https://go-review.googlesource.com/c/tools/+/174977 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 74 +++++++++++++++++------------- internal/lsp/cache/file.go | 50 ++++++++++++++++++++ internal/lsp/cache/pkg.go | 4 ++ internal/lsp/diagnostics.go | 69 +++++++++++++++------------- internal/lsp/source/diagnostics.go | 32 ++++++++++--- internal/lsp/source/view.go | 5 ++ 6 files changed, 163 insertions(+), 71 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 0f99248d..7a40c68f 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -57,31 +57,6 @@ func (v *View) parse(ctx context.Context, f *File) ([]packages.Error, error) { return nil, nil } -func (v *View) cachePackage(ctx context.Context, pkg *Package) { - for _, file := range pkg.GetSyntax() { - // TODO: If a file is in multiple packages, which package do we store? - if !file.Pos().IsValid() { - v.Logger().Errorf(ctx, "invalid position for file %v", file.Name) - continue - } - tok := v.Config.Fset.File(file.Pos()) - if tok == nil { - v.Logger().Errorf(ctx, "no token.File for %v", file.Name) - continue - } - fURI := span.FileURI(tok.Name()) - f, err := v.getFile(fURI) - if err != nil { - v.Logger().Errorf(ctx, "no file: %v", err) - continue - } - f.token = tok - f.ast = file - f.imports = f.ast.Imports - f.pkg = pkg - } -} - func (v *View) checkMetadata(ctx context.Context, f *File) ([]packages.Error, error) { if v.reparseImports(ctx, f, f.filename) { cfg := v.Config @@ -271,21 +246,56 @@ func (imp *importer) typeCheck(pkgPath string) (*Package, error) { check.Files(pkg.syntax) // Add every file in this package to our cache. - imp.view.cachePackage(imp.ctx, pkg) + imp.view.cachePackage(imp.ctx, pkg, meta) + + return pkg, nil +} + +func (v *View) cachePackage(ctx context.Context, pkg *Package, meta *metadata) { + for _, file := range pkg.GetSyntax() { + // TODO: If a file is in multiple packages, which package do we store? + if !file.Pos().IsValid() { + v.Logger().Errorf(ctx, "invalid position for file %v", file.Name) + continue + } + tok := v.Config.Fset.File(file.Pos()) + if tok == nil { + v.Logger().Errorf(ctx, "no token.File for %v", file.Name) + continue + } + fURI := span.FileURI(tok.Name()) + f, err := v.getFile(fURI) + if err != nil { + v.Logger().Errorf(ctx, "no file: %v", err) + continue + } + f.token = tok + f.ast = file + f.imports = f.ast.Imports + f.pkg = pkg + } + + v.pcache.mu.Lock() + defer v.pcache.mu.Unlock() + + // Cache the entry for this package. + // All dependencies are cached through calls to *imp.Import. + e := &entry{ + pkg: pkg, + err: nil, + ready: make(chan struct{}), + } + close(e.ready) + v.pcache.packages[pkg.pkgPath] = e // Set imports of package to correspond to cached packages. // We lock the package cache, but we shouldn't get any inconsistencies // because we are still holding the lock on the view. - imp.view.pcache.mu.Lock() - defer imp.view.pcache.mu.Unlock() - for importPath := range meta.children { - if importEntry, ok := imp.view.pcache.packages[importPath]; ok { + if importEntry, ok := v.pcache.packages[importPath]; ok { pkg.imports[importPath] = importEntry.pkg } } - - return pkg, nil } func (v *View) appendPkgError(pkg *Package, err error) { diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index d7429c9f..0a9b303d 100644 --- a/internal/lsp/cache/file.go +++ b/internal/lsp/cache/file.go @@ -88,6 +88,7 @@ func (f *File) GetAST(ctx context.Context) *ast.File { func (f *File) GetPackage(ctx context.Context) source.Package { f.view.mu.Lock() defer f.view.mu.Unlock() + if f.pkg == nil || len(f.view.contentChanges) > 0 { if errs, err := f.view.parse(ctx, f); err != nil { // Create diagnostics for errors if we are able to. @@ -134,3 +135,52 @@ func (f *File) read(ctx context.Context) { func (f *File) isPopulated() bool { return f.ast != nil && f.token != nil && f.pkg != nil && f.meta != nil && f.imports != nil } + +func (f *File) GetActiveReverseDeps(ctx context.Context) []source.File { + pkg := f.GetPackage(ctx) + if pkg == nil { + return nil + } + + f.view.mu.Lock() + defer f.view.mu.Unlock() + + f.view.mcache.mu.Lock() + defer f.view.mcache.mu.Unlock() + + seen := make(map[string]struct{}) // visited packages + results := make(map[*File]struct{}) + f.view.reverseDeps(ctx, seen, results, pkg.PkgPath()) + + files := make([]source.File, 0, len(results)) + for rd := range results { + if rd == nil { + continue + } + // Don't return any of the active file's in this package. + if rd.pkg != nil && rd.pkg == pkg { + continue + } + files = append(files, rd) + } + return files +} + +func (v *View) reverseDeps(ctx context.Context, seen map[string]struct{}, results map[*File]struct{}, pkgPath string) { + if _, ok := seen[pkgPath]; ok { + return + } + seen[pkgPath] = struct{}{} + m, ok := v.mcache.packages[pkgPath] + if !ok { + return + } + for _, filename := range m.files { + if f, err := v.getFile(span.FileURI(filename)); err == nil && f.active { + results[f] = struct{}{} + } + } + for parentPkgPath := range m.parents { + v.reverseDeps(ctx, seen, results, parentPkgPath) + } +} diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 573be719..a1d1a305 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -124,6 +124,10 @@ func (pkg *Package) GetActionGraph(ctx context.Context, a *analysis.Analyzer) (* return e.Action, nil } +func (pkg *Package) PkgPath() string { + return pkg.pkgPath +} + func (pkg *Package) GetFilenames() []string { return pkg.files } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index ee634a65..040c3837 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -18,45 +18,48 @@ func (s *Server) cacheAndDiagnose(ctx context.Context, uri span.URI, content str if err := view.SetContent(ctx, uri, []byte(content)); err != nil { return err } - go func() { ctx := view.BackgroundContext() - if ctx.Err() != nil { - s.log.Errorf(ctx, "canceling diagnostics for %s: %v", uri, ctx.Err()) - return - } - reports, err := source.Diagnostics(ctx, view, uri) - if err != nil { - s.log.Errorf(ctx, "failed to compute diagnostics for %s: %v", uri, err) - return - } - - s.undeliveredMu.Lock() - defer s.undeliveredMu.Unlock() - - for uri, diagnostics := range reports { - if err := s.publishDiagnostics(ctx, view, uri, diagnostics); err != nil { - if s.undelivered == nil { - s.undelivered = make(map[span.URI][]source.Diagnostic) - } - s.undelivered[uri] = diagnostics - continue - } - // In case we had old, undelivered diagnostics. - delete(s.undelivered, uri) - } - // Anytime we compute diagnostics, make sure to also send along any - // undelivered ones (only for remaining URIs). - for uri, diagnostics := range s.undelivered { - s.publishDiagnostics(ctx, view, uri, diagnostics) - - // If we fail to deliver the same diagnostics twice, just give up. - delete(s.undelivered, uri) - } + s.Diagnostics(ctx, view, uri) }() return nil } +func (s *Server) Diagnostics(ctx context.Context, view *cache.View, uri span.URI) { + if ctx.Err() != nil { + s.log.Errorf(ctx, "canceling diagnostics for %s: %v", uri, ctx.Err()) + return + } + reports, err := source.Diagnostics(ctx, view, uri) + if err != nil { + s.log.Errorf(ctx, "failed to compute diagnostics for %s: %v", uri, err) + return + } + + s.undeliveredMu.Lock() + defer s.undeliveredMu.Unlock() + + for uri, diagnostics := range reports { + if err := s.publishDiagnostics(ctx, view, uri, diagnostics); err != nil { + if s.undelivered == nil { + s.undelivered = make(map[span.URI][]source.Diagnostic) + } + s.undelivered[uri] = diagnostics + continue + } + // In case we had old, undelivered diagnostics. + delete(s.undelivered, uri) + } + // Anytime we compute diagnostics, make sure to also send along any + // undelivered ones (only for remaining URIs). + for uri, diagnostics := range s.undelivered { + s.publishDiagnostics(ctx, view, uri, diagnostics) + + // If we fail to deliver the same diagnostics twice, just give up. + delete(s.undelivered, uri) + } +} + func (s *Server) publishDiagnostics(ctx context.Context, view *cache.View, uri span.URI, diagnostics []source.Diagnostic) error { protocolDiagnostics, err := toProtocolDiagnostics(ctx, view, diagnostics) if err != nil { diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 25946e6d..4b3b94cc 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -64,6 +64,25 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag for _, filename := range pkg.GetFilenames() { reports[span.FileURI(filename)] = []Diagnostic{} } + // Run diagnostics for the package that this URI belongs to. + if !diagnostics(ctx, v, pkg, reports) { + // If we don't have any list, parse, or type errors, run analyses. + if err := analyses(ctx, v, pkg, reports); err != nil { + return singleDiagnostic(uri, "failed to run analyses for %s", uri), nil + } + } + // Updates to the diagnostics for this package may need to be propagated. + for _, f := range f.GetActiveReverseDeps(ctx) { + pkg := f.GetPackage(ctx) + for _, filename := range pkg.GetFilenames() { + reports[span.FileURI(filename)] = []Diagnostic{} + } + diagnostics(ctx, v, pkg, reports) + } + return reports, nil +} + +func diagnostics(ctx context.Context, v View, pkg Package, reports map[span.URI][]Diagnostic) bool { var listErrors, parseErrors, typeErrors []packages.Error for _, err := range pkg.GetErrors() { switch err.Kind { @@ -97,9 +116,11 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag reports[spn.URI()] = append(reports[spn.URI()], diagnostic) } } - if len(diags) > 0 { - return reports, nil - } + // Returns true if we've sent non-empty diagnostics. + return len(diags) != 0 +} + +func analyses(ctx context.Context, v View, pkg Package, reports map[span.URI][]Diagnostic) error { // Type checking and parsing succeeded. Run analyses. if err := runAnalyses(ctx, v, pkg, func(a *analysis.Analyzer, diag analysis.Diagnostic) error { r := span.NewRange(v.FileSet(), diag.Pos, 0) @@ -120,10 +141,9 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag }) return nil }); err != nil { - return singleDiagnostic(uri, "unable to run analyses for %s: %v", uri, err), nil + return err } - - return reports, nil + return nil } func pointToSpan(ctx context.Context, v View, spn span.Span) span.Span { diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index b9ce7e74..6661696d 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -41,11 +41,16 @@ type File interface { GetPackage(ctx context.Context) Package GetToken(ctx context.Context) *token.File GetContent(ctx context.Context) []byte + + // GetActiveReverseDeps returns the active files belonging to the reverse + // dependencies of this file's package. + GetActiveReverseDeps(ctx context.Context) []File } // Package represents a Go package that has been type-checked. It maintains // only the relevant fields of a *go/packages.Package. type Package interface { + PkgPath() string GetFilenames() []string GetSyntax() []*ast.File GetErrors() []packages.Error