go/types: correctly determine if panic call refers to built-in

R=go1.11

The terminating statement check for functions that declare result
parameters was using the wrong scope to look up calls to `panic`
which in esoteric cases lead to a false positive.

Instead of looking up a panic call again at a time when correct
scope information would have to be recomputed, collect calls to
predeclared panic in a set when type-checking that call.

Fixes #23218.

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

diff --git a/src/go/types/builtins.go b/src/go/types/builtins.go
index 66548231fe45a8fa08e8984afa3195f00d34761e..f22851e240ecef431c09624d0e80c683c9fe31f1 100644 (file)
--- a/src/go/types/builtins.go
+++ b/src/go/types/builtins.go
@@ -470,6 +470,19 @@ func (check *Checker) builtin(x *operand, call *ast.CallExpr, id builtinId) (_ b
 
        case _Panic:
                // panic(x)
+               // record panic call if inside a function with result parameters
+               // (for use in Checker.isTerminating)
+               if check.sig.results.Len() > 0 {
+                       // function has result parameters
+                       p := check.isPanic
+                       if p == nil {
+                               // allocate lazily
+                               p = make(map[*ast.CallExpr]bool)
+                               check.isPanic = p
+                       }
+                       p[call] = true
+               }
+
                check.assignment(x, &emptyInterface, "argument to panic")
                if x.mode == invalid {
                        return

diff --git a/src/go/types/check.go b/src/go/types/check.go
index aa0fd123e6bbb71457e134c8d44429aad73696ab..d1b7155cf594e89fc0f170504637b15c68816717 100644 (file)
--- a/src/go/types/check.go
+++ b/src/go/types/check.go
@@ -41,13 +41,14 @@ type exprInfo struct {
 
 // A context represents the context within which an object is type-checked.
 type context struct {
-       decl          *declInfo      // package-level declaration whose init expression/function body is checked
-       scope         *Scope         // top-most scope for lookups
-       pos           token.Pos      // if valid, identifiers are looked up as if at position pos (used by Eval)
-       iota          constant.Value // value of iota in a constant declaration; nil otherwise
-       sig           *Signature     // function signature if inside a function; nil otherwise
-       hasLabel      bool           // set if a function makes use of labels (only ~1% of functions); unused outside functions
-       hasCallOrRecv bool           // set if an expression contains a function call or channel receive operation
+       decl          *declInfo              // package-level declaration whose init expression/function body is checked
+       scope         *Scope                 // top-most scope for lookups
+       pos           token.Pos              // if valid, identifiers are looked up as if at position pos (used by Eval)
+       iota          constant.Value         // value of iota in a constant declaration; nil otherwise
+       sig           *Signature             // function signature if inside a function; nil otherwise
+       isPanic       map[*ast.CallExpr]bool // set of panic call expressions (used for termination check)
+       hasLabel      bool                   // set if a function makes use of labels (only ~1% of functions); unused outside functions
+       hasCallOrRecv bool                   // set if an expression contains a function call or channel receive operation
 }
 
 // lookup looks up name in the current context and returns the matching object, or nil.

diff --git a/src/go/types/return.go b/src/go/types/return.go
index 0c1447f89b3d9d66e18083af9cc668c0cc4c268e..2d34a70b98ba6bf429ee71c413b444deae14049e 100644 (file)
--- a/src/go/types/return.go
+++ b/src/go/types/return.go
@@ -28,15 +28,9 @@ func (check *Checker) isTerminating(s ast.Stmt, label string) bool {
                return check.isTerminating(s.Stmt, s.Label.Name)
 
        case *ast.ExprStmt:
-               // the predeclared (possibly parenthesized) panic() function is terminating
-               if call, _ := unparen(s.X).(*ast.CallExpr); call != nil {
-                       if id, _ := call.Fun.(*ast.Ident); id != nil {
-                               if _, obj := check.scope.LookupParent(id.Name, token.NoPos); obj != nil {
-                                       if b, _ := obj.(*Builtin); b != nil && b.id == _Panic {
-                                               return true
-                                       }
-                               }
-                       }
+               // calling the predeclared (possibly parenthesized) panic() function is terminating
+               if call, ok := unparen(s.X).(*ast.CallExpr); ok && check.isPanic[call] {
+                       return true
                }
 
        case *ast.ReturnStmt:

diff --git a/src/go/types/testdata/stmt1.src b/src/go/types/testdata/stmt1.src
index 24ad6ebdf18633e6f819fa1bbd6eb1544eef78d2..f79f92058bb6725befc51bd0854ad3b03f3b9c20 100644 (file)
--- a/src/go/types/testdata/stmt1.src
+++ b/src/go/types/testdata/stmt1.src
@@ -239,3 +239,21 @@ L: select {
        }
        ; ; ;
 } /* ERROR "missing return" */
+
+func parenPanic() int {
+       ((((((panic)))(0))))
+}
+
+func issue23218a() int {
+       {
+               panic := func(interface{}){}
+               panic(0)
+       }
+} /* ERROR "missing return" */
+
+func issue23218b() int {
+       {
+               panic := func(interface{}){}
+               ((((panic))))(0)
+       }
+} /* ERROR "missing return" */