go/importer: associate exported field and interface methods with correct package

In gc export data, exported struct field and interface method names appear
in unqualified form (i.e., w/o package name). The (gc)importer assumed that
unqualified exported names automatically belong to the package being imported.
This is not the case if the field or method belongs to a struct or interface
that was declared in another package and re-exported.

The issue becomes visible if a type T (say an interface with a method M)
is declared in a package A, indirectly re-exported by a package B (which
imports A), and then imported in C. If C imports both A and B, if A is
imported before B, T.M gets associated with the correct package A. If B
is imported before A, T.M appears to be exported by B (even though T itself
is correctly marked as coming from A). If T.M is imported again via the
import of A if gets dropped (as it should) because it was imported already.

The fix is to pass down the parent package when we parse imported types
so that the importer can use the correct package when creating fields
and methods.

Fixes #13898.

Change-Id: I7ec2ee2dda15859c582b65db221c3841899776e1
Reviewed-on: https://go-review.googlesource.com/18549
Reviewed-by: Alan Donovan <adonovan@google.com>

diff --git a/src/go/internal/gcimporter/gcimporter.go b/src/go/internal/gcimporter/gcimporter.go
index 46006c5c2012abb3530e5513287437e7d6b83af1..2365d84931cd86fcd6bc70df61f3b596b15cb839 100644 (file)
--- a/src/go/internal/gcimporter/gcimporter.go
+++ b/src/go/internal/gcimporter/gcimporter.go
@@ -413,11 +413,11 @@ func (p *parser) parseBasicType() types.Type {
 
 // ArrayType = "[" int_lit "]" Type .
 //
-func (p *parser) parseArrayType() types.Type {
+func (p *parser) parseArrayType(parent *types.Package) types.Type {
        // "[" already consumed and lookahead known not to be "]"
        lit := p.expect(scanner.Int)
        p.expect(']')
-       elem := p.parseType()
+       elem := p.parseType(parent)
        n, err := strconv.ParseInt(lit, 10, 64)
        if err != nil {
                p.error(err)
@@ -427,35 +427,43 @@ func (p *parser) parseArrayType() types.Type {
 
 // MapType = "map" "[" Type "]" Type .
 //
-func (p *parser) parseMapType() types.Type {
+func (p *parser) parseMapType(parent *types.Package) types.Type {
        p.expectKeyword("map")
        p.expect('[')
-       key := p.parseType()
+       key := p.parseType(parent)
        p.expect(']')
-       elem := p.parseType()
+       elem := p.parseType(parent)
        return types.NewMap(key, elem)
 }
 
 // Name = identifier | "?" | QualifiedName .
 //
-// For unqualified names, the returned package is the imported package.
+// For unqualified and anonymous names, the returned package is the parent
+// package unless parent == nil, in which case the returned package is the
+// package being imported. (The parent package is not nil if the the name
+// is an unqualified struct field or interface method name belonging to a
+// type declared in another package.)
+//
 // For qualified names, the returned package is nil (and not created if
 // it doesn't exist yet) unless materializePkg is set (which creates an
-// unnamed package). In the latter case, a subequent import clause is
-// expected to provide a name for the package.
+// unnamed package with valid package path). In the latter case, a
+// subequent import clause is expected to provide a name for the package.
 //
-func (p *parser) parseName(materializePkg bool) (pkg *types.Package, name string) {
+func (p *parser) parseName(parent *types.Package, materializePkg bool) (pkg *types.Package, name string) {
+       pkg = parent
+       if pkg == nil {
+               pkg = p.sharedPkgs[p.id]
+       }
        switch p.tok {
        case scanner.Ident:
-               pkg = p.sharedPkgs[p.id]
                name = p.lit
                p.next()
        case '?':
                // anonymous
-               pkg = p.sharedPkgs[p.id]
                p.next()
        case '@':
                // exported name prefixed with package path
+               pkg = nil
                var id string
                id, name = p.parseQualifiedName()
                if materializePkg {
@@ -476,9 +484,9 @@ func deref(typ types.Type) types.Type {
 
 // Field = Name Type [ string_lit ] .
 //
-func (p *parser) parseField() (*types.Var, string) {
-       pkg, name := p.parseName(true)
-       typ := p.parseType()
+func (p *parser) parseField(parent *types.Package) (*types.Var, string) {
+       pkg, name := p.parseName(parent, true)
+       typ := p.parseType(parent)
        anonymous := false
        if name == "" {
                // anonymous field - typ must be T or *T and T must be a type name
@@ -487,7 +495,9 @@ func (p *parser) parseField() (*types.Var, string) {
                        pkg = nil
                        name = typ.Name()
                case *types.Named:
-                       name = typ.Obj().Name()
+                       obj := typ.Obj()
+                       pkg = obj.Pkg()
+                       name = obj.Name()
                default:
                        p.errorf("anonymous field expected")
                }
@@ -508,7 +518,7 @@ func (p *parser) parseField() (*types.Var, string) {
 // StructType = "struct" "{" [ FieldList ] "}" .
 // FieldList  = Field { ";" Field } .
 //
-func (p *parser) parseStructType() types.Type {
+func (p *parser) parseStructType(parent *types.Package) types.Type {
        var fields []*types.Var
        var tags []string
 
@@ -518,7 +528,7 @@ func (p *parser) parseStructType() types.Type {
                if i > 0 {
                        p.expect(';')
                }
-               fld, tag := p.parseField()
+               fld, tag := p.parseField(parent)
                if tag != "" && tags == nil {
                        tags = make([]string, i)
                }
@@ -535,7 +545,7 @@ func (p *parser) parseStructType() types.Type {
 // Parameter = ( identifier | "?" ) [ "..." ] Type [ string_lit ] .
 //
 func (p *parser) parseParameter() (par *types.Var, isVariadic bool) {
-       _, name := p.parseName(false)
+       _, name := p.parseName(nil, false)
        // remove gc-specific parameter numbering
        if i := strings.Index(name, "·"); i >= 0 {
                name = name[:i]
@@ -544,7 +554,7 @@ func (p *parser) parseParameter() (par *types.Var, isVariadic bool) {
                p.expectSpecial("...")
                isVariadic = true
        }
-       typ := p.parseType()
+       typ := p.parseType(nil)
        if isVariadic {
                typ = types.NewSlice(typ)
        }
@@ -607,7 +617,7 @@ func (p *parser) parseSignature(recv *types.Var) *types.Signature {
 // by the compiler and thus embedded interfaces are never
 // visible in the export data.
 //
-func (p *parser) parseInterfaceType() types.Type {
+func (p *parser) parseInterfaceType(parent *types.Package) types.Type {
        var methods []*types.Func
 
        p.expectKeyword("interface")
@@ -616,7 +626,7 @@ func (p *parser) parseInterfaceType() types.Type {
                if i > 0 {
                        p.expect(';')
                }
-               pkg, name := p.parseName(true)
+               pkg, name := p.parseName(parent, true)
                sig := p.parseSignature(nil)
                methods = append(methods, types.NewFunc(token.NoPos, pkg, name, sig))
        }
@@ -629,7 +639,7 @@ func (p *parser) parseInterfaceType() types.Type {
 
 // ChanType = ( "chan" [ "<-" ] | "<-" "chan" ) Type .
 //
-func (p *parser) parseChanType() types.Type {
+func (p *parser) parseChanType(parent *types.Package) types.Type {
        dir := types.SendRecv
        if p.tok == scanner.Ident {
                p.expectKeyword("chan")
@@ -642,7 +652,7 @@ func (p *parser) parseChanType() types.Type {
                p.expectKeyword("chan")
                dir = types.RecvOnly
        }
-       elem := p.parseType()
+       elem := p.parseType(parent)
        return types.NewChan(dir, elem)
 }
 
@@ -657,24 +667,24 @@ func (p *parser) parseChanType() types.Type {
 // PointerType = "*" Type .
 // FuncType    = "func" Signature .
 //
-func (p *parser) parseType() types.Type {
+func (p *parser) parseType(parent *types.Package) types.Type {
        switch p.tok {
        case scanner.Ident:
                switch p.lit {
                default:
                        return p.parseBasicType()
                case "struct":
-                       return p.parseStructType()
+                       return p.parseStructType(parent)
                case "func":
                        // FuncType
                        p.next()
                        return p.parseSignature(nil)
                case "interface":
-                       return p.parseInterfaceType()
+                       return p.parseInterfaceType(parent)
                case "map":
-                       return p.parseMapType()
+                       return p.parseMapType(parent)
                case "chan":
-                       return p.parseChanType()
+                       return p.parseChanType(parent)
                }
        case '@':
                // TypeName
@@ -685,19 +695,19 @@ func (p *parser) parseType() types.Type {
                if p.tok == ']' {
                        // SliceType
                        p.next()
-                       return types.NewSlice(p.parseType())
+                       return types.NewSlice(p.parseType(parent))
                }
-               return p.parseArrayType()
+               return p.parseArrayType(parent)
        case '*':
                // PointerType
                p.next()
-               return types.NewPointer(p.parseType())
+               return types.NewPointer(p.parseType(parent))
        case '<':
-               return p.parseChanType()
+               return p.parseChanType(parent)
        case '(':
                // "(" Type ")"
                p.next()
-               typ := p.parseType()
+               typ := p.parseType(parent)
                p.expect(')')
                return typ
        }
@@ -779,7 +789,8 @@ func (p *parser) parseConstDecl() {
 
        var typ0 types.Type
        if p.tok != '=' {
-               typ0 = p.parseType()
+               // constant types are never structured - no need for parent type
+               typ0 = p.parseType(nil)
        }
 
        p.expect('=')
@@ -853,7 +864,7 @@ func (p *parser) parseTypeDecl() {
        // structure, but throw it away if the object already has a type.
        // This ensures that all imports refer to the same type object for
        // a given type declaration.
-       typ := p.parseType()
+       typ := p.parseType(pkg)
 
        if name := obj.Type().(*types.Named); name.Underlying() == nil {
                name.SetUnderlying(typ)
@@ -865,7 +876,7 @@ func (p *parser) parseTypeDecl() {
 func (p *parser) parseVarDecl() {
        p.expectKeyword("var")
        pkg, name := p.parseExportedName()
-       typ := p.parseType()
+       typ := p.parseType(pkg)
        pkg.Scope().Insert(types.NewVar(token.NoPos, pkg, name, typ))
 }
 
@@ -901,7 +912,7 @@ func (p *parser) parseMethodDecl() {
        base := deref(recv.Type()).(*types.Named)
 
        // parse method name, signature, and possibly inlined body
-       _, name := p.parseName(false)
+       _, name := p.parseName(nil, false)
        sig := p.parseFunc(recv)
 
        // methods always belong to the same package as the base type object

diff --git a/src/go/internal/gcimporter/gcimporter_test.go b/src/go/internal/gcimporter/gcimporter_test.go
index 926242db05351fadcb5bdddb41f27e0e34d008d2..e56720b0d525435215a4dcd9e66f915d5d024d49 100644 (file)
--- a/src/go/internal/gcimporter/gcimporter_test.go
+++ b/src/go/internal/gcimporter/gcimporter_test.go
@@ -311,3 +311,53 @@ func TestIssue13566(t *testing.T) {
                }
        }
 }
+
+func TestIssue13898(t *testing.T) {
+       skipSpecialPlatforms(t)
+
+       // This package only handles gc export data.
+       if runtime.Compiler != "gc" {
+               t.Skipf("gc-built packages not available (compiler = %s)", runtime.Compiler)
+               return
+       }
+
+       // import go/internal/gcimporter which imports go/types partially
+       imports := make(map[string]*types.Package)
+       _, err := Import(imports, "go/internal/gcimporter", ".")
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       // look for go/types package
+       var goTypesPkg *types.Package
+       for path, pkg := range imports {
+               if path == "go/types" {
+                       goTypesPkg = pkg
+                       break
+               }
+       }
+       if goTypesPkg == nil {
+               t.Fatal("go/types not found")
+       }
+
+       // look for go/types.Object type
+       obj := goTypesPkg.Scope().Lookup("Object")
+       if obj == nil {
+               t.Fatal("go/types.Object not found")
+       }
+       typ, ok := obj.Type().(*types.Named)
+       if !ok {
+               t.Fatalf("go/types.Object type is %v; wanted named type", typ)
+       }
+
+       // lookup go/types.Object.Pkg method
+       m, _, _ := types.LookupFieldOrMethod(typ, false, nil, "Pkg")
+       if m == nil {
+               t.Fatal("go/types.Object.Pkg not found")
+       }
+
+       // the method must belong to go/types
+       if m.Pkg().Path() != "go/types" {
+               t.Fatalf("found %v; want go/types", m.Pkg())
+       }
+}

diff --git a/src/go/types/issues_test.go b/src/go/types/issues_test.go
index 672c78dfc200331ea90a56eee052008e9bcca778..388473511856f9d979142b88ebe477dee9626483 100644 (file)
--- a/src/go/types/issues_test.go
+++ b/src/go/types/issues_test.go
@@ -11,6 +11,7 @@ import (
        "go/ast"
        "go/importer"
        "go/parser"
+       "internal/testenv"
        "sort"
        "strings"
        "testing"
@@ -204,3 +205,90 @@ L7 uses var z int`
                t.Errorf("Unexpected defs/uses\ngot:\n%s\nwant:\n%s", got, want)
        }
 }
+
+// This tests that the package associated with the types.Object.Pkg method
+// is the type's package independent of the order in which the imports are
+// listed in the sources src1, src2 below.
+// The actual issue is in go/internal/gcimporter which has a corresponding
+// test; we leave this test here to verify correct behavior at the go/types
+// level.
+func TestIssue13898(t *testing.T) {
+       testenv.MustHaveGoBuild(t)
+
+       const src0 = `
+package main
+
+import "go/types"
+
+func main() {
+       var info types.Info
+       for _, obj := range info.Uses {
+               _ = obj.Pkg()
+       }
+}
+`
+       // like src0, but also imports go/importer
+       const src1 = `
+package main
+
+import (
+       "go/types"
+       _ "go/importer"
+)
+
+func main() {
+       var info types.Info
+       for _, obj := range info.Uses {
+               _ = obj.Pkg()
+       }
+}
+`
+       // like src1 but with different import order
+       // (used to fail with this issue)
+       const src2 = `
+package main
+
+import (
+       _ "go/importer"
+       "go/types"
+)
+
+func main() {
+       var info types.Info
+       for _, obj := range info.Uses {
+               _ = obj.Pkg()
+       }
+}
+`
+       f := func(test, src string) {
+               f, err := parser.ParseFile(fset, "", src, 0)
+               if err != nil {
+                       t.Fatal(err)
+               }
+               cfg := Config{Importer: importer.Default()}
+               info := Info{Uses: make(map[*ast.Ident]Object)}
+               _, err = cfg.Check("main", fset, []*ast.File{f}, &info)
+               if err != nil {
+                       t.Fatal(err)
+               }
+
+               var pkg *Package
+               count := 0
+               for id, obj := range info.Uses {
+                       if id.Name == "Pkg" {
+                               pkg = obj.Pkg()
+                               count++
+                       }
+               }
+               if count != 1 {
+                       t.Fatalf("%s: got %d entries named Pkg; want 1", test, count)
+               }
+               if pkg.Name() != "types" {
+                       t.Fatalf("%s: got %v; want package types", test, pkg)
+               }
+       }
+
+       f("src0", src0)
+       f("src1", src1)
+       f("src2", src2)
+}