]> Cypherpunks.ru repositories - gostls13.git/commitdiff
types2: disambiguate package qualifiers in error messages
authorRobert Griesemer <gri@golang.org>
Tue, 27 Apr 2021 19:54:39 +0000 (12:54 -0700)
committerRobert Griesemer <gri@golang.org>
Wed, 28 Apr 2021 18:50:47 +0000 (18:50 +0000)
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
src/cmd/compile/internal/types2/errors.go
src/cmd/compile/internal/types2/issues_test.go
src/cmd/compile/internal/types2/resolver.go

index c8ca118c3ccbc96f252dc078df6a94f8ccb00ed1..8d6cd1edab9d4c61cd1f1d5d7a99798def7fdcde 100644 (file)
@@ -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
index d66528a8fd2edc67078d75fa7ad31ff375660500..af4ecb2300a58cbfe5e8eeae32220fe72129ba2d 100644 (file)
@@ -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...)
 }
index 643d6789b513eea1a2527a7064f47c90973b3032..e716a48038510a2320c02524c4375d0bdaa9ef6f 100644 (file)
@@ -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)
+       }
+}
index 86eeb72b21c0b62f8abc3f5f35900bff140cb0e2..fa30650bd444f3d995b1939e897783e6ef50b768 100644 (file)
@@ -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
        }