go.tools/go: separate interface construction from method set construction

We introduce a method (*Interface).Complete(), which is intended
to be called from clients after all embedded interfaces have been
fully defined. For importers, this will definitely be the case
after the import has finished, so each importer have been updated
to do so, with the exception of the gcimporter, which does not use
embedded interfaces, therefore Complete() can be called immediately
after construction.

Building the method set separately from the constructor type caused
some problems with go/importer, which copies the types.Interface
object, leading to there existing two almost-identical interface
types referenced from interface method receivers, only one of which
has been completed. To avoid this situation, the importer has been
modified to construct the interface object only once.

Fixes golang/go#8177.

LGTM=gri
R=gri, dave, gordon.klaus, adonovan
CC=golang-codereviews
https://golang.org/cl/105060044

go/gccgoimporter/parser.go

@@ -839,6 +839,11 @@ func (p *parser) parsePackage() *types.Package {
 	for p.tok != scanner.EOF {
 		p.parseDirective()
 	}
+	for _, typ := range p.typeMap {
+		if it, ok := typ.(*types.Interface); ok {
+			it.Complete()
+		}
+	}
 	p.pkg.MarkComplete()
 	return p.pkg
 }

go/gcimporter/gcimporter.go

@@ -598,7 +598,9 @@ func (p *parser) parseInterfaceType() types.Type {
 	}
 	p.expect('}')
 
-	return types.NewInterface(methods, nil)
+	// Complete requires the type's embedded interfaces to be fully defined,
+	// but we do not define any
+	return types.NewInterface(methods, nil).Complete()
 }
 
 // ChanType = ( "chan" [ "<-" ] | "<-" "chan" ) Type .

go/importer/export.go

@@ -328,13 +328,14 @@ func (p *exporter) qualifiedName(pkg *types.Package, name string) {
 }
 
 func (p *exporter) signature(sig *types.Signature) {
-	// TODO(gri) We only need to record the receiver type
+	// We need the receiver information (T vs *T)
-	//           for interface methods if we flatten them
-	//           out. If we track embedded types instead,
-	//           the information is already present.
-	// We do need the receiver information (T vs *T)
 	// for methods associated with named types.
+	// We do not record interface receiver types in the
+	// export data because 1) the importer can derive them
+	// from the interface type and 2) they create cycles
+	// in the type graph.
 	if recv := sig.Recv(); recv != nil {
+		if _, ok := recv.Type().Underlying().(*types.Interface); !ok {
 			// 1-element tuple
 			p.int(1)
 			p.param(recv)
@@ -342,6 +343,10 @@ func (p *exporter) signature(sig *types.Signature) {
 			// 0-element tuple
 			p.int(0)
 		}
+	} else {
+		// 0-element tuple
+		p.int(0)
+	}
 	p.tuple(sig.Params())
 	p.tuple(sig.Results())
 	if sig.Variadic() {

go/importer/import.go

@@ -72,6 +72,13 @@ func ImportData(imports map[string]*types.Package, data []byte) (int, *types.Pac
 		p.obj(pkg)
 	}
 
+	// complete interfaces
+	for _, typ := range p.typList {
+		if it, ok := typ.(*types.Interface); ok {
+			it.Complete()
+		}
+	}
+
 	// package was imported completely and without errors
 	pkg.MarkComplete()
 
@@ -254,8 +261,12 @@ func (p *importer) typ() types.Type {
 		return t
 
 	case interfaceTag:
-		t := new(types.Interface)
+		// Create a dummy entry in the type list. This is safe because we
-		p.record(t)
+		// 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.
+		n := len(p.typList)
+		p.record(nil)
 
 		// read embedded interfaces
 		embeddeds := make([]*types.Named, p.int())
@@ -270,7 +281,8 @@ func (p *importer) typ() types.Type {
 			methods[i] = types.NewFunc(token.NoPos, pkg, name, p.typ().(*types.Signature))
 		}
 
-		*t = *types.NewInterface(methods, embeddeds)
+		t := types.NewInterface(methods, embeddeds)
+		p.typList[n] = t
 		return t
 
 	case mapTag:

go/importer/import_test.go

@@ -61,6 +61,8 @@ var tests = []string{
 	`package p; type T chan int`,
 	`package p; type T <-chan complex64`,
 	`package p; type T chan<- map[int]string`,
+	// test case for issue 8177
+	`package p; type T1 interface { F(T2) }; type T2 interface { T1 }`,
 
 	// vars
 	`package p; var X int`,

go/pointer/gen.go

@@ -20,7 +20,7 @@ import (
 )
 
 var (
-	tEface     = types.NewInterface(nil, nil)
+	tEface     = types.NewInterface(nil, nil).Complete()
 	tInvalid   = types.Typ[types.Invalid]
 	tUnsafePtr = types.Typ[types.UnsafePointer]
 )

go/types/type.go

@@ -263,29 +263,12 @@ func NewInterface(methods []*Func, embeddeds []*Named) *Interface {
 	}
 	sort.Sort(byUniqueMethodName(methods))
 
-	var allMethods []*Func
 	if embeddeds == nil {
-		allMethods = methods
-	} else {
-		allMethods = append(allMethods, methods...)
-		for _, t := range embeddeds {
-			it := t.Underlying().(*Interface)
-			for _, tm := range it.allMethods {
-				// Make a copy of the method and adjust its receiver type.
-				newm := *tm
-				newmtyp := *tm.typ.(*Signature)
-				newm.typ = &newmtyp
-				newmtyp.recv = NewVar(newm.pos, newm.pkg, "", typ)
-				allMethods = append(allMethods, &newm)
-			}
-		}
 		sort.Sort(byUniqueTypeName(embeddeds))
-		sort.Sort(byUniqueMethodName(allMethods))
 	}
 
 	typ.methods = methods
 	typ.embeddeds = embeddeds
-	typ.allMethods = allMethods
 	return typ
 }
 
@@ -313,6 +296,43 @@ func (t *Interface) Method(i int) *Func { return t.allMethods[i] }
 // Empty returns true if t is the empty interface.
 func (t *Interface) Empty() bool { return len(t.allMethods) == 0 }
 
+// Complete computes the interface's method set. It must be called by users of
+// NewInterface after the interface's embedded types are fully defined and
+// before using the interface type in any way other than to form other types.
+// Complete returns the receiver.
+func (t *Interface) Complete() *Interface {
+	if t.allMethods != nil {
+		return t
+	}
+
+	var allMethods []*Func
+	if t.embeddeds == nil {
+		if t.methods == nil {
+			allMethods = make([]*Func, 0, 1)
+		} else {
+			allMethods = t.methods
+		}
+	} else {
+		allMethods = append(allMethods, t.methods...)
+		for _, et := range t.embeddeds {
+			it := et.Underlying().(*Interface)
+			it.Complete()
+			for _, tm := range it.allMethods {
+				// Make a copy of the method and adjust its receiver type.
+				newm := *tm
+				newmtyp := *tm.typ.(*Signature)
+				newm.typ = &newmtyp
+				newmtyp.recv = NewVar(newm.pos, newm.pkg, "", t)
+				allMethods = append(allMethods, &newm)
+			}
+		}
+		sort.Sort(byUniqueMethodName(allMethods))
+	}
+	t.allMethods = allMethods
+
+	return t
+}
+
 // A Map represents a map type.
 type Map struct {
 	key, elem Type

go/types/universe.go

@@ -69,7 +69,7 @@ func defPredeclaredTypes() {
 	res := NewVar(token.NoPos, nil, "", Typ[String])
 	sig := &Signature{results: NewTuple(res)}
 	err := NewFunc(token.NoPos, nil, "Error", sig)
-	typ := &Named{underlying: NewInterface([]*Func{err}, nil)}
+	typ := &Named{underlying: NewInterface([]*Func{err}, nil).Complete()}
 	sig.recv = NewVar(token.NoPos, nil, "", typ)
 	def(NewTypeName(token.NoPos, nil, "error", typ))
 }