From 4e620158a20b8f8d69d1fdaa9da4ec21de677fed Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 9 Jan 2014 08:40:32 -0800 Subject: [PATCH] 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 | 1 + go/types/expr.go | 9 ++- go/types/resolver.go | 111 +++++++++++------------------ go/types/stmt.go | 35 +++++++++ go/types/testdata/importdecl0a.src | 11 ++- go/types/testdata/init0.src | 2 +- go/types/testdata/issues.src | 16 +++++ 7 files changed, 109 insertions(+), 76 deletions(-) create mode 100644 go/types/testdata/issues.src diff --git a/go/types/check_test.go b/go/types/check_test.go index 3c5f6967..0d0afce8 100644 --- a/go/types/check_test.go +++ b/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() diff --git a/go/types/expr.go b/go/types/expr.go index af96bbdf..920b1f13 100644 --- a/go/types/expr.go +++ b/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 - // info. - check.later(nil, check.decl, sig, e.Body) + // them: use existing package-level declaration info. + check.funcBody("", sig, e.Body) + x.mode = value + x.typ = sig } else { check.invalidAST(e.Pos(), "invalid function literal %s", e) goto Error diff --git a/go/types/resolver.go b/go/types/resolver.go index dc6c15a0..f4d215d4 100644 --- a/go/types/resolver.go +++ b/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 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 := "" - 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 + for _, f := range check.funcList { check.decl = f.info - check.stmtList(0, f.body.List) - - if check.hasLabel { - check.labels(f.body) - } - - if f.sig.results.Len() > 0 && !check.isTerminating(f.body, "") { - check.errorf(f.body.Rbrace, "missing return") - } + check.funcBody(f.name, f.sig, f.body) } // 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. - // Note: must happen after checking all functions because closures may affect outer scopes + // Phase 6: Check for declared but not used packages and function variables. + // 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." - - // We must skip this check if we didn't look at function bodies. - - if !check.conf.IgnoreFuncBodies { - for i, scope := range fileScopes { - var usedDotImports map[*Package]bool // lazily allocated - for _, obj := range scope.elems { - switch obj := obj.(type) { - case *PkgName: - // Unused "blank imports" are automatically ignored - // since _ identifiers are not entered into scopes. - if !obj.used { - check.errorf(obj.pos, "%q imported but not used", obj.pkg.path) - } - default: - // All other objects in the file scope must be dot- - // imported. If an object was used, mark its package - // as used. - if obj.isUsed() { - if usedDotImports == nil { - usedDotImports = make(map[*Package]bool) - } - usedDotImports[obj.Pkg()] = true + for i, scope := range fileScopes { + var usedDotImports map[*Package]bool // lazily allocated + for _, obj := range scope.elems { + switch obj := obj.(type) { + case *PkgName: + // Unused "blank imports" are automatically ignored + // since _ identifiers are not entered into scopes. + if !obj.used { + check.errorf(obj.pos, "%q imported but not used", obj.pkg.path) + } + default: + // All other objects in the file scope must be dot- + // imported. If an object was used, mark its package + // as used. + if obj.isUsed() { + if usedDotImports == nil { + usedDotImports = make(map[*Package]bool) } + usedDotImports[obj.Pkg()] = true } } - // Iterate through all dot-imports for this file and - // check if the corresponding package was used. - for pkg, pos := range dotImports[i] { - if !usedDotImports[pkg] { - check.errorf(pos, "%q imported but not used", pkg.path) - } + } + // Iterate through all dot-imports for this file and + // check if the corresponding package was used. + for pkg, pos := range dotImports[i] { + 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) { diff --git a/go/types/stmt.go b/go/types/stmt.go index 5ef682b1..6601393e 100644 --- a/go/types/stmt.go +++ b/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 = "" + } + fmt.Printf("--- %s: %s {\n", name, sig) + defer fmt.Println("--- ") + } + + // 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 diff --git a/go/types/testdata/importdecl0a.src b/go/types/testdata/importdecl0a.src index c6b82b7e..49a35fec 100644 --- a/go/types/testdata/importdecl0a.src +++ b/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" + } } diff --git a/go/types/testdata/init0.src b/go/types/testdata/init0.src index b6c0a72b..9fe0571e 100644 --- a/go/types/testdata/init0.src +++ b/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() diff --git a/go/types/testdata/issues.src b/go/types/testdata/issues.src new file mode 100644 index 00000000..abcd11e1 --- /dev/null +++ b/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 +}