cmd/go/internal/load: pass the importer's package path when checking visibility

A module like "gopkg.in/macaroon.v2" might have a test with a "_test" package
suffix (see https://golang.org/cmd/go/#hdr-Test_packages).
When we compile that test, its ImportStack entry includes the "_test" suffix
even though nothing else can actually import it via that path.
When we look up the module containing such a package, we must use the original
path, not the suffixed one.

On the other hand, an actual importable package may also be named with the
suffix "_test", so we need to be careful not to strip the suffix if it is
legitimately part of the path. We cannot distinguish that case by examining
srcDir or the ImportStack: the srcDir contaning a module doesn't necessarily
bear any relationship to its import path, and the ImportStack doesn't tell us
whether the suffix is part of the original path.

Fortunately, LoadImport usually has more information that we can use: it
receives a parent *Package that includes the original import path.

Fixes #26722

Change-Id: I1f7a4b37dbcb70e46af1caf9a496dfdd59ae8b17
Reviewed-on: https://go-review.googlesource.com/127796
Reviewed-by: Russ Cox <[email protected]>

Author: Bryan C. Mills

src/cmd/go/internal/load/pkg.go

@@ -319,7 +319,8 @@ func (p *PackageError) Error() string {
 }
 
 // An ImportStack is a stack of import paths, possibly with the suffix " (test)" appended.
-// TODO(bcmills): When the tree opens for 1.12, replace the suffixed string with a struct.
+// The import path of a test package is the import path of the corresponding
+// non-test package with the suffix "_test" added.
 type ImportStack []string
 
 func (s *ImportStack) Push(p string) {
@@ -468,6 +469,11 @@ func LoadImport(path, srcDir string, parent *Package, stk *ImportStack, importPo
 		}
 	}
 
+	parentPath := ""
+	if parent != nil {
+		parentPath = parent.ImportPath
+	}
+
 	// Determine canonical identifier for this package.
 	// For a local import the identifier is the pseudo-import path
 	// we create from the full directory to the package.
@@ -481,10 +487,6 @@ func LoadImport(path, srcDir string, parent *Package, stk *ImportStack, importPo
 	if isLocal {
 		importPath = dirToImportPath(filepath.Join(srcDir, path))
 	} else if cfg.ModulesEnabled {
-		parentPath := ""
-		if parent != nil {
-			parentPath = parent.ImportPath
-		}
 		var p string
 		modDir, p, modErr = ModLookup(parentPath, path)
 		if modErr == nil {
@@ -557,11 +559,11 @@ func LoadImport(path, srcDir string, parent *Package, stk *ImportStack, importPo
 	}
 
 	// Checked on every import because the rules depend on the code doing the importing.
-	if perr := disallowInternal(srcDir, p, stk); perr != p {
+	if perr := disallowInternal(srcDir, parentPath, p, stk); perr != p {
 		return setErrorPos(perr, importPos)
 	}
 	if mode&ResolveImport != 0 {
-		if perr := disallowVendor(srcDir, origPath, p, stk); perr != p {
+		if perr := disallowVendor(srcDir, parentPath, origPath, p, stk); perr != p {
 			return setErrorPos(perr, importPos)
 		}
 	}
@@ -922,10 +924,11 @@ func reusePackage(p *Package, stk *ImportStack) *Package {
 	return p
 }
 
-// disallowInternal checks that srcDir is allowed to import p.
+// disallowInternal checks that srcDir (containing package importerPath, if non-empty)
+// is allowed to import p.
 // If the import is allowed, disallowInternal returns the original package p.
 // If not, it returns a new package containing just an appropriate error.
-func disallowInternal(srcDir string, p *Package, stk *ImportStack) *Package {
+func disallowInternal(srcDir, importerPath string, p *Package, stk *ImportStack) *Package {
 	// golang.org/s/go14internal:
 	// An import of a path containing the element “internal”
 	// is disallowed if the importing code is outside the tree
@@ -969,7 +972,6 @@ func disallowInternal(srcDir string, p *Package, stk *ImportStack) *Package {
 		i-- // rewind over slash in ".../internal"
 	}
 
-	var where string
 	if p.Module == nil {
 		parent := p.Dir[:i+len(p.Dir)-len(p.ImportPath)]
 
@@ -987,18 +989,16 @@ func disallowInternal(srcDir string, p *Package, stk *ImportStack) *Package {
 		// p is in a module, so make it available based on the import path instead
 		// of the file path (https://golang.org/issue/23970).
 		parent := p.ImportPath[:i]
-		importer := strings.TrimSuffix((*stk)[len(*stk)-2], " (test)")
-		if str.HasPathPrefix(importer, parent) {
+		if str.HasPathPrefix(importerPath, parent) {
 			return p
 		}
-		where = " in " + importer
 	}
 
 	// Internal is present, and srcDir is outside parent's tree. Not allowed.
 	perr := *p
 	perr.Error = &PackageError{
 		ImportStack: stk.Copy(),
-		Err:         "use of internal package " + p.ImportPath + " not allowed" + where,
+		Err:         "use of internal package " + p.ImportPath + " not allowed",
 	}
 	perr.Incomplete = true
 	return &perr
@@ -1023,10 +1023,11 @@ func findInternal(path string) (index int, ok bool) {
 	return 0, false
 }
 
-// disallowVendor checks that srcDir is allowed to import p as path.
+// disallowVendor checks that srcDir (containing package importerPath, if non-empty)
+// is allowed to import p as path.
 // If the import is allowed, disallowVendor returns the original package p.
 // If not, it returns a new package containing just an appropriate error.
-func disallowVendor(srcDir, path string, p *Package, stk *ImportStack) *Package {
+func disallowVendor(srcDir, importerPath, path string, p *Package, stk *ImportStack) *Package {
 	// The stack includes p.ImportPath.
 	// If that's the only thing on the stack, we started
 	// with a name given on the command line, not an
@@ -1035,13 +1036,12 @@ func disallowVendor(srcDir, path string, p *Package, stk *ImportStack) *Package
 		return p
 	}
 
-	if p.Standard && ModPackageModuleInfo != nil {
+	if p.Standard && ModPackageModuleInfo != nil && importerPath != "" {
 		// Modules must not import vendor packages in the standard library,
 		// but the usual vendor visibility check will not catch them
 		// because the module loader presents them with an ImportPath starting
 		// with "golang_org/" instead of "vendor/".
-		importer := strings.TrimSuffix((*stk)[len(*stk)-2], " (test)")
-		if mod := ModPackageModuleInfo(importer); mod != nil {
+		if mod := ModPackageModuleInfo(importerPath); mod != nil {
 			dir := p.Dir
 			if relDir, err := filepath.Rel(p.Root, p.Dir); err == nil {
 				dir = relDir

src/cmd/go/testdata/script/mod_internal.txt

@@ -5,17 +5,22 @@ rm go.mod
 go mod init golang.org/x/anything
 go build .
 
+# ...and their tests...
+go test
+stdout PASS
+
 # ...but that should not leak into other modules.
 ! go build ./baddep
-stderr 'use of internal package golang.org/x/.* not allowed in golang.org/notx/useinternal$'
+stderr golang.org[/\\]notx[/\\]useinternal
+stderr 'use of internal package golang.org/x/.* not allowed'
 
 # Internal packages in the standard library should not leak into modules.
 ! go build ./fromstd
-stderr 'use of internal package internal/testenv not allowed$'
+stderr 'use of internal package internal/testenv not allowed'
 
 # Packages found via standard-library vendoring should not leak.
 ! go build ./fromstdvendor
-stderr 'use of vendored package golang_org/x/net/http/httpguts not allowed$'
+stderr 'use of vendored package golang_org/x/net/http/httpguts not allowed'
 
 
 # Dependencies should be able to use their own internal modules...
@@ -25,12 +30,12 @@ go build ./throughdep
 
 # ... but other modules should not, even if they have transitive dependencies.
 ! go build .
-stderr 'use of internal package golang.org/x/.* not allowed in golang.org/notx$'
+stderr 'use of internal package golang.org/x/.* not allowed'
 
 # And transitive dependencies still should not leak.
 ! go build ./baddep
-stderr 'use of internal package golang.org/x/.* not allowed in golang.org/notx/useinternal$'
-
+stderr golang.org[/\\]notx[/\\]useinternal
+stderr 'use of internal package golang.org/x/.* not allowed'
 
 # Replacing an internal module should keep it internal to the same paths.
 rm go.mod
@@ -39,18 +44,29 @@ go mod edit -replace golang.org/x/internal=./replace/golang.org/notx/internal
 go build ./throughdep
 
 ! go build ./baddep
-stderr 'use of internal package golang.org/x/.* not allowed in golang.org/notx/useinternal$'
+stderr golang.org[/\\]notx[/\\]useinternal
+stderr 'use of internal package golang.org/x/.* not allowed'
 
 go mod edit -replace golang.org/x/internal=./vendor/golang.org/x/internal
 go build ./throughdep
 
 ! go build ./baddep
-stderr 'use of internal package golang.org/x/.* not allowed in golang.org/notx/useinternal$'
+stderr golang.org[/\\]notx[/\\]useinternal
+stderr 'use of internal package golang.org/x/.* not allowed'
 
 -- useinternal.go --
 package useinternal
 import _ "golang.org/x/internal/subtle"
 
+-- useinternal_test.go --
+package useinternal_test
+import (
+	"testing"
+	_ "golang.org/x/internal/subtle"
+)
+
+func Test(*testing.T) {}
+
 -- throughdep/useinternal.go --
 package throughdep
 import _ "golang.org/x/useinternal"

src/cmd/go/testdata/script/mod_test.txt

@@ -1,11 +1,27 @@
 env GO111MODULE=on
 
+# A test in the module's root package should work.
 cd a/
 go test
 stdout PASS
 
+# A test with the "_test" suffix in the module root should also work.
+cd ../b/
+go test
+stdout PASS
+
+# A test with the "_test" suffix of a *package* with a "_test" suffix should
+# even work (not that you should ever do that).
+cd ../c_test
+go test
+stdout PASS
+
+cd ../d_test
+go test
+stdout PASS
+
 -- a/go.mod --
-module github.com/user/a
+module example.com/user/a
 
 -- a/a.go --
 package a
@@ -16,3 +32,59 @@ package a
 import "testing"
 
 func Test(t *testing.T) {}
+
+-- b/go.mod --
+module example.com/user/b
+
+-- b/b.go --
+package b
+
+-- b/b_test.go --
+package b_test
+
+import "testing"
+
+func Test(t *testing.T) {}
+
+-- c_test/go.mod --
+module example.com/c_test
+
+-- c_test/umm.go --
+// Package c_test is the non-test package for its import path!
+package c_test
+
+-- c_test/c_test_test.go --
+package c_test_test
+
+import "testing"
+
+func Test(t *testing.T) {}
+
+-- d_test/go.mod --
+// Package d is an ordinary package in a deceptively-named directory.
+module example.com/d
+
+-- d_test/d.go --
+package d
+
+-- d_test/d_test.go --
+package d_test
+
+import "testing"
+
+func Test(t *testing.T) {}
+
+-- e/go.mod --
+module example.com/e_test
+
+-- e/wat.go --
+// Package e_test is the non-test package for its import path,
+// in a deceptively-named directory!
+package e_test
+
+-- e/e_test.go --
+package e_test_test
+
+import "testing"
+
+func Test(t *testing.T) {}