internal/lsp/cache: always type check in default mode

Every package has a default type checking mode dictated by whether it's
in the workspace or not. Some features force full rather than exported
type checking, but AFAICT that ends up being more harm than good. For
example, let's say we want to Find References on fmt.Printf in the stdlib.
Before this CL, we'd force a new type check of the fmt package, then
find no references because nothing else would have been checked against
that new version.

While there may be some features that work better in the current regime,
I can't think of any, and we have no test coverage for them. So I'd
rather start with what makes sense, and if we want to change it maybe
let's write some tests.

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

internal/lsp/cache/check.go

@@ -56,7 +56,11 @@ type packageData struct {
 }
 
 // buildPackageHandle returns a packageHandle for a given package and mode.
-func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) {
+func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID) (*packageHandle, error) {
+	mode := source.ParseExported
+	if _, ok := s.isWorkspacePackage(id); ok {
+		mode = source.ParseFull
+	}
 	if ph := s.getPackage(id, mode); ph != nil {
 		return ph, nil
 	}
@@ -141,11 +145,7 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
 	// Begin computing the key by getting the depKeys for all dependencies.
 	var depKeys []packageHandleKey
 	for _, depID := range depList {
-		mode := source.ParseExported
-		if _, ok := s.isWorkspacePackage(depID); ok {
-			mode = source.ParseFull
-		}
-		depHandle, err := s.buildPackageHandle(ctx, depID, mode)
+		depHandle, err := s.buildPackageHandle(ctx, depID)
 		if err != nil {
 			event.Error(ctx, "no dep handle", err, tag.Package.Of(string(depID)))
 			if ctx.Err() != nil {

internal/lsp/cache/load.go

@@ -152,7 +152,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error {
 		if err != nil {
 			return err
 		}
-		if _, err := s.buildPackageHandle(ctx, m.id, source.ParseFull); err != nil {
+		if _, err := s.buildPackageHandle(ctx, m.id); err != nil {
 			return err
 		}
 	}

internal/lsp/cache/snapshot.go

@@ -283,7 +283,7 @@ func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI) ([]source.
 	// Get the list of IDs from the snapshot again, in case it has changed.
 	var pkgs []source.Package
 	for _, id := range s.getIDsForURI(uri) {
-		pkg, err := s.checkedPackage(ctx, id, source.ParseFull)
+		pkg, err := s.checkedPackage(ctx, id)
 		if err != nil {
 			return nil, err
 		}
@@ -292,8 +292,8 @@ func (s *snapshot) PackagesForFile(ctx context.Context, uri span.URI) ([]source.
 	return pkgs, nil
 }
 
-func (s *snapshot) checkedPackage(ctx context.Context, id packageID, mode source.ParseMode) (*pkg, error) {
-	ph, err := s.buildPackageHandle(ctx, id, mode)
+func (s *snapshot) checkedPackage(ctx context.Context, id packageID) (*pkg, error) {
+	ph, err := s.buildPackageHandle(ctx, id)
 	if err != nil {
 		return nil, err
 	}
@@ -312,7 +312,7 @@ func (s *snapshot) GetReverseDependencies(ctx context.Context, id string) ([]sou
 
 	var pkgs []source.Package
 	for id := range ids {
-		pkg, err := s.checkedPackage(ctx, id, source.ParseFull)
+		pkg, err := s.checkedPackage(ctx, id)
 		if err != nil {
 			return nil, err
 		}
@@ -448,7 +448,7 @@ func (s *snapshot) WorkspacePackages(ctx context.Context) ([]source.Package, err
 	}
 	var pkgs []source.Package
 	for _, pkgID := range s.workspacePackageIDs() {
-		pkg, err := s.checkedPackage(ctx, pkgID, source.ParseFull)
+		pkg, err := s.checkedPackage(ctx, pkgID)
 		if err != nil {
 			return nil, err
 		}
@@ -464,27 +464,19 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.Package, error)
 
 	// The WorkspaceSymbols implementation relies on this function returning
 	// workspace packages first.
-	wsPackages := s.workspacePackageIDs()
-	var otherPackages []packageID
+	ids := s.workspacePackageIDs()
 	s.mu.Lock()
 	for id := range s.metadata {
 		if _, ok := s.workspacePackages[id]; ok {
 			continue
 		}
-		otherPackages = append(otherPackages, id)
+		ids = append(ids, id)
 	}
 	s.mu.Unlock()
 
 	var pkgs []source.Package
-	for _, id := range wsPackages {
-		pkg, err := s.checkedPackage(ctx, id, source.ParseFull)
-		if err != nil {
-			return nil, err
-		}
-		pkgs = append(pkgs, pkg)
-	}
-	for _, id := range otherPackages {
-		pkg, err := s.checkedPackage(ctx, id, source.ParseExported)
+	for _, id := range ids {
+		pkg, err := s.checkedPackage(ctx, id)
 		if err != nil {
 			return nil, err
 		}

internal/lsp/regtest/references_test.go

@@ -0,0 +1,41 @@
+// Copyright 2020 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 regtest
+
+import (
+	"testing"
+)
+
+func TestNonWorkspaceReferences(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.com
+
+-- main.go --
+package main
+
+import "fmt"
+
+func main() {
+	fmt.Print()
+}
+`
+
+	run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("main.go")
+		file, pos := env.GoToDefinition("main.go", env.RegexpSearch("main.go", `fmt.(Print)`))
+		refs, err := env.Editor.References(env.Ctx, file, pos)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(refs) != 2 {
+			t.Fatalf("got %v reference(s), want 2", len(refs))
+		}
+		// The first reference is guaranteed to be the definition.
+		if got, want := refs[1].URI, env.Sandbox.Workdir.URI("main.go"); got != want {
+			t.Errorf("found reference in %v, wanted %v", got, want)
+		}
+	})
+}