go.tools/go/types: type-check interfaces in reverse dependency order

Side-effect: Because interfaces are now type-checked in reverse order,
cycle errors in interface declarations appear at the "end" rather than
at the "beginning" of the cycle in the source code. This is harmless.
Eventually we may want to do dependency order determination and thus
cycle detection for all types before fully type-checking them, which
might simplify some code and also produce consistently positioned cycle
errors again.

Fixes golang/go#7158.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://golang.org/cl/83640043

go/types/check.go

@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"go/ast"
 	"go/token"
-	"sort"
 
 	"code.google.com/p/go.tools/go/exact"
 )
@@ -200,17 +199,6 @@ func (check *checker) handleBailout(err *error) {
 	}
 }
 
-func mapObjects(m map[Object]*declInfo) []Object {
-	list := make([]Object, len(m))
-	i := 0
-	for obj := range m {
-		list[i] = obj
-		i++
-	}
-	sort.Sort(inSourceOrder(list))
-	return list
-}
-
 // Files checks the provided files as part of the checker's package.
 func (check *checker) Files(files []*ast.File) (err error) {
 	defer check.handleBailout(&err)
@@ -219,7 +207,7 @@ func (check *checker) Files(files []*ast.File) (err error) {
 
 	check.collectObjects()
 
-	objList := mapObjects(check.objMap)
+	objList := check.resolveOrder()
 
 	check.packageObjects(objList)
 

go/types/decl.go

@@ -50,7 +50,7 @@ func (check *checker) objDecl(obj Object, def *Named, path []*TypeName) {
 
 	d := check.objMap[obj]
 	if d == nil {
-		check.dump("%s: %s should have been declared", obj.Pos(), obj)
+		check.dump("%s: %s should have been declared", obj.Pos(), obj.Name())
 		unreachable()
 	}
 

go/types/ordering.go

@@ -0,0 +1,108 @@
+// 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) identifer 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))
+	if debug {
+		for _, obj := range ifaces {
+			assert(check.objMap[obj].mark == 0)
+		}
+	}
+	for _, obj := range ifaces {
+		check.appendInPostOrder(&order, obj)
+	}
+
+	// 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) {
+	d := check.objMap[obj]
+	if d.mark != 0 {
+		// 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
+	}
+	d.mark = 1
+
+	for _, obj := range orderedSetObjects(d.deps) {
+		check.appendInPostOrder(order, obj)
+	}
+
+	*order = append(*order, obj)
+}

go/types/resolver.go

@@ -25,8 +25,8 @@ type declInfo struct {
 	init  ast.Expr      // init expression, or nil
 	fdecl *ast.FuncDecl // func declaration, or nil
 
-	deps map[Object]bool // init dependencies; lazily allocated
+	deps map[Object]bool // type and init dependencies; lazily allocated
-	mark int             // see check.dependencies
+	mark int             // for dependency analysis
 }
 
 // hasInitializer reports whether the declared object has an initialization
@@ -437,7 +437,7 @@ func (check *checker) unusedImports() {
 	}
 }
 
-func setObjects(set map[Object]bool) []Object {
+func orderedSetObjects(set map[Object]bool) []Object {
 	list := make([]Object, len(set))
 	i := 0
 	for obj := range set {
@@ -449,11 +449,6 @@ func setObjects(set map[Object]bool) []Object {
 	return list
 }
 
-// TODO(gri) Instead of this support function, introduce type
-// that combines an map[Object]*declInfo and []Object so that
-// we can encapsulate this and don't have to depend on correct
-// Pos() information.
-
 // inSourceOrder implements the sort.Sort interface.
 type inSourceOrder []Object
 
@@ -526,7 +521,7 @@ func (check *checker) dependencies(obj Object, path []Object) {
 
 	path = append(path, obj) // len(path) > 0
 	init.mark = len(path)    // init.mark > 0
-	for _, obj := range setObjects(init.deps) {
+	for _, obj := range orderedSetObjects(init.deps) {
 		check.dependencies(obj, path)
 	}
 	init.mark = -1 // init.mark < 0

go/types/testdata/cycles.src

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

go/types/testdata/cycles4.src

@@ -66,3 +66,45 @@ type T4 interface {
 func _(x T1, y T3) {
 	x = y
 }
+
+// Check that interfaces are type-checked in order of
+// (embedded interface) dependencies (was issue 7158).
+
+var x1 T5 = T7(nil)
+
+type T5 interface {
+	T6
+}
+
+type T6 interface {
+	m() T7
+}
+type T7 interface {
+	T5
+}
+
+// Actual test case from issue 7158.
+
+func wrapNode() Node {
+	return wrapElement()
+}
+
+func wrapElement() Element {
+	return nil
+}
+
+type EventTarget interface {
+	AddEventListener(Event)
+}
+
+type Node interface {
+	EventTarget
+}
+
+type Element interface {
+	Node
+}
+
+type Event interface {
+	Target() Element
+}

go/types/testdata/decls0.src

@@ -155,18 +155,14 @@ type (
 	I8 /* ERROR "illegal cycle" */ interface {
 		I8
 	}
-	// Use I09 (rather than I9) because it appears lexically before
+	I9 interface {
-	// I10 so that we get the illegal cycle here rather then in the
-	// declaration of I10. If the implementation sorts by position
-	// rather than name, the error message will still be here.
-	I09 /* ERROR "illegal cycle" */ interface {
 		I10
 	}
 	I10 interface {
 		I11
 	}
-	I11 interface {
+	I11 /* ERROR "illegal cycle" */ interface {
-		I09
+		I9
 	}
 
 	C1 chan int