From 42812a2feec29873aa6ee594d1355948e78e92a3 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 27 Apr 2021 12:54:39 -0700 Subject: [PATCH] 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 Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/check.go | 21 ++++++-- src/cmd/compile/internal/types2/errors.go | 27 +++++++++- .../compile/internal/types2/issues_test.go | 52 +++++++++++++++++-- src/cmd/compile/internal/types2/resolver.go | 6 ++- 4 files changed, 94 insertions(+), 12 deletions(-) diff --git a/src/cmd/compile/internal/types2/check.go b/src/cmd/compile/internal/types2/check.go index c8ca118c3c..8d6cd1edab 100644 --- a/src/cmd/compile/internal/types2/check.go +++ b/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 diff --git a/src/cmd/compile/internal/types2/errors.go b/src/cmd/compile/internal/types2/errors.go index d66528a8fd..af4ecb2300 100644 --- a/src/cmd/compile/internal/types2/errors.go +++ b/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...) } diff --git a/src/cmd/compile/internal/types2/issues_test.go b/src/cmd/compile/internal/types2/issues_test.go index 643d6789b5..e716a48038 100644 --- a/src/cmd/compile/internal/types2/issues_test.go +++ b/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) @@ -482,14 +482,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 @@ -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) + } +} diff --git a/src/cmd/compile/internal/types2/resolver.go b/src/cmd/compile/internal/types2/resolver.go index 86eeb72b21..fa30650bd4 100644 --- a/src/cmd/compile/internal/types2/resolver.go +++ b/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 } -- 2.48.1