Actually implement most of the PEP and fix the tests

python · ncoghlan · Sep 18, 2017 · Nov 5, 2017 · Nov 5, 2017 · Nov 5, 2017
commit fe928915d4c8fad7426ebcab33a115cf06252886
diff --git a/Include/frameobject.h b/Include/frameobject.h
@@ -71,16 +71,6 @@ PyAPI_FUNC(PyTryBlock *) PyFrame_BlockPop(PyFrameObject *);
 
 PyAPI_FUNC(PyObject **) PyFrame_ExtendStack(PyFrameObject *, int, int);
 
-/* Conversions between "fast locals" and locals in dictionary */
-
-PyAPI_FUNC(void) PyFrame_LocalsToFast(PyFrameObject *, int);
-
-PyAPI_FUNC(int) PyFrame_FastToLocalsWithError(PyFrameObject *f);
-PyAPI_FUNC(void) PyFrame_FastToLocals(PyFrameObject *);
-#ifdef Py_BUILD_CORE
-int _PyFrame_FastToLocalsInternal(PyFrameObject *f, int); /* For sys.settrace */
-#endif
-
 PyAPI_FUNC(int) PyFrame_ClearFreeList(void);
 
 PyAPI_FUNC(void) _PyFrame_DebugMallocStats(FILE *out);
@@ -89,13 +79,29 @@ PyAPI_FUNC(void) _PyFrame_DebugMallocStats(FILE *out);
 PyAPI_FUNC(int) PyFrame_GetLineNumber(PyFrameObject *);
 
 
+/* Conversions between "fast locals" and locals in dictionary */
+PyAPI_FUNC(int) PyFrame_FastToLocalsWithError(PyFrameObject *f);
+PyAPI_FUNC(void) PyFrame_FastToLocals(PyFrameObject *);
+
 #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03080000
 /* Fast locals proxy for reliable write-through from trace functions */
 PyTypeObject PyFastLocalsProxy_Type;
 #define _PyFastLocalsProxy_CheckExact(self) \
     (Py_TYPE(self) == &PyFastLocalsProxy_Type)
+
+/* Access the frame locals mapping */
+PyAPI_FUNC(PyObject *) PyFrame_GetPyLocals(PyFrameObject *); // = locals()
+PyAPI_FUNC(PyObject *) PyFrame_GetLocalsAttr(PyFrameObject *);  // = frame.f_locals
+#endif
+
+#ifdef Py_BUILD_CORE
+PyObject *_PyFrame_BorrowPyLocals(PyFrameObject *f); /* For PyEval_GetLocals() */
 #endif
 
+
+/* This always raises RuntimeError now (use PyFrame_GetLocalsAttr() instead) */
+PyAPI_FUNC(void) PyFrame_LocalsToFast(PyFrameObject *, int);
+
 #ifdef __cplusplus
 }
 #endif

diff --git a/Lib/test/test_scope.py b/Lib/test/test_scope.py
@@ -804,7 +804,9 @@ def function_local_semantics():
             return locals()
 
         global_ns = globals()
+        self.assertIsInstance(global_ns, dict)
         local_ns = function_local_semantics()
+        self.assertIsInstance(local_ns, dict)
         self.assertIs(local_ns["global_ns"], global_ns)
         self.assertIs(local_ns["local_ns"], local_ns)
         self.assertEqual(local_ns["known_var"], "original")

diff --git a/Lib/test/test_sys_settrace.py b/Lib/test/test_sys_settrace.py
@@ -1447,11 +1447,45 @@ def tracefunc(frame, event, arg):
         self.assertEqual(x_from_nested_ref, [2, 6])
         self.assertEqual(x_from_nested_locals, [3, 7])
 
-    def test_locals_writeback_support(self):
+    def test_locals_writethrough_proxy(self):
         # To support interactive debuggers, trace functions are expected to
         # be able to reliably modify function locals. However, this should
         # NOT enable writebacks via locals() at function scope.
         #
+        # This is a simple scenario to test the core behaviour of the
+        # writethrough proxy
+        local_var = "original"
+        # Check regular locals() snapshot
+        locals_snapshot = locals()
+        self.assertEqual(locals_snapshot["local_var"], "original")
+        locals_snapshot["local_var"] = "modified"
+        self.assertEqual(local_var, "original")
+        # Check writethrough proxy on frame
+        locals_proxy = sys._getframe().f_locals
+        self.assertEqual(locals_proxy["local_var"], "original")
+        locals_proxy["local_var"] = "modified"
+        self.assertEqual(local_var, "modified")
+        # Check handling of closures
+        def nested_scope():
+            nonlocal local_var
+            return local_var, locals(), sys._getframe().f_locals
+        closure_var, inner_snapshot, inner_proxy = nested_scope()
+        self.assertEqual(closure_var, "modified")
+        self.assertEqual(inner_snapshot["local_var"], "modified")
+        self.assertEqual(inner_proxy["local_var"], "modified")
+        inner_snapshot["local_var"] = "modified_again"
+        self.assertEqual(local_var, "modified")
+        self.assertEqual(inner_snapshot["local_var"], "modified_again")
+        self.assertEqual(inner_proxy["local_var"], "modified_again") # Q: Is this desirable?
+        inner_proxy["local_var"] = "modified_yet_again"
+        self.assertEqual(local_var, "modified_yet_again")
+        self.assertEqual(inner_snapshot["local_var"], "modified_yet_again")
+        self.assertEqual(inner_proxy["local_var"], "modified_yet_again")
+
+    def test_locals_writeback_complex_scenario(self):
+        # Further locals writeback testing using a more complex scenario
+        # involving multiple scopes of different kinds
+        #
         # Note: the sample values have numbers in them so mixing up variable
         #       names in the checks can't accidentally make the test pass -
         #       you'd have to get both the name *and* expected number wrong
@@ -1471,7 +1505,7 @@ def outer():
                 a_nonlocal = 'original5'       # We expect this to be retained
                 another_nonlocal = 'original6' # Trace func will modify this
                 def inner():
-                    nonlocal another_nonlocal
+                    nonlocal a_nonlocal, another_nonlocal
                     a_local = 'original7'          # We expect this to be retained
                     another_local = 'original8'    # Trace func will modify this
                     ns = locals()
@@ -1492,29 +1526,36 @@ def inner():
             a_local, another_local, inner_ns = inner_result
             a_local_via_ns = inner_ns['a_local']
             a_nonlocal_via_inner_ns = inner_ns['a_nonlocal']
+            print(a_nonlocal_via_inner_ns)
             another_nonlocal_via_inner_ns = inner_ns['another_nonlocal']
             another_local_via_ns = inner_ns['another_local']
             a_new_local_via_ns = inner_ns['a_new_local']
             del outer, outer_result, outer_ns, inner_result, inner_ns
             """
         )
         def tracefunc(frame, event, arg):
+            ns = frame.f_locals
+            co_name = frame.f_code.co_name
             if event == "return":
-                # We leave any state manipulation to the very end
-                ns = frame.f_locals
-                co_name = frame.f_code.co_name
+                # We leave most state manipulation to the very end
                 if co_name == "C":
                     # Modify class attributes
                     ns["another_attr"] = "modified4"
                 elif co_name == "inner":
-                    # Modify local and nonlocal variables
+                    # Modify variables in outer scope
                     ns["another_nonlocal"] = "modified6"
-                    ns["another_local"] = "modified8"
                     outer_ns = frame.f_back.f_locals
                     outer_ns["outer_local"] = "modified10"
                 elif co_name == "<module>":
                     # Modify globals
                     ns["another_global"] = "modified2"
+            elif event == "line" and co_name == "inner":
+                # This is the one item we can't leave to the end, as the
+                # return tuple is already built by the time the return event
+                # fires, and we want to manipulate one of the entries in that
+                # So instead, we just mutate it on every line trace event
+                ns["another_local"] = "modified8"
+                print(event)
             return tracefunc
         actual_ns = {}
         sys.settrace(tracefunc)
@@ -1525,9 +1566,6 @@ def tracefunc(frame, event, arg):
         for k in list(actual_ns.keys()):
             if k.startswith("_"):
                 del actual_ns[k]
-        # CURRENT FAILURE STATUS:
-        # - trace functions mutating state as expected
-        # - proxy is incorrectly being returned by locals(), so that also mutates
         expected_ns = {
             "a_global": "created1",
             "another_global": "modified2",
@@ -1547,7 +1585,8 @@ def tracefunc(frame, event, arg):
             "outer_local": "modified10",
             "outer_local_via_ns": "modified10",
         }
-        self.assertEqual(actual_ns, expected_ns)
+        # Expected is first so any error diff is easier to read
+        self.assertEqual(expected_ns, actual_ns)
 
 
 if __name__ == "__main__":