From 15105dd4b56610e18fa4b5744c7deca069085bc5 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 26 Apr 2021 19:23:05 -0400 Subject: [PATCH] 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 Trust: Robert Griesemer Run-TryBot: Robert Findley TryBot-Result: Go Bot Reviewed-by: Robert Griesemer --- src/go/types/check.go | 21 ++++++++++---- src/go/types/errors.go | 27 +++++++++++++++++- src/go/types/issues_test.go | 55 +++++++++++++++++++++++++++++++++---- src/go/types/resolver.go | 6 +++- 4 files changed, 97 insertions(+), 12 deletions(-) diff --git a/src/go/types/check.go b/src/go/types/check.go index 2c8f683ad5..4053fe2f4a 100644 --- a/src/go/types/check.go +++ b/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 diff --git a/src/go/types/errors.go b/src/go/types/errors.go index a956256762..19e9ae8d44 100644 --- a/src/go/types/errors.go +++ b/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) { diff --git a/src/go/types/issues_test.go b/src/go/types/issues_test.go index a773a362c7..44926919ef 100644 --- a/src/go/types/issues_test.go +++ b/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) + } +} diff --git a/src/go/types/resolver.go b/src/go/types/resolver.go index 43d2c739a5..f67fc65cd1 100644 --- a/src/go/types/resolver.go +++ b/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 } -- 2.50.0