internal/lsp/cache: hardcode parse modes instead of guessing them

This change largely reverts CL 217139, which attempted to guess a
package's parse mode based on whether or not it was in the user's
workspace. This ignored the fact that a user may jump to the definition
of a file outside of their workspace.

Fixes golang/go#37045

Change-Id: Icb6b9d055bd1f260013227db1a6a34873c45b680
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218499
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: stamblerre

internal/lsp/cache/analysis.go

@@ -82,11 +82,10 @@ type packageFactKey struct {
 }
 
 func (s *snapshot) actionHandle(ctx context.Context, id packageID, a *analysis.Analyzer) (*actionHandle, error) {
-	ph := s.getPackage(id)
+	ph := s.getPackage(id, source.ParseFull)
 	if ph == nil {
 		return nil, errors.Errorf("no PackageHandle for %s", id)
 	}
-	expectMode(ph, source.ParseFull)
 	act := s.getActionHandle(id, ph.mode, a)
 	if act != nil {
 		return act, nil

internal/lsp/cache/check.go

@@ -63,13 +63,13 @@ type packageData struct {
 }
 
 // buildPackageHandle returns a source.PackageHandle for a given package and config.
-func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID) (*packageHandle, error) {
-	if ph := s.getPackage(id); ph != nil {
+func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) {
+	if ph := s.getPackage(id, mode); ph != nil {
 		return ph, nil
 	}
 
 	// Build the PackageHandle for this ID and its dependencies.
-	ph, deps, err := s.buildKey(ctx, id)
+	ph, deps, err := s.buildKey(ctx, id, mode)
 	if err != nil {
 		return nil, err
 	}
@@ -83,7 +83,6 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID) (*packa
 	//
 
 	m := ph.m
-	mode := ph.mode
 	goFiles := ph.goFiles
 	compiledGoFiles := ph.compiledGoFiles
 	key := ph.key
@@ -109,12 +108,11 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID) (*packa
 }
 
 // buildKey computes the key for a given packageHandle.
-func (s *snapshot) buildKey(ctx context.Context, id packageID) (*packageHandle, map[packagePath]*packageHandle, error) {
+func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, map[packagePath]*packageHandle, error) {
 	m := s.getMetadata(id)
 	if m == nil {
 		return nil, nil, errors.Errorf("no metadata for %s", id)
 	}
-	mode := s.packageMode(id)
 	goFiles, err := s.parseGoHandles(ctx, m.goFiles, mode)
 	if err != nil {
 		return nil, nil, err
@@ -140,7 +138,11 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID) (*packageHandle,
 	// Begin computing the key by getting the depKeys for all dependencies.
 	var depKeys []packageHandleKey
 	for _, depID := range depList {
-		depHandle, err := s.buildPackageHandle(ctx, depID)
+		mode := source.ParseExported
+		if _, ok := s.isWorkspacePackage(depID); ok {
+			mode = source.ParseFull
+		}
+		depHandle, err := s.buildPackageHandle(ctx, depID, mode)
 		if err != nil {
 			log.Error(ctx, "no dep handle", err, telemetry.Package.Of(depID))
 

internal/lsp/cache/load.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	"golang.org/x/tools/go/packages"
+	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/lsp/telemetry"
 	"golang.org/x/tools/internal/packagesinternal"
 	"golang.org/x/tools/internal/span"
@@ -119,7 +120,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error {
 		if err != nil {
 			return err
 		}
-		if _, err := s.buildPackageHandle(ctx, m.id); err != nil {
+		if _, err := s.buildPackageHandle(ctx, m.id, source.ParseFull); err != nil {
 			return err
 		}
 	}

internal/lsp/cache/snapshot.go

@@ -159,12 +159,7 @@ func (s *snapshot) packageHandle(ctx context.Context, id packageID) (*packageHan
 		return nil, errors.Errorf("%s needs loading", id)
 	}
 	if check {
-		ph, err := s.buildPackageHandle(ctx, m.id)
-		if err != nil {
-			return nil, err
-		}
-		expectMode(ph, source.ParseFull)
-		return ph, nil
+		return s.buildPackageHandle(ctx, m.id, source.ParseFull)
 	}
 	var result *packageHandle
 	for _, ph := range phs {
@@ -202,11 +197,10 @@ func (s *snapshot) packageHandles(ctx context.Context, uri span.URI, meta []*met
 	var results []*packageHandle
 	if check {
 		for _, m := range meta {
-			ph, err := s.buildPackageHandle(ctx, m.id)
+			ph, err := s.buildPackageHandle(ctx, m.id, source.ParseFull)
 			if err != nil {
 				return nil, err
 			}
-			expectMode(ph, source.ParseFull)
 			results = append(results, ph)
 		}
 	} else {
@@ -252,11 +246,10 @@ func (s *snapshot) shouldCheck(m []*metadata) (phs []*packageHandle, load, check
 	// If a single PackageHandle is missing, re-check all of them.
 	// TODO: Optimize this by only checking the necessary packages.
 	for _, m := range m {
-		ph := s.getPackage(m.id)
+		ph := s.getPackage(m.id, source.ParseFull)
 		if ph == nil {
 			return nil, false, true
 		}
-		expectMode(ph, source.ParseFull)
 		phs = append(phs, ph)
 	}
 	// If the metadata for the package had missing dependencies,
@@ -414,24 +407,15 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.PackageHandle, e
 	for pkgID := range pkgIDs {
 		// Metadata for these packages should already be up-to-date,
 		// so just build the package handle directly (without a reload).
-		ph, err := s.buildPackageHandle(ctx, pkgID)
+		ph, err := s.buildPackageHandle(ctx, pkgID, source.ParseExported)
 		if err != nil {
 			return nil, err
 		}
-		expectMode(ph, source.ParseExported)
 		results = append(results, ph)
 	}
 	return results, nil
 }
 
-// expectMode is a defensive check to make sure that we mark workspace packages
-// correctly. TODO(rstambler): Remove this once we're confident this works.
-func expectMode(ph *packageHandle, mode source.ParseMode) {
-	if ph.mode != mode {
-		panic(fmt.Sprintf("unexpected parse mode for %s", ph.m.id))
-	}
-}
-
 func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Package, error) {
 	// Don't reload workspace package metadata.
 	// This function is meant to only return currently cached information.
@@ -460,31 +444,17 @@ func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Pac
 	return results, nil
 }
 
-func (s *snapshot) getPackage(id packageID) *packageHandle {
+func (s *snapshot) getPackage(id packageID, mode source.ParseMode) *packageHandle {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
 	key := packageKey{
 		id:   id,
-		mode: s.packageModeLocked(id),
+		mode: mode,
 	}
 	return s.packages[key]
 }
 
-func (s *snapshot) packageMode(id packageID) source.ParseMode {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	return s.packageModeLocked(id)
-}
-
-func (s *snapshot) packageModeLocked(id packageID) source.ParseMode {
-	if _, ok := s.workspacePackages[id]; ok {
-		return source.ParseFull
-	}
-	return source.ParseExported
-}
-
 func (s *snapshot) getActionHandle(id packageID, m source.ParseMode, a *analysis.Analyzer) *actionHandle {
 	s.mu.Lock()
 	defer s.mu.Unlock()

internal/lsp/source/view.go

@@ -61,6 +61,8 @@ type Snapshot interface {
 	CachedImportPaths(ctx context.Context) (map[string]Package, error)
 
 	// KnownPackages returns all the packages loaded in this snapshot.
+	// Workspace packages may be parsed in ParseFull mode, whereas transitive
+	// dependencies will be in ParseExported mode.
 	KnownPackages(ctx context.Context) ([]PackageHandle, error)
 
 	// WorkspacePackages returns the PackageHandles for the snapshot's