go.tools/ssa: fix a package-level var initialization order bug.

buildDecl was visiting all decls in source order, but the spec
calls for visiting all vars and init() funcs in order, then
all remaining functions.  These two passes are now called
buildInit(), buildFuncDecl().

+ Test.

Also:
- Added workaround to gcimporter for Func with pkg==nil.
- Prog.concreteMethods has been merged into Pkg.values.
- Prog.concreteMethod() renamed declaredFunc().
- s/mfunc/obj/ (name cleanup from recent gri CL)

R=gri
CC=golang-dev
https://golang.org/cl/12030044

go/types/gcimporter.go

@@ -611,6 +611,10 @@ func (p *gcParser) parseInterfaceType() Type {
 		// TODO(gri) Ideally, we should use a named type here instead of
 		// typ, for less verbose printing of interface method signatures.
 		sig.recv = NewVar(token.NoPos, pkg, "", typ)
+		// TODO(gri): fix: pkg may be nil!
+		if pkg == nil {
+			pkg = p.imports[p.id]
+		}
 		m := NewFunc(token.NoPos, pkg, name, sig)
 		if alt := mset.insert(m); alt != nil {
 			p.errorf("multiple methods named %s.%s", alt.Pkg().name, alt.Name())
@@ -906,6 +910,10 @@ func (p *gcParser) parseMethodDecl() {
 	// and method exists already
 	// TODO(gri) This is a quadratic algorithm - ok for now because method counts are small.
 	if _, m := lookupMethod(base.methods, pkg, name); m == nil {
+		// TODO(gri): fix: pkg may be nil.
+		if pkg == nil {
+			pkg = p.imports[p.id]
+		}
 		base.methods = append(base.methods, NewFunc(token.NoPos, pkg, name, sig))
 	}
 }

ssa/builder.go

@@ -646,10 +646,6 @@ func (b *builder) expr(fn *Function, e ast.Expr) Value {
 			c.setType(fn.Pkg.typeOf(e))
 			return fn.emit(c)
 
-			// TODO(adonovan): add more tests for
-			// interaction of bound interface method
-			// closures and promotion.
-
 		case types.FieldVal:
 			indices := sel.Index()
 			last := len(indices) - 1
@@ -803,7 +799,7 @@ func (b *builder) setCallFunc(fn *Function, e *ast.CallExpr, c *CallCommon) {
 				c.Method = obj
 			} else {
 				// "Call"-mode call.
-				c.Value = fn.Prog.concreteMethod(obj)
+				c.Value = fn.Prog.declaredFunc(obj)
 				c.Args = append(c.Args, v)
 			}
 			return
@@ -2175,48 +2171,26 @@ func (b *builder) buildFunction(fn *Function) {
 	fn.finishBody()
 }
 
-// buildDecl builds SSA code for all globals, functions or methods
+// buildInit emits to init any initialization code needed for
-// declared by decl in package pkg.
+// declaration decl, causing SSA-building of any functions or methods
+// it references transitively.
 //
-func (b *builder) buildDecl(pkg *Package, decl ast.Decl) {
+func (b *builder) buildInit(init *Function, decl ast.Decl) {
 	switch decl := decl.(type) {
 	case *ast.GenDecl:
-		switch decl.Tok {
+		if decl.Tok == token.VAR {
-		// Nothing to do for CONST, IMPORT.
-		case token.VAR:
 			for _, spec := range decl.Specs {
-				b.globalValueSpec(pkg.init, spec.(*ast.ValueSpec), nil, nil)
+				b.globalValueSpec(init, spec.(*ast.ValueSpec), nil, nil)
-			}
-		case token.TYPE:
-			for _, spec := range decl.Specs {
-				id := spec.(*ast.TypeSpec).Name
-				if isBlankIdent(id) {
-					continue
-				}
-				nt := pkg.objectOf(id).Type().(*types.Named)
-				for i, n := 0, nt.NumMethods(); i < n; i++ {
-					b.buildFunction(pkg.Prog.concreteMethod(nt.Method(i)))
-				}
 			}
 		}
 
 	case *ast.FuncDecl:
-		id := decl.Name
+		if decl.Recv == nil && decl.Name.Name == "init" {
-		if decl.Recv != nil {
-			return // method declaration
-		}
-		if isBlankIdent(id) {
-			// no-op
-
-			// TODO(adonovan): test: can references within
-			// the blank functions' body affect the program?
-
-		} else if id.Name == "init" {
 			// init() block
-			if pkg.Prog.mode&LogSource != 0 {
+			if init.Prog.mode&LogSource != 0 {
-				fmt.Fprintln(os.Stderr, "build init block @", pkg.Prog.Fset.Position(decl.Pos()))
+				fmt.Fprintln(os.Stderr, "build init block @",
+					init.Prog.Fset.Position(decl.Pos()))
 			}
-			init := pkg.init
 
 			// A return statement within an init block is
 			// treated like a "goto" to the the next init
@@ -2234,14 +2208,25 @@ func (b *builder) buildDecl(pkg *Package, decl ast.Decl) {
 			emitJump(init, next)
 			init.targets = init.targets.tail
 			init.currentBlock = next
+		}
+	}
+}
 
-		} else {
+// buildFuncDecl builds SSA code for the function or method declared
-			// Package-level function.
+// by decl in package pkg.
+//
+func (b *builder) buildFuncDecl(pkg *Package, decl *ast.FuncDecl) {
+	id := decl.Name
+	if isBlankIdent(id) {
+		// TODO(gri): workaround for missing object,
+		// see e.g. $GOROOT/test/blank.go:27.
+		return
+	}
+	if decl.Recv == nil && id.Name == "init" {
+		return // init() block: already done in pass 1
+	}
 	b.buildFunction(pkg.values[pkg.objectOf(id)].(*Function))
 }
-	}
-
-}
 
 // BuildAll calls Package.Build() for each package in prog.
 // Building occurs in parallel unless the BuildSerially mode flag was set.
@@ -2303,28 +2288,35 @@ func (p *Package) Build() {
 		nTo1Vars: make(map[*ast.ValueSpec]bool),
 	}
 
-	// Visit the package's var decls and init funcs in source
+	// Pass 1: visit the package's var decls and init funcs in
-	// order.  This causes init() code to be generated in
+	// source order, causing init() code to be generated in
-	// topological order.  We visit them transitively through
+	// topological order.  We visit package-level vars
-	// functions of the same package, but we don't treat functions
+	// transitively through functions and methods, building them
-	// as roots.
+	// as we go.
-	//
-	// We also ensure all functions and methods are built, even if
-	// they are unreachable.
 	for _, file := range p.info.Files {
 		for _, decl := range file.Decls {
-			b.buildDecl(p, decl)
+			b.buildInit(init, decl)
 		}
 	}
 
-	p.info = nil // We no longer need ASTs or go/types deductions.
+	// Finish up init().
-
-	// Finish up.
 	emitJump(init, done)
 	init.currentBlock = done
 	init.emit(new(RunDefers))
 	init.emit(new(Ret))
 	init.finishBody()
+
+	// Pass 2: build all remaining package-level functions and
+	// methods in source order, including unreachable/blank ones.
+	for _, file := range p.info.Files {
+		for _, decl := range file.Decls {
+			if decl, ok := decl.(*ast.FuncDecl); ok {
+				b.buildFuncDecl(p, decl)
+			}
+		}
+	}
+
+	p.info = nil // We no longer need ASTs or go/types deductions.
 }
 
 // Only valid during p's create and build phases.

ssa/create.go

@@ -40,7 +40,6 @@ func NewProgram(fset *token.FileSet, mode BuilderMode) *Program {
 		PackagesByPath:      make(map[string]*Package),
 		packages:            make(map[*types.Package]*Package),
 		builtins:            make(map[types.Object]*Builtin),
-		concreteMethods:     make(map[*types.Func]*Function),
 		boundMethodWrappers: make(map[*types.Func]*Function),
 		ifaceMethodWrappers: make(map[*types.Func]*Function),
 		mode:                mode,
@@ -101,25 +100,19 @@ func memberFromObject(pkg *Package, obj types.Object, syntax ast.Node) {
 				body:      decl.Body,
 			}
 		}
-		sig := obj.Type().(*types.Signature)
 		fn := &Function{
 			name:      name,
 			object:    obj,
-			Signature: sig,
+			Signature: obj.Type().(*types.Signature),
 			Synthetic: synthetic,
 			pos:       obj.Pos(), // (iff syntax)
 			Pkg:       pkg,
 			Prog:      pkg.Prog,
 			syntax:    fs,
 		}
-		if recv := sig.Recv(); recv == nil {
-			// Function declaration.
 		pkg.values[obj] = fn
-			pkg.Members[name] = fn
+		if fn.Signature.Recv() == nil {
-		} else {
+			pkg.Members[name] = fn // package-level function
-			// Concrete method.
-			_ = deref(recv.Type()).(*types.Named) // assertion
-			pkg.Prog.concreteMethods[obj] = fn
 		}
 
 	default: // (incl. *types.Package)

ssa/interp/interp_test.go

@@ -134,6 +134,7 @@ var testdataTests = []string{
 	"fieldprom.go",
 	"ifaceconv.go",
 	"ifaceprom.go",
+	"initorder.go",
 	"methprom.go",
 	"mrvchain.go",
 }

ssa/interp/testdata/ifaceprom.go

@@ -1,7 +1,7 @@
 package main
 
 // Test of promotion of methods of an interface embedded within a
-// struct.  In particular, this test excercises that the correct
+// struct.  In particular, this test exercises that the correct
 // method is called.
 
 type I interface {

ssa/interp/testdata/initorder.go

@@ -0,0 +1,55 @@
+package main
+
+// Test of initialization order of package-level vars.
+
+type T int
+
+var counter int
+
+func next() int {
+	c := counter
+	counter++
+	return c
+}
+
+func (T) next() int {
+	return next()
+}
+
+var t T
+
+func makeOrder1() [6]int {
+	return [6]int{f1, b1, d1, e1, c1, a1}
+}
+
+func makeOrder2() [6]int {
+	return [6]int{f2, b2, d2, e2, c2, a2}
+}
+
+var order1 = makeOrder1()
+
+func main() {
+	// order1 is a package-level variable:
+	// [a-f]1 are initialized is reference order.
+	if order1 != [6]int{0, 1, 2, 3, 4, 5} {
+		panic(order1)
+	}
+
+	// order2 is a local variable:
+	// [a-f]2 are initialized in lexical order.
+	var order2 = makeOrder2()
+	if order2 != [6]int{11, 7, 9, 10, 8, 6} {
+		panic(order2)
+	}
+}
+
+// The references traversal visits through calls to package-level
+// functions (next), method expressions (T.next) and methods (t.next).
+
+var a1, b1 = next(), next()
+var c1, d1 = T.next(0), T.next(0)
+var e1, f1 = t.next(), t.next()
+
+var a2, b2 = next(), next()
+var c2, d2 = T.next(0), T.next(0)
+var e2, f2 = t.next(), t.next()

ssa/promote.go

@@ -118,16 +118,14 @@ func (prog *Program) LookupMethod(meth *types.Selection) *Function {
 	return prog.populateMethodSet(meth.Recv(), meth)[meth.Obj().Id()]
 }
 
-// concreteMethod returns the concrete method denoted by obj.
+// declaredFunc returns the concrete function/method denoted by obj.
-// Panic ensues if there is no such method (e.g. it's a standalone
+// Panic ensues if there is none.
-// function).
 //
-func (prog *Program) concreteMethod(obj *types.Func) *Function {
+func (prog *Program) declaredFunc(obj *types.Func) *Function {
-	fn := prog.concreteMethods[obj]
+	if v := prog.packageLevelValue(obj); v != nil {
-	if fn == nil {
+		return v.(*Function)
-		panic("no concrete method: " + obj.String())
 	}
-	return fn
+	panic("no concrete method: " + obj.String())
 }
 
 // findMethod returns the concrete Function for the method meth,
@@ -137,18 +135,18 @@ func (prog *Program) concreteMethod(obj *types.Func) *Function {
 //
 func findMethod(prog *Program, meth *types.Selection) *Function {
 	needsPromotion := len(meth.Index()) > 1
-	mfunc := meth.Obj().(*types.Func)
+	obj := meth.Obj().(*types.Func)
-	needsIndirection := !isPointer(recvType(mfunc)) && isPointer(meth.Recv())
+	needsIndirection := !isPointer(recvType(obj)) && isPointer(meth.Recv())
 
 	if needsPromotion || needsIndirection {
 		return makeWrapper(prog, meth.Recv(), meth)
 	}
 
 	if _, ok := meth.Recv().Underlying().(*types.Interface); ok {
-		return interfaceMethodWrapper(prog, meth.Recv(), mfunc)
+		return interfaceMethodWrapper(prog, meth.Recv(), obj)
 	}
 
-	return prog.concreteMethod(mfunc)
+	return prog.declaredFunc(obj)
 }
 
 // makeWrapper returns a synthetic wrapper Function that optionally
@@ -173,21 +171,21 @@ func findMethod(prog *Program, meth *types.Selection) *Function {
 // EXCLUSIVE_LOCKS_REQUIRED(prog.methodsMu)
 //
 func makeWrapper(prog *Program, typ types.Type, meth *types.Selection) *Function {
-	mfunc := meth.Obj().(*types.Func)
+	obj := meth.Obj().(*types.Func)
-	old := mfunc.Type().(*types.Signature)
+	old := obj.Type().(*types.Signature)
 	sig := types.NewSignature(nil, types.NewVar(token.NoPos, nil, "recv", typ), old.Params(), old.Results(), old.IsVariadic())
 
-	description := fmt.Sprintf("wrapper for %s", mfunc)
+	description := fmt.Sprintf("wrapper for %s", obj)
 	if prog.mode&LogSource != 0 {
 		defer logStack("make %s to (%s)", description, typ)()
 	}
 	fn := &Function{
-		name:      mfunc.Name(),
+		name:      obj.Name(),
 		method:    meth,
 		Signature: sig,
 		Synthetic: description,
 		Prog:      prog,
-		pos:       mfunc.Pos(),
+		pos:       obj.Pos(),
 	}
 	fn.startBody()
 	fn.addSpilledParam(sig.Recv())
@@ -220,10 +218,10 @@ func makeWrapper(prog *Program, typ types.Type, meth *types.Selection) *Function
 		if !isPointer(old.Recv().Type()) {
 			v = emitLoad(fn, v)
 		}
-		c.Call.Value = prog.concreteMethod(mfunc)
+		c.Call.Value = prog.declaredFunc(obj)
 		c.Call.Args = append(c.Call.Args, v)
 	} else {
-		c.Call.Method = mfunc
+		c.Call.Method = obj
 		c.Call.Value = emitLoad(fn, v)
 	}
 	for _, arg := range fn.Params[1:] {
@@ -357,7 +355,7 @@ func boundMethodWrapper(prog *Program, obj *types.Func) *Function {
 		var c Call
 
 		if _, ok := recvType(obj).Underlying().(*types.Interface); !ok { // concrete
-			c.Call.Value = prog.concreteMethod(obj)
+			c.Call.Value = prog.declaredFunc(obj)
 			c.Call.Args = []Value{cap}
 		} else {
 			c.Call.Value = cap

ssa/source.go

@@ -220,14 +220,10 @@ func (prog *Program) FuncValue(obj *types.Func) Value {
 	if v, ok := prog.builtins[obj]; ok {
 		return v
 	}
-	// Package-level function?
+	// Package-level function or declared method?
 	if v := prog.packageLevelValue(obj); v != nil {
 		return v
 	}
-	// Concrete method?
-	if v := prog.concreteMethods[obj]; v != nil {
-		return v
-	}
 	// TODO(adonovan): interface method wrappers?  other wrappers?
 	return nil
 }

ssa/ssa.go

@@ -22,7 +22,6 @@ type Program struct {
 	PackagesByPath map[string]*Package         // all loaded Packages, keyed by import path
 	packages       map[*types.Package]*Package // all loaded Packages, keyed by object
 	builtins       map[types.Object]*Builtin   // all built-in functions, keyed by typechecker objects.
-	concreteMethods map[*types.Func]*Function   // maps declared concrete methods to their code
 	mode           BuilderMode                 // set of mode bits for SSA construction
 
 	methodsMu           sync.Mutex                // guards the following maps:
@@ -40,7 +39,7 @@ type Package struct {
 	Prog    *Program               // the owning program
 	Object  *types.Package         // the type checker's package object for this package
 	Members map[string]Member      // all package members keyed by name
-	values  map[types.Object]Value // package-level vars and funcs, keyed by object
+	values  map[types.Object]Value // package-level vars & funcs (incl. methods), keyed by object
 	init    *Function              // Func("init"); the package's (concatenated) init function
 
 	// The following fields are set transiently, then cleared