internal/lsp: use snapshot to get reverse dependencies

This change modifies the behavior of the GetReverseDependencies function
used for diagnostics. Since we now return diagnostics for the entire
workspace, we don't have to worry if a file is open to show errors in
it. This change requires the addition of a new (*snapshot).PackageHandle
function that gets a CheckPackageHandle for a given package ID. This
function does not cause a re-load of the package metadata, though if we
feel that this is something we need in the future we can add it.

Change-Id: I863bdf284d15f2317d8fae395928a90b9455146b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208102
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/gofile.go

@@ -1,137 +0,0 @@
-// Copyright 2019 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package cache
-
-import (
-	"context"
-
-	"golang.org/x/tools/internal/lsp/source"
-	"golang.org/x/tools/internal/lsp/telemetry"
-	"golang.org/x/tools/internal/span"
-	errors "golang.org/x/xerrors"
-)
-
-func (s *snapshot) PackageHandles(ctx context.Context, f source.File) ([]source.CheckPackageHandle, error) {
-	ctx = telemetry.File.With(ctx, f.URI())
-
-	fh := s.Handle(ctx, f)
-
-	// Determine if we need to type-check the package.
-	metadata, cphs, load, check := s.shouldCheck(fh)
-
-	// We may need to re-load package metadata.
-	// We only need to this if it has been invalidated, and is therefore unvailable.
-	if load {
-		var err error
-		m, err := s.load(ctx, source.FileURI(f.URI()))
-		if err != nil {
-			return nil, err
-		}
-		// If load has explicitly returned nil metadata and no error,
-		// it means that we should not re-type-check the packages.
-		// Return the cached CheckPackageHandles, if we had them.
-		if m == nil && len(cphs) > 0 {
-			return cphs, nil
-		}
-		// If metadata was returned, from the load call, use it.
-		if m != nil {
-			metadata = m
-		}
-	}
-	if check {
-		var results []source.CheckPackageHandle
-		for _, m := range metadata {
-			cph, err := s.checkPackageHandle(ctx, m.id, source.ParseFull)
-			if err != nil {
-				return nil, err
-			}
-			results = append(results, cph)
-		}
-		cphs = results
-	}
-	if len(cphs) == 0 {
-		return nil, errors.Errorf("no CheckPackageHandles for %s", f)
-	}
-	return cphs, nil
-}
-
-func (s *snapshot) shouldCheck(fh source.FileHandle) (m []*metadata, cphs []source.CheckPackageHandle, load, check bool) {
-	// Get the metadata for the given file.
-	m = s.getMetadataForURI(fh.Identity().URI)
-
-	// If there is no metadata for the package, we definitely need to type-check again.
-	if len(m) == 0 {
-		return nil, nil, true, true
-	}
-
-	// If the metadata for the package had missing dependencies,
-	// we _may_ need to re-check. If the missing dependencies haven't changed
-	// since previous load, we will not check again.
-	for _, m := range m {
-		if len(m.missingDeps) != 0 {
-			load = true
-			check = true
-		}
-	}
-	// We expect to see a checked package for each package ID,
-	// and it should be parsed in full mode.
-	cphs = s.getPackages(source.FileURI(fh.Identity().URI), source.ParseFull)
-	if len(cphs) < len(m) {
-		return m, nil, load, true
-	}
-	return m, cphs, load, check
-}
-
-func (v *view) GetActiveReverseDeps(ctx context.Context, f source.File) (results []source.CheckPackageHandle) {
-	var (
-		s     = v.getSnapshot()
-		rdeps = transitiveReverseDependencies(ctx, f.URI(), s)
-		files = v.openFiles(ctx, rdeps)
-		seen  = make(map[span.URI]struct{})
-	)
-	for _, f := range files {
-		if _, ok := seen[f.URI()]; ok {
-			continue
-		}
-		cphs, err := s.PackageHandles(ctx, f)
-		if err != nil {
-			continue
-		}
-		cph, err := source.WidestCheckPackageHandle(cphs)
-		if err != nil {
-			continue
-		}
-		for _, ph := range cph.CompiledGoFiles() {
-			seen[ph.File().Identity().URI] = struct{}{}
-		}
-		results = append(results, cph)
-	}
-	return results
-}
-
-func transitiveReverseDependencies(ctx context.Context, uri span.URI, s *snapshot) (result []span.URI) {
-	var (
-		seen         = make(map[packageID]struct{})
-		uris         = make(map[span.URI]struct{})
-		topLevelURIs = make(map[span.URI]struct{})
-	)
-
-	metadata := s.getMetadataForURI(uri)
-
-	for _, m := range metadata {
-		for _, uri := range m.compiledGoFiles {
-			topLevelURIs[uri] = struct{}{}
-		}
-		s.reverseDependencies(m.id, uris, seen)
-	}
-	// Filter out the URIs that belong to the original package.
-	for uri := range uris {
-		if _, ok := topLevelURIs[uri]; ok {
-			continue
-		}
-		result = append(result, uri)
-	}
-	return result
-}

internal/lsp/cache/snapshot.go

@@ -10,6 +10,7 @@ import (
 	"golang.org/x/tools/internal/lsp/telemetry"
 	"golang.org/x/tools/internal/span"
 	"golang.org/x/tools/internal/telemetry/log"
+	errors "golang.org/x/xerrors"
 )
 
 type snapshot struct {
@@ -59,6 +60,131 @@ func (s *snapshot) View() source.View {
 	return s.view
 }
 
+func (s *snapshot) PackageHandles(ctx context.Context, f source.File) ([]source.CheckPackageHandle, error) {
+	ctx = telemetry.File.With(ctx, f.URI())
+
+	fh := s.Handle(ctx, f)
+	metadata := s.getMetadataForURI(fh.Identity().URI)
+
+	// Determine if we need to type-check the package.
+	cphs, load, check := s.shouldCheck(metadata)
+
+	// We may need to re-load package metadata.
+	// We only need to this if it has been invalidated, and is therefore unvailable.
+	if load {
+		var err error
+		m, err := s.load(ctx, source.FileURI(f.URI()))
+		if err != nil {
+			return nil, err
+		}
+		// If metadata was returned, from the load call, use it.
+		if m != nil {
+			metadata = m
+		}
+	}
+	if check {
+		var results []source.CheckPackageHandle
+		for _, m := range metadata {
+			cph, err := s.checkPackageHandle(ctx, m.id, source.ParseFull)
+			if err != nil {
+				return nil, err
+			}
+			results = append(results, cph)
+		}
+		cphs = results
+	}
+	if len(cphs) == 0 {
+		return nil, errors.Errorf("no CheckPackageHandles for %s", f)
+	}
+	return cphs, nil
+}
+
+func (s *snapshot) PackageHandle(ctx context.Context, id string) (source.CheckPackageHandle, error) {
+	ctx = telemetry.Package.With(ctx, id)
+
+	m := s.getMetadata(packageID(id))
+	if m == nil {
+		return nil, errors.Errorf("no known metadata for %s", id)
+	}
+	// Determine if we need to type-check the package.
+	cphs, load, check := s.shouldCheck([]*metadata{m})
+	if load {
+		return nil, errors.Errorf("outdated metadata for %s, needs re-load", id)
+	}
+	if check {
+		return s.checkPackageHandle(ctx, m.id, source.ParseFull)
+	}
+	if len(cphs) == 0 {
+		return nil, errors.Errorf("no check package handle for %s", id)
+	}
+	if len(cphs) > 1 {
+		return nil, errors.Errorf("multiple check package handles for a single id: %s", id)
+	}
+	return cphs[0], nil
+}
+
+// shouldCheck determines if the packages provided by the metadata
+// need to be re-loaded or re-type-checked.
+func (s *snapshot) shouldCheck(m []*metadata) (cphs []source.CheckPackageHandle, load, check bool) {
+	// No metadata. Re-load and re-check.
+	if len(m) == 0 {
+		return nil, true, true
+	}
+	// We expect to see a checked package for each package ID,
+	// and it should be parsed in full mode.
+	// If a single CheckPackageHandle is missing, re-check all of them.
+	// TODO: Optimize this by only checking the necessary packages.
+	for _, metadata := range m {
+		cph := s.getPackage(metadata.id, source.ParseFull)
+		if cph == nil {
+			return nil, false, true
+		}
+		cphs = append(cphs, cph)
+	}
+	// If the metadata for the package had missing dependencies,
+	// we _may_ need to re-check. If the missing dependencies haven't changed
+	// since previous load, we will not check again.
+	if len(cphs) < len(m) {
+		for _, m := range m {
+			if len(m.missingDeps) != 0 {
+				return nil, true, true
+			}
+		}
+	}
+	return cphs, false, false
+}
+
+func (s *snapshot) GetReverseDependencies(id string) []string {
+	ids := make(map[packageID]struct{})
+	s.transitiveReverseDependencies(packageID(id), ids)
+
+	// Make sure to delete the original package ID from the map.
+	delete(ids, packageID(id))
+
+	var results []string
+	for id := range ids {
+		results = append(results, string(id))
+	}
+	return results
+}
+
+// transitiveReverseDependencies populates the uris map with file URIs
+// belonging to the provided package and its transitive reverse dependencies.
+func (s *snapshot) transitiveReverseDependencies(id packageID, ids map[packageID]struct{}) {
+	if _, ok := ids[id]; ok {
+		return
+	}
+	m := s.getMetadata(id)
+	if m == nil {
+		return
+	}
+	ids[id] = struct{}{}
+	importedBy := s.getImportedBy(id)
+	for _, parentID := range importedBy {
+		s.transitiveReverseDependencies(parentID, ids)
+	}
+}
+
 func (s *snapshot) getImportedBy(id packageID) []packageID {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -84,25 +210,6 @@ func (s *snapshot) addPackage(cph *checkPackageHandle) {
 	s.packages[cph.packageKey()] = cph
 }
 
-func (s *snapshot) getPackages(uri source.FileURI, m source.ParseMode) (cphs []source.CheckPackageHandle) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	if ids, ok := s.ids[uri.URI()]; ok {
-		for _, id := range ids {
-			key := packageKey{
-				id:   id,
-				mode: m,
-			}
-			cph, ok := s.packages[key]
-			if ok {
-				cphs = append(cphs, cph)
-			}
-		}
-	}
-	return cphs
-}
-
 // checkWorkspacePackages checks the initial set of packages loaded when
 // the view is created. This is needed because
 // (*snapshot).CheckPackageHandle makes the assumption that every package that's
@@ -445,14 +552,20 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, kind source
 		}
 		originalFH = nil
 	}
-
 	if len(ids) == 0 {
 		return false
 	}
 
 	// Remove the package and all of its reverse dependencies from the cache.
+	reverseDependencies := make(map[packageID]struct{})
 	for id := range ids {
-		v.snapshot.reverseDependencies(id, withoutTypes, map[packageID]struct{}{})
+		v.snapshot.transitiveReverseDependencies(id, reverseDependencies)
+	}
+	for id := range reverseDependencies {
+		m := v.snapshot.getMetadata(id)
+		for _, uri := range m.compiledGoFiles {
+			withoutTypes[uri] = struct{}{}
+		}
 	}
 
 	// Make sure to clear out the content if there has been a deletion.
@@ -476,26 +589,6 @@ func (v *view) invalidateContent(ctx context.Context, f source.File, kind source
 	return true
 }
 
-// reverseDependencies populates the uris map with file URIs belonging to the
-// provided package and its transitive reverse dependencies.
-func (s *snapshot) reverseDependencies(id packageID, uris map[span.URI]struct{}, seen map[packageID]struct{}) {
-	if _, ok := seen[id]; ok {
-		return
-	}
-	m := s.getMetadata(id)
-	if m == nil {
-		return
-	}
-	seen[id] = struct{}{}
-	importedBy := s.getImportedBy(id)
-	for _, parentID := range importedBy {
-		s.reverseDependencies(parentID, uris, seen)
-	}
-	for _, uri := range m.compiledGoFiles {
-		uris[uri] = struct{}{}
-	}
-}
-
 func (s *snapshot) clearAndRebuildImportGraph() {
 	s.mu.Lock()
 	defer s.mu.Unlock()

internal/lsp/cache/view.go

@@ -432,19 +432,6 @@ func (v *view) mapFile(uri span.URI, f viewFile) {
 	}
 }
 
-func (v *view) openFiles(ctx context.Context, uris []span.URI) (results []source.File) {
-	v.mu.Lock()
-	defer v.mu.Unlock()
-
-	for _, uri := range uris {
-		// Call unlocked version of getFile since we hold the lock on the view.
-		if f, err := v.getFile(ctx, uri, source.Go); err == nil && v.session.IsOpen(uri) {
-			results = append(results, f)
-		}
-	}
-	return results
-}
-
 func (v *view) FindPosInPackage(searchpkg source.Package, pos token.Pos) (*ast.File, *protocol.ColumnMapper, source.Package, error) {
 	tok := v.session.cache.fset.File(pos)
 	if tok == nil {

internal/lsp/lsp_test.go

@@ -356,6 +356,9 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span) {
 		t.Fatal(err)
 	}
 	// TODO: This test should probably be able to handle multiple code actions.
+	if len(actions) == 0 {
+		t.Fatal("no code actions returned")
+	}
 	if len(actions) > 1 {
 		t.Fatal("expected only 1 code action")
 	}

internal/lsp/source/diagnostics.go

@@ -66,13 +66,11 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse
 		log.Error(ctx, "no package for file", err)
 		return singleDiagnostic(fh.Identity(), "%s is not part of a package", f.URI()), "", nil
 	}
-
 	// Prepare the reports we will send for the files in this package.
 	reports := make(map[FileIdentity][]Diagnostic)
 	for _, fh := range pkg.CompiledGoFiles() {
 		clearReports(snapshot, reports, fh.File().Identity())
 	}
-
 	// Prepare any additional reports for the errors in this package.
 	for _, e := range pkg.GetErrors() {
 		if e.Kind != ListError {
@@ -80,7 +78,6 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse
 		}
 		clearReports(snapshot, reports, e.File)
 	}
-
 	// Run diagnostics for the package that this URI belongs to.
 	if !diagnostics(ctx, snapshot, pkg, reports) {
 		// If we don't have any list, parse, or type errors, run analyses.
@@ -89,8 +86,11 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse
 		}
 	}
 	// Updates to the diagnostics for this package may need to be propagated.
-	revDeps := snapshot.View().GetActiveReverseDeps(ctx, f)
-	for _, cph := range revDeps {
+	for _, id := range snapshot.GetReverseDependencies(pkg.ID()) {
+		cph, err := snapshot.PackageHandle(ctx, id)
+		if err != nil {
+			return nil, warningMsg, err
+		}
 		pkg, err := cph.Check(ctx)
 		if err != nil {
 			return nil, warningMsg, err

internal/lsp/source/view.go

@@ -20,14 +20,12 @@ import (
 
 // Snapshot represents the current state for the given view.
 type Snapshot interface {
-	ID() uint64
+	// View returns the View associated with this snapshot.
+	View() View
 
 	// Handle returns the FileHandle for the given file.
 	Handle(ctx context.Context, f File) FileHandle
 
-	// View returns the View associated with this snapshot.
-	View() View
-
 	// Analyze runs the analyses for the given package at this snapshot.
 	Analyze(ctx context.Context, id string, analyzers []*analysis.Analyzer) ([]*Error, error)
 
@@ -35,10 +33,17 @@ type Snapshot interface {
 	// This is used to get the SuggestedFixes associated with that error.
 	FindAnalysisError(ctx context.Context, id string, diag protocol.Diagnostic) (*Error, error)
 
-	// PackageHandles returns the PackageHandles for the packages
+	// PackageHandle returns the CheckPackageHandle for the given package ID.
+	PackageHandle(ctx context.Context, id string) (CheckPackageHandle, error)
+
+	// PackageHandles returns the CheckPackageHandles for the packages
 	// that this file belongs to.
 	PackageHandles(ctx context.Context, f File) ([]CheckPackageHandle, error)
 
+	// GetActiveReverseDeps returns the active files belonging to the reverse
+	// dependencies of this file's package.
+	GetReverseDependencies(id string) []string
+
 	// KnownImportPaths returns all the imported packages loaded in this snapshot,
 	// indexed by their import path.
 	KnownImportPaths() map[string]Package
@@ -119,10 +124,6 @@ type View interface {
 	// original one will be.
 	SetOptions(context.Context, Options) (View, error)
 
-	// GetActiveReverseDeps returns the active files belonging to the reverse
-	// dependencies of this file's package.
-	GetActiveReverseDeps(ctx context.Context, f File) []CheckPackageHandle
-
 	// FindFileInPackage returns the AST and type information for a file that may
 	// belong to or be part of a dependency of the given package.
 	FindPosInPackage(pkg Package, pos token.Pos) (*ast.File, *protocol.ColumnMapper, Package, error)