[go] cmd/go/internal/modload: set errors for packages with invalid import paths

[Bryan Mills (Gerrit)]

Bryan Mills submitted this change.

View Change

Approvals: Russ Cox: Looks good to me, approved Bryan Mills: Trusted; Run TryBots Gopher Robot: TryBots succeeded

cmd/go/internal/modload: set errors for packages with invalid import paths

Prior to CL 339170, relative errors in module mode resulted in a
base.Fatalf from the module loader, which caused unrecoverable errors
from 'go list -e' but successfully rejected relative imports (which
were never intended to work in module mode in the first place).

After that CL, the base.Fatalf is no longer present, but some errors
that had triggered that base.Fatalf were no longer diagnosed at all:
the module loader left them for the package loader to report, and the
package loader assumed that the module loader would report them.

Since the module loader already knows that the paths are invalid,
it now reports those errors itself.

Fixes #51125

Change-Id: I70e5818cfcfeea0ac70e17274427b08a74fd7c13
Reviewed-on: https://go-review.googlesource.com/c/go/+/386176
Trust: Bryan Mills <bcm...@google.com>
Run-TryBot: Bryan Mills <bcm...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Russ Cox <r...@golang.org>
---
M src/cmd/compile/internal/test/testdata/ptrsort.go
M src/cmd/go/internal/load/pkg.go
M src/cmd/go/internal/modload/import.go
M src/cmd/go/internal/modload/load.go
M src/cmd/go/testdata/script/build_internal.txt
A src/cmd/go/testdata/script/run_issue51125.txt
M src/cmd/go/testdata/script/test_relative_cmdline.txt
7 files changed, 109 insertions(+), 25 deletions(-)

diff --git a/src/cmd/compile/internal/test/testdata/ptrsort.go b/src/cmd/compile/internal/test/testdata/ptrsort.go
index 6cc7ba4..d26ba58 100644
--- a/src/cmd/compile/internal/test/testdata/ptrsort.go
+++ b/src/cmd/compile/internal/test/testdata/ptrsort.go
@@ -6,7 +6,7 @@
 import (
 	"fmt"
 
-	"./mysort"
+	"cmd/compile/internal/test/testdata/mysort"
 )
 
 type MyString struct {
diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go
index 214502d..d68f43a 100644
--- a/src/cmd/go/internal/load/pkg.go
+++ b/src/cmd/go/internal/load/pkg.go
@@ -819,11 +819,11 @@
 	}
 	r := resolvedImportCache.Do(importKey, func() any {
 		var r resolvedImport
-		if build.IsLocalImport(path) {
+		if cfg.ModulesEnabled {
+			r.dir, r.path, r.err = modload.Lookup(parentPath, parentIsStd, path)
+		} else if build.IsLocalImport(path) {
 			r.dir = filepath.Join(parentDir, path)
 			r.path = dirToImportPath(r.dir)
-		} else if cfg.ModulesEnabled {
-			r.dir, r.path, r.err = modload.Lookup(parentPath, parentIsStd, path)
 		} else if mode&ResolveImport != 0 {
 			// We do our own path resolution, because we want to
 			// find out the key to use in packageCache without the
@@ -1113,6 +1113,7 @@
 	}
 
 	if !str.HasFilePathPrefix(dir, root) || len(dir) <= len(root) || dir[len(root)] != filepath.Separator || path != "command-line-arguments" && !build.IsLocalImport(path) && filepath.Join(root, path) != dir {
+		debug.PrintStack()
 		base.Fatalf("unexpected directory layout:\n"+
 			"	import path: %s\n"+
 			"	root: %s\n"+
diff --git a/src/cmd/go/internal/modload/import.go b/src/cmd/go/internal/modload/import.go
index 812e48a..4862f62 100644
--- a/src/cmd/go/internal/modload/import.go
+++ b/src/cmd/go/internal/modload/import.go
@@ -248,12 +248,26 @@
 // return the module, its root directory, and a list of other modules that
 // lexically could have provided the package but did not.
 func importFromModules(ctx context.Context, path string, rs *Requirements, mg *ModuleGraph) (m module.Version, dir string, altMods []module.Version, err error) {
+	invalidf := func(format string, args ...interface{}) (module.Version, string, []module.Version, error) {
+		return module.Version{}, "", nil, &invalidImportError{
+			importPath: path,
+			err:        fmt.Errorf(format, args...),
+		}
+	}
+
 	if strings.Contains(path, "@") {
-		return module.Version{}, "", nil, fmt.Errorf("import path should not have @version")
+		return invalidf("import path %q should not have @version", path)
 	}
 	if build.IsLocalImport(path) {
-		return module.Version{}, "", nil, fmt.Errorf("relative import not supported")
+		return invalidf("%q is relative, but relative import paths are not supported in module mode", path)
 	}
+	if filepath.IsAbs(path) {
+		return invalidf("%q is not a package path; see 'go help packages'", path)
+	}
+	if search.IsMetaPackage(path) {
+		return invalidf("%q is not an importable package; see 'go help packages'", path)
+	}
+
 	if path == "C" {
 		// There's no directory for import "C".
 		return module.Version{}, "", nil, nil
diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go
index a4a7cb2..d4847ef 100644
--- a/src/cmd/go/internal/modload/load.go
+++ b/src/cmd/go/internal/modload/load.go
@@ -1675,24 +1675,6 @@
 
 // load loads an individual package.
 func (ld *loader) load(ctx context.Context, pkg *loadPkg) {
-	if strings.Contains(pkg.path, "@") {
-		// Leave for error during load.
-		return
-	}
-	if build.IsLocalImport(pkg.path) || filepath.IsAbs(pkg.path) {
-		// Leave for error during load.
-		// (Module mode does not allow local imports.)
-		return
-	}
-
-	if search.IsMetaPackage(pkg.path) {
-		pkg.err = &invalidImportError{
-			importPath: pkg.path,
-			err:        fmt.Errorf("%q is not an importable package; see 'go help packages'", pkg.path),
-		}
-		return
-	}
-
 	var mg *ModuleGraph
 	if ld.requirements.pruning == unpruned {
 		var err error
diff --git a/src/cmd/go/testdata/script/build_internal.txt b/src/cmd/go/testdata/script/build_internal.txt
index 25aa18c..5b786f2 100644
--- a/src/cmd/go/testdata/script/build_internal.txt
+++ b/src/cmd/go/testdata/script/build_internal.txt
@@ -10,8 +10,10 @@
 
 # Test internal packages outside GOROOT are respected
 cd ../testinternal2
+env GO111MODULE=off
 ! go build -v .
 stderr 'p\.go:3:8: use of internal package .*internal/w not allowed'
+env GO111MODULE=''
 
 [gccgo] skip # gccgo does not have GOROOT
 cd ../testinternal
diff --git a/src/cmd/go/testdata/script/run_issue51125.txt b/src/cmd/go/testdata/script/run_issue51125.txt
new file mode 100644
index 0000000..8fa4486
--- /dev/null
+++ b/src/cmd/go/testdata/script/run_issue51125.txt
@@ -0,0 +1,54 @@
+# Regression test for https://go.dev/issue/51125:
+# Relative import paths (a holdover from GOPATH) were accidentally allowed in module mode.
+
+cd $WORK
+
+# Relative imports should not be allowed with a go.mod file.
+
+! go run driver.go
+stderr '^driver.go:3:8: "./mypkg" is relative, but relative import paths are not supported in module mode$'
+
+go list -e -f '{{with .Error}}{{.}}{{end}}' -deps driver.go
+stdout '^driver.go:3:8: "./mypkg" is relative, but relative import paths are not supported in module mode$'
+! stderr .
+
+
+# Relative imports should not be allowed in module mode even without a go.mod file.
+rm go.mod
+
+! go run driver.go
+stderr '^driver.go:3:8: "./mypkg" is relative, but relative import paths are not supported in module mode$'
+
+go list -e -f '{{with .Error}}{{.}}{{end}}' -deps driver.go
+stdout '^driver.go:3:8: "./mypkg" is relative, but relative import paths are not supported in module mode$'
+! stderr .
+
+
+# In GOPATH mode, they're still allowed (but only outside of GOPATH/src).
+env GO111MODULE=off
+
+[!short] go run driver.go
+
+go list -deps driver.go
+
+
+-- $WORK/go.mod --
+module example
+
+go 1.17
+-- $WORK/driver.go --
+package main
+
+import "./mypkg"
+
+func main() {
+	mypkg.MyFunc()
+}
+-- $WORK/mypkg/code.go --
+package mypkg
+
+import "fmt"
+
+func MyFunc() {
+	fmt.Println("Hello, world!")
+}
diff --git a/src/cmd/go/testdata/script/test_relative_cmdline.txt b/src/cmd/go/testdata/script/test_relative_cmdline.txt
index 2f9c80f..96f7b87 100644
--- a/src/cmd/go/testdata/script/test_relative_cmdline.txt
+++ b/src/cmd/go/testdata/script/test_relative_cmdline.txt
@@ -1,5 +1,7 @@
 # Relative imports in command line package
 
+env GO111MODULE=off
+
 # Run tests outside GOPATH.
 env GOPATH=$WORK/tmp
 
@@ -47,4 +49,4 @@
 	if F() != p2.F() {
 		t.Fatal(F())
 	}
-}
\ No newline at end of file
+}

To view, visit change 386176. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I70e5818cfcfeea0ac70e17274427b08a74fd7c13

Gerrit-Change-Number: 386176

Gerrit-PatchSet: 3

Gerrit-Owner: Bryan Mills <bcm...@google.com>

Gerrit-Reviewer: Bryan Mills <bcm...@google.com>

Gerrit-Reviewer: Gopher Robot <go...@golang.org>

Gerrit-Reviewer: Michael Matloob <mat...@golang.org>

Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-MessageType: merged