]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: apply import restrictions to test code too
authorRuss Cox <rsc@golang.org>
Fri, 21 Oct 2016 16:04:31 +0000 (12:04 -0400)
committerRuss Cox <rsc@golang.org>
Fri, 28 Oct 2016 20:32:29 +0000 (20:32 +0000)
We reject import of main packages, but we missed tests.
Reject in all tests except test of that main package.

We reject local (relative) imports from code with a
non-local import path, but again we missed tests.
Reject those too.

Fixes #14811.
Fixes #15795.
Fixes #17475.

Change-Id: I535ff26889520276a891904f54f1a85b2c40207d
Reviewed-on: https://go-review.googlesource.com/31821
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Quentin Smith <quentin@golang.org>
src/cmd/go/go_test.go
src/cmd/go/pkg.go
src/cmd/go/testdata/src/importmain/ismain/main.go [new file with mode: 0644]
src/cmd/go/testdata/src/importmain/test/test.go [new file with mode: 0644]
src/cmd/go/testdata/src/importmain/test/test_test.go [new file with mode: 0644]

index b02581be7baa58b5dfcd1ac024d8bfdadf5e8fb9..c9bd9be03cc23837db76a7f5b9f2da84594fc31a 100644 (file)
@@ -135,7 +135,15 @@ func testgo(t *testing.T) *testgoData {
                t.Skip("skipping external tests on %s/%s", runtime.GOOS, runtime.GOARCH)
        }
 
-       return &testgoData{t: t}
+       tg := &testgoData{t: t}
+
+       // Hide user's local .gitconfig from git invocations.
+       // In particular, people using Github 2FA may configure
+       // https://github.com/ to redirect to ssh://git@github.com/
+       // using an insteadOf configuration, and that will break various
+       // of our tests.
+       tg.setenv("HOME", "/test-go-home-does-not-exist")
+       return tg
 }
 
 // must gives a fatal error if err is not nil.
@@ -2063,6 +2071,16 @@ func TestCoverageUsesActualSettingToOverrideEvenForRace(t *testing.T) {
        checkCoverage(tg, data)
 }
 
+func TestCoverageImportMainLoop(t *testing.T) {
+       tg := testgo(t)
+       defer tg.cleanup()
+       tg.setenv("GOPATH", filepath.Join(tg.pwd(), "testdata"))
+       tg.runFail("test", "importmain/test")
+       tg.grepStderr("not an importable package", "did not detect import main")
+       tg.runFail("test", "-cover", "importmain/test")
+       tg.grepStderr("not an importable package", "did not detect import main")
+}
+
 func TestBuildDryRunWithCgo(t *testing.T) {
        if !canCgo {
                t.Skip("skipping because cgo not enabled")
@@ -2462,21 +2480,238 @@ func TestGoGetHTTPS404(t *testing.T) {
 }
 
 // Test that you cannot import a main package.
-func TestIssue4210(t *testing.T) {
+// See golang.org/issue/4210 and golang.org/issue/17475.
+func TestImportMain(t *testing.T) {
        tg := testgo(t)
        defer tg.cleanup()
+
+       // Importing package main from that package main's test should work.
        tg.tempFile("src/x/main.go", `package main
                var X int
                func main() {}`)
-       tg.tempFile("src/y/main.go", `package main
-               import "fmt"
+       tg.tempFile("src/x/main_test.go", `package main_test
                import xmain "x"
-               func main() {
-                       fmt.Println(xmain.X)
-               }`)
+               import "testing"
+               var _ = xmain.X
+               func TestFoo(t *testing.T) {}
+       `)
        tg.setenv("GOPATH", tg.path("."))
-       tg.runFail("build", "y")
-       tg.grepBoth("is a program", `did not find expected error message ("is a program")`)
+       tg.run("build", "x")
+       tg.run("test", "x")
+
+       // Importing package main from another package should fail.
+       tg.tempFile("src/p1/p.go", `package p1
+               import xmain "x"
+               var _ = xmain.X
+       `)
+       tg.runFail("build", "p1")
+       tg.grepStderr("import \"x\" is a program, not an importable package", "did not diagnose package main")
+
+       // ... even in that package's test.
+       tg.tempFile("src/p2/p.go", `package p2
+       `)
+       tg.tempFile("src/p2/p_test.go", `package p2
+               import xmain "x"
+               import "testing"
+               var _ = xmain.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "p2")
+       tg.runFail("test", "p2")
+       tg.grepStderr("import \"x\" is a program, not an importable package", "did not diagnose package main")
+
+       // ... even if that package's test is an xtest.
+       tg.tempFile("src/p3/p.go", `package p
+       `)
+       tg.tempFile("src/p3/p_test.go", `package p_test
+               import xmain "x"
+               import "testing"
+               var _ = xmain.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "p3")
+       tg.runFail("test", "p3")
+       tg.grepStderr("import \"x\" is a program, not an importable package", "did not diagnose package main")
+
+       // ... even if that package is a package main
+       tg.tempFile("src/p4/p.go", `package main
+       func main() {}
+       `)
+       tg.tempFile("src/p4/p_test.go", `package main
+               import xmain "x"
+               import "testing"
+               var _ = xmain.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "p4")
+       tg.runFail("test", "p4")
+       tg.grepStderr("import \"x\" is a program, not an importable package", "did not diagnose package main")
+
+       // ... even if that package is a package main using an xtest.
+       tg.tempFile("src/p5/p.go", `package main
+       func main() {}
+       `)
+       tg.tempFile("src/p5/p_test.go", `package main_test
+               import xmain "x"
+               import "testing"
+               var _ = xmain.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "p5")
+       tg.runFail("test", "p5")
+       tg.grepStderr("import \"x\" is a program, not an importable package", "did not diagnose package main")
+}
+
+// Test that you cannot use a local import in a package
+// accessed by a non-local import (found in a GOPATH/GOROOT).
+// See golang.org/issue/17475.
+func TestImportLocal(t *testing.T) {
+       tg := testgo(t)
+       defer tg.cleanup()
+
+       // Importing package main from that package main's test should work.
+       tg.tempFile("src/dir/x/x.go", `package x
+               var X int
+       `)
+       tg.setenv("GOPATH", tg.path("."))
+       tg.run("build", "dir/x")
+
+       // Ordinary import should work.
+       tg.tempFile("src/dir/p0/p.go", `package p0
+               import "dir/x"
+               var _ = x.X
+       `)
+       tg.run("build", "dir/p0")
+
+       // Relative import should not.
+       tg.tempFile("src/dir/p1/p.go", `package p1
+               import "../x"
+               var _ = x.X
+       `)
+       tg.runFail("build", "dir/p1")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in a test.
+       tg.tempFile("src/dir/p2/p.go", `package p2
+       `)
+       tg.tempFile("src/dir/p2/p_test.go", `package p2
+               import "../x"
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir/p2")
+       tg.runFail("test", "dir/p2")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in an xtest.
+       tg.tempFile("src/dir/p2/p_test.go", `package p2_test
+               import "../x"
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir/p2")
+       tg.runFail("test", "dir/p2")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // Relative import starting with ./ should not work either.
+       tg.tempFile("src/dir/d.go", `package dir
+               import "./x"
+               var _ = x.X
+       `)
+       tg.runFail("build", "dir")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in a test.
+       tg.tempFile("src/dir/d.go", `package dir
+       `)
+       tg.tempFile("src/dir/d_test.go", `package dir
+               import "./x"
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir")
+       tg.runFail("test", "dir")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in an xtest.
+       tg.tempFile("src/dir/d_test.go", `package dir_test
+               import "./x"
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir")
+       tg.runFail("test", "dir")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // Relative import plain ".." should not work.
+       tg.tempFile("src/dir/x/y/y.go", `package dir
+               import ".."
+               var _ = x.X
+       `)
+       tg.runFail("build", "dir/x/y")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in a test.
+       tg.tempFile("src/dir/x/y/y.go", `package y
+       `)
+       tg.tempFile("src/dir/x/y/y_test.go", `package y
+               import ".."
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir/x/y")
+       tg.runFail("test", "dir/x/y")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in an x test.
+       tg.tempFile("src/dir/x/y/y_test.go", `package y_test
+               import ".."
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir/x/y")
+       tg.runFail("test", "dir/x/y")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // Relative import "." should not work.
+       tg.tempFile("src/dir/x/xx.go", `package x
+               import "."
+               var _ = x.X
+       `)
+       tg.runFail("build", "dir/x")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in a test.
+       tg.tempFile("src/dir/x/xx.go", `package x
+       `)
+       tg.tempFile("src/dir/x/xx_test.go", `package x
+               import "."
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir/x")
+       tg.runFail("test", "dir/x")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
+
+       // ... even in an xtest.
+       tg.tempFile("src/dir/x/xx.go", `package x
+       `)
+       tg.tempFile("src/dir/x/xx_test.go", `package x_test
+               import "."
+               import "testing"
+               var _ = x.X
+               func TestFoo(t *testing.T) {}
+       `)
+       tg.run("build", "dir/x")
+       tg.runFail("test", "dir/x")
+       tg.grepStderr("local import.*in non-local package", "did not diagnose local import")
 }
 
 func TestGoGetInsecure(t *testing.T) {
index 22241f597f55aca9eda39bf57a4275550935e0fd..7505a43f2e23840c90f6d5869744d680c1f4dcb0 100644 (file)
@@ -341,50 +341,52 @@ func loadImport(path, srcDir string, parent *Package, stk *importStack, importPo
                importPath = path
        }
 
-       if p := packageCache[importPath]; p != nil {
-               if perr := disallowInternal(srcDir, p, stk); perr != p {
-                       return perr
+       p := packageCache[importPath]
+       if p != nil {
+               p = reusePackage(p, stk)
+       } else {
+               p = new(Package)
+               p.local = isLocal
+               p.ImportPath = importPath
+               packageCache[importPath] = p
+
+               // Load package.
+               // Import always returns bp != nil, even if an error occurs,
+               // in order to return partial information.
+               //
+               // TODO: After Go 1, decide when to pass build.AllowBinary here.
+               // See issue 3268 for mistakes to avoid.
+               buildMode := build.ImportComment
+               if mode&useVendor == 0 || path != origPath {
+                       // Not vendoring, or we already found the vendored path.
+                       buildMode |= build.IgnoreVendor
                }
-               if mode&useVendor != 0 {
-                       if perr := disallowVendor(srcDir, origPath, p, stk); perr != p {
-                               return perr
-                       }
+               bp, err := buildContext.Import(path, srcDir, buildMode)
+               bp.ImportPath = importPath
+               if gobin != "" {
+                       bp.BinDir = gobin
+               }
+               if err == nil && !isLocal && bp.ImportComment != "" && bp.ImportComment != path &&
+                       !strings.Contains(path, "/vendor/") && !strings.HasPrefix(path, "vendor/") {
+                       err = fmt.Errorf("code in directory %s expects import %q", bp.Dir, bp.ImportComment)
+               }
+               p.load(stk, bp, err)
+               if p.Error != nil && p.Error.Pos == "" && len(importPos) > 0 {
+                       pos := importPos[0]
+                       pos.Filename = shortPath(pos.Filename)
+                       p.Error.Pos = pos.String()
                }
-               return reusePackage(p, stk)
-       }
-
-       p := new(Package)
-       p.local = isLocal
-       p.ImportPath = importPath
-       packageCache[importPath] = p
 
-       // Load package.
-       // Import always returns bp != nil, even if an error occurs,
-       // in order to return partial information.
-       //
-       // TODO: After Go 1, decide when to pass build.AllowBinary here.
-       // See issue 3268 for mistakes to avoid.
-       buildMode := build.ImportComment
-       if mode&useVendor == 0 || path != origPath {
-               // Not vendoring, or we already found the vendored path.
-               buildMode |= build.IgnoreVendor
-       }
-       bp, err := buildContext.Import(path, srcDir, buildMode)
-       bp.ImportPath = importPath
-       if gobin != "" {
-               bp.BinDir = gobin
-       }
-       if err == nil && !isLocal && bp.ImportComment != "" && bp.ImportComment != path &&
-               !strings.Contains(path, "/vendor/") && !strings.HasPrefix(path, "vendor/") {
-               err = fmt.Errorf("code in directory %s expects import %q", bp.Dir, bp.ImportComment)
-       }
-       p.load(stk, bp, err)
-       if p.Error != nil && p.Error.Pos == "" && len(importPos) > 0 {
-               pos := importPos[0]
-               pos.Filename = shortPath(pos.Filename)
-               p.Error.Pos = pos.String()
+               if origPath != cleanImport(origPath) {
+                       p.Error = &PackageError{
+                               ImportStack: stk.copy(),
+                               Err:         fmt.Sprintf("non-canonical import path: %q should be %q", origPath, pathpkg.Clean(origPath)),
+                       }
+                       p.Incomplete = true
+               }
        }
 
+       // Checked on every import because the rules depend on the code doing the importing.
        if perr := disallowInternal(srcDir, p, stk); perr != p {
                return perr
        }
@@ -394,12 +396,32 @@ func loadImport(path, srcDir string, parent *Package, stk *importStack, importPo
                }
        }
 
-       if origPath != cleanImport(origPath) {
-               p.Error = &PackageError{
+       if p.Name == "main" && parent != nil && parent.Dir != p.Dir {
+               perr := *p
+               perr.Error = &PackageError{
                        ImportStack: stk.copy(),
-                       Err:         fmt.Sprintf("non-canonical import path: %q should be %q", origPath, pathpkg.Clean(origPath)),
+                       Err:         fmt.Sprintf("import %q is a program, not an importable package", path),
                }
-               p.Incomplete = true
+               if len(importPos) > 0 {
+                       pos := importPos[0]
+                       pos.Filename = shortPath(pos.Filename)
+                       perr.Error.Pos = pos.String()
+               }
+               return &perr
+       }
+
+       if p.local && parent != nil && !parent.local {
+               perr := *p
+               perr.Error = &PackageError{
+                       ImportStack: stk.copy(),
+                       Err:         fmt.Sprintf("local import %q in non-local package", path),
+               }
+               if len(importPos) > 0 {
+                       pos := importPos[0]
+                       pos.Filename = shortPath(pos.Filename)
+                       perr.Error.Pos = pos.String()
+               }
+               return &perr
        }
 
        return p
@@ -445,7 +467,7 @@ func vendoredImportPath(parent *Package, path string) (found string) {
                root = expandPath(root)
        }
 
-       if !hasFilePathPrefix(dir, root) || len(dir) <= len(root) || dir[len(root)] != filepath.Separator || parent.ImportPath != "command-line-arguments" && filepath.Join(root, parent.ImportPath) != dir {
+       if !hasFilePathPrefix(dir, root) || len(dir) <= len(root) || dir[len(root)] != filepath.Separator || parent.ImportPath != "command-line-arguments" && !parent.local && filepath.Join(root, parent.ImportPath) != dir {
                fatalf("unexpected directory layout:\n"+
                        "       import path: %s\n"+
                        "       root: %s\n"+
@@ -974,7 +996,8 @@ func (p *Package) load(stk *importStack, bp *build.Package, err error) *Package
                // The same import path could produce an error or not,
                // depending on what tries to import it.
                // Prefer to record entries with errors, so we can report them.
-               if deps[path] == nil || p1.Error != nil {
+               p0 := deps[path]
+               if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
                        deps[path] = p1
                }
        }
@@ -984,28 +1007,6 @@ func (p *Package) load(stk *importStack, bp *build.Package, err error) *Package
                        continue
                }
                p1 := loadImport(path, p.Dir, p, stk, p.build.ImportPos[path], useVendor)
-               if p1.Name == "main" {
-                       p.Error = &PackageError{
-                               ImportStack: stk.copy(),
-                               Err:         fmt.Sprintf("import %q is a program, not an importable package", path),
-                       }
-                       pos := p.build.ImportPos[path]
-                       if len(pos) > 0 {
-                               p.Error.Pos = pos[0].String()
-                       }
-               }
-               if p1.local {
-                       if !p.local && p.Error == nil {
-                               p.Error = &PackageError{
-                                       ImportStack: stk.copy(),
-                                       Err:         fmt.Sprintf("local import %q in non-local package", path),
-                               }
-                               pos := p.build.ImportPos[path]
-                               if len(pos) > 0 {
-                                       p.Error.Pos = pos[0].String()
-                               }
-                       }
-               }
                if p.Standard && p.Error == nil && !p1.Standard && p1.Error == nil {
                        p.Error = &PackageError{
                                ImportStack: stk.copy(),
diff --git a/src/cmd/go/testdata/src/importmain/ismain/main.go b/src/cmd/go/testdata/src/importmain/ismain/main.go
new file mode 100644 (file)
index 0000000..bf01907
--- /dev/null
@@ -0,0 +1,5 @@
+package main
+
+import _ "importmain/test"
+
+func main() {}
diff --git a/src/cmd/go/testdata/src/importmain/test/test.go b/src/cmd/go/testdata/src/importmain/test/test.go
new file mode 100644 (file)
index 0000000..56e5404
--- /dev/null
@@ -0,0 +1 @@
+package test
diff --git a/src/cmd/go/testdata/src/importmain/test/test_test.go b/src/cmd/go/testdata/src/importmain/test/test_test.go
new file mode 100644 (file)
index 0000000..2268a82
--- /dev/null
@@ -0,0 +1,6 @@
+package test_test
+
+import "testing"
+import _ "importmain/ismain"
+
+func TestCase(t *testing.T) {}