go/types: correctly compute method set of some recursive interfaces

R=go1.11.

The existing algorithm for type-checking interfaces breaks down in
complex cases of recursive types, e.g.:

	package issue21804

	type (
		_ interface{ m(B) }
		A interface{ a(D) }
		B interface{ A }
		C interface{ B }
		D interface{ C }
	)

	var _ A = C(nil)

The underlying problem is that the method set of C is computed by
following a chain of embedded interfaces at a point when the method
set for one of those interfaces is not yet complete. A more general
problem is largely avoided by topological sorting of interfaces
depending on their dependencies on embedded interfaces (but not
method parameters).

This change fixes this problem by fundamentally changing how
interface method sets are computed: Instead of determining them
based on recursively type-checking embedded interfaces, the new
approach computes the method sets of interfaces separately,
based on syntactic structure and name resolution; and using type-
checked types only when readily available (e.g., for local types
which can at best refer to themselves, and imported interfaces).

This approach avoids cyclic dependencies arising in the method
sets by separating the collection of embedded methods (which
fundamentally cannot have cycles in correct programs) and type-
checking of the method's signatures (which may have arbitrary
cycles).

As a consequence, type-checking interfaces can rely on the
pre-computed method sets which makes the code simpler: Type-
checking of embedded interface types doesn't have to happen
anymore during interface construction since we already have
all methods and now is delayed to the end of type-checking.
Also, the topological sort of global interfaces is not needed
anymore.

Fixes #18395.

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

src/go/types/check.go

@@ -85,11 +85,12 @@ type Checker struct {
 	files            []*ast.File                       // package files
 	unusedDotImports map[*Scope]map[*Package]token.Pos // positions of unused dot-imported packages for each file scope
 
-	firstErr error                 // first error encountered
-	methods  map[string][]*Func    // maps type names to associated methods
-	untyped  map[ast.Expr]exprInfo // map of expressions without final type
-	funcs    []funcInfo            // list of functions to type-check
-	delayed  []func()              // delayed checks requiring fully setup types
+	firstErr   error                    // first error encountered
+	methods    map[string][]*Func       // maps package scope type names to associated non-blank, non-interface methods
+	interfaces map[*TypeName]*ifaceInfo // maps interface type names to corresponding interface infos
+	untyped    map[ast.Expr]exprInfo    // map of expressions without final type
+	funcs      []funcInfo               // list of functions to type-check
+	delayed    []func()                 // delayed checks requiring fully setup types
 
 	// context within which the current object is type-checked
 	// (valid only for the duration of type-checking a specific object)
@@ -186,6 +187,7 @@ func (check *Checker) initFiles(files []*ast.File) {
 
 	check.firstErr = nil
 	check.methods = nil
+	check.interfaces = nil
 	check.untyped = nil
 	check.funcs = nil
 	check.delayed = nil
@@ -236,19 +238,21 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
 
 	check.collectObjects()
 
-	check.packageObjects(check.resolveOrder())
+	check.packageObjects()
 
 	check.functionBodies()
 
 	check.initOrder()
 
-	if !check.conf.DisableUnusedImportCheck {
-		check.unusedImports()
+	// perform delayed checks
+	// (cannot use range - delayed checks may add more delayed checks;
+	// e.g., when type-checking delayed embedded interfaces)
+	for i := 0; i < len(check.delayed); i++ {
+		check.delayed[i]()
 	}
 
-	// perform delayed checks
-	for _, f := range check.delayed {
-		f()
+	if !check.conf.DisableUnusedImportCheck {
+		check.unusedImports()
 	}
 
 	check.recordUntyped()

src/go/types/check_test.go

@@ -61,6 +61,7 @@ var tests = [][]string{
 	{"testdata/cycles2.src"},
 	{"testdata/cycles3.src"},
 	{"testdata/cycles4.src"},
+	{"testdata/cycles5.src"},
 	{"testdata/init0.src"},
 	{"testdata/init1.src"},
 	{"testdata/init2.src"},

src/go/types/decl.go

@@ -37,6 +37,18 @@ func (check *Checker) declare(scope *Scope, id *ast.Ident, obj Object, pos token
 	}
 }
 
+// pathString returns a string of the form a->b-> ... ->g for a path [a, b, ... g].
+func pathString(path []*TypeName) string {
+	var s string
+	for i, p := range path {
+		if i > 0 {
+			s += "->"
+		}
+		s += p.Name()
+	}
+	return s
+}
+
 // objDecl type-checks the declaration of obj in its respective (file) context.
 // See check.typ for the details on def and path.
 func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
@@ -45,7 +57,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 	}
 
 	if trace {
-		check.trace(obj.Pos(), "-- declaring %s", obj.Name())
+		check.trace(obj.Pos(), "-- declaring %s (path = %s)", obj.Name(), pathString(path))
 		check.indent++
 		defer func() {
 			check.indent--

src/go/types/ordering.go

-// Copyright 2014 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-// This file implements resolveOrder.
-
-package types
-
-import (
-	"go/ast"
-	"sort"
-)
-
-// resolveOrder computes the order in which package-level objects
-// must be type-checked.
-//
-// Interface types appear first in the list, sorted topologically
-// by dependencies on embedded interfaces that are also declared
-// in this package, followed by all other objects sorted in source
-// order.
-//
-// TODO(gri) Consider sorting all types by dependencies here, and
-// in the process check _and_ report type cycles. This may simplify
-// the full type-checking phase.
-//
-func (check *Checker) resolveOrder() []Object {
-	var ifaces, others []Object
-
-	// collect interface types with their dependencies, and all other objects
-	for obj := range check.objMap {
-		if ityp := check.interfaceFor(obj); ityp != nil {
-			ifaces = append(ifaces, obj)
-			// determine dependencies on embedded interfaces
-			for _, f := range ityp.Methods.List {
-				if len(f.Names) == 0 {
-					// Embedded interface: The type must be a (possibly
-					// qualified) identifier denoting another interface.
-					// Imported interfaces are already fully resolved,
-					// so we can ignore qualified identifiers.
-					if ident, _ := f.Type.(*ast.Ident); ident != nil {
-						embedded := check.pkg.scope.Lookup(ident.Name)
-						if check.interfaceFor(embedded) != nil {
-							check.objMap[obj].addDep(embedded)
-						}
-					}
-				}
-			}
-		} else {
-			others = append(others, obj)
-		}
-	}
-
-	// final object order
-	var order []Object
-
-	// sort interface types topologically by dependencies,
-	// and in source order if there are no dependencies
-	sort.Sort(inSourceOrder(ifaces))
-	visited := make(objSet)
-	for _, obj := range ifaces {
-		check.appendInPostOrder(&order, obj, visited)
-	}
-
-	// sort everything else in source order
-	sort.Sort(inSourceOrder(others))
-
-	return append(order, others...)
-}
-
-// interfaceFor returns the AST interface denoted by obj, or nil.
-func (check *Checker) interfaceFor(obj Object) *ast.InterfaceType {
-	tname, _ := obj.(*TypeName)
-	if tname == nil {
-		return nil // not a type
-	}
-	d := check.objMap[obj]
-	if d == nil {
-		check.dump("%s: %s should have been declared", obj.Pos(), obj.Name())
-		unreachable()
-	}
-	if d.typ == nil {
-		return nil // invalid AST - ignore (will be handled later)
-	}
-	ityp, _ := d.typ.(*ast.InterfaceType)
-	return ityp
-}
-
-func (check *Checker) appendInPostOrder(order *[]Object, obj Object, visited objSet) {
-	if visited[obj] {
-		// We've already seen this object; either because it's
-		// already added to order, or because we have a cycle.
-		// In both cases we stop. Cycle errors are reported
-		// when type-checking types.
-		return
-	}
-	visited[obj] = true
-
-	d := check.objMap[obj]
-	for _, obj := range orderedSetObjects(d.deps) {
-		check.appendInPostOrder(order, obj, visited)
-	}
-
-	*order = append(*order, obj)
-}
-
-func orderedSetObjects(set objSet) []Object {
-	list := make([]Object, len(set))
-	i := 0
-	for obj := range set {
-		// we don't care about the map element value
-		list[i] = obj
-		i++
-	}
-	sort.Sort(inSourceOrder(list))
-	return list
-}
-
-// inSourceOrder implements the sort.Sort interface.
-type inSourceOrder []Object
-
-func (a inSourceOrder) Len() int           { return len(a) }
-func (a inSourceOrder) Less(i, j int) bool { return a[i].order() < a[j].order() }
-func (a inSourceOrder) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }

src/go/types/resolver.go

@@ -9,6 +9,7 @@ import (
 	"go/ast"
 	"go/constant"
 	"go/token"
+	"sort"
 	"strconv"
 	"strings"
 	"unicode"
@@ -453,8 +454,17 @@ func (check *Checker) collectObjects() {
 	}
 }
 
-// packageObjects typechecks all package objects in objList, but not function bodies.
-func (check *Checker) packageObjects(objList []Object) {
+// packageObjects typechecks all package objects, but not function bodies.
+func (check *Checker) packageObjects() {
+	// process package objects in source order for reproducible results
+	objList := make([]Object, len(check.objMap))
+	i := 0
+	for obj := range check.objMap {
+		objList[i] = obj
+		i++
+	}
+	sort.Sort(inSourceOrder(objList))
+
 	// add new methods to already type-checked types (from a prior Checker.Files call)
 	for _, obj := range objList {
 		if obj, _ := obj.(*TypeName); obj != nil && obj.typ != nil {
@@ -476,6 +486,13 @@ func (check *Checker) packageObjects(objList []Object) {
 	check.methods = nil
 }
 
+// inSourceOrder implements the sort.Sort interface.
+type inSourceOrder []Object
+
+func (a inSourceOrder) Len() int           { return len(a) }
+func (a inSourceOrder) Less(i, j int) bool { return a[i].order() < a[j].order() }
+func (a inSourceOrder) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
+
 // functionBodies typechecks all function bodies.
 func (check *Checker) functionBodies() {
 	for _, f := range check.funcs {

src/go/types/testdata/cycles.src

@@ -52,9 +52,9 @@ type (
 	// interfaces
 	I0 /* ERROR cycle */ interface{ I0 }
 
-	I1 interface{ I2 }
+	I1 /* ERROR cycle */ interface{ I2 }
 	I2 interface{ I3 }
-	I3 /* ERROR cycle */ interface{ I1 }
+	I3 interface{ I1 }
 
 	I4 interface{ f(I4) }
 

src/go/types/testdata/cycles4.src

@@ -108,15 +108,3 @@ type Element interface {
 type Event interface {
 	Target() Element
 }
-
-// Recognize issue #13895.
-
-type (
-	_ interface{ m(B1) }
-	A1 interface{ a(D1) }
-	B1 interface{ A1 }
-	C1 interface{ B1 /* ERROR issue #18395 */ }
-	D1 interface{ C1 }
-)
-
-var _ A1 = C1 /* ERROR cannot use C1 */ (nil)
\ No newline at end of file

src/go/types/testdata/cycles5.src

+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+import "unsafe"
+
+// test case from issue #18395
+
+type (
+	A interface { B }
+	B interface { C }
+	C interface { D; F() A }
+	D interface { G() B }
+)
+
+var _ = A(nil).G // G must be found
+
+
+// test case from issue #21804
+
+type sourceBridge interface {
+	listVersions() ([]Version, error)
+}
+
+type Constraint interface {
+	copyTo(*ConstraintMsg)
+}
+
+type ConstraintMsg struct{}
+
+func (m *ConstraintMsg) asUnpairedVersion() UnpairedVersion {
+	return nil
+}
+
+type Version interface {
+	Constraint
+}
+
+type UnpairedVersion interface {
+	Version
+}
+
+var _ Constraint = UnpairedVersion(nil)
+
+
+// derived test case from issue #21804
+
+type (
+	_ interface{ m(B1) }
+	A1 interface{ a(D1) }
+	B1 interface{ A1 }
+	C1 interface{ B1 }
+	D1 interface{ C1 }
+)
+
+var _ A1 = C1(nil)
+
+
+// derived test case from issue #22701
+
+func F(x I4) interface{} {
+	return x.Method()
+}
+
+type Unused interface {
+	RefersToI1(a I1)
+}
+
+type I1 interface {
+	I2
+	I3
+}
+
+type I2 interface {
+	RefersToI4() I4
+}
+
+type I3 interface {
+	Method() interface{}
+}
+
+type I4 interface {
+	I1
+}
+
+
+// check embedding of error interface
+
+type Error interface{ error }
+
+var err Error
+var _ = err.Error()
+
+
+// more esoteric cases
+
+type (
+	T1 interface { T2 /* ERROR not an interface */ }
+	T2 /* ERROR cycle */ T2
+)
+
+type (
+	T3 interface { T4 /* ERROR not an interface */ }
+	T4 /* ERROR cycle */ T5
+	T5 = T6
+	T6 = T7
+	T7 = T4
+)
+
+
+// arbitrary code may appear inside an interface
+
+type I interface {
+	m([unsafe.Sizeof(func() { I.m(nil) })]byte) // should report an error (see #22992)
+}

src/go/types/testdata/decls0.src

@@ -159,13 +159,13 @@ type (
 	I8 /* ERROR "illegal cycle" */ interface {
 		I8
 	}
-	I9 interface {
+	I9 /* ERROR "illegal cycle" */ interface {
 		I10
 	}
 	I10 interface {
 		I11
 	}
-	I11 /* ERROR "illegal cycle" */ interface {
+	I11 interface {
 		I9
 	}
 

src/go/types/typexpr.go

@@ -69,20 +69,10 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 
 	case *TypeName:
 		x.mode = typexpr
-		// check for cycle
-		// (it's ok to iterate forward because each named type appears at most once in path)
-		for i, prev := range path {
-			if prev == obj {
-				check.errorf(obj.pos, "illegal cycle in declaration of %s", obj.name)
-				// print cycle
-				for _, obj := range path[i:] {
-					check.errorf(obj.Pos(), "\t%s refers to", obj.Name()) // secondary error, \t indented
-				}
-				check.errorf(obj.Pos(), "\t%s", obj.Name())
-				// maintain x.mode == typexpr despite error
-				typ = Typ[Invalid]
-				break
-			}
+		if check.cycle(obj, path, true) {
+			// maintain x.mode == typexpr despite error
+			typ = Typ[Invalid]
+			break
 		}
 
 	case *Var:
@@ -116,6 +106,26 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 	x.typ = typ
 }
 
+// cycle reports whether obj appears in path or not.
+// If it does, and report is set, it also reports a cycle error.
+func (check *Checker) cycle(obj *TypeName, path []*TypeName, report bool) bool {
+	// (it's ok to iterate forward because each named type appears at most once in path)
+	for i, prev := range path {
+		if prev == obj {
+			if report {
+				check.errorf(obj.pos, "illegal cycle in declaration of %s", obj.name)
+				// print cycle
+				for _, obj := range path[i:] {
+					check.errorf(obj.Pos(), "\t%s refers to", obj.Name()) // secondary error, \t indented
+				}
+				check.errorf(obj.Pos(), "\t%s", obj.Name())
+			}
+			return true
+		}
+	}
+	return false
+}
+
 // typExpr type-checks the type expression e and returns its type, or Typ[Invalid].
 // If def != nil, e is the type specification for the named type def, declared
 // in a type declaration, and def.underlying will be set to the type of e before
@@ -456,44 +466,86 @@ func (check *Checker) declareInSet(oset *objset, pos token.Pos, obj Object) bool
 	return true
 }
 
-func (check *Checker) interfaceType(iface *Interface, ityp *ast.InterfaceType, def *Named, path []*TypeName) {
-	// empty interface: common case
-	if ityp.Methods == nil {
+func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, def *Named, path []*TypeName) {
+	// fast-track empty interface
+	if iface.Methods == nil {
+		ityp.allMethods = markComplete
 		return
 	}
 
-	// The parser ensures that field tags are nil and we don't
-	// care if a constructed AST contains non-nil tags.
+	// collect embedded interfaces
+	// Only needed for printing and API. Delay collection
+	// to end of type-checking when all types are complete.
+	interfaceScope := check.scope // capture for use in delayed function
+	check.delay(func() {
+		check.scope = interfaceScope
+		if trace {
+			check.trace(iface.Pos(), "-- delayed checking embedded interfaces of %s", iface)
+			check.indent++
+			defer func() {
+				check.indent--
+			}()
+		}
+		for _, f := range iface.Methods.List {
+			if len(f.Names) == 0 {
+				typ := check.typ(f.Type)
+				// typ should be a named type denoting an interface
+				// (the parser will make sure it's a name type but
+				// constructed ASTs may be wrong).
+				if typ == Typ[Invalid] {
+					continue // error reported before
+				}
+				if !isNamed(typ) {
+					check.invalidAST(f.Type.Pos(), "%s is not a named type", f.Type)
+					continue
+				}
+				embed, _ := typ.Underlying().(*Interface)
+				if embed == nil {
+					check.errorf(f.Type.Pos(), "%s is not an interface", typ)
+					continue
+				}
+				// Correct embedded interfaces must be complete -
+				// don't just assert, but report error since this
+				// used to be the underlying cause for issue #18395.
+				if embed.allMethods == nil {
+					check.dump("%s: incomplete embedded interface %s", f.Type.Pos(), typ)
+					unreachable()
+				}
+				// collect interface
+				// (at this point we know that typ must be a named, non-basic type)
+				ityp.embeddeds = append(ityp.embeddeds, typ.(*Named))
+			}
+		}
+		// sort to match NewInterface
+		// TODO(gri) we may be able to switch to source order
+		sort.Sort(byUniqueTypeName(ityp.embeddeds))
+	})
+
+	// compute method set
+	var tname *TypeName
+	if def != nil {
+		tname = def.obj
+	}
+	info := check.infoFromTypeLit(iface, tname, path)
+	if info == nil || info == &emptyIfaceInfo {
+		// error or empty interface - exit early
+		ityp.allMethods = markComplete
+		return
+	}
 
 	// use named receiver type if available (for better error messages)
-	var recvTyp Type = iface
+	var recvTyp Type = ityp
 	if def != nil {
 		recvTyp = def
 	}
 
-	// Phase 1: Collect explicitly declared methods, the corresponding
-	//          signature (AST) expressions, and the list of embedded
-	//          type (AST) expressions. Do not resolve signatures or
-	//          embedded types yet to avoid cycles referring to this
-	//          interface.
-
-	var (
-		mset       objset
-		signatures []ast.Expr // list of corresponding method signatures
-		embedded   []ast.Expr // list of embedded types
-	)
-	for _, f := range ityp.Methods.List {
-		if len(f.Names) > 0 {
-			// The parser ensures that there's only one method
-			// and we don't care if a constructed AST has more.
-			name := f.Names[0]
+	// collect methods
+	var sigfix []*methodInfo
+	for i, minfo := range info.methods {
+		fun := minfo.fun
+		if fun == nil {
+			name := minfo.src.Names[0]
 			pos := name.Pos()
-			// spec: "As with all method sets, in an interface type,
-			// each method must have a unique non-blank name."
-			if name.Name == "_" {
-				check.errorf(pos, "invalid method name _")
-				continue
-			}
 			// Don't type-check signature yet - use an
 			// empty signature now and update it later.
 			// Since we know the receiver, set it up now
@@ -504,91 +556,42 @@ func (check *Checker) interfaceType(iface *Interface, ityp *ast.InterfaceType, d
 			// also the T4 and T5 tests in testdata/cycles2.src.
 			sig := new(Signature)
 			sig.recv = NewVar(pos, check.pkg, "", recvTyp)
-			m := NewFunc(pos, check.pkg, name.Name, sig)
-			if check.declareInSet(&mset, pos, m) {
-				iface.methods = append(iface.methods, m)
-				iface.allMethods = append(iface.allMethods, m)
-				signatures = append(signatures, f.Type)
-				check.recordDef(name, m)
-			}
-		} else {
-			// embedded type
-			embedded = append(embedded, f.Type)
+			fun = NewFunc(pos, check.pkg, name.Name, sig)
+			minfo.fun = fun
+			check.recordDef(name, fun)
+			sigfix = append(sigfix, minfo)
 		}
-	}
-
-	// Phase 2: Resolve embedded interfaces. Because an interface must not
-	//          embed itself (directly or indirectly), each embedded interface
-	//          can be fully resolved without depending on any method of this
-	//          interface (if there is a cycle or another error, the embedded
-	//          type resolves to an invalid type and is ignored).
-	//          In particular, the list of methods for each embedded interface
-	//          must be complete (it cannot depend on this interface), and so
-	//          those methods can be added to the list of all methods of this
-	//          interface.
-
-	for _, e := range embedded {
-		pos := e.Pos()
-		typ := check.typExpr(e, nil, path)
-		// Determine underlying embedded (possibly incomplete) type
-		// by following its forward chain.
-		named, _ := typ.(*Named)
-		under := underlying(named)
-		embed, _ := under.(*Interface)
-		if embed == nil {
-			if typ != Typ[Invalid] {
-				check.errorf(pos, "%s is not an interface", typ)
-			}
-			continue
-		}
-		iface.embeddeds = append(iface.embeddeds, named)
-		// collect embedded methods
-		if embed.allMethods == nil {
-			check.errorf(pos, "internal error: incomplete embedded interface %s (issue #18395)", named)
-		}
-		for _, m := range embed.allMethods {
-			if check.declareInSet(&mset, pos, m) {
-				iface.allMethods = append(iface.allMethods, m)
-			}
+		// fun != nil
+		if i < info.explicits {
+			ityp.methods = append(ityp.methods, fun)
 		}
+		ityp.allMethods = append(ityp.allMethods, fun)
 	}
 
-	// Phase 3: At this point all methods have been collected for this interface.
-	//          It is now safe to type-check the signatures of all explicitly
-	//          declared methods, even if they refer to this interface via a cycle
-	//          and embed the methods of this interface in a parameter of interface
-	//          type.
-
-	for i, m := range iface.methods {
-		expr := signatures[i]
-		typ := check.typ(expr)
+	// fix signatures now that we have collected all methods
+	for _, minfo := range sigfix {
+		typ := check.typ(minfo.src.Type)
 		sig, _ := typ.(*Signature)
 		if sig == nil {
 			if typ != Typ[Invalid] {
-				check.invalidAST(expr.Pos(), "%s is not a method signature", typ)
+				check.invalidAST(minfo.src.Type.Pos(), "%s is not a method signature", typ)
 			}
 			continue // keep method with empty method signature
 		}
 		// update signature, but keep recv that was set up before
-		old := m.typ.(*Signature)
+		old := minfo.fun.typ.(*Signature)
 		sig.recv = old.recv
-		*old = *sig // update signature (don't replace it!)
+		*old = *sig // update signature (don't replace pointer!)
 	}
 
-	// TODO(gri) The list of explicit methods is only sorted for now to
-	// produce the same Interface as NewInterface. We may be able to
-	// claim source order in the future. Revisit.
-	sort.Sort(byUniqueMethodName(iface.methods))
-
-	// TODO(gri) The list of embedded types is only sorted for now to
-	// produce the same Interface as NewInterface. We may be able to
-	// claim source order in the future. Revisit.
-	sort.Sort(byUniqueTypeName(iface.embeddeds))
+	// sort to match NewInterface
+	// TODO(gri) we may be able to switch to source order
+	sort.Sort(byUniqueMethodName(ityp.methods))
 
-	if iface.allMethods == nil {
-		iface.allMethods = make([]*Func, 0) // mark interface as complete
+	if ityp.allMethods == nil {
+		ityp.allMethods = markComplete
 	} else {
-		sort.Sort(byUniqueMethodName(iface.allMethods))
+		sort.Sort(byUniqueMethodName(ityp.allMethods))
 	}
 }