go/packages: add missing test variants to fallback loader

For all packages p, q where there's an dependency path of
p.test -> ... -> q -> ... -> p, Go creates test variants
q [p.test] of each q and replaces the dependency on q with
a dependency on q [p.test], and replaces p with the expanded
test variant p[p.test]. Fix the fallback logic to add
these missing test variants. (Before this change, it was
only producing the variant of p: p [p.test].)

Fixes golang/go#27670

Change-Id: Ic56ba35fadcdf8c5928ec76f5a7b0ebe650c9f02
Reviewed-on: https://go-review.googlesource.com/136176
Reviewed-by: Ian Cottrell <iancottrell@google.com>

go/packages/golist_fallback.go

@@ -33,14 +33,20 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 	}
 
 	var tmpdir string // used for generated cgo files
+	var needsTestVariant []struct {
+		pkg, xtestPkg *Package
+	}
 
 	var response driverResponse
+	allPkgs := make(map[string]bool)
 	addPackage := func(p *jsonPackage) {
-		if p.Name == "" {
+		id := p.ImportPath
+
+		if p.Name == "" || allPkgs[id] {
 			return
 		}
+		allPkgs[id] = true
 
-		id := p.ImportPath
 		isRoot := original[id] != nil
 		pkgpath := id
 
@@ -110,7 +116,7 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 		if isRoot {
 			response.Roots = append(response.Roots, id)
 		}
-		response.Packages = append(response.Packages, &Package{
+		pkg := &Package{
 			ID:              id,
 			Name:            p.Name,
 			GoFiles:         absJoin(p.Dir, p.GoFiles, p.CgoFiles),
@@ -119,13 +125,12 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 			PkgPath:         pkgpath,
 			Imports:         importMap(p.Imports),
 			// TODO(matloob): set errors on the Package to cgoErrors
-		})
+		}
-		if cfg.Tests {
+		response.Packages = append(response.Packages, pkg)
+		if cfg.Tests && isRoot {
 			testID := fmt.Sprintf("%s [%s.test]", id, id)
 			if len(p.TestGoFiles) > 0 || len(p.XTestGoFiles) > 0 {
-				if isRoot {
+				response.Roots = append(response.Roots, testID)
-					response.Roots = append(response.Roots, testID)
-				}
 				testPkg := &Package{
 					ID:              testID,
 					Name:            p.Name,
@@ -140,32 +145,28 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 				var xtestPkg *Package
 				if len(p.XTestGoFiles) > 0 {
 					xtestID := fmt.Sprintf("%s_test [%s.test]", id, id)
-					if isRoot {
+					response.Roots = append(response.Roots, xtestID)
-						response.Roots = append(response.Roots, xtestID)
+					// Generate test variants for all packages q where a path exists
-					}
+					// such that xtestPkg -> ... -> q -> ... -> p (where p is the package under test)
-					// Rewrite import to package under test to refer to test variant.
+					// and rewrite all import map entries of p to point to testPkg (the test variant of
-					imports := importMap(p.XTestImports)
+					// p), and of each q  to point to the test variant of that q.
-					for imp := range imports {
-						if imp == p.ImportPath {
-							imports[imp] = &Package{ID: testID}
-							break
-						}
-					}
 					xtestPkg = &Package{
 						ID:              xtestID,
 						Name:            p.Name + "_test",
 						GoFiles:         absJoin(p.Dir, p.XTestGoFiles),
 						CompiledGoFiles: absJoin(p.Dir, p.XTestGoFiles),
 						PkgPath:         pkgpath + "_test",
-						Imports:         imports,
+						Imports:         importMap(p.XTestImports),
 					}
+					// Add to list of packages we need to rewrite imports for to refer to test variants.
+					// We may need to create a test variant of a package that hasn't been loaded yet, so
+					// the test variants need to be created later.
+					needsTestVariant = append(needsTestVariant, struct{ pkg, xtestPkg *Package }{pkg, xtestPkg})
 					response.Packages = append(response.Packages, xtestPkg)
 				}
 				// testmain package
 				testmainID := id + ".test"
-				if isRoot {
+				response.Roots = append(response.Roots, testmainID)
-					response.Roots = append(response.Roots, testmainID)
-				}
 				imports := map[string]*Package{}
 				imports[testPkg.PkgPath] = &Package{ID: testPkg.ID}
 				if xtestPkg != nil {
@@ -185,13 +186,13 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 					return
 				}
 				testmain := filepath.Join(outdir, "testmain.go")
-				extradeps, err := generateTestmain(testmain, testPkg, xtestPkg)
+				extraimports, extradeps, err := generateTestmain(testmain, testPkg, xtestPkg)
 				if err != nil {
 					testmainPkg.Errors = append(testmainPkg.Errors,
 						Error{"-", fmt.Sprintf("failed to generate testmain: %v", err)})
 				}
 				deps = append(deps, extradeps...)
-				for _, imp := range extradeps { // testing, testing/internal/testdeps, and maybe os
+				for _, imp := range extraimports { // testing, testing/internal/testdeps, and maybe os
 					imports[imp] = &Package{ID: imp}
 				}
 				testmainPkg.GoFiles = []string{testmain}
@@ -222,6 +223,10 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 		addPackage(p)
 	}
 
+	for _, v := range needsTestVariant {
+		createTestVariants(&response, v.pkg, v.xtestPkg)
+	}
+
 	// TODO(matloob): Is this the right ordering?
 	sort.SliceStable(response.Packages, func(i, j int) bool {
 		return response.Packages[i].PkgPath < response.Packages[j].PkgPath
@@ -230,6 +235,54 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error)
 	return &response, nil
 }
 
+func createTestVariants(response *driverResponse, pkgUnderTest, xtestPkg *Package) {
+	allPkgs := make(map[string]*Package)
+	for _, pkg := range response.Packages {
+		allPkgs[pkg.ID] = pkg
+	}
+	needsTestVariant := make(map[string]bool)
+	needsTestVariant[pkgUnderTest.ID] = true
+	var needsVariantRec func(p *Package) bool
+	needsVariantRec = func(p *Package) bool {
+		if needsTestVariant[p.ID] {
+			return true
+		}
+		for _, imp := range p.Imports {
+			if needsVariantRec(allPkgs[imp.ID]) {
+				// Don't break because we want to make sure all dependencies
+				// have been processed, and all required test variants of our dependencies
+				// exist.
+				needsTestVariant[p.ID] = true
+			}
+		}
+		if !needsTestVariant[p.ID] {
+			return false
+		}
+		// Create a clone of the package. It will share the same strings and lists of source files,
+		// but that's okay. It's only necessary for the Imports map to have a separate identity.
+		testVariant := *p
+		testVariant.ID = fmt.Sprintf("%s [%s.test]", p.ID, pkgUnderTest.ID)
+		testVariant.Imports = make(map[string]*Package)
+		for imp, pkg := range p.Imports {
+			testVariant.Imports[imp] = pkg
+			if needsTestVariant[pkg.ID] {
+				testVariant.Imports[imp] = &Package{ID: fmt.Sprintf("%s [%s.test]", pkg.ID, pkgUnderTest.ID)}
+			}
+		}
+		response.Packages = append(response.Packages, &testVariant)
+		return needsTestVariant[p.ID]
+	}
+	// finally, update the xtest package's imports
+	for imp, pkg := range xtestPkg.Imports {
+		if allPkgs[pkg.ID] == nil {
+			fmt.Printf("for %s: package %s doesn't exist\n", xtestPkg.ID, pkg.ID)
+		}
+		if needsVariantRec(allPkgs[pkg.ID]) {
+			xtestPkg.Imports[imp] = &Package{ID: fmt.Sprintf("%s [%s.test]", pkg.ID, pkgUnderTest.ID)}
+		}
+	}
+}
+
 // vendorlessPath returns the devendorized version of the import path ipath.
 // For example, VendorlessPath("foo/bar/vendor/a/b") returns "a/b".
 // Copied from golang.org/x/tools/imports/fix.go.

go/packages/golist_fallback_testmain.go

@@ -27,16 +27,66 @@ import (
 // This file complements golist_fallback.go by providing
 // support for generating testmains.
 
-func generateTestmain(out string, testPkg, xtestPkg *Package) (extradeps []string, err error) {
+func generateTestmain(out string, testPkg, xtestPkg *Package) (extraimports, extradeps []string, err error) {
 	testFuncs, err := loadTestFuncs(testPkg, xtestPkg)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	extradeps = []string{"testing/internal/testdeps", "testing"}
+	extraimports = []string{"testing", "testing/internal/testdeps"}
 	if testFuncs.TestMain == nil {
-		extradeps = append(extradeps, "os")
+		extraimports = append(extraimports, "os")
 	}
-	return extradeps, writeTestmain(out, testFuncs)
+	// Transitive dependencies of ("testing", "testing/internal/testdeps").
+	// os is part of the transitive closure so it and its transitive dependencies are
+	// included regardless of whether it's imported in the template below.
+	extradeps = []string{
+		"errors",
+		"internal/cpu",
+		"unsafe",
+		"internal/bytealg",
+		"internal/race",
+		"runtime/internal/atomic",
+		"runtime/internal/sys",
+		"runtime",
+		"sync/atomic",
+		"sync",
+		"io",
+		"unicode",
+		"unicode/utf8",
+		"bytes",
+		"math",
+		"syscall",
+		"time",
+		"internal/poll",
+		"internal/syscall/unix",
+		"internal/testlog",
+		"os",
+		"math/bits",
+		"strconv",
+		"reflect",
+		"fmt",
+		"sort",
+		"strings",
+		"flag",
+		"runtime/debug",
+		"context",
+		"runtime/trace",
+		"testing",
+		"bufio",
+		"regexp/syntax",
+		"regexp",
+		"compress/flate",
+		"encoding/binary",
+		"hash",
+		"hash/crc32",
+		"compress/gzip",
+		"path/filepath",
+		"io/ioutil",
+		"text/tabwriter",
+		"runtime/pprof",
+		"testing/internal/testdeps",
+	}
+	return extraimports, extradeps, writeTestmain(out, testFuncs)
 }
 
 // The following is adapted from the cmd/go testmain generation code.

go/packages/packages_test.go

@@ -240,10 +240,6 @@ func TestLoadImportsGraph(t *testing.T) {
 }
 
 func TestLoadImportsTestVariants(t *testing.T) {
-	if usesOldGolist {
-		t.Skip("not yet supported in pre-Go 1.10.4 golist fallback implementation")
-	}
-
 	tmp, cleanup := makeTree(t, map[string]string{
 		"src/a/a.go":       `package a; import _ "b"`,
 		"src/b/b.go":       `package b`,