gopls/internal/cache: improve missing import error message
Make the missing import error knowledgeable of the view type, so that it
can correctly reference modules, GOROOT, GOPATH, or go/packages driver
as applicable.
While at it, fix some duplicated and broken logic for determining if the
view is in go/packages driver mode, consolidate on representing the
driver accurately as GoEnv.EffectiveGOPACKAGESDRIVER.
Fixes golang/go#64980
Change-Id: I7961aade981173098ab02cbe1862ac2eca2c394b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/589215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go
index 11d30d4..3340306 100644
--- a/gopls/internal/cache/check.go
+++ b/gopls/internal/cache/check.go
@@ -1356,9 +1356,9 @@
// TODO(rfindley): consider storing less data in gobDiagnostics, and
// interpreting each diagnostic in the context of a fixed set of options.
// Then these fields need not be part of the type checking inputs.
- relatedInformation bool
- linkTarget string
- moduleMode bool
+ supportsRelatedInformation bool
+ linkTarget string
+ viewType ViewType
}
func (s *Snapshot) typeCheckInputs(ctx context.Context, mp *metadata.Package) (typeCheckInputs, error) {
@@ -1396,9 +1396,9 @@
depsByImpPath: mp.DepsByImpPath,
goVersion: goVersion,
- relatedInformation: s.Options().RelatedInformationSupported,
- linkTarget: s.Options().LinkTarget,
- moduleMode: s.view.moduleMode(),
+ supportsRelatedInformation: s.Options().RelatedInformationSupported,
+ linkTarget: s.Options().LinkTarget,
+ viewType: s.view.typ,
}, nil
}
@@ -1455,9 +1455,9 @@
maxAlign := inputs.sizes.Alignof(types.NewPointer(types.Typ[types.Int64]))
fmt.Fprintf(hasher, "sizes: %d %d\n", wordSize, maxAlign)
- fmt.Fprintf(hasher, "relatedInformation: %t\n", inputs.relatedInformation)
+ fmt.Fprintf(hasher, "relatedInformation: %t\n", inputs.supportsRelatedInformation)
fmt.Fprintf(hasher, "linkTarget: %s\n", inputs.linkTarget)
- fmt.Fprintf(hasher, "moduleMode: %t\n", inputs.moduleMode)
+ fmt.Fprintf(hasher, "viewType: %d\n", inputs.viewType)
var hash [sha256.Size]byte
hasher.Sum(hash[:0])
@@ -1595,7 +1595,7 @@
}
}
- diags := typeErrorsToDiagnostics(pkg, pkg.typeErrors, inputs.linkTarget, inputs.moduleMode, inputs.relatedInformation)
+ diags := typeErrorsToDiagnostics(pkg, inputs, pkg.typeErrors)
for _, diag := range diags {
// If the file didn't parse cleanly, it is highly likely that type
// checking errors will be confusing or redundant. But otherwise, type
@@ -1630,7 +1630,7 @@
depPH := b.handles[id]
if depPH == nil {
// e.g. missing metadata for dependencies in buildPackageHandle
- return nil, missingPkgError(inputs.id, path, inputs.moduleMode)
+ return nil, missingPkgError(inputs.id, path, inputs.viewType)
}
if !metadata.IsValidImport(inputs.pkgPath, depPH.mp.PkgPath) {
return nil, fmt.Errorf("invalid use of internal package %q", path)
@@ -1825,20 +1825,23 @@
// missingPkgError returns an error message for a missing package that varies
// based on the user's workspace mode.
-func missingPkgError(from PackageID, pkgPath string, moduleMode bool) error {
- // TODO(rfindley): improve this error. Previous versions of this error had
- // access to the full snapshot, and could provide more information (such as
- // the initialization error).
- if moduleMode {
+func missingPkgError(from PackageID, pkgPath string, viewType ViewType) error {
+ switch viewType {
+ case GoModView, GoWorkView:
if metadata.IsCommandLineArguments(from) {
return fmt.Errorf("current file is not included in a workspace module")
} else {
// Previously, we would present the initialization error here.
return fmt.Errorf("no required module provides package %q", pkgPath)
}
- } else {
- // Previously, we would list the directories in GOROOT and GOPATH here.
+ case AdHocView:
+ return fmt.Errorf("cannot find package %q in GOROOT", pkgPath)
+ case GoPackagesDriverView:
+ return fmt.Errorf("go/packages driver could not load %q", pkgPath)
+ case GOPATHView:
return fmt.Errorf("cannot find package %q in GOROOT or GOPATH", pkgPath)
+ default:
+ return fmt.Errorf("unable to load package")
}
}
@@ -1852,9 +1855,8 @@
// to the previous error in the errs slice (such as if they were printed in
// sequence to a terminal).
//
-// The linkTarget, moduleMode, and supportsRelatedInformation parameters affect
-// the construction of protocol objects (see the code for details).
-func typeErrorsToDiagnostics(pkg *syntaxPackage, errs []types.Error, linkTarget string, moduleMode, supportsRelatedInformation bool) []*Diagnostic {
+// Fields in typeCheckInputs may affect the resulting diagnostics.
+func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs typeCheckInputs, errs []types.Error) []*Diagnostic {
var result []*Diagnostic
// batch records diagnostics for a set of related types.Errors.
@@ -1944,7 +1946,7 @@
}
msg := related[0].Msg // primary
if i > 0 {
- if supportsRelatedInformation {
+ if inputs.supportsRelatedInformation {
msg += " (see details)"
} else {
msg += fmt.Sprintf(" (this error: %v)", e.Msg)
@@ -1959,16 +1961,16 @@
}
if code != 0 {
diag.Code = code.String()
- diag.CodeHref = typesCodeHref(linkTarget, code)
+ diag.CodeHref = typesCodeHref(inputs.linkTarget, code)
}
if code == typesinternal.UnusedVar || code == typesinternal.UnusedImport {
diag.Tags = append(diag.Tags, protocol.Unnecessary)
}
if match := importErrorRe.FindStringSubmatch(e.Msg); match != nil {
- diag.SuggestedFixes = append(diag.SuggestedFixes, goGetQuickFixes(moduleMode, pgf.URI, match[1])...)
+ diag.SuggestedFixes = append(diag.SuggestedFixes, goGetQuickFixes(inputs.viewType.usesModules(), pgf.URI, match[1])...)
}
if match := unsupportedFeatureRe.FindStringSubmatch(e.Msg); match != nil {
- diag.SuggestedFixes = append(diag.SuggestedFixes, editGoDirectiveQuickFix(moduleMode, pgf.URI, match[1])...)
+ diag.SuggestedFixes = append(diag.SuggestedFixes, editGoDirectiveQuickFix(inputs.viewType.usesModules(), pgf.URI, match[1])...)
}
// Link up related information. For the primary error, all related errors
diff --git a/gopls/internal/cache/errors.go b/gopls/internal/cache/errors.go
index acb5385..7aa1e2c 100644
--- a/gopls/internal/cache/errors.go
+++ b/gopls/internal/cache/errors.go
@@ -129,9 +129,9 @@
var importErrorRe = regexp.MustCompile(`could not import ([^\s]+)`)
var unsupportedFeatureRe = regexp.MustCompile(`.*require.* go(\d+\.\d+) or later`)
-func goGetQuickFixes(moduleMode bool, uri protocol.DocumentURI, pkg string) []SuggestedFix {
+func goGetQuickFixes(haveModule bool, uri protocol.DocumentURI, pkg string) []SuggestedFix {
// Go get only supports module mode for now.
- if !moduleMode {
+ if !haveModule {
return nil
}
title := fmt.Sprintf("go get package %v", pkg)
@@ -147,9 +147,9 @@
return []SuggestedFix{SuggestedFixFromCommand(cmd, protocol.QuickFix)}
}
-func editGoDirectiveQuickFix(moduleMode bool, uri protocol.DocumentURI, version string) []SuggestedFix {
+func editGoDirectiveQuickFix(haveModule bool, uri protocol.DocumentURI, version string) []SuggestedFix {
// Go mod edit only supports module mode.
- if !moduleMode {
+ if !haveModule {
return nil
}
title := fmt.Sprintf("go mod edit -go=%s", version)
diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go
index 5995cce..e42290d 100644
--- a/gopls/internal/cache/load.go
+++ b/gopls/internal/cache/load.go
@@ -676,7 +676,7 @@
//
// For module views (of type GoMod or GoWork), packages must in any case be
// in a workspace module (enforced below).
- if !s.view.moduleMode() || !s.Options().ExpandWorkspaceToModule {
+ if !s.view.typ.usesModules() || !s.Options().ExpandWorkspaceToModule {
folder := s.view.folder.Dir.Path()
inFolder := false
for uri := range uris {
@@ -692,7 +692,7 @@
// In module mode, a workspace package must be contained in a workspace
// module.
- if s.view.moduleMode() {
+ if s.view.typ.usesModules() {
var modURI protocol.DocumentURI
if pkg.Module != nil {
modURI = protocol.URIFromPath(pkg.Module.GoMod)
diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go
index b300827..9f05cbb 100644
--- a/gopls/internal/cache/snapshot.go
+++ b/gopls/internal/cache/snapshot.go
@@ -878,7 +878,7 @@
watchGoFiles := fmt.Sprintf("**/*.{%s}", extensions)
var dirs []string
- if s.view.moduleMode() {
+ if s.view.typ.usesModules() {
if s.view.typ == GoWorkView {
workVendorDir := filepath.Join(s.view.gowork.Dir().Path(), "vendor")
workVendorURI := protocol.URIFromPath(workVendorDir)
diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go
index 56e26bc..6f76c55 100644
--- a/gopls/internal/cache/view.go
+++ b/gopls/internal/cache/view.go
@@ -72,8 +72,19 @@
GoVersionOutput string // complete go version output
// OS environment variables (notably not go env).
- GOWORK string
- GOPACKAGESDRIVER string
+
+ // ExplicitGOWORK is the GOWORK value set explicitly in the environment. This
+ // may differ from `go env GOWORK` when the GOWORK value is implicit from the
+ // working directory.
+ ExplicitGOWORK string
+
+ // EffectiveGOPACKAGESDRIVER is the effective go/packages driver binary that
+ // will be used. This may be set via GOPACKAGESDRIVER, or may be discovered
+ // via os.LookPath("gopackagesdriver"). The latter functionality is
+ // undocumented and may be removed in the future.
+ //
+ // If GOPACKAGESDRIVER is set to "off", EffectiveGOPACKAGESDRIVER is "".
+ EffectiveGOPACKAGESDRIVER string
}
// View represents a single build for a workspace.
@@ -315,9 +326,9 @@
}
}
-// moduleMode reports whether the view uses Go modules.
-func (w viewDefinition) moduleMode() bool {
- switch w.typ {
+// usesModules reports whether the view uses Go modules.
+func (typ ViewType) usesModules() bool {
+ switch typ {
case GoModView, GoWorkView:
return true
default:
@@ -829,9 +840,9 @@
var err error
dirURI := protocol.URIFromPath(dir)
goworkFromEnv := false
- if folder.Env.GOWORK != "off" && folder.Env.GOWORK != "" {
+ if folder.Env.ExplicitGOWORK != "off" && folder.Env.ExplicitGOWORK != "" {
goworkFromEnv = true
- def.gowork = protocol.URIFromPath(folder.Env.GOWORK)
+ def.gowork = protocol.URIFromPath(folder.Env.ExplicitGOWORK)
} else {
def.gowork, err = findRootPattern(ctx, dirURI, "go.work", fs)
if err != nil {
@@ -855,20 +866,10 @@
// - 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
- }
+ if def.folder.Env.EffectiveGOPACKAGESDRIVER != "" {
+ def.typ = GoPackagesDriverView
+ def.root = dirURI
+ return def, nil
}
// From go.dev/ref/mod, module mode is active if GO111MODULE=on, or
@@ -894,7 +895,7 @@
// Prefer a go.work file if it is available and contains the module relevant
// to forURI.
- if def.adjustedGO111MODULE() != "off" && folder.Env.GOWORK != "off" && def.gowork != "" {
+ if def.adjustedGO111MODULE() != "off" && folder.Env.ExplicitGOWORK != "off" && def.gowork != "" {
def.typ = GoWorkView
if goworkFromEnv {
// The go.work file could be anywhere, which can lead to confusing error
@@ -997,18 +998,20 @@
// The value of GOPACKAGESDRIVER is not returned through the go command.
if driver, ok := opts.Env["GOPACKAGESDRIVER"]; ok {
- env.GOPACKAGESDRIVER = driver
- } else {
- env.GOPACKAGESDRIVER = os.Getenv("GOPACKAGESDRIVER")
+ if driver != "off" {
+ env.EffectiveGOPACKAGESDRIVER = driver
+ }
+ } else if driver := os.Getenv("GOPACKAGESDRIVER"); driver != "off" {
+ env.EffectiveGOPACKAGESDRIVER = driver
// 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.
- if env.GOPACKAGESDRIVER == "" {
+ if env.EffectiveGOPACKAGESDRIVER == "" {
tool, err := exec.LookPath("gopackagesdriver")
if err == nil && tool != "" {
- env.GOPACKAGESDRIVER = tool
+ env.EffectiveGOPACKAGESDRIVER = tool
}
}
}
@@ -1017,9 +1020,9 @@
// between an explicit GOWORK value and one which is implicit from the file
// system. The former doesn't change unless the environment changes.
if gowork, ok := opts.Env["GOWORK"]; ok {
- env.GOWORK = gowork
+ env.ExplicitGOWORK = gowork
} else {
- env.GOWORK = os.Getenv("GOWORK")
+ env.ExplicitGOWORK = os.Getenv("GOWORK")
}
return env, nil
}
diff --git a/gopls/internal/test/integration/diagnostics/diagnostics_test.go b/gopls/internal/test/integration/diagnostics/diagnostics_test.go
index 2862a86..195089f 100644
--- a/gopls/internal/test/integration/diagnostics/diagnostics_test.go
+++ b/gopls/internal/test/integration/diagnostics/diagnostics_test.go
@@ -43,7 +43,13 @@
// This test is very basic: start with a clean Go program, make an error, and
// get a diagnostic for that error. However, it also demonstrates how to
// combine Expectations to await more complex state in the editor.
- Run(t, exampleProgram, func(t *testing.T, env *Env) {
+ RunMultiple{
+ {"golist", WithOptions(Modes(Default))},
+ {"gopackages", WithOptions(
+ Modes(Default),
+ FakeGoPackagesDriver(t),
+ )},
+ }.Run(t, exampleProgram, func(t *testing.T, env *Env) {
// Deleting the 'n' at the end of Println should generate a single error
// diagnostic.
env.OpenFile("main.go")
@@ -84,7 +90,15 @@
const Foo = "abc
`
- Run(t, brokenFile, func(t *testing.T, env *Env) {
+ RunMultiple{
+ {"golist", WithOptions(Modes(Default))},
+ // Since this test requires loading an overlay,
+ // it verifies that the fake go/packages driver honors overlays.
+ {"gopackages", WithOptions(
+ Modes(Default),
+ FakeGoPackagesDriver(t),
+ )},
+ }.Run(t, brokenFile, func(t *testing.T, env *Env) {
env.CreateBuffer("broken.go", brokenFile)
env.AfterChange(Diagnostics(env.AtRegexp("broken.go", "\"abc")))
})
@@ -559,7 +573,7 @@
NoOutstandingWork(IgnoreTelemetryPromptWork),
Diagnostics(
env.AtRegexp("a.go", `"mod.com`),
- WithMessage("GOROOT or GOPATH"),
+ WithMessage("in GOROOT"),
),
)
// Deleting the import dismisses the warning.
diff --git a/gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go b/gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go
new file mode 100644
index 0000000..7ed6d2a
--- /dev/null
+++ b/gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go
@@ -0,0 +1,48 @@
+// Copyright 2024 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 diagnostics
+
+import (
+ "testing"
+
+ . "golang.org/x/tools/gopls/internal/test/integration"
+)
+
+// Test that the import error does not mention GOPATH when building with
+// go/packages driver.
+func TestBrokenWorkspace_GOPACKAGESDRIVER(t *testing.T) {
+ // A go.mod file is actually needed here, because the fake go/packages driver
+ // uses go list behind the scenes, and we load go/packages driver workspaces
+ // with ./...
+ const files = `
+-- go.mod --
+module m
+go 1.12
+
+-- a.go --
+package foo
+
+import "mod.com/hello"
+
+func f() {
+}
+`
+ WithOptions(
+ FakeGoPackagesDriver(t),
+ ).Run(t, files, func(t *testing.T, env *Env) {
+ env.OpenFile("a.go")
+ env.AfterChange(
+ Diagnostics(
+ env.AtRegexp("a.go", `"mod.com`),
+ WithMessage("go/packages driver"),
+ ),
+ )
+ // Deleting the import removes the error.
+ env.RegexpReplace("a.go", `import "mod.com/hello"`, "")
+ env.AfterChange(
+ NoDiagnostics(ForFile("a.go")),
+ )
+ })
+}
diff --git a/gopls/internal/test/integration/options.go b/gopls/internal/test/integration/options.go
index d6c21e6..87be211 100644
--- a/gopls/internal/test/integration/options.go
+++ b/gopls/internal/test/integration/options.go
@@ -5,8 +5,12 @@
package integration
import (
+ "strings"
+ "testing"
+
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/test/integration/fake"
+ "golang.org/x/tools/internal/drivertest"
)
type runConfig struct {
@@ -161,6 +165,18 @@
}
}
+// FakeGoPackagesDriver configures gopls to run with a fake GOPACKAGESDRIVER
+// environment variable.
+func FakeGoPackagesDriver(t *testing.T) RunOption {
+ env := drivertest.Env(t)
+ vars := make(EnvVars)
+ for _, e := range env {
+ kv := strings.SplitN(e, "=", 2)
+ vars[kv[0]] = kv[1]
+ }
+ return vars
+}
+
// InGOPATH configures the workspace working directory to be GOPATH, rather
// than a separate working directory for use with modules.
func InGOPATH() RunOption {
diff --git a/gopls/internal/test/integration/regtest.go b/gopls/internal/test/integration/regtest.go
index 96c1044..b676fd4 100644
--- a/gopls/internal/test/integration/regtest.go
+++ b/gopls/internal/test/integration/regtest.go
@@ -15,6 +15,7 @@
"golang.org/x/tools/gopls/internal/cache"
"golang.org/x/tools/gopls/internal/cmd"
+ "golang.org/x/tools/internal/drivertest"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/memoize"
"golang.org/x/tools/internal/testenv"
@@ -45,12 +46,6 @@
var runner *Runner
-// The integrationTestRunner interface abstracts the Run operation,
-// enables decorators for various optional features.
-type integrationTestRunner interface {
- Run(t *testing.T, files string, f TestFunc)
-}
-
func Run(t *testing.T, files string, f TestFunc) {
runner.Run(t, files, f)
}
@@ -79,9 +74,15 @@
runner.Run(t, files, f, r.opts...)
}
+// RunMultiple runs a test multiple times, with different options.
+// The runner should be constructed with [WithOptions].
+//
+// TODO(rfindley): replace Modes with selective use of RunMultiple.
type RunMultiple []struct {
Name string
- Runner integrationTestRunner
+ Runner interface {
+ Run(t *testing.T, files string, f TestFunc)
+ }
}
func (r RunMultiple) Run(t *testing.T, files string, f TestFunc) {
@@ -112,6 +113,9 @@
// Main sets up and tears down the shared integration test state.
func Main(m *testing.M) (code int) {
+ // Provide an entrypoint for tests that use a fake go/packages driver.
+ drivertest.RunIfChild()
+
defer func() {
if runner != nil {
if err := runner.Close(); err != nil {
