types2: disambiguate package qualifiers in error messages

This is a port of the go/types CL https://golang.org/cl/313035
with minor adjustments (use of package syntax rather than go/ast).

Change-Id: I89410efb3d27be85fdbe827f966c2c91ee5693b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/314410
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

src/cmd/compile/internal/types2/check.go

@@ -87,7 +87,16 @@ type Checker struct {
 	impMap  map[importKey]*Package      // maps (import path, source directory) to (complete or fake) package
 	posMap  map[*Interface][]syntax.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;
@@ -181,7 +190,6 @@ func NewChecker(conf *Config, pkg *Package, info *Info) *Checker {
 		impMap:  make(map[importKey]*Package),
 		posMap:  make(map[*Interface][]syntax.Pos),
 		typMap:  make(map[string]*Named),
-		pkgCnt:  make(map[string]int),
 	}
 }
 
@@ -271,9 +279,6 @@ func (check *Checker) checkFiles(files []*syntax.File) (err error) {
 		print("== unusedImports ==")
 		check.unusedImports()
 	}
-	// no longer needed - release memory
-	check.imports = nil
-	check.dotImportMap = nil
 
 	print("== recordUntyped ==")
 	check.recordUntyped()
@@ -285,6 +290,12 @@ func (check *Checker) checkFiles(files []*syntax.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(gri) There's more memory we should release at this point.
 
 	return

src/cmd/compile/internal/types2/errors.go

@@ -108,8 +108,13 @@ func sprintf(qf Qualifier, format string, args ...interface{}) string {
 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
@@ -117,6 +122,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 {
 	return sprintf(check.qualifier, format, args...)
 }

src/cmd/compile/internal/types2/issues_test.go

@@ -474,7 +474,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.PkgName.Value, []*syntax.File{bast}, nil)
 	if err != nil {
 		t.Errorf("package %s failed to typecheck: %v", b.Name(), err)
@@ -483,13 +483,17 @@ func TestIssue34151(t *testing.T) {
 
 type importHelper struct {
 	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
@@ -513,7 +517,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.PkgName.Value, []*syntax.File{f}, nil)
 		if err != nil {
 			t.Errorf("%q failed to typecheck: %v", src, err)
@@ -568,3 +572,41 @@ func TestIssue44515(t *testing.T) {
 		t.Errorf("got %q; want %q", got, want)
 	}
 }
+
+func TestIssue43124(t *testing.T) {
+	// 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: defaultImporter()}}
+
+	// Packages should be fully qualified when there is ambiguity within the
+	// error string itself.
+	bast := mustParse(t, bsrc)
+	_, err = conf.Check(bast.PkgName.Value, []*syntax.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.PkgName.Value, []*syntax.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/cmd/compile/internal/types2/resolver.go

@@ -179,7 +179,11 @@ func (check *Checker) importPackage(pos syntax.Pos, 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
 	}