internal/gcimporter: updates how imports are handled in unified IR

The imports of the main package are now all of the packages that
were referenced during reading the unified IR. This is similar to
the non-bundled option for iimport.

Without this imports of some packages are not the transitive closure.

Fixes golang/go#58296

Change-Id: I4c58c99a5538a9f990ca7325baa1637e7141a97e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/465936
Run-TryBot: Tim King <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: timothy-king

internal/gcimporter/gcimporter_test.go

@@ -23,6 +23,7 @@ import (
 	"path"
 	"path/filepath"
 	"runtime"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -847,6 +848,60 @@ func TestIssue57729(t *testing.T) {
 	}
 }
 
+func TestIssue58296(t *testing.T) {
+	// Compiles packages c, b, and a where c imports b and b imports a,
+	// then imports c with stub *types.Packages for b and a, and checks that
+	// both a and b are in the Imports() of c.
+	//
+	// This is how go/packages can read the exportdata when NeedDeps is off.
+
+	// This package only handles gc export data.
+	needsCompiler(t, "gc")
+	testenv.NeedsGoBuild(t) // to find stdlib export data in the build cache
+
+	// On windows, we have to set the -D option for the compiler to avoid having a drive
+	// letter and an illegal ':' in the import path - just skip it (see also issue #3483).
+	if runtime.GOOS == "windows" {
+		t.Skip("avoid dealing with relative paths/drive letters on windows")
+	}
+
+	tmpdir := mktmpdir(t)
+	defer os.RemoveAll(tmpdir)
+	testoutdir := filepath.Join(tmpdir, "testdata")
+
+	apkg := filepath.Join(testoutdir, "a")
+	bpkg := filepath.Join(testoutdir, "b")
+	cpkg := filepath.Join(testoutdir, "c")
+
+	srcdir := filepath.Join("testdata", "issue58296")
+	compilePkg(t, filepath.Join(srcdir, "a"), "a.go", testoutdir, nil, apkg)
+	compilePkg(t, filepath.Join(srcdir, "b"), "b.go", testoutdir, map[string]string{apkg: filepath.Join(testoutdir, "a.o")}, bpkg)
+	compilePkg(t, filepath.Join(srcdir, "c"), "c.go", testoutdir, map[string]string{bpkg: filepath.Join(testoutdir, "b.o")}, cpkg)
+
+	// The export data reader for c cannot rely on Package.Imports
+	// being populated for a or b. (For the imports {a,b} it is unset.)
+	imports := map[string]*types.Package{
+		apkg: types.NewPackage(apkg, "a"),
+		bpkg: types.NewPackage(bpkg, "b"),
+	}
+
+	// make sure a and b are both imported by c.
+	pkg, err := Import(imports, "./c", testoutdir, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var names []string
+	for _, imp := range pkg.Imports() {
+		names = append(names, imp.Name())
+	}
+	sort.Strings(names)
+
+	if got, want := strings.Join(names, ","), "a,b"; got != want {
+		t.Errorf("got imports %v for package c. wanted %v", names, want)
+	}
+}
+
 // apkg returns the package "a" prefixed by (as a package) testoutdir
 func apkg(testoutdir string) string {
 	apkg := testoutdir + "/a"

internal/gcimporter/testdata/issue58296/a/a.go

@@ -0,0 +1,9 @@
+// Copyright 2023 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 a
+
+type A int
+
+func (A) f() {}

internal/gcimporter/testdata/issue58296/b/b.go

@@ -0,0 +1,11 @@
+// Copyright 2023 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 b
+
+import "./a"
+
+type B struct {
+	a a.A
+}

internal/gcimporter/testdata/issue58296/c/c.go

@@ -0,0 +1,11 @@
+// Copyright 2023 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 c
+
+import "./b"
+
+type C struct {
+	b b.B
+}

internal/gcimporter/ureader_yes.go

@@ -12,6 +12,7 @@ package gcimporter
 import (
 	"go/token"
 	"go/types"
+	"sort"
 	"strings"
 
 	"golang.org/x/tools/internal/pkgbits"
@@ -121,6 +122,16 @@ func readUnifiedPackage(fset *token.FileSet, ctxt *types.Context, imports map[st
 		iface.Complete()
 	}
 
+	// Imports() of pkg are all of the transitive packages that were loaded.
+	var imps []*types.Package
+	for _, imp := range pr.pkgs {
+		if imp != nil && imp != pkg {
+			imps = append(imps, imp)
+		}
+	}
+	sort.Sort(byPath(imps))
+	pkg.SetImports(imps)
+
 	pkg.MarkComplete()
 	return pkg
 }
@@ -260,39 +271,9 @@ func (r *reader) doPkg() *types.Package {
 	pkg := types.NewPackage(path, name)
 	r.p.imports[path] = pkg
 
-	imports := make([]*types.Package, r.Len())
-	for i := range imports {
-		imports[i] = r.pkg()
-	}
-	pkg.SetImports(flattenImports(imports))
-
 	return pkg
 }
 
-// flattenImports returns the transitive closure of all imported
-// packages rooted from pkgs.
-func flattenImports(pkgs []*types.Package) []*types.Package {
-	var res []*types.Package
-	seen := make(map[*types.Package]struct{})
-	for _, pkg := range pkgs {
-		if _, ok := seen[pkg]; ok {
-			continue
-		}
-		seen[pkg] = struct{}{}
-		res = append(res, pkg)
-
-		// pkg.Imports() is already flattened.
-		for _, pkg := range pkg.Imports() {
-			if _, ok := seen[pkg]; ok {
-				continue
-			}
-			seen[pkg] = struct{}{}
-			res = append(res, pkg)
-		}
-	}
-	return res
-}
-
 // @@@ Types
 
 func (r *reader) typ() types.Type {