go/importer: use named receiver types for methods of named interfaces

griesemer · griesemer · commit ea012d100bad · 2018-02-12T21:43:04.000Z
R=go1.11 Once approved, this change must be ported to golang.org/x/tools/go/gcimporter15. Fixes #13829. Change-Id: I26a0094d2bfd38b97f2b64bae84b9f428fc9cdf1 Reviewed-on: https://go-review.googlesource.com/85318 Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/src/go/internal/gcimporter/bimport.go b/src/go/internal/gcimporter/bimport.go
@@ -257,24 +257,24 @@ func (p *importer) obj(tag int) {
 	case constTag:
 		pos := p.pos()
 		pkg, name := p.qualifiedName()
-		typ := p.typ(nil)
+		typ := p.typ(nil, nil)
 		val := p.value()
 		p.declare(types.NewConst(pos, pkg, name, typ, val))
 
 	case aliasTag:
 		// TODO(gri) verify type alias hookup is correct
 		pos := p.pos()
 		pkg, name := p.qualifiedName()
-		typ := p.typ(nil)
+		typ := p.typ(nil, nil)
 		p.declare(types.NewTypeName(pos, pkg, name, typ))
 
 	case typeTag:
-		p.typ(nil)
+		p.typ(nil, nil)
 
 	case varTag:
 		pos := p.pos()
 		pkg, name := p.qualifiedName()
-		typ := p.typ(nil)
+		typ := p.typ(nil, nil)
 		p.declare(types.NewVar(pos, pkg, name, typ))
 
 	case funcTag:
@@ -379,7 +379,11 @@ func (t *dddSlice) String() string         { return "..." + t.elem.String() }
 // the package currently imported. The parent package is needed for
 // exported struct fields and interface methods which don't contain
 // explicit package information in the export data.
-func (p *importer) typ(parent *types.Package) types.Type {
+//
+// A non-nil tname is used as the "owner" of the result type; i.e.,
+// the result type is the underlying type of tname. tname is used
+// to give interface methods a named receiver type where possible.
+func (p *importer) typ(parent *types.Package, tname *types.Named) types.Type {
 	// if the type was seen before, i is its index (>= 0)
 	i := p.tagOrIndex()
 	if i >= 0 {
@@ -409,15 +413,15 @@ func (p *importer) typ(parent *types.Package) types.Type {
 		t0 := types.NewNamed(obj.(*types.TypeName), nil, nil)
 
 		// but record the existing type, if any
-		t := obj.Type().(*types.Named)
-		p.record(t)
+		tname := obj.Type().(*types.Named) // tname is either t0 or the existing type
+		p.record(tname)
 
 		// read underlying type
-		t0.SetUnderlying(p.typ(parent))
+		t0.SetUnderlying(p.typ(parent, t0))
 
 		// interfaces don't have associated methods
 		if types.IsInterface(t0) {
-			return t
+			return tname
 		}
 
 		// read associated methods
@@ -438,7 +442,7 @@ func (p *importer) typ(parent *types.Package) types.Type {
 			t0.AddMethod(types.NewFunc(pos, parent, name, sig))
 		}
 
-		return t
+		return tname
 
 	case arrayTag:
 		t := new(types.Array)
@@ -447,7 +451,7 @@ func (p *importer) typ(parent *types.Package) types.Type {
 		}
 
 		n := p.int64()
-		*t = *types.NewArray(p.typ(parent), n)
+		*t = *types.NewArray(p.typ(parent, nil), n)
 		return t
 
 	case sliceTag:
@@ -456,7 +460,7 @@ func (p *importer) typ(parent *types.Package) types.Type {
 			p.record(t)
 		}
 
-		*t = *types.NewSlice(p.typ(parent))
+		*t = *types.NewSlice(p.typ(parent, nil))
 		return t
 
 	case dddTag:
@@ -465,7 +469,7 @@ func (p *importer) typ(parent *types.Package) types.Type {
 			p.record(t)
 		}
 
-		t.elem = p.typ(parent)
+		t.elem = p.typ(parent, nil)
 		return t
 
 	case structTag:
@@ -483,7 +487,7 @@ func (p *importer) typ(parent *types.Package) types.Type {
 			p.record(t)
 		}
 
-		*t = *types.NewPointer(p.typ(parent))
+		*t = *types.NewPointer(p.typ(parent, nil))
 		return t
 
 	case signatureTag:
@@ -502,6 +506,8 @@ func (p *importer) typ(parent *types.Package) types.Type {
 		// cannot expect the interface type to appear in a cycle, as any
 		// such cycle must contain a named type which would have been
 		// first defined earlier.
+		// TODO(gri) Is this still true now that we have type aliases?
+		// See issue #23225.
 		n := len(p.typList)
 		if p.trackAllTypes {
 			p.record(nil)
@@ -510,10 +516,10 @@ func (p *importer) typ(parent *types.Package) types.Type {
 		var embeddeds []*types.Named
 		for n := p.int(); n > 0; n-- {
 			p.pos()
-			embeddeds = append(embeddeds, p.typ(parent).(*types.Named))
+			embeddeds = append(embeddeds, p.typ(parent, nil).(*types.Named))
 		}
 
-		t := types.NewInterface(p.methodList(parent), embeddeds)
+		t := types.NewInterface(p.methodList(parent, tname), embeddeds)
 		p.interfaceList = append(p.interfaceList, t)
 		if p.trackAllTypes {
 			p.typList[n] = t
@@ -526,8 +532,8 @@ func (p *importer) typ(parent *types.Package) types.Type {
 			p.record(t)
 		}
 
-		key := p.typ(parent)
-		val := p.typ(parent)
+		key := p.typ(parent, nil)
+		val := p.typ(parent, nil)
 		*t = *types.NewMap(key, val)
 		return t
 
@@ -549,7 +555,7 @@ func (p *importer) typ(parent *types.Package) types.Type {
 		default:
 			errorf("unexpected channel dir %d", d)
 		}
-		val := p.typ(parent)
+		val := p.typ(parent, nil)
 		*t = *types.NewChan(dir, val)
 		return t
 
@@ -573,7 +579,7 @@ func (p *importer) fieldList(parent *types.Package) (fields []*types.Var, tags [
 func (p *importer) field(parent *types.Package) (*types.Var, string) {
 	pos := p.pos()
 	pkg, name, alias := p.fieldName(parent)
-	typ := p.typ(parent)
+	typ := p.typ(parent, nil)
 	tag := p.string()
 
 	anonymous := false
@@ -597,22 +603,30 @@ func (p *importer) field(parent *types.Package) (*types.Var, string) {
 	return types.NewField(pos, pkg, name, typ, anonymous), tag
 }
 
-func (p *importer) methodList(parent *types.Package) (methods []*types.Func) {
+func (p *importer) methodList(parent *types.Package, baseType *types.Named) (methods []*types.Func) {
 	if n := p.int(); n > 0 {
 		methods = make([]*types.Func, n)
 		for i := range methods {
-			methods[i] = p.method(parent)
+			methods[i] = p.method(parent, baseType)
 		}
 	}
 	return
 }
 
-func (p *importer) method(parent *types.Package) *types.Func {
+func (p *importer) method(parent *types.Package, baseType *types.Named) *types.Func {
 	pos := p.pos()
 	pkg, name, _ := p.fieldName(parent)
+	// If we don't have a baseType, use a nil receiver.
+	// A receiver using the actual interface type (which
+	// we don't know yet) will be filled in when we call
+	// types.Interface.Complete.
+	var recv *types.Var
+	if baseType != nil {
+		recv = types.NewVar(token.NoPos, parent, "", baseType)
+	}
 	params, isddd := p.paramList()
 	result, _ := p.paramList()
-	sig := types.NewSignature(nil, params, result, isddd)
+	sig := types.NewSignature(recv, params, result, isddd)
 	return types.NewFunc(pos, pkg, name, sig)
 }
 
@@ -668,7 +682,7 @@ func (p *importer) paramList() (*types.Tuple, bool) {
 }
 
 func (p *importer) param(named bool) (*types.Var, bool) {
-	t := p.typ(nil)
+	t := p.typ(nil, nil)
 	td, isddd := t.(*dddSlice)
 	if isddd {
 		t = types.NewSlice(td.elem)
diff --git a/src/go/internal/gcimporter/gcimporter_test.go b/src/go/internal/gcimporter/gcimporter_test.go
@@ -200,11 +200,22 @@ var importedObjectTests = []struct {
 	name string
 	want string
 }{
+	// non-interfaces
+	{"crypto.Hash", "type Hash uint"},
+	{"go/ast.ObjKind", "type ObjKind int"},
+	{"go/types.Qualifier", "type Qualifier func(*Package) string"},
+	{"go/types.Comparable", "func Comparable(T Type) bool"},
 	{"math.Pi", "const Pi untyped float"},
+	{"math.Sin", "func Sin(x float64) float64"},
+
+	// interfaces
+	{"context.Context", "type Context interface{Deadline() (deadline time.Time, ok bool); Done() <-chan struct{}; Err() error; Value(key interface{}) interface{}}"},
+	{"crypto.Decrypter", "type Decrypter interface{Decrypt(rand io.Reader, msg []byte, opts DecrypterOpts) (plaintext []byte, err error); Public() PublicKey}"},
+	{"encoding.BinaryMarshaler", "type BinaryMarshaler interface{MarshalBinary() (data []byte, err error)}"},
 	{"io.Reader", "type Reader interface{Read(p []byte) (n int, err error)}"},
 	{"io.ReadWriter", "type ReadWriter interface{Reader; Writer}"},
-	{"math.Sin", "func Sin(x float64) float64"},
-	// TODO(gri) add more tests
+	{"go/ast.Node", "type Node interface{End() go/token.Pos; Pos() go/token.Pos}"},
+	{"go/types.Type", "type Type interface{String() string; Underlying() Type}"},
 }
 
 func TestImportedTypes(t *testing.T) {
@@ -239,6 +250,44 @@ func TestImportedTypes(t *testing.T) {
 		if got != test.want {
 			t.Errorf("%s: got %q; want %q", test.name, got, test.want)
 		}
+
+		if named, _ := obj.Type().(*types.Named); named != nil {
+			verifyInterfaceMethodRecvs(t, named, 0)
+		}
+	}
+}
+
+// verifyInterfaceMethodRecvs verifies that method receiver types
+// are named if the methods belong to a named interface type.
+func verifyInterfaceMethodRecvs(t *testing.T, named *types.Named, level int) {
+	// avoid endless recursion in case of an embedding bug that lead to a cycle
+	if level > 10 {
+		t.Errorf("%s: embeds itself", named)
+		return
+	}
+
+	iface, _ := named.Underlying().(*types.Interface)
+	if iface == nil {
+		return // not an interface
+	}
+
+	// check explicitly declared methods
+	for i := 0; i < iface.NumExplicitMethods(); i++ {
+		m := iface.ExplicitMethod(i)
+		recv := m.Type().(*types.Signature).Recv()
+		if recv == nil {
+			t.Errorf("%s: missing receiver type", m)
+			continue
+		}
+		if recv.Type() != named {
+			t.Errorf("%s: got recv type %s; want %s", m, recv.Type(), named)
+		}
+	}
+
+	// check embedded interfaces (they are named, too)
+	for i := 0; i < iface.NumEmbeddeds(); i++ {
+		// embedding of interfaces cannot have cycles; recursion will terminate
+		verifyInterfaceMethodRecvs(t, iface.Embedded(i), level+1)
 	}
 }
 
diff --git a/src/go/internal/srcimporter/srcimporter_test.go b/src/go/internal/srcimporter/srcimporter_test.go
@@ -130,6 +130,44 @@ func TestImportedTypes(t *testing.T) {
 		if got != test.want {
 			t.Errorf("%s: got %q; want %q", test.name, got, test.want)
 		}
+
+		if named, _ := obj.Type().(*types.Named); named != nil {
+			verifyInterfaceMethodRecvs(t, named, 0)
+		}
+	}
+}
+
+// verifyInterfaceMethodRecvs verifies that method receiver types
+// are named if the methods belong to a named interface type.
+func verifyInterfaceMethodRecvs(t *testing.T, named *types.Named, level int) {
+	// avoid endless recursion in case of an embedding bug that lead to a cycle
+	if level > 10 {
+		t.Errorf("%s: embeds itself", named)
+		return
+	}
+
+	iface, _ := named.Underlying().(*types.Interface)
+	if iface == nil {
+		return // not an interface
+	}
+
+	// check explicitly declared methods
+	for i := 0; i < iface.NumExplicitMethods(); i++ {
+		m := iface.ExplicitMethod(i)
+		recv := m.Type().(*types.Signature).Recv()
+		if recv == nil {
+			t.Errorf("%s: missing receiver type", m)
+			continue
+		}
+		if recv.Type() != named {
+			t.Errorf("%s: got recv type %s; want %s", m, recv.Type(), named)
+		}
+	}
+
+	// check embedded interfaces (they are named, too)
+	for i := 0; i < iface.NumEmbeddeds(); i++ {
+		// embedding of interfaces cannot have cycles; recursion will terminate
+		verifyInterfaceMethodRecvs(t, iface.Embedded(i), level+1)
 	}
 }
 
diff --git a/src/go/types/type.go b/src/go/types/type.go
@@ -254,7 +254,8 @@ var emptyInterface = Interface{allMethods: markComplete}
 var markComplete = make([]*Func, 0)
 
 // NewInterface returns a new (incomplete) interface for the given methods and embedded types.
-// To compute the method set of the interface, Complete must be called.
+// NewInterface takes ownership of the provided methods and may modify their types by setting
+// missing receivers. To compute the method set of the interface, Complete must be called.
 func NewInterface(methods []*Func, embeddeds []*Named) *Interface {
 	typ := new(Interface)
 
@@ -267,10 +268,10 @@ func NewInterface(methods []*Func, embeddeds []*Named) *Interface {
 		if mset.insert(m) != nil {
 			panic("multiple methods with the same name")
 		}
-		// set receiver
-		// TODO(gri) Ideally, we should use a named type here instead of
-		// typ, for less verbose printing of interface method signatures.
-		m.typ.(*Signature).recv = NewVar(m.pos, m.pkg, "", typ)
+		// set receiver if we don't have one
+		if sig := m.typ.(*Signature); sig.recv == nil {
+			sig.recv = NewVar(m.pos, m.pkg, "", typ)
+		}
 	}
 	sort.Sort(byUniqueMethodName(methods))
 
