go.tools/go/types: track init cycles through functions

- Info.InitOrder now provides list of Initializers
  (vars + init expr), handling n:1 decls and blank
  identifiers
- added respective API test
- cycles detected through function "mentions"

Missing: cycles through method "mentions" and via
         closures

R=adonovan
CC=golang-dev
https://golang.org/cl/21810043

go/types/api.go

@@ -162,11 +162,20 @@ type Info struct {
 	//
 	Scopes map[ast.Node]*Scope
 
-	// InitOrder is the list of package-level variables in the order in which
-	// they must be initialized. Variables related by an initialization dependency
-	// appear in topological order, and unrelated variables appear in source order.
-	// TODO(gri) find a better name
-	InitOrder []*Var
+	// InitOrder is the list of package-level initializers in the order in which
+	// they must be executed. Initializers referring to variables related by an
+	// initialization dependency appear in topological order, the others appear
+	// in source order. Variables without an initialization expression do not
+	// appear in this list.
+	InitOrder []*Initializer
+}
+
+// An Initializer describes a package-level variable, or a list of variables in case
+// of a multi-valued initialization expression, and the corresponding initialization
+// expression.
+type Initializer struct {
+	Lhs []*Var // var Lhs = Rhs
+	Rhs ast.Expr
 }
 
 // Check type-checks a package and returns the resulting package object,

go/types/api_test.go

@@ -209,3 +209,50 @@ func TestScopesInfo(t *testing.T) {
 		}
 	}
 }
+
+func TestInitOrder(t *testing.T) {
+	var tests = []struct {
+		src  string
+		vars [][]string
+		// TODO(gri) check also init expression
+	}{
+		{`package p; var (x = 1; y = x)`, [][]string{{"x"}, {"y"}}},
+		{`package p; var (a = 1; b = 2; c = 3)`, [][]string{{"a"}, {"b"}, {"c"}}},
+		{`package p; var (a, b, c = 1, 2, 3)`, [][]string{{"a"}, {"b"}, {"c"}}},
+		{`package p; var _ = f(); func f() int { return 1 }`, [][]string{{"_"}}}, // blank var
+		{`package p; var (a = 0; x = y; y = z; z = 0)`, [][]string{{"a"}, {"z"}, {"y"}, {"x"}}},
+		{`package p; var (a, _ = m[0]; m map[int]string)`, [][]string{{"a", "_"}}}, // blank var
+		{`package p; var a, b = f(); func f() (_, _ int) { return z, z }; var z = 0`, [][]string{{"z"}, {"a", "b"}}},
+		// TODO(gri) add more tests (incl. methods, closures, init functions)
+	}
+
+	for i, test := range tests {
+		path := fmt.Sprintf("InitOrder%d", i)
+		info := Info{}
+		mustTypecheck(t, path, test.src, &info)
+
+		// number of initializers must match
+		if len(info.InitOrder) != len(test.vars) {
+			t.Errorf("%s: got %d initializers; want %d", path, len(info.InitOrder), len(test.vars))
+			continue
+		}
+
+		// initializer order and initialized variables must match
+		for i, got := range info.InitOrder {
+			want := test.vars[i]
+			// number of variables must match
+			if len(got.Lhs) != len(want) {
+				t.Errorf("%s, %d.th initializer: got %d variables; want %d", path, i, len(got.Lhs), len(want))
+				continue
+			}
+
+			// variable names must match
+			for j, got := range got.Lhs {
+				want := want[j]
+				if got.name != want {
+					t.Errorf("%s, %d.th initializer: got name %s; want %s", path, i, got.name, want)
+				}
+			}
+		}
+	}
+}

go/types/check.go

@@ -34,20 +34,20 @@ type checker struct {
 	fset *token.FileSet
 	pkg  *Package
 
-	methods      map[string][]*Func         // maps type names to associated methods
-	conversions  map[*ast.CallExpr]bool     // set of type-checked conversions (to distinguish from calls)
-	dependencies map[Object]map[Object]bool // set of object initialization dependencies (obj depends on {obj1, obj2, ...})
-	untyped      map[ast.Expr]exprInfo      // map of expressions without final type
-	lhsVarsList  [][]*Var                   // type switch lhs variable sets, for 'declared but not used' errors
-	delayed      []func()                   // delayed checks that require fully setup types
+	methods     map[string][]*Func     // maps type names to associated methods
+	conversions map[*ast.CallExpr]bool // set of type-checked conversions (to distinguish from calls)
+	untyped     map[ast.Expr]exprInfo  // map of expressions without final type
+	lhsVarsList [][]*Var               // type switch lhs variable sets, for 'declared but not used' errors
+	delayed     []func()               // delayed checks that require fully setup types
 
 	firstErr error // first error encountered
 	Info           // collected type info
 
-	objMap   map[Object]declInfo // if set we are in the package-level declaration phase (otherwise all objects seen must be declared)
-	topScope *Scope              // current topScope for lookups
-	iota     exact.Value         // current value of iota in a constant declaration; nil otherwise
-	init     Object              // current variable or function for which init expression or body is type-checked
+	objMap   map[Object]*declInfo // if set we are in the package-level declaration phase (otherwise all objects seen must be declared)
+	initMap  map[Object]*declInfo // map of variables/functions with init expressions/bodies
+	topScope *Scope               // current topScope for lookups
+	iota     exact.Value          // current value of iota in a constant declaration; nil otherwise
+	decl     *declInfo            // current package-level declaration whose init expression/body is type-checked
 
 	// functions
 	funcList []funcInfo // list of functions/methods with correct signatures and non-empty bodies
@@ -60,43 +60,32 @@ type checker struct {
 
 func newChecker(conf *Config, fset *token.FileSet, pkg *Package) *checker {
 	return &checker{
-		conf:         conf,
-		fset:         fset,
-		pkg:          pkg,
-		methods:      make(map[string][]*Func),
-		conversions:  make(map[*ast.CallExpr]bool),
-		dependencies: make(map[Object]map[Object]bool),
-		untyped:      make(map[ast.Expr]exprInfo),
+		conf:        conf,
+		fset:        fset,
+		pkg:         pkg,
+		methods:     make(map[string][]*Func),
+		conversions: make(map[*ast.CallExpr]bool),
+		untyped:     make(map[ast.Expr]exprInfo),
 	}
 }
 
-func isVarOrFunc(obj Object) bool {
-	switch obj.(type) {
-	case *Var, *Func:
-		return true
+// addDeclDep adds the dependency edge (check.decl -> to)
+// if check.decl exists and to has an init expression.
+func (check *checker) addDeclDep(to Object) {
+	from := check.decl
+	if from == nil {
+		return // not in a package-level init expression
 	}
-	return false
-}
-
-// addInitDep adds the dependency edge (to -> check.init)
-// if check.init is a package-level variable or function.
-func (check *checker) addInitDep(to Object) {
-	from := check.init
-	if from == nil || from.Parent() == nil {
-		return // not a package-level variable or function
+	init := check.initMap[to]
+	if init == nil {
+		return // to does not have a package-level init expression
 	}
-
-	if debug {
-		assert(isVarOrFunc(from))
-		assert(isVarOrFunc(to))
-	}
-
-	m := check.dependencies[from] // lazily allocated
+	m := from.deps
 	if m == nil {
-		m = make(map[Object]bool)
-		check.dependencies[from] = m
+		m = make(map[Object]*declInfo)
+		from.deps = m
 	}
-	m[to] = true
+	m[to] = init
 }
 
 func (check *checker) delay(f func()) {

go/types/expr.go

@@ -969,7 +969,8 @@ func (check *checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind {
 		if sig, ok := check.typ(e.Type, nil, false).(*Signature); ok {
 			x.mode = value
 			x.typ = sig
-			check.later(nil, sig, e.Body)
+			// TODO(gri) provide correct *declInfo here
+			check.later(nil, nil, sig, e.Body)
 		} else {
 			check.invalidAST(e.Pos(), "invalid function literal %s", e)
 			goto Error

go/types/resolver.go

@@ -37,31 +37,27 @@ func (check *checker) declare(scope *Scope, id *ast.Ident, obj Object) {
 // A declInfo describes a package-level const, type, var, or func declaration.
 type declInfo struct {
 	file  *Scope        // scope of file containing this declaration
+	lhs   []*Var        // lhs of n:1 variable declarations, or nil
 	typ   ast.Expr      // type, or nil
-	init  ast.Expr      // initialization expression, or nil
-	fdecl *ast.FuncDecl // function declaration, or nil
-}
+	init  ast.Expr      // init expression, or nil
+	fdecl *ast.FuncDecl // func declaration, or nil
 
-// A multiExpr describes the lhs variables and a single but
-// (expected to be) multi-valued rhs init expr of a variable
-// declaration.
-type multiExpr struct {
-	lhs      []*Var
-	rhs      []ast.Expr // len(rhs) == 1
-	ast.Expr            // dummy to satisfy ast.Expr interface
+	deps map[Object]*declInfo // init dependencies; lazily allocated
+	mark byte                 // see check.dependencies
 }
 
 type funcInfo struct {
-	obj  *Func // for debugging/tracing only
+	obj  *Func     // for debugging/tracing only
+	info *declInfo // for cycle detection
 	sig  *Signature
 	body *ast.BlockStmt
 }
 
 // later appends a function with non-empty body to check.funcList.
-func (check *checker) later(f *Func, sig *Signature, body *ast.BlockStmt) {
+func (check *checker) later(f *Func, info *declInfo, sig *Signature, body *ast.BlockStmt) {
 	// functions implemented elsewhere (say in assembly) have no body
 	if !check.conf.IgnoreFuncBodies && body != nil {
-		check.funcList = append(check.funcList, funcInfo{f, sig, body})
+		check.funcList = append(check.funcList, funcInfo{f, info, sig, body})
 	}
 }
 
@@ -107,14 +103,14 @@ func (check *checker) resolveFiles(files []*ast.File) {
 	//          base type names.
 
 	var (
-		fileScope *Scope                      // current file scope
-		objList   []Object                    // list of package-level objects to type-check
-		objMap    = make(map[Object]declInfo) // declaration info for each package-level object
+		objList []Object                     // list of package-level objects, in source order
+		objMap  = make(map[Object]*declInfo) // corresponding declaration info
+		initMap = make(map[Object]*declInfo) // declaration info for variables with initializers
 	)
 
 	// declare declares obj in the package scope, records its ident -> obj mapping,
 	// and updates objList and objMap. The object must not be a function or method.
-	declare := func(ident *ast.Ident, obj Object, typ, init ast.Expr) {
+	declare := func(ident *ast.Ident, obj Object, d *declInfo) {
 		assert(ident.Name == obj.Name())
 
 		// spec: "A package-scope or file-scope identifier with name init
@@ -126,7 +122,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 
 		check.declare(pkg.scope, ident, obj)
 		objList = append(objList, obj)
-		objMap[obj] = declInfo{fileScope, typ, init, nil}
+		objMap[obj] = d
 	}
 
 	importer := check.conf.Import
@@ -145,7 +141,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 		// but there is no corresponding package object.
 		check.recordObject(file.Name, nil)
 
-		fileScope = NewScope(pkg.scope)
+		fileScope := NewScope(pkg.scope)
 		check.recordScope(file, fileScope)
 		fileScopes = append(fileScopes, fileScope)
 		dotImports = append(dotImports, nil) // element (map) is lazily allocated
@@ -253,36 +249,46 @@ func (check *checker) resolveFiles(files []*ast.File) {
 									init = last.Values[i]
 								}
 
-								declare(name, obj, last.Type, init)
+								declare(name, obj, &declInfo{file: fileScope, typ: last.Type, init: init})
 							}
 
 							check.arityMatch(s, last)
 
 						case token.VAR:
-							// declare all variables
 							lhs := make([]*Var, len(s.Names))
+							// If there's exactly one rhs initializer, use
+							// the same declInfo d1 for all lhs variables
+							// so that each lhs variable depends on the same
+							// rhs initializer (n:1 var declaration).
+							var d1 *declInfo
+							if len(s.Values) == 1 {
+								// The lhs elements are only set up after the foor loop below,
+								// but that's ok because declareVar only collects the declInfo
+								// for a later phase.
+								d1 = &declInfo{file: fileScope, lhs: lhs, typ: s.Type, init: s.Values[0]}
+							}
+
+							// declare all variables
 							for i, name := range s.Names {
 								obj := NewVar(name.Pos(), pkg, name.Name, nil)
 								lhs[i] = obj
 
+								d := d1
 								var init ast.Expr
-								switch len(s.Values) {
-								case len(s.Names):
-									// lhs and rhs match
-									init = s.Values[i]
-								case 1:
-									// rhs must be a multi-valued expression
-									// (lhs may not be fully set up yet, but
-									// that's fine because declare simply collects
-									// the information for later processing.)
-									init = &multiExpr{lhs, s.Values, nil}
-								default:
+								if d == nil {
+									// individual assignments
 									if i < len(s.Values) {
 										init = s.Values[i]
 									}
+									d = &declInfo{file: fileScope, typ: s.Type, init: init}
 								}
 
-								declare(name, obj, s.Type, init)
+								declare(name, obj, d)
+
+								// remember obj if it has an initializer
+								if d1 != nil || init != nil {
+									initMap[obj] = d
+								}
 							}
 
 							check.arityMatch(s, nil)
@@ -293,7 +299,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 
 					case *ast.TypeSpec:
 						obj := NewTypeName(s.Name.Pos(), pkg, s.Name.Name, nil)
-						declare(s.Name, obj, s.Type, nil)
+						declare(s.Name, obj, &declInfo{file: fileScope, typ: s.Type})
 
 					default:
 						check.invalidAST(s.Pos(), "unknown ast.Spec node %T", s)
@@ -333,7 +339,12 @@ func (check *checker) resolveFiles(files []*ast.File) {
 					}
 				}
 				objList = append(objList, obj)
-				objMap[obj] = declInfo{fileScope, nil, nil, d}
+				info := &declInfo{file: fileScope, fdecl: d}
+				objMap[obj] = info
+				// remember obj if it has a body (= initializer)
+				if d.Body != nil {
+					initMap[obj] = info
+				}
 
 			default:
 				check.invalidAST(d.Pos(), "unknown ast.Decl node %T", d)
@@ -354,6 +365,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 	// Phase 3: Typecheck all objects in objList, but not function bodies.
 
 	check.objMap = objMap // indicate that we are checking package-level declarations (objects may not have a type yet)
+	check.initMap = initMap
 	for _, obj := range objList {
 		if obj.Type() == nil {
 			check.objDecl(obj, nil, false)
@@ -387,6 +399,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 		check.topScope = f.sig.scope // open function scope
 		check.funcSig = f.sig
 		check.hasLabel = false
+		check.decl = f.info
 		check.stmtList(0, f.body.List)
 
 		if check.hasLabel {
@@ -402,13 +415,17 @@ func (check *checker) resolveFiles(files []*ast.File) {
 	// Note: must happen after checking all functions because function bodies
 	// may introduce dependencies
 
-	state := make(map[Object]uint8)
+	// For source order and reproducible error messages,
+	// iterate through objList rather than initMap.
 	for _, obj := range objList {
-		if isVarOrFunc(obj) && state[obj] == 0 {
-			check.objDependencies(obj, state)
+		if obj, _ := obj.(*Var); obj != nil {
+			if init := initMap[obj]; init != nil {
+				check.dependencies(obj, init)
+			}
 		}
 	}
-	objList = nil // not needed anymore
+	objList = nil       // not needed anymore
+	check.initMap = nil // not needed anymore
 
 	// Phase 6: Check for declared but not used packages and variables.
 	// Note: must happen after checking all functions because closures may affect outer scopes
@@ -473,23 +490,36 @@ func (check *checker) resolveFiles(files []*ast.File) {
 	}
 }
 
-func (check *checker) objDependencies(obj Object, state map[Object]uint8) {
+func (check *checker) dependencies(obj Object, init *declInfo) {
 	const inProgress, done = 1, 2
-	if state[obj] == inProgress {
-		check.errorf(obj.Pos(), "initialization cycle for %s", obj.Name())
-		// TODO(gri) print the cycle
-		state[obj] = done // avoid further errors
+
+	if init.mark == done {
 		return
 	}
 
-	state[obj] = inProgress
-	for dep := range check.dependencies[obj] {
-		check.objDependencies(dep, state)
+	if init.mark == inProgress {
+		// cycle detected: end recursion and report error for variables
+		if obj, _ := obj.(*Var); obj != nil {
+			check.errorf(obj.pos, "initialization cycle for %s", obj.name)
+			// TODO(gri) print the cycle
+			init.mark = done // avoid further errors
+		}
+		return
 	}
-	state[obj] = done
 
-	if v, _ := obj.(*Var); v != nil {
-		check.Info.InitOrder = append(check.Info.InitOrder, v)
+	init.mark = inProgress
+	for obj, dep := range init.deps {
+		check.dependencies(obj, dep)
+	}
+	init.mark = done
+
+	// record the init order for variables
+	if lhs, _ := obj.(*Var); lhs != nil {
+		initLhs := init.lhs // possibly nil (see declInfo.lhs field comment)
+		if initLhs == nil {
+			initLhs = []*Var{lhs}
+		}
+		check.Info.InitOrder = append(check.Info.InitOrder, &Initializer{initLhs, init.init})
 	}
 }
 
@@ -517,19 +547,25 @@ func (check *checker) objDecl(obj Object, def *Named, cycleOk bool) {
 	oldIota := check.iota
 	check.iota = nil
 
+	// save current decl
+	oldDecl := check.decl
+	check.decl = nil
+
 	switch obj := obj.(type) {
 	case *Const:
 		check.constDecl(obj, d.typ, d.init)
 	case *Var:
-		check.varDecl(obj, d.typ, d.init)
+		check.decl = d // new package-level var decl
+		check.varDecl(obj, d.lhs, d.typ, d.init)
 	case *TypeName:
 		check.typeDecl(obj, d.typ, def, cycleOk)
 	case *Func:
-		check.funcDecl(obj, d.fdecl)
+		check.funcDecl(obj, d)
 	default:
 		unreachable()
 	}
 
+	check.decl = oldDecl
 	check.iota = oldIota
 	check.topScope = oldScope
 }
@@ -561,7 +597,8 @@ func (check *checker) constDecl(obj *Const, typ, init ast.Expr) {
 	check.iota = nil
 }
 
-func (check *checker) varDecl(obj *Var, typ, init ast.Expr) {
+// TODO(gri) document arguments
+func (check *checker) varDecl(obj *Var, lhs []*Var, typ, init ast.Expr) {
 	if obj.visited {
 		check.errorf(obj.Pos(), "illegal cycle in initialization of variable %s", obj.name)
 		obj.typ = Typ[Invalid]
@@ -586,19 +623,28 @@ func (check *checker) varDecl(obj *Var, typ, init ast.Expr) {
 		return
 	}
 
-	// save current init object
-	oldInit := check.init
-	check.init = obj
-
-	if m, _ := init.(*multiExpr); m != nil {
-		check.initVars(m.lhs, m.rhs, token.NoPos)
-	} else {
+	if lhs == nil || len(lhs) == 1 {
+		assert(lhs == nil || lhs[0] == obj)
 		var x operand
 		check.expr(&x, init)
 		check.initVar(obj, &x)
+		return
 	}
 
-	check.init = oldInit
+	if debug {
+		// obj must be one of lhs
+		found := false
+		for _, lhs := range lhs {
+			if obj == lhs {
+				found = true
+				break
+			}
+		}
+		if !found {
+			panic("inconsistent lhs")
+		}
+	}
+	check.initVars(lhs, []ast.Expr{init}, token.NoPos)
 }
 
 func (check *checker) typeDecl(obj *TypeName, typ ast.Expr, def *Named, cycleOk bool) {
@@ -696,11 +742,12 @@ func (check *checker) typeDecl(obj *TypeName, typ ast.Expr, def *Named, cycleOk
 	delete(check.methods, obj.name) // we don't need them anymore
 }
 
-func (check *checker) funcDecl(obj *Func, fdecl *ast.FuncDecl) {
+func (check *checker) funcDecl(obj *Func, info *declInfo) {
 	// func declarations cannot use iota
 	assert(check.iota == nil)
 
 	obj.typ = Typ[Invalid] // guard against cycles
+	fdecl := info.fdecl
 	sig := check.funcType(fdecl.Recv, fdecl.Type, nil)
 	if sig.recv == nil && obj.name == "init" && (sig.params.Len() > 0 || sig.results.Len() > 0) {
 		check.errorf(fdecl.Pos(), "func init must have no arguments and no return values")
@@ -708,7 +755,7 @@ func (check *checker) funcDecl(obj *Func, fdecl *ast.FuncDecl) {
 	}
 	obj.typ = sig
 
-	check.later(obj, sig, fdecl.Body)
+	check.later(obj, info, sig, fdecl.Body)
 }
 
 func (check *checker) declStmt(decl ast.Decl) {
@@ -754,15 +801,14 @@ func (check *checker) declStmt(decl ast.Decl) {
 					}
 
 				case token.VAR:
-					// For varDecl called with a multiExpr we need the fully
-					// initialized lhs. Compute it in a separate pre-pass.
-					lhs := make([]*Var, len(s.Names))
+					lhs0 := make([]*Var, len(s.Names))
 					for i, name := range s.Names {
-						lhs[i] = NewVar(name.Pos(), pkg, name.Name, nil)
+						lhs0[i] = NewVar(name.Pos(), pkg, name.Name, nil)
 					}
 
-					// declare all variables
-					for i, obj := range lhs {
+					// initialize all variables
+					for i, obj := range lhs0 {
+						var lhs []*Var
 						var init ast.Expr
 						switch len(s.Values) {
 						case len(s.Names):
@@ -770,20 +816,22 @@ func (check *checker) declStmt(decl ast.Decl) {
 							init = s.Values[i]
 						case 1:
 							// rhs is expected to be a multi-valued expression
-							init = &multiExpr{lhs, s.Values, nil}
+							lhs = lhs0
+							init = s.Values[0]
 						default:
 							if i < len(s.Values) {
 								init = s.Values[i]
 							}
 						}
-
-						check.varDecl(obj, s.Type, init)
+						check.varDecl(obj, lhs, s.Type, init)
 					}
 
 					check.arityMatch(s, nil)
 
+					// declare all variables
+					// (only at this point are the variable scopes (parents) set)
 					for i, name := range s.Names {
-						check.declare(check.topScope, name, lhs[i])
+						check.declare(check.topScope, name, lhs0[i])
 					}
 
 				default:

go/types/testdata/init0.src

@@ -36,7 +36,11 @@ type S1 struct {
 var x3 /* ERROR initialization cycle */ S1 = S1{x3.f}
 
 // cycles via functions
-var x4 = f1() // TODO(gri) recognize cycle
-func f1() int {
-	return x4
-}
+var x4 = x5
+var x5 /* ERROR initialization cycle */ = f1()
+func f1() int { return x5*10 }
+
+var x6, x7 /* ERROR initialization cycle */ = f2()
+var x8 = x7
+func f2() (int, int) { return f3() + f3(), 0 }
+func f3() int { return x8 }

go/types/typexpr.go

@@ -89,15 +89,16 @@ func (check *checker) ident(x *operand, e *ast.Ident, def *Named, cycleOk bool)
 	case *Var:
 		obj.used = true
 		x.mode = variable
-		if obj.parent.parent == Universe && typ != Typ[Invalid] {
-			// obj is a package-level variable, and there are no other errors
-			// (such as a type-checking cycle)
-			check.addInitDep(obj)
+		if typ != Typ[Invalid] {
+			check.addDeclDep(obj)
 		}
 
 	case *Func:
 		obj.used = true
 		x.mode = value
+		if typ != Typ[Invalid] {
+			check.addDeclDep(obj)
+		}
 
 	case *Builtin:
 		obj.used = true // for built-ins defined by package unsafe