go/types: fix computation of initialization order

The old algorithm operated on a dependency graph that included
all objects (including functions) for simplicity: it was based
directly on the dependencies collected for each object during
type checking an object's initialization expression. It also
used that graph to compute the objects involved in an erroneous
initialization cycle.

Cycles that consist only of (mutually recursive) functions are
permitted in initialization code; so those cycles were silently
ignored if encountered. However, such cycles still inflated the
number of dependencies a variable might have (due to the cycle),
which in some cases lead to the wrong variable being scheduled
for initialization before the one with the inflated dependency
count.

Correcting for the cycle when it is found is too late since at
that point another variable may have already been scheduled.

The new algorithm computes the initialization dependency graph as
before but adds an extra pass during which functions are eliminated
from the graph (and their dependencies are "back-propagated").
This eliminates the problem of cycles only involving functions
(there are no functions).

When a cycle is found, the new code computes the cycle path from
the original object dependencies so it can still include functions
on the path as before, for the same detailed error message.

The new code also more clearly distinguishes between objects that
can be in the dependency graph (constants, variables, functions),
and objects that cannot, by introducing the dependency type, a new
subtype of Object. As a consequence, the dependency graph is smaller.

Fixes #10709.

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

src/go/types/api_test.go

@@ -573,26 +573,25 @@ func TestInitOrderInfo(t *testing.T) {
 			"a = next()", "b = next()", "c = next()", "d = next()", "e = next()", "f = next()", "_ = makeOrder()",
 		}},
 		// test case for issue 10709
-		// TODO(gri) enable once the issue is fixed
+		{`package p13
-		// {`package p13
 
-		// var (
+		var (
-		//     v = t.m()
+		    v = t.m()
-		//     t = makeT(0)
+		    t = makeT(0)
-		// )
+		)
 
-		// type T struct{}
+		type T struct{}
 
-		// func (T) m() int { return 0 }
+		func (T) m() int { return 0 }
 
-		// func makeT(n int) T {
+		func makeT(n int) T {
-		//     if n > 0 {
+		    if n > 0 {
-		//         return makeT(n-1)
+		        return makeT(n-1)
-		//     }
+		    }
-		//     return T{}
+		    return T{}
-		// }`, []string{
+		}`, []string{
-		// 	"t = makeT(0)", "v = t.m()",
+			"t = makeT(0)", "v = t.m()",
-		// }},
+		}},
 		// test case for issue 10709: same as test before, but variable decls swapped
 		{`package p14
 
@@ -613,6 +612,24 @@ func TestInitOrderInfo(t *testing.T) {
 		}`, []string{
 			"t = makeT(0)", "v = t.m()",
 		}},
+		// another candidate possibly causing problems with issue 10709
+		{`package p15
+
+		var y1 = f1()
+
+		func f1() int { return g1() }
+		func g1() int { f1(); return x1 }
+
+		var x1 = 0
+
+		var y2 = f2()
+
+		func f2() int { return g2() }
+		func g2() int { return x2 }
+
+		var x2 = 0`, []string{
+			"x1 = 0", "y1 = f1()", "x2 = 0", "y2 = f2()",
+		}},
 	}
 
 	for _, test := range tests {

src/go/types/initorder.go

@@ -15,7 +15,7 @@ func (check *Checker) initOrder() {
 	// built from several calls to (*Checker).Files. Clear it.
 	check.Info.InitOrder = check.Info.InitOrder[:0]
 
-	// Compute the transposed object dependency graph and initialize
+	// Compute the object dependency graph and initialize
 	// a priority queue with the list of graph nodes.
 	pq := nodeQueue(dependencyGraph(check.objMap))
 	heap.Init(&pq)
@@ -25,6 +25,8 @@ func (check *Checker) initOrder() {
 		fmt.Printf("Computing initialization order for %s\n\n", check.pkg)
 		fmt.Println("Object dependency graph:")
 		for obj, d := range check.objMap {
+			// only print objects that may appear in the dependency graph
+			if obj, _ := obj.(dependency); obj != nil {
 				if len(d.deps) > 0 {
 					fmt.Printf("\t%s depends on\n", obj.Name())
 					for dep := range d.deps {
@@ -34,13 +36,14 @@ func (check *Checker) initOrder() {
 					fmt.Printf("\t%s has no dependencies\n", obj.Name())
 				}
 			}
+		}
 		fmt.Println()
 
-		fmt.Println("Transposed object dependency graph:")
+		fmt.Println("Transposed object dependency graph (functions eliminated):")
 		for _, n := range pq {
-			fmt.Printf("\t%s depends on %d nodes\n", n.obj.Name(), n.in)
+			fmt.Printf("\t%s depends on %d nodes\n", n.obj.Name(), n.ndeps)
-			for _, out := range n.out {
+			for p := range n.pred {
-				fmt.Printf("\t\t%s is dependent\n", out.obj.Name())
+				fmt.Printf("\t\t%s is dependent\n", p.obj.Name())
 			}
 		}
 		fmt.Println()
@@ -54,34 +57,40 @@ func (check *Checker) initOrder() {
 	// In a valid Go program, those nodes always have zero dependencies (after
 	// removing all incoming dependencies), otherwise there are initialization
 	// cycles.
-	mark := 0
 	emitted := make(map[*declInfo]bool)
 	for len(pq) > 0 {
 		// get the next node
-		n := heap.Pop(&pq).(*objNode)
+		n := heap.Pop(&pq).(*graphNode)
 
 		if debug {
 			fmt.Printf("\t%s (src pos %d) depends on %d nodes now\n",
-				n.obj.Name(), n.obj.order(), n.in)
+				n.obj.Name(), n.obj.order(), n.ndeps)
 		}
 
 		// if n still depends on other nodes, we have a cycle
-		if n.in > 0 {
+		if n.ndeps > 0 {
-			mark++ // mark nodes using a different value each time
+			cycle := findPath(check.objMap, n.obj, n.obj, make(map[Object]bool))
-			cycle := findPath(n, n, mark)
+			// If n.obj is not part of the cycle (e.g., n.obj->b->c->d->c),
-			if i := valIndex(cycle); i >= 0 {
+			// cycle will be nil. Don't report anything in that case since
-				check.reportCycle(cycle, i)
+			// the cycle is reported when the algorithm gets to an object
+			// in the cycle.
+			// Furthermore, once an object in the cycle is encountered,
+			// the cycle will be broken (dependency count will be reduced
+			// below), and so the remaining nodes in the cycle don't trigger
+			// another error (unless they are part of multiple cycles).
+			if cycle != nil {
+				check.reportCycle(cycle)
 			}
-			// ok to continue, but the variable initialization order
+			// Ok to continue, but the variable initialization order
 			// will be incorrect at this point since it assumes no
-			// cycle errors
+			// cycle errors.
 		}
 
 		// reduce dependency count of all dependent nodes
 		// and update priority queue
-		for _, out := range n.out {
+		for p := range n.pred {
-			out.in--
+			p.ndeps--
-			heap.Fix(&pq, out.index)
+			heap.Fix(&pq, p.index)
 		}
 
 		// record the init order for variables with initializers only
@@ -118,102 +127,147 @@ func (check *Checker) initOrder() {
 	}
 }
 
-// findPath returns the (reversed) list of nodes z, ... c, b, a,
+// findPath returns the (reversed) list of objects []Object{to, ... from}
-// such that there is a path (list of edges) from a to z.
+// such that there is a path of object dependencies from 'from' to 'to'.
 // If there is no such path, the result is nil.
-// Nodes marked with the value mark are considered "visited";
+func findPath(objMap map[Object]*declInfo, from, to Object, visited map[Object]bool) []Object {
-// unvisited nodes are marked during the graph search.
+	if visited[from] {
-func findPath(a, z *objNode, mark int) []*objNode {
-	if a.mark == mark {
 		return nil // node already seen
 	}
-	a.mark = mark
+	visited[from] = true
 
-	for _, n := range a.out {
+	for d := range objMap[from].deps {
-		if n == z {
+		if d == to {
-			return []*objNode{z}
+			return []Object{d}
 		}
-		if P := findPath(n, z, mark); P != nil {
+		if P := findPath(objMap, d, to, visited); P != nil {
-			return append(P, n)
+			return append(P, d)
 		}
 	}
 
 	return nil
 }
 
-// valIndex returns the index of the first constant or variable in a,
+// reportCycle reports an error for the given cycle.
-// if any; or a value < 0.
+func (check *Checker) reportCycle(cycle []Object) {
-func valIndex(a []*objNode) int {
+	obj := cycle[0]
-	for i, n := range a {
-		switch n.obj.(type) {
-		case *Const, *Var:
-			return i
-		}
-	}
-	return -1
-}
-
-// reportCycle reports an error for the cycle starting at i.
-func (check *Checker) reportCycle(cycle []*objNode, i int) {
-	obj := cycle[i].obj
 	check.errorf(obj.Pos(), "initialization cycle for %s", obj.Name())
-	// print cycle
+	// subtle loop: print cycle[i] for i = 0, n-1, n-2, ... 1 for len(cycle) = n
-	for _ = range cycle {
+	for i := len(cycle) - 1; i >= 0; i-- {
 		check.errorf(obj.Pos(), "\t%s refers to", obj.Name()) // secondary error, \t indented
-		i++
+		obj = cycle[i]
-		if i >= len(cycle) {
-			i = 0
-		}
-		obj = cycle[i].obj
 	}
+	// print cycle[0] again to close the cycle
 	check.errorf(obj.Pos(), "\t%s", obj.Name())
 }
 
-// An objNode represents a node in the object dependency graph.
+// ----------------------------------------------------------------------------
-// Each node b in a.out represents an edge a->b indicating that
+// Object dependency graph
-// b depends on a.
+
-// Nodes may be marked for cycle detection. A node n is marked
+// A dependency is an object that may be a dependency in an initialization
-// if n.mark corresponds to the current mark value.
+// expression. Only constants, variables, and functions can be dependencies.
-type objNode struct {
+// Constants are here because constant expression cycles are reported during
-	obj   Object     // object represented by this node
+// initialization order computation.
-	in    int        // number of nodes this node depends on
+type dependency interface {
-	out   []*objNode // list of nodes that depend on this node
+	Object
-	index int        // node index in list of nodes
+	isDependency()
-	mark  int        // for cycle detection
 }
 
-// dependencyGraph computes the transposed object dependency graph
+// A graphNode represents a node in the object dependency graph.
-// from the given objMap. The transposed graph is returned as a list
+// Each node p in n.pred represents an edge p->n, and each node
-// of nodes; an edge d->n indicates that node n depends on node d.
+// s in n.succ represents an edge n->s; with a->b indicating that
-func dependencyGraph(objMap map[Object]*declInfo) []*objNode {
+// a depends on b.
-	// M maps each object to its corresponding node
+type graphNode struct {
-	M := make(map[Object]*objNode, len(objMap))
+	obj        dependency // object represented by this node
+	pred, succ nodeSet    // consumers and dependencies of this node (lazily initialized)
+	index      int        // node index in graph slice/priority queue
+	ndeps      int        // number of outstanding dependencies before this object can be initialized
+}
+
+type nodeSet map[*graphNode]bool
+
+func (s *nodeSet) add(p *graphNode) {
+	if *s == nil {
+		*s = make(nodeSet)
+	}
+	(*s)[p] = true
+}
+
+// dependencyGraph computes the object dependency graph from the given objMap,
+// with any function nodes removed. The resulting graph contains only constants
+// and variables.
+func dependencyGraph(objMap map[Object]*declInfo) []*graphNode {
+	// M is the dependency (Object) -> graphNode mapping
+	M := make(map[dependency]*graphNode)
 	for obj := range objMap {
-		M[obj] = &objNode{obj: obj}
+		// only consider nodes that may be an initialization dependency
+		if obj, _ := obj.(dependency); obj != nil {
+			M[obj] = &graphNode{obj: obj}
+		}
 	}
 
-	// G is the graph of nodes n
+	// compute edges for graph M
-	G := make([]*objNode, len(M))
+	// (We need to include all nodes, even isolated ones, because they still need
-	i := 0
+	// to be scheduled for initialization in correct order relative to other nodes.)
 	for obj, n := range M {
-		deps := objMap[obj].deps
+		// for each dependency obj -> d (= deps[i]), create graph edges n->s and s->n
-		n.in = len(deps)
+		for d := range objMap[obj].deps {
-		for d := range deps {
+			// only consider nodes that may be an initialization dependency
-			d := M[d]                // node n depends on node d
+			if d, _ := d.(dependency); d != nil {
-			d.out = append(d.out, n) // add edge d->n
+				d := M[d]
+				n.succ.add(d)
+				d.pred.add(n)
+			}
+		}
 	}
 
-		G[i] = n
+	// remove function nodes and collect remaining graph nodes in G
+	// (Mutually recursive functions may introduce cycles among themselves
+	// which are permitted. Yet such cycles may incorrectly inflate the dependency
+	// count for variables which in turn may not get scheduled for initialization
+	// in correct order.)
+	var G []*graphNode
+	for obj, n := range M {
+		if _, ok := obj.(*Func); ok {
+			// connect each predecessor p of n with each successor s
+			// and drop the function node (don't collect it in G)
+			for p := range n.pred {
+				// ignore self-cycles
+				if p != n {
+					// Each successor s of n becomes a successor of p, and
+					// each predecessor p of n becomes a predecessor of s.
+					for s := range n.succ {
+						// ignore self-cycles
+						if s != n {
+							p.succ.add(s)
+							s.pred.add(p)
+							delete(s.pred, n) // remove edge to n
+						}
+					}
+					delete(p.succ, n) // remove edge to n
+				}
+			}
+		} else {
+			// collect non-function nodes
+			G = append(G, n)
+		}
+	}
+
+	// fill in index and ndeps fields
+	for i, n := range G {
 		n.index = i
-		i++
+		n.ndeps = len(n.succ)
 	}
 
 	return G
 }
 
+// ----------------------------------------------------------------------------
+// Priority queue
+
 // nodeQueue implements the container/heap interface;
 // a nodeQueue may be used as a priority queue.
-type nodeQueue []*objNode
+type nodeQueue []*graphNode
 
 func (a nodeQueue) Len() int { return len(a) }
 
@@ -227,7 +281,7 @@ func (a nodeQueue) Less(i, j int) bool {
 	x, y := a[i], a[j]
 	// nodes are prioritized by number of incoming dependencies (1st key)
 	// and source order (2nd key)
-	return x.in < y.in || x.in == y.in && x.obj.order() < y.obj.order()
+	return x.ndeps < y.ndeps || x.ndeps == y.ndeps && x.obj.order() < y.obj.order()
 }
 
 func (a *nodeQueue) Push(x interface{}) {

src/go/types/object.go

@@ -153,6 +153,8 @@ func NewConst(pos token.Pos, pkg *Package, name string, typ Type, val constant.V
 
 func (obj *Const) Val() constant.Value { return obj.val }
 
+func (*Const) isDependency() {} // a constant may be a dependency of an initialization expression
+
 // A TypeName represents a declared type.
 type TypeName struct {
 	object
@@ -187,6 +189,8 @@ func (obj *Var) Anonymous() bool { return obj.anonymous }
 
 func (obj *Var) IsField() bool { return obj.isField }
 
+func (*Var) isDependency() {} // a variable may be a dependency of an initialization expression
+
 // A Func represents a declared function, concrete method, or abstract
 // (interface) method. Its Type() is always a *Signature.
 // An abstract method may belong to many interfaces due to embedding.
@@ -215,6 +219,8 @@ func (obj *Func) Scope() *Scope {
 	return obj.typ.(*Signature).scope
 }
 
+func (*Func) isDependency() {} // a function may be a dependency of an initialization expression
+
 // A Label represents a declared label.
 type Label struct {
 	object