go/types: walk all imports when determining package name ambiguity

CL 209578 disambiguated paths among imported packages, but as
demonstrated in #43119, formatted types may reference packages that are
not directly imported.

Fix this by recursively walking all imports to determine whether there
is any ambiguity in the import graph. This might result in
over-qualification of names, but it is straightforward and should
eliminate any ambiguity.

In general this should be fine, but might introduce risk of infinite
recursion in the case of an importer bug, or performance problems for
very large import graphs. Mitigate the former by tracking seen packages,
and the latter by only walking the import graph once an error has been
produced.

Fixes #43119

Change-Id: If874f050ad0e808db8e354c2ffc88bc6d64fd277
Reviewed-on: https://go-review.googlesource.com/c/go/+/313035
Trust: Robert Findley <rfindley@google.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

src/go/types/check.go

@@ -91,7 +91,16 @@ type Checker struct {
 	impMap  map[importKey]*Package     // maps (import path, source directory) to (complete or fake) package
 	posMap  map[*Interface][]token.Pos // maps interface types to lists of embedded interface positions
 	typMap  map[string]*Named          // maps an instantiated named type hash to a *Named type
-	pkgCnt  map[string]int             // counts number of imported packages with a given name (for better error messages)
+
+	// pkgPathMap maps package names to the set of distinct import paths we've
+	// seen for that name, anywhere in the import graph. It is used for
+	// disambiguating package names in error messages.
+	//
+	// pkgPathMap is allocated lazily, so that we don't pay the price of building
+	// it on the happy path. seenPkgMap tracks the packages that we've already
+	// walked.
+	pkgPathMap map[string]map[string]bool
+	seenPkgMap map[*Package]bool
 
 	// information collected during type-checking of a set of package files
 	// (initialized by Files, valid only for the duration of check.Files;
@@ -187,7 +196,6 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch
 		impMap:  make(map[importKey]*Package),
 		posMap:  make(map[*Interface][]token.Pos),
 		typMap:  make(map[string]*Named),
-		pkgCnt:  make(map[string]int),
 	}
 }
 
@@ -265,9 +273,6 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
 	if !check.conf.DisableUnusedImportCheck {
 		check.unusedImports()
 	}
-	// no longer needed - release memory
-	check.imports = nil
-	check.dotImportMap = nil
 
 	check.recordUntyped()
 
@@ -277,6 +282,12 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
 
 	check.pkg.complete = true
 
+	// no longer needed - release memory
+	check.imports = nil
+	check.dotImportMap = nil
+	check.pkgPathMap = nil
+	check.seenPkgMap = nil
+
 	// TODO(rFindley) There's more memory we should release at this point.
 
 	return

src/go/types/errors.go

@@ -28,8 +28,13 @@ func unreachable() {
 func (check *Checker) qualifier(pkg *Package) string {
 	// Qualify the package unless it's the package being type-checked.
 	if pkg != check.pkg {
+		if check.pkgPathMap == nil {
+			check.pkgPathMap = make(map[string]map[string]bool)
+			check.seenPkgMap = make(map[*Package]bool)
+			check.markImports(pkg)
+		}
 		// If the same package name was used by multiple packages, display the full path.
-		if check.pkgCnt[pkg.name] > 1 {
+		if len(check.pkgPathMap[pkg.name]) > 1 {
 			return strconv.Quote(pkg.path)
 		}
 		return pkg.name
@@ -37,6 +42,26 @@ func (check *Checker) qualifier(pkg *Package) string {
 	return ""
 }
 
+// markImports recursively walks pkg and its imports, to record unique import
+// paths in pkgPathMap.
+func (check *Checker) markImports(pkg *Package) {
+	if check.seenPkgMap[pkg] {
+		return
+	}
+	check.seenPkgMap[pkg] = true
+
+	forName, ok := check.pkgPathMap[pkg.name]
+	if !ok {
+		forName = make(map[string]bool)
+		check.pkgPathMap[pkg.name] = forName
+	}
+	forName[pkg.path] = true
+
+	for _, imp := range pkg.imports {
+		check.markImports(imp)
+	}
+}
+
 func (check *Checker) sprintf(format string, args ...interface{}) string {
 	for i, arg := range args {
 		switch a := arg.(type) {

src/go/types/issues_test.go

@@ -477,7 +477,7 @@ func TestIssue34151(t *testing.T) {
 	}
 
 	bast := mustParse(t, bsrc)
-	conf := Config{Importer: importHelper{a}}
+	conf := Config{Importer: importHelper{pkg: a}}
 	b, err := conf.Check(bast.Name.Name, fset, []*ast.File{bast}, nil)
 	if err != nil {
 		t.Errorf("package %s failed to typecheck: %v", b.Name(), err)
@@ -485,14 +485,18 @@ func TestIssue34151(t *testing.T) {
 }
 
 type importHelper struct {
-	pkg *Package
+	pkg      *Package
+	fallback Importer
 }
 
 func (h importHelper) Import(path string) (*Package, error) {
-	if path != h.pkg.Path() {
+	if path == h.pkg.Path() {
+		return h.pkg, nil
+	}
+	if h.fallback == nil {
 		return nil, fmt.Errorf("got package path %q; want %q", path, h.pkg.Path())
 	}
-	return h.pkg, nil
+	return h.fallback.Import(path)
 }
 
 // TestIssue34921 verifies that we don't update an imported type's underlying
@@ -516,7 +520,7 @@ func TestIssue34921(t *testing.T) {
 	var pkg *Package
 	for _, src := range sources {
 		f := mustParse(t, src)
-		conf := Config{Importer: importHelper{pkg}}
+		conf := Config{Importer: importHelper{pkg: pkg}}
 		res, err := conf.Check(f.Name.Name, fset, []*ast.File{f}, nil)
 		if err != nil {
 			t.Errorf("%q failed to typecheck: %v", src, err)
@@ -571,3 +575,44 @@ func TestIssue44515(t *testing.T) {
 		t.Errorf("got %q; want %q", got, want)
 	}
 }
+
+func TestIssue43124(t *testing.T) {
+	// TODO(rFindley) enhance the testdata tests to be able to express this type
+	//                of setup.
+
+	// All involved packages have the same name (template). Error messages should
+	// disambiguate between text/template and html/template by printing the full
+	// path.
+	const (
+		asrc = `package a; import "text/template"; func F(template.Template) {}; func G(int) {}`
+		bsrc = `package b; import ("a"; "html/template"); func _() { a.F(template.Template{}) }`
+		csrc = `package c; import ("a"; "html/template"); func _() { a.G(template.Template{}) }`
+	)
+
+	a, err := pkgFor("a", asrc, nil)
+	if err != nil {
+		t.Fatalf("package a failed to typecheck: %v", err)
+	}
+	conf := Config{Importer: importHelper{pkg: a, fallback: importer.Default()}}
+
+	// Packages should be fully qualified when there is ambiguity within the
+	// error string itself.
+	bast := mustParse(t, bsrc)
+	_, err = conf.Check(bast.Name.Name, fset, []*ast.File{bast}, nil)
+	if err == nil {
+		t.Fatal("package b had no errors")
+	}
+	if !strings.Contains(err.Error(), "text/template") || !strings.Contains(err.Error(), "html/template") {
+		t.Errorf("type checking error for b does not disambiguate package template: %q", err)
+	}
+
+	// ...and also when there is any ambiguity in reachable packages.
+	cast := mustParse(t, csrc)
+	_, err = conf.Check(cast.Name.Name, fset, []*ast.File{cast}, nil)
+	if err == nil {
+		t.Fatal("package c had no errors")
+	}
+	if !strings.Contains(err.Error(), "html/template") {
+		t.Errorf("type checking error for c does not disambiguate package template: %q", err)
+	}
+}

src/go/types/resolver.go

@@ -192,7 +192,11 @@ func (check *Checker) importPackage(at positioner, path, dir string) *Package {
 	// package should be complete or marked fake, but be cautious
 	if imp.complete || imp.fake {
 		check.impMap[key] = imp
-		check.pkgCnt[imp.name]++
+		// Once we've formatted an error message once, keep the pkgPathMap
+		// up-to-date on subsequent imports.
+		if check.pkgPathMap != nil {
+			check.markImports(imp)
+		}
 		return imp
 	}