Skip to content

Commit

Permalink
gopls/internal/lsp/cache: add views for unused modules in selectViews
Browse files Browse the repository at this point in the history 
Update the zero-config algorithm to define views for modules that are
unused by the relevant go.work file. In these cases, load the module
independently via GOWORK=off.

For golang/go#57979

Change-Id: I7b126c41694131660a91380340fa9ceeeeef948d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550915
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
@findleyr @gopherbot
findleyr authored and gopherbot committed Dec 19, 2023
1 parent f801f12 commit 160631f
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 54 deletions.
46 changes: 32 additions & 14 deletions gopls/internal/lsp/cache/session_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
// fields exported for cmp.Diff
Type ViewType
Root string
Env []string
}

type folderSummary struct {
Expand Down Expand Up @@ -52,7 +53,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "."}},
nil,
[]viewSummary{{GoWorkView, "."}},
[]viewSummary{{GoWorkView, ".", nil}},
},
{
"basic go.mod workspace",
Expand All @@ -61,7 +62,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "."}},
nil,
[]viewSummary{{GoModView, "."}},
[]viewSummary{{GoModView, ".", nil}},
},
{
"basic GOPATH workspace",
Expand All @@ -80,7 +81,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
}},
[]string{"src/golang.org/a//a.go", "src/golang.org/b/b.go"},
[]viewSummary{{GOPATHView, "src"}},
[]viewSummary{{GOPATHView, "src", nil}},
},
{
"basic AdHoc workspace",
Expand All @@ -89,7 +90,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "."}},
nil,
[]viewSummary{{AdHocView, "."}},
[]viewSummary{{AdHocView, ".", nil}},
},
{
"multi-folder workspace",
Expand All @@ -99,7 +100,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "a"}, {dir: "b"}},
nil,
[]viewSummary{{GoModView, "a"}, {GoModView, "b"}},
[]viewSummary{{GoModView, "a", nil}, {GoModView, "b", nil}},
},
{
"multi-module workspace",
Expand All @@ -109,7 +110,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "."}},
nil,
[]viewSummary{{AdHocView, "."}},
[]viewSummary{{AdHocView, ".", nil}},
},
{
"zero-config open module",
Expand All @@ -122,8 +123,8 @@ func TestZeroConfigAlgorithm(t *testing.T) {
[]folderSummary{{dir: "."}},
[]string{"a/a.go"},
[]viewSummary{
{AdHocView, "."},
{GoModView, "a"},
{AdHocView, ".", nil},
{GoModView, "a", nil},
},
},
{
Expand All @@ -137,9 +138,9 @@ func TestZeroConfigAlgorithm(t *testing.T) {
[]folderSummary{{dir: "."}},
[]string{"a/a.go", "b/b.go"},
[]viewSummary{
{AdHocView, "."},
{GoModView, "a"},
{GoModView, "b"},
{AdHocView, ".", nil},
{GoModView, "a", nil},
{GoModView, "b", nil},
},
},
{
Expand All @@ -153,7 +154,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "."}},
[]string{"a/a.go", "b/b.go"},
[]viewSummary{{GoWorkView, "."}},
[]viewSummary{{GoWorkView, ".", nil}},
},
{
"go.work from env",
Expand All @@ -175,7 +176,23 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
}},
[]string{"a/a.go", "b/b.go"},
[]viewSummary{{GoWorkView, "."}},
[]viewSummary{{GoWorkView, ".", nil}},
},
{
"independent module view",
map[string]string{
"go.work": "go 1.18\nuse (\n\t./a\n)\n", // not using b
"a/go.mod": "module golang.org/a\ngo 1.18\n",
"a/a.go": "package a",
"b/go.mod": "module golang.org/a\ngo 1.18\n",
"b/b.go": "package b",
},
[]folderSummary{{dir: "."}},
[]string{"a/a.go", "b/b.go"},
[]viewSummary{
{GoWorkView, ".", nil},
{GoModView, "b", []string{"GOWORK=off"}},
},
},
{
"multiple go.work",
Expand All @@ -190,7 +207,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
},
[]folderSummary{{dir: "."}},
[]string{"a/a.go", "b/b.go", "b/c/c.go"},
[]viewSummary{{GoWorkView, "."}, {GoWorkView, "b/c"}},
[]viewSummary{{GoWorkView, ".", nil}, {GoWorkView, "b/c", nil}},
},
}

Expand Down Expand Up @@ -237,6 +254,7 @@ func TestZeroConfigAlgorithm(t *testing.T) {
got = append(got, viewSummary{
Type: view.Type(),
Root: rel.RelPath(view.root.Path()),
Env: view.envOverlay,
})
}
if diff := cmp.Diff(test.want, got); diff != "" {
Expand Down
18 changes: 13 additions & 5 deletions gopls/internal/lsp/cache/snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"golang.org/x/tools/gopls/internal/util/maps"
"golang.org/x/tools/gopls/internal/util/pathutil"
"golang.org/x/tools/gopls/internal/util/persistent"
"golang.org/x/tools/gopls/internal/util/slices"
"golang.org/x/tools/gopls/internal/vulncheck"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/event/label"
Expand Down Expand Up @@ -498,12 +499,19 @@ func (s *Snapshot) goCommandInvocation(ctx context.Context, flags InvocationFlag
allowModfileModificationOption := s.Options().AllowModfileModifications
allowNetworkOption := s.Options().AllowImplicitNetworkAccess

// TODO(rfindley): this is very hard to follow, and may not even be doing the
// right thing: should inv.Env really trample view.options? Do we ever invoke
// this with a non-empty inv.Env?
// TODO(rfindley): it's not clear that this is doing the right thing.
// Should inv.Env really overwrite view.options? Should s.view.envOverlay
// overwrite inv.Env? (Do we ever invoke this with a non-empty inv.Env?)
//
// We should refactor to make it clearer that the correct env is being used.
inv.Env = append(append(append(os.Environ(), s.Options().EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.adjustedGO111MODULE)
// We should survey existing uses and write down rules for how env is
// applied.
inv.Env = slices.Concat(
os.Environ(),
s.Options().EnvSlice(),
inv.Env,
[]string{"GO111MODULE=" + s.view.adjustedGO111MODULE},
s.view.envOverlay,
)
inv.BuildFlags = append([]string{}, s.Options().BuildFlags...)
cleanup = func() {} // fallback

Expand Down
115 changes: 80 additions & 35 deletions gopls/internal/lsp/cache/view.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ type viewDefinition struct {
// TODO(rfindley): should we just run `go list -m` to compute this set?
workspaceModFiles map[protocol.DocumentURI]struct{}
workspaceModFilesErr error // error encountered computing workspaceModFiles

// envOverlay holds additional environment to apply to this viewDefinition.
envOverlay []string
}

type viewDef struct {
Expand Down Expand Up @@ -191,6 +194,14 @@ func viewDefinitionsEqual(x, y *viewDefinition) bool {
} else if !maps.SameKeys(x.workspaceModFiles, y.workspaceModFiles) {
return false
}
if len(x.envOverlay) != len(y.envOverlay) {
return false
}
for i, xv := range x.envOverlay {
if xv != y.envOverlay[i] {
return false
}
}
return x.viewDef == y.viewDef
}

Expand Down Expand Up @@ -875,25 +886,6 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forURI prot
def.adjustedGO111MODULE = "auto"
}

hasGoPackagesDriver := false
{
// The value of GOPACKAGESDRIVER is not returned through the go command.
gopackagesdriver := os.Getenv("GOPACKAGESDRIVER")
// A user may also have a gopackagesdriver binary on their machine, which
// works the same way as setting GOPACKAGESDRIVER.
tool, _ := exec.LookPath("gopackagesdriver")
hasGoPackagesDriver = gopackagesdriver != "off" && (gopackagesdriver != "" || tool != "")
}

// Check if the workspace is within any GOPATH directory.
inGOPATH := false
for _, gp := range filepath.SplitList(def.gopath) {
if pathutil.InDir(filepath.Join(gp, "src"), dir) {
inGOPATH = true
break
}
}

dirURI := protocol.URIFromPath(dir)
goworkFromEnv := false
if rawGoWork != "off" && rawGoWork != "" {
Expand Down Expand Up @@ -927,18 +919,38 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forURI prot
}

// Determine how we load and where to load package information for this view
// (i.e. the ViewType and root).
//
// If GOPACKAGESDRIVER is set it takes precedence. Otherwise, check if we're
// in one of the module modes (go.mod or go.work). From go.dev/ref/mod,
// module mode is active if GO111MODULE is on, or in {auto,""} and we are
// inside a module or have a GOWORK value. But gopls is less strict, allowing
// GOPATH mode if GO111MODULE="", and AdHoc views if no module is found.
switch {
case hasGoPackagesDriver:
def.typ = GoPackagesDriverView
def.root = dirURI
case def.adjustedGO111MODULE != "off" && rawGoWork != "off" && def.gowork != "":
// Specifically, set
// - def.typ
// - def.root
// - def.workspaceModFiles, and
// - def.envOverlay.

// If GOPACKAGESDRIVER is set it takes precedence.
{
// The value of GOPACKAGESDRIVER is not returned through the go command.
gopackagesdriver := os.Getenv("GOPACKAGESDRIVER")
// A user may also have a gopackagesdriver binary on their machine, which
// works the same way as setting GOPACKAGESDRIVER.
//
// TODO(rfindley): remove this call to LookPath. We should not support this
// undocumented method of setting GOPACKAGESDRIVER.
tool, err := exec.LookPath("gopackagesdriver")
if gopackagesdriver != "off" && (gopackagesdriver != "" || (err == nil && tool != "")) {
def.typ = GoPackagesDriverView
def.root = dirURI
return def, nil
}
}

// From go.dev/ref/mod, module mode is active if GO111MODULE=on, or
// GO111MODULE=auto or "" and we are inside a module or have a GOWORK value.
// But gopls is less strict, allowing GOPATH mode if GO111MODULE="", and
// AdHoc views if no module is found.

// Prefer a go.work file if it is available and contains the module relevant
// to forURI.
if def.adjustedGO111MODULE != "off" && rawGoWork != "off" && def.gowork != "" {
def.typ = GoWorkView
if goworkFromEnv {
// The go.work file could be anywhere, which can lead to confusing error
Expand All @@ -948,18 +960,51 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forURI prot
def.root = def.gowork.Dir()
}
def.workspaceModFiles, def.workspaceModFilesErr = goWorkModules(ctx, def.gowork, fs)
case def.adjustedGO111MODULE != "off" && def.gomod != "":

// If forURI is in a module but that module is not
// included in the go.work file, use a go.mod view with GOWORK=off.
if forURI != "" && def.workspaceModFilesErr == nil && def.gomod != "" {
if _, ok := def.workspaceModFiles[def.gomod]; !ok {
def.typ = GoModView
def.root = def.gomod.Dir()
def.workspaceModFiles = map[protocol.DocumentURI]unit{def.gomod: {}}
def.envOverlay = []string{"GOWORK=off"}
}
}
return def, nil
}

// Otherwise, use the active module, if in module mode.
//
// Note, we could override GO111MODULE here via envOverlay if we wanted to
// support the case where someone opens a module with GO111MODULE=off. But
// that is probably not worth worrying about (at this point, folks probably
// shouldn't be setting GO111MODULE).
if def.adjustedGO111MODULE != "off" && def.gomod != "" {
def.typ = GoModView
def.root = def.gomod.Dir()
def.workspaceModFiles = map[protocol.DocumentURI]struct{}{def.gomod: {}}
case def.adjustedGO111MODULE != "on" && inGOPATH:
return def, nil
}

// Check if the workspace is within any GOPATH directory.
inGOPATH := false
for _, gp := range filepath.SplitList(def.gopath) {
if pathutil.InDir(filepath.Join(gp, "src"), dir) {
inGOPATH = true
break
}
}
if def.adjustedGO111MODULE != "on" && inGOPATH {
def.typ = GOPATHView
def.root = dirURI
default:
def.typ = AdHocView
def.root = dirURI
return def, nil
}

// We're not in a workspace, module, or GOPATH, so have no better choice than
// an ad-hoc view.
def.typ = AdHocView
def.root = dirURI
return def, nil
}

Expand Down

0 comments on commit 160631f

Please sign in to comment.