go.tools/go/types: use correct outer scope state for inner functions

Inner function bodies must be type-checked "in-place" for
them to see the correct state of the surrounding scopes.

Fixes golang/go#7035.

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

go/types/check_test.go

@@ -77,6 +77,7 @@ var tests = [][]string{
 	{"testdata/stmt1.src"},
 	{"testdata/gotos.src"},
 	{"testdata/labels.src"},
+	{"testdata/issues.src"},
 }
 
 var fset = token.NewFileSet()

go/types/expr.go

@@ -967,13 +967,12 @@ func (check *checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind {
 
 	case *ast.FuncLit:
 		if sig, ok := check.typ(e.Type, nil, false).(*Signature); ok {
-			x.mode = value
-			x.typ = sig
 			// Anonymous functions are considered part of the
 			// init expression/func declaration which contains
-			// them: use the current package-level declaration
+			// them: use existing package-level declaration info.
-			// info.
+			check.funcBody("", sig, e.Body)
-			check.later(nil, check.decl, sig, e.Body)
+			x.mode = value
+			x.typ = sig
 		} else {
 			check.invalidAST(e.Pos(), "invalid function literal %s", e)
 			goto Error

go/types/resolver.go

@@ -49,20 +49,12 @@ type declInfo struct {
 }
 
 type funcInfo struct {
-	obj  *Func     // for debugging/tracing only
+	name string    // for 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, 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, info, sig, body})
-	}
-}
-
 // arityMatch checks that the lhs and rhs of a const or var decl
 // have the appropriate number of names and init exprs. For const
 // decls, init is the value spec providing the init exprs; for
@@ -408,34 +400,9 @@ func (check *checker) resolveFiles(files []*ast.File) {
 
 	// Phase 4: Typecheck all functions bodies.
 
-	// Note: funcList may grow while iterating through it - cannot use range clause.
+	for _, f := range check.funcList {
-	for i := 0; i < len(check.funcList); i++ {
-		// TODO(gri) Factor out this code into a dedicated function
-		// with its own context so that it can be run concurrently
-		// eventually.
-
-		f := check.funcList[i]
-		if trace {
-			s := "<function literal>"
-			if f.obj != nil {
-				s = f.obj.name
-			}
-			fmt.Println("---", s)
-		}
-
-		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)
+		check.funcBody(f.name, f.sig, f.body)
-
-		if check.hasLabel {
-			check.labels(f.body)
-		}
-
-		if f.sig.results.Len() > 0 && !check.isTerminating(f.body, "") {
-			check.errorf(f.body.Rbrace, "missing return")
-		}
 	}
 
 	// Phase 5: Check initialization dependencies.
@@ -455,44 +422,46 @@ func (check *checker) resolveFiles(files []*ast.File) {
 	objList = nil       // not needed anymore
 	check.initMap = nil // not needed anymore
 
-	// Phase 6: Check for declared but not used packages and variables.
+	// Phase 6: Check for declared but not used packages and function variables.
-	// Note: must happen after checking all functions because closures may affect outer scopes
+	// Note: must happen after checking all functions because function bodies
+	// may introduce package uses
+
+	// If function bodies are not checked, packages' uses are likely missing,
+	// and there are no unused function variables. Nothing left to do.
+	if check.conf.IgnoreFuncBodies {
+		return
+	}
 
 	// spec: "It is illegal (...) to directly import a package without referring to
 	// any of its exported identifiers. To import a package solely for its side-effects
 	// (initialization), use the blank identifier as explicit package name."
-
+	for i, scope := range fileScopes {
-	// We must skip this check if we didn't look at function bodies.
+		var usedDotImports map[*Package]bool // lazily allocated
-
+		for _, obj := range scope.elems {
-	if !check.conf.IgnoreFuncBodies {
+			switch obj := obj.(type) {
-		for i, scope := range fileScopes {
+			case *PkgName:
-			var usedDotImports map[*Package]bool // lazily allocated
+				// Unused "blank imports" are automatically ignored
-			for _, obj := range scope.elems {
+				// since _ identifiers are not entered into scopes.
-				switch obj := obj.(type) {
+				if !obj.used {
-				case *PkgName:
+					check.errorf(obj.pos, "%q imported but not used", obj.pkg.path)
-					// Unused "blank imports" are automatically ignored
+				}
-					// since _ identifiers are not entered into scopes.
+			default:
-					if !obj.used {
+				// All other objects in the file scope must be dot-
-						check.errorf(obj.pos, "%q imported but not used", obj.pkg.path)
+				// imported. If an object was used, mark its package
-					}
+				// as used.
-				default:
+				if obj.isUsed() {
-					// All other objects in the file scope must be dot-
+					if usedDotImports == nil {
-					// imported. If an object was used, mark its package
+						usedDotImports = make(map[*Package]bool)
-					// as used.
-					if obj.isUsed() {
-						if usedDotImports == nil {
-							usedDotImports = make(map[*Package]bool)
-						}
-						usedDotImports[obj.Pkg()] = true
 					}
+					usedDotImports[obj.Pkg()] = true
 				}
 			}
-			// Iterate through all dot-imports for this file and
+		}
-			// check if the corresponding package was used.
+		// Iterate through all dot-imports for this file and
-			for pkg, pos := range dotImports[i] {
+		// check if the corresponding package was used.
-				if !usedDotImports[pkg] {
+		for pkg, pos := range dotImports[i] {
-					check.errorf(pos, "%q imported but not used", pkg.path)
+			if !usedDotImports[pkg] {
-				}
+				check.errorf(pos, "%q imported but not used", pkg.path)
 			}
 		}
 	}
@@ -500,6 +469,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 	// Each set of implicitly declared lhs variables in a type switch acts collectively
 	// as a single lhs variable. If any one was 'used', all of them are 'used'. Handle
 	// them before the general analysis.
+	// Note: This check could be done on a per-function basis.
 	for _, vars := range check.lhsVarsList {
 		// len(vars) > 0
 		var used bool
@@ -517,6 +487,7 @@ func (check *checker) resolveFiles(files []*ast.File) {
 
 	// spec: "Implementation restriction: A compiler may make it illegal to
 	// declare a variable inside a function body if the variable is never used."
+	// Note: Inner functions are handled via the child scopes of the enclosing function.
 	for _, f := range check.funcList {
 		check.usage(f.sig.scope)
 	}
@@ -839,7 +810,11 @@ func (check *checker) funcDecl(obj *Func, info *declInfo) {
 	}
 	obj.typ = sig
 
-	check.later(obj, info, sig, fdecl.Body)
+	// function body must be type-checked after global declarations
+	// (functions implemented elsewhere have no body)
+	if !check.conf.IgnoreFuncBodies && fdecl.Body != nil {
+		check.funcList = append(check.funcList, funcInfo{obj.name, info, sig, fdecl.Body})
+	}
 }
 
 func (check *checker) declStmt(decl ast.Decl) {

go/types/stmt.go

@@ -7,10 +7,45 @@
 package types
 
 import (
+	"fmt"
 	"go/ast"
 	"go/token"
 )
 
+func (check *checker) funcBody(name string, sig *Signature, body *ast.BlockStmt) {
+	if trace {
+		if name == "" {
+			name = "<function literal>"
+		}
+		fmt.Printf("--- %s: %s {\n", name, sig)
+		defer fmt.Println("--- <end>")
+	}
+
+	// save/restore outer function state
+	defer func(scope *Scope, sig *Signature, label bool, indent int) {
+		check.topScope = scope
+		check.funcSig = sig
+		check.hasLabel = label
+		check.indent = indent
+	}(check.topScope, check.funcSig, check.hasLabel, check.indent)
+
+	// setup inner function state
+	check.topScope = sig.scope
+	check.funcSig = sig
+	check.hasLabel = false
+	check.indent = 0
+
+	check.stmtList(0, body.List)
+
+	if check.hasLabel {
+		check.labels(body)
+	}
+
+	if sig.results.Len() > 0 && !check.isTerminating(body, "") {
+		check.errorf(body.Rbrace, "missing return")
+	}
+}
+
 // stmtContext is a bitset describing the environment
 // (outer statements) containing a statement.
 type stmtContext uint

go/types/testdata/importdecl0a.src

@@ -27,7 +27,8 @@ import (
 )
 
 import "fmt"
-import f "fmt"
+import f1 "fmt"
+import f2 "fmt"
 
 // reflect.flag must not be visible in this package
 type flag int
@@ -39,5 +40,11 @@ type Value /* ERROR "already declared in this file" */ struct{}
 var _ = fmt.Println // use "fmt"
 
 func _() {
-	f.Println() // use "fmt"
+	f1.Println() // use "fmt"
+}
+
+func _() {
+	_ = func() {
+		f2.Println() // use "fmt"
+	}
 }

go/types/testdata/init0.src

@@ -53,7 +53,7 @@ func f3() int { return x8 }
 
 // cycles via closures
 
-var x9 /* ERROR initialization cycle */ = func() int { return x9 }()
+var x9 /* ERROR illegal cycle */ = func() int { return x9 }()
 
 var x10 /* ERROR initialization cycle */ = f4()
 

go/types/testdata/issues.src

@@ -0,0 +1,16 @@
+// 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.
+
+package issues
+
+import "fmt"
+
+func issue7035() {
+	type T struct{ X int }
+	_ = func() {
+		fmt.Println() // must refer to imported fmt rather than the fmt below
+	}
+	fmt := new(T)
+	_ = fmt.X
+}