[inferpython] fixing bug with WITH_CLEANUP_START/POP_FINALLY

Summary:
this an old bug. The answer is in the C implementation of CPython,
much clearer that the spec. Unfortunatly This fix reveals an other problem.
See PyIRTestException.ml. I will fix that in an other diff.

Reviewed By: nbenton

Differential Revision: D63840270

fbshipit-source-id: fe6538d8f3c6607c6210b841e1e4d2fe519a7ddd

infer/src/python/PyIR.ml

@@ -427,6 +427,7 @@ module Error = struct
     | CallKeywordNotString0 of Const.t
     | CallKeywordNotString1 of Exp.t
     | MakeFunctionInvalidDefaults of Exp.t
+    | WithCleanupStart of Exp.t
     | WithCleanupFinish of Exp.t
     | RaiseExceptionInvalid of int
 
@@ -466,6 +467,8 @@ module Error = struct
         F.fprintf fmt "CALL_FUNCTION_KW: keyword is not a tuple of strings: %a" Exp.pp exp
     | MakeFunctionInvalidDefaults exp ->
         F.fprintf fmt "MAKE_FUNCTION: expecting tuple of default values but got %a" Exp.pp exp
+    | WithCleanupStart exp ->
+        F.fprintf fmt "WITH_CLEANUP_START/TODO: unsupported scenario with %a" Exp.pp exp
     | WithCleanupFinish exp ->
         F.fprintf fmt "WITH_CLEANUP_FINISH/TODO: unsupported scenario with %a" Exp.pp exp
     | RaiseExceptionInvalid n ->
@@ -571,6 +574,10 @@ module Stack = struct
         None
     | elt :: rest ->
         if Int.equal 0 n then Some elt else peek (n - 1) rest
+
+
+  let pp ~pp fmt list = F.fprintf fmt "[%a]" (Pp.semicolon_seq pp) (List.rev list)
+  [@@warning "-unused-value-declaration"]
 end
 
 module Terminator = struct
@@ -1219,34 +1226,32 @@ let for_iter st delta next_offset_opt =
            ; else_= Terminator.mk_jump other_label other_stack } ) )
 
 
-(** Extract from Python3.8 specification:
-
-    Starts cleaning up the stack when a with statement block exits. At the top of the stack are
-    either [NULL] (pushed by [BEGIN_FINALLY]) or 6 values pushed if an exception has been raised in
-    the with block. Below is the context manager’s [__exit__()] or [__aexit__()] bound method.
-
-    If top-of-stack is [NULL], calls [SECOND(None, None, None)], removes the function from the
-    stack, leaving top-of-stack, and pushes [NULL] to the stack. Otherwise calls
-    [SEVENTH(TOP, SECOND, THIRD)], shifts the bottom 3 values of the stack down, replaces the empty
-    spot with [NULL] and pushes top-of-stack. Finally pushes the result of the call.
-
-    vsiles notes:
-
-    - the doc is quite confusing. Here is my source of truth
-      https://github.com/python/cpython/blob/3.8/Python/ceval.c#L3300
-    - we only support the first case with [NULL]
-    - we only support [__exit__].
-    - TODO: learn how [__aexit__] works
-    - TODO: duplicate the handler to deal with the except case *)
+(* See https://github.com/python/cpython/blob/3.8/Python/ceval.c#L3300 *)
 let with_cleanup_start st =
   let open IResult.Let_syntax in
+  let* tos, st = State.pop st in
+  let* () =
+    match (tos : Exp.t) with
+    | Const PYCNone ->
+        Ok ()
+    | _ ->
+        internal_error st (Error.WithCleanupStart tos)
+  in
   let* context_manager_exit, st = State.pop st in
-  let* lhs, st =
-    call_function_with_unnamed_args st context_manager_exit
+  let* context_manager =
+    match context_manager_exit with
+    | Exp.ContextManagerExit exp ->
+        Ok exp
+    | exp ->
+        internal_error st (Error.WithCleanupStart exp)
+  in
+  let* id, st =
+    call_method st PyCommon.enter context_manager
       [Const Const.none; Const Const.none; Const Const.none]
   in
   let st = State.push st (Const Const.none) in
-  let st = State.push st (Exp.Temp lhs) in
+  let st = State.push st (Const Const.none) in
+  let st = State.push st (Temp id) in
   Ok (st, None)
 
 
@@ -1721,16 +1726,17 @@ let parse_bytecode st ({FFI.Code.co_consts; co_names; co_varnames} as code)
       let {State.stack} = st in
       Ok (st, Some (Terminator.mk_jump label stack))
   | "POP_FINALLY" ->
-      let* return_value, st =
+      (* see https://github.com/python/cpython/blob/3.8/Python/ceval.c#L2129 *)
+      let* st =
         if arg <> 0 then
           let* ret, st = State.pop st in
-          Ok (Some ret, st)
-        else Ok (None, st)
+          let* _, st = State.pop st in
+          let st = State.push st ret in
+          Ok st
+        else
+          let* _, st = State.pop st in
+          Ok st
       in
-      (* MUST BE FINALLY BLOCK *)
-      (* TODO: insert popping of tos when CALL_FINALLY is supported. Right now
-         we didn't push anything on the stack so we don't pop anything *)
-      let st = match return_value with Some ret -> State.push st ret | None -> st in
       Ok (st, None)
   | "WITH_CLEANUP_START" ->
       with_cleanup_start st

infer/src/python/unit/PyIRTest.ml

@@ -406,21 +406,21 @@ def f(x, y, l, bar, toto):
           if n11 then jmp b3 else jmp b4
 
         b3:
-          n16 <- PYCNone(PYCNone, PYCNone, PYCNone)
-          n17 <- PYCNone(PYCNone, PYCNone, PYCNone)
-          jmp b1(n2, CM(n6).__exit__, CM(n9).__exit__)
+          n16 <- n9.__enter__(PYCNone, PYCNone, PYCNone)
+          n17 <- n6.__enter__(PYCNone, PYCNone, PYCNone)
+          jmp b1(n2)
 
         b4:
           n12 <- GLOBAL[print]
           n13 <- n12(PYCString ("nop"))
           jmp b5
 
         b5:
-          n14 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n14 <- n9.__enter__(PYCNone, PYCNone, PYCNone)
           jmp b6
 
         b6:
-          n15 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n15 <- n6.__enter__(PYCNone, PYCNone, PYCNone)
           jmp b1(n2)
 
         b7:
@@ -864,11 +864,11 @@ def f(foo, bar):
           jmp b1
 
         b1:
-          n9 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n9 <- n4.__enter__(PYCNone, PYCNone, PYCNone)
           n10 <- GLOBAL[print]
           n11 <- LOCAL[foo0]
           n12 <- n10(n11)
-          n13 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n13 <- n1.__enter__(PYCNone, PYCNone, PYCNone)
           return PYCInt (42) |}]
 
 
@@ -1593,5 +1593,43 @@ async def foo():
 |}
   in
   PyIR.test source ;
-  [%expect {|
-    IR error: UNEXPECTED_EXPRESSION: CM(n8).__exit__ |}]
+  [%expect
+    {|
+    module dummy:
+
+      toplevel:
+        b0:
+          TOPLEVEL[foo] <- $FuncObj(foo, dummy.foo, {})
+          return PYCNone
+
+
+      dummy.foo:
+        b0:
+          n0 <- GLOBAL[range]
+          n1 <- GLOBAL[num]
+          n2 <- n0(n1)
+          n3 <- $GetIter(n2)
+          jmp b1
+
+        b1:
+          n4 <- $NextIter(n3)
+          n5 <- $HasNextIter(n3)
+          if n5 then jmp b2 else jmp b4
+
+        b2:
+          LOCAL[i] <- n4
+          n6 <- GLOBAL[read]
+          n7 <- n6()
+          n8 <- $GetAwaitable(n7)
+          n9 <- $YieldFrom(n8, PYCNone)
+          n10 <- n8.__enter__()
+          n11 <- $GetAwaitable(n10)
+          n12 <- $YieldFrom(n11, PYCNone)
+          LOCAL[f] <- n11
+          n13 <- n8.__enter__(PYCNone, PYCNone, PYCNone)
+          n14 <- $GetAwaitable(n13)
+          n15 <- $YieldFrom(n14, PYCNone)
+          return PYCNone
+
+        b4:
+          return PYCNone |}]

infer/src/python/unit/PyIRTestBasic.ml

@@ -338,7 +338,7 @@ with open("foo.txt", "wt") as fp:
           jmp b1
 
         b1:
-          n5 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n5 <- n1.__enter__(PYCNone, PYCNone, PYCNone)
           return PYCNone |}]
 
 

infer/src/python/unit/PyIRTestException.ml

@@ -214,45 +214,8 @@ with open("foo", "r") as fp:
         |}
   in
   PyIR.test source ;
-  [%expect
-    {|
-    module dummy:
-
-      toplevel:
-        b0:
-          n0 <- $ImportName(foo)(PYCTuple ([|PYCString ("ERROR")|]), PYCInt (0))
-          n1 <- $ImportFrom(ERROR)(n0)
-          TOPLEVEL[ERROR] <- n1
-          n2 <- TOPLEVEL[open]
-          n3 <- n2(PYCString ("foo"), PYCString ("r"))
-          n4 <- n3.__enter__()
-          TOPLEVEL[fp] <- n4
-          n5 <- TOPLEVEL[fp]
-          n6 <- $GetIter(n5)
-          jmp b1(CM(n3).__exit__, n6)
-
-        b1(n7, n8):
-          n9 <- $NextIter(n8)
-          n10 <- $HasNextIter(n8)
-          if n10 then jmp b2 else jmp b7
-
-        b2:
-          TOPLEVEL[line] <- n9
-          n12 <- TOPLEVEL[print]
-          n13 <- n12(PYCString ("TRY"))
-          jmp b6
-
-        b6:
-          n14 <- TOPLEVEL[print]
-          n15 <- n14(PYCString ("ELSE"))
-          jmp b1(n7, n8)
-
-        b7:
-          jmp b8
-
-        b8:
-          n11 <- PYCNone(PYCNone, PYCNone, PYCNone)
-          return PYCNone |}]
+  [%expect {|
+    IR error: WITH_CLEANUP_START/TODO: unsupported scenario with n7 |}]
 
 
 let%expect_test _ =
@@ -375,7 +338,7 @@ async def async_with(filename):
           jmp b1
 
         b1:
-          n10 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n10 <- n2.__enter__(PYCNone, PYCNone, PYCNone)
           n11 <- $GetAwaitable(n10)
           n12 <- $YieldFrom(n11, PYCNone)
           return PYCNone |}]

infer/src/python/unit/PyIRTestLoop.ml

@@ -90,21 +90,21 @@ def f(x, y, l, bar, toto):
           if n11 then jmp b3 else jmp b4
 
         b3:
-          n16 <- PYCNone(PYCNone, PYCNone, PYCNone)
-          n17 <- PYCNone(PYCNone, PYCNone, PYCNone)
-          jmp b1(n2, CM(n6).__exit__, CM(n9).__exit__)
+          n16 <- n9.__enter__(PYCNone, PYCNone, PYCNone)
+          n17 <- n6.__enter__(PYCNone, PYCNone, PYCNone)
+          jmp b1(n2)
 
         b4:
           n12 <- GLOBAL[print]
           n13 <- n12(PYCString ("nop"))
           jmp b5
 
         b5:
-          n14 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n14 <- n9.__enter__(PYCNone, PYCNone, PYCNone)
           jmp b6
 
         b6:
-          n15 <- PYCNone(PYCNone, PYCNone, PYCNone)
+          n15 <- n6.__enter__(PYCNone, PYCNone, PYCNone)
           jmp b1(n2)
 
         b7: