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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 0f99248..7a40c68 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -57,31 +57,6 @@
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 @@
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 d7429c9..0a9b303 100644
--- a/internal/lsp/cache/file.go
+++ b/internal/lsp/cache/file.go
@@ -88,6 +88,7 @@
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) 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 573be71..a1d1a30 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -124,6 +124,10 @@
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 ee634a6..040c383 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -18,45 +18,48 @@
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 25946e6..4b3b94c 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -64,6 +64,25 @@
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 @@
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 @@
})
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 b9ce7e7..6661696 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -41,11 +41,16 @@
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
