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>

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) {

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)
+	}
+}

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
 }

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.Diagnostics(ctx, view, uri)
-			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)
-		}
 	}()
 	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 {

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 {
+	// Returns true if we've sent non-empty diagnostics.
-		return reports, nil
+	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 nil
-	return reports, nil
 }
 
 func pointToSpan(ctx context.Context, v View, spn span.Span) span.Span {

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