gopls/internal/lsp/cache: load modules by dir, not module path

As uncovered in golang/go#59458, loading modules by <modulepath>/... can
be incorrect in cases where the package pattern matches more than one
module in the module graph. In such cases, it is possible that we will
query modules that do not have a corresponding entry in the go.sum file
(due to pruning).

Fix this by loading <dir>/... for each module root directory.

Fixes golang/go#59458

Change-Id: Ia163f4ab18847289941385e4eb9233906a4363c6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/485840
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

Diff for: gopls/internal/lsp/cache/load.go

@@ -72,14 +72,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 			}
 
 		case moduleLoadScope:
-			switch scope {
-			case "std", "cmd":
-				query = append(query, string(scope))
-			default:
-				modQuery := fmt.Sprintf("%s/...", scope)
-				query = append(query, modQuery)
-				moduleQueries[modQuery] = string(scope)
-			}
+			modQuery := fmt.Sprintf("%s%c...", scope.dir, filepath.Separator)
+			query = append(query, modQuery)
+			moduleQueries[modQuery] = string(scope.modulePath)
 
 		case viewLoadScope:
 			// If we are outside of GOPATH, a module, or some other known

Diff for: gopls/internal/lsp/cache/pkg.go

@@ -74,8 +74,11 @@ type loadScope interface {
 type (
 	fileLoadScope    span.URI // load packages containing a file (including command-line-arguments)
 	packageLoadScope string   // load a specific package (the value is its PackageID)
-	moduleLoadScope  string   // load packages in a specific module
-	viewLoadScope    span.URI // load the workspace
+	moduleLoadScope  struct {
+		dir        string // dir containing the go.mod file
+		modulePath string // parsed module path
+	}
+	viewLoadScope span.URI // load the workspace
 )
 
 // Implement the loadScope interface.

Diff for: gopls/internal/lsp/cache/view.go

@@ -702,6 +702,12 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr
 
 	if len(s.workspaceModFiles) > 0 {
 		for modURI := range s.workspaceModFiles {
+			// Verify that the modfile is valid before trying to load it.
+			//
+			// TODO(rfindley): now that we no longer need to parse the modfile in
+			// order to load scope, we could move these diagnostics to a more general
+			// location where we diagnose problems with modfiles or the workspace.
+			//
 			// Be careful not to add context cancellation errors as critical module
 			// errors.
 			fh, err := s.ReadFile(ctx, modURI)
@@ -722,8 +728,11 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr
 				addError(modURI, fmt.Errorf("no module path for %s", modURI))
 				continue
 			}
-			path := parsed.File.Module.Mod.Path
-			scopes = append(scopes, moduleLoadScope(path))
+			moduleDir := filepath.Dir(modURI.Filename())
+			// Previously, we loaded <modulepath>/... for each module path, but that
+			// is actually incorrect when the pattern may match packages in more than
+			// one module. See golang/go#59458 for more details.
+			scopes = append(scopes, moduleLoadScope{dir: moduleDir, modulePath: parsed.File.Module.Mod.Path})
 		}
 	} else {
 		scopes = append(scopes, viewLoadScope("LOAD_VIEW"))

Diff for: gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -1814,7 +1814,8 @@ func main() {}
 	Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("go.mod")
 		env.AfterChange(
-			LogMatching(protocol.Info, `.*query=\[builtin mod.com/...\].*`, 1, false),
+			// Check that we have only loaded "<dir>/..." once.
+			LogMatching(protocol.Info, `.*query=.*\.\.\..*`, 1, false),
 		)
 	})
 }

Diff for: gopls/internal/regtest/workspace/fromenv_test.go

@@ -5,6 +5,8 @@
 package workspace
 
 import (
+	"fmt"
+	"path/filepath"
 	"testing"
 
 	. "golang.org/x/tools/gopls/internal/lsp/regtest"
@@ -14,7 +16,12 @@ import (
 // Test that setting go.work via environment variables or settings works.
 func TestUseGoWorkOutsideTheWorkspace(t *testing.T) {
 	testenv.NeedsGo1Point(t, 18)
-	const files = `
+
+	// As discussed in
+	// https://github.com/golang/go/issues/59458#issuecomment-1513794691, we must
+	// use \-separated paths in go.work use directives for this test to work
+	// correctly on windows.
+	var files = fmt.Sprintf(`
 -- work/a/go.mod --
 module a.com
 
@@ -41,15 +48,19 @@ package c
 go 1.18
 
 use (
-	$SANDBOX_WORKDIR/work/a
-	$SANDBOX_WORKDIR/work/b
-	$SANDBOX_WORKDIR/other/c
+	%s
+	%s
+	%s
 )
-`
+`,
+		filepath.Join("$SANDBOX_WORKDIR", "work", "a"),
+		filepath.Join("$SANDBOX_WORKDIR", "work", "b"),
+		filepath.Join("$SANDBOX_WORKDIR", "other", "c"),
+	)
 
 	WithOptions(
 		WorkspaceFolders("work"), // use a nested workspace dir, so that GOWORK is outside the workspace
-		EnvVars{"GOWORK": "$SANDBOX_WORKDIR/config/go.work"},
+		EnvVars{"GOWORK": filepath.Join("$SANDBOX_WORKDIR", "config", "go.work")},
 	).Run(t, files, func(t *testing.T, env *Env) {
 		// When we have an explicit GOWORK set, we should get a file watch request.
 		env.OnceMet(

Diff for: gopls/internal/regtest/workspace/metadata_test.go

@@ -179,3 +179,76 @@ func Hello() int {
 		}
 	})
 }
+
+// Test for golang/go#59458. With lazy module loading, we may not need
+// transitively required modules.
+func TestNestedModuleLoading_Issue59458(t *testing.T) {
+	testenv.NeedsGo1Point(t, 17) // needs lazy module loading
+
+	// In this test, module b.com/nested requires b.com/other, which in turn
+	// requires b.com, but b.com/nested does not reach b.com through the package
+	// graph. Therefore, b.com/nested does not need b.com on 1.17 and later,
+	// thanks to graph pruning.
+	//
+	// We verify that we can load b.com/nested successfully. Previously, we
+	// couldn't, because loading the pattern b.com/nested/... matched the module
+	// b.com, which exists in the module graph but does not have a go.sum entry.
+
+	const proxy = `
+-- [email protected]/go.mod --
+module b.com
+
+go 1.18
+-- [email protected]/b/b.go --
+package b
+
+func Hello() {}
+
+-- b.com/[email protected]/go.mod --
+module b.com/other
+
+go 1.18
+
+require b.com v1.2.3
+-- b.com/[email protected]/go.sun --
+b.com v1.2.3 h1:AGjCxWRJLUuJiZ21IUTByr9buoa6+B6Qh5LFhVLKpn4=
+-- b.com/[email protected]/bar/bar.go --
+package bar
+
+import "b.com/b"
+
+func _() {
+	b.Hello()
+}
+-- b.com/[email protected]/foo/foo.go --
+package foo
+
+const Foo = 0
+`
+
+	const files = `
+-- go.mod --
+module b.com/nested
+
+go 1.18
+
+require b.com/other v1.4.6
+-- go.sum --
+b.com/other v1.4.6 h1:pHXSzGsk6DamYXp9uRdDB9A/ZQqAN9it+JudU0sBf94=
+b.com/other v1.4.6/go.mod h1:T0TYuGdAHw4p/l0+1P/yhhYHfZRia7PaadNVDu58OWM=
+-- nested.go --
+package nested
+
+import "b.com/other/foo"
+
+const C = foo.Foo
+`
+	WithOptions(
+		ProxyFiles(proxy),
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.OnceMet(
+			InitialWorkspaceLoad,
+			NoDiagnostics(),
+		)
+	})
+}