Finish public C API, start dict API tests

python · ncoghlan · Sep 18, 2017 · Nov 5, 2017 · Nov 5, 2017 · Nov 5, 2017
commit 1484c100ef9f5669f9e83b58c06a2dce4bfdec37
diff --git a/Include/cpython/frameobject.h b/Include/cpython/frameobject.h
@@ -91,22 +91,13 @@ PyAPI_DATA(PyTypeObject) _PyFastLocalsProxy_Type;
 // TODO: Add specific test cases for these (as any PyLocals_* tests won't cover
 //       checking the status of a frame other than the currently active one)
 PyAPI_FUNC(PyObject *) PyFrame_GetLocals(PyFrameObject *);
-
-// TODO: Implement the rest of these, and add API tests
 PyAPI_FUNC(PyObject *) PyFrame_GetLocalsCopy(PyFrameObject *);
 PyAPI_FUNC(PyObject *) PyFrame_GetLocalsView(PyFrameObject *);
 PyAPI_FUNC(int) PyFrame_GetLocalsReturnsCopy(PyFrameObject *);
 
 // Underlying API supporting PyEval_GetLocals()
 PyAPI_FUNC(PyObject *) _PyFrame_BorrowLocals(PyFrameObject *);
 
-/* Force an update of any selectively updated views previously returned by
- * PyFrame_GetLocalsView(frame). Currently also needed in CPython when
- * accessing the f_locals attribute directly and it is not a plain dict
- * instance (otherwise it may report stale information).
- */
-PyAPI_FUNC(int) PyFrame_RefreshLocalsView(PyFrameObject *);
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py
@@ -5,6 +5,7 @@
 import weakref
 
 from test import support
+from test.support import import_helper
 
 
 class ClearTest(unittest.TestCase):
@@ -195,6 +196,149 @@ def test_f_lineno_del_segfault(self):
         with self.assertRaises(AttributeError):
             del f.f_lineno
 
 class FastLocalsProxyTest(unittest.TestCase):
+
+    def check_proxy_contents(self, proxy, expected_contents):
+        # These checks should never implicitly resync the frame proxy's cache,
+        # even if the proxy is referenced as a local variable in the frame
+        self.assertEqual(len(proxy), len(expected_contents))
+        self.assertCountEqual(proxy, expected_contents)
+        self.assertCountEqual(proxy.keys(), expected_contents.keys())
+        self.assertCountEqual(proxy.values(), expected_contents.values())
+        self.assertCountEqual(proxy.items(), expected_contents.items())
+
+    def test_dict_operations(self):
+        # No real iteration order guarantees for the locals proxy, as it
+        # depends on exactly how the compiler composes the frame locals array
+        proxy = sys._getframe().f_locals
+        # Not yet set values aren't visible in the proxy
+        self.check_proxy_contents(proxy, {"self": self})
+
+        # Ensuring copying the proxy produces a plain dict instance
+        dict_copy = proxy.copy()
+        self.assertIsInstance(dict_copy, dict)
+        self.assertEqual(dict_copy.keys(), {"proxy", "self"})
+        # The proxy automatically updates its cache for O(n) operations like copying,
+        # but won't pick up new local variables until it is resync'ed with the frame
+        # or that particular key is accessed or queried
+        self.check_proxy_contents(proxy, dict_copy)
+        self.assertIn("dict_copy", proxy) # Implicitly updates cache for this key
+        dict_copy["dict_copy"] = dict_copy
+        self.check_proxy_contents(proxy, dict_copy)
+
+        self.fail("Test not finished yet")
+
+    def test_active_frame_c_apis(self):
+        # Use ctypes to access the C APIs under test
+        ctypes = import_helper.import_module('ctypes')
+        Py_IncRef = ctypes.pythonapi.Py_IncRef
+        PyEval_GetLocals = ctypes.pythonapi.PyEval_GetLocals
+        PyLocals_Get = ctypes.pythonapi.PyLocals_Get
+        PyLocals_GetReturnsCopy = ctypes.pythonapi.PyLocals_GetReturnsCopy
+        PyLocals_GetCopy = ctypes.pythonapi.PyLocals_GetCopy
+        PyLocals_GetView = ctypes.pythonapi.PyLocals_GetView
+        for capi_func in (Py_IncRef,):
+            capi_func.argtypes = (ctypes.py_object,)
+        for capi_func in (PyEval_GetLocals,
+                          PyLocals_Get, PyLocals_GetCopy, PyLocals_GetView):
+            capi_func.restype = ctypes.py_object
+
+        # PyEval_GetLocals() always accesses the running frame,
+        # so Py_IncRef has to be called inline (no helper function)
+
+        # This test covers the retrieval APIs, the behavioural tests are covered
+        # elsewhere using the `frame.f_locals` attribute and the locals() builtin
+
+        # Test retrieval API behaviour in an optimised scope
+        print("Retrieving C locals cache for frame")
+        c_locals_cache = PyEval_GetLocals()
+        Py_IncRef(c_locals_cache) # Make the borrowed reference a real one
        Py_IncRef(c_locals_cache) # Account for next check's borrowed reference
+        self.assertIs(PyEval_GetLocals(), c_locals_cache)
+        self.assertTrue(PyLocals_GetReturnsCopy())
+        locals_get = PyLocals_Get()
+        self.assertIsInstance(locals_get, dict)
+        self.assertIsNot(locals_get, c_locals_cache)
+        locals_copy = PyLocals_GetCopy()
+        self.assertIsInstance(locals_copy, dict)
+        self.assertIsNot(locals_copy, c_locals_cache)
+        locals_view = PyLocals_GetView()
+        self.assertIsInstance(locals_view, types.MappingProxyType)
+
+        # Test API behaviour in an unoptimised scope
+        class ExecFrame:
+            c_locals_cache = PyEval_GetLocals()
+            Py_IncRef(c_locals_cache) # Make the borrowed reference a real one
+            Py_IncRef(c_locals_cache) # Account for next check's borrowed reference
+            self.assertIs(PyEval_GetLocals(), c_locals_cache)
+            self.assertFalse(PyLocals_GetReturnsCopy())
+            locals_get = PyLocals_Get()
+            self.assertIs(locals_get, c_locals_cache)
+            locals_copy = PyLocals_GetCopy()
+            self.assertIsInstance(locals_copy, dict)
+            self.assertIsNot(locals_copy, c_locals_cache)
+            locals_view = PyLocals_GetView()
+            self.assertIsInstance(locals_view, types.MappingProxyType)
+
+    def test_arbitrary_frame_c_apis(self):
+        # Use ctypes to access the C APIs under test
+        ctypes = import_helper.import_module('ctypes')
+        Py_IncRef = ctypes.pythonapi.Py_IncRef
+        _PyFrame_BorrowLocals = ctypes.pythonapi._PyFrame_BorrowLocals
+        PyFrame_GetLocals = ctypes.pythonapi.PyFrame_GetLocals
+        PyFrame_GetLocalsReturnsCopy = ctypes.pythonapi.PyFrame_GetLocalsReturnsCopy
+        PyFrame_GetLocalsCopy = ctypes.pythonapi.PyFrame_GetLocalsCopy
+        PyFrame_GetLocalsView = ctypes.pythonapi.PyFrame_GetLocalsView
+        for capi_func in (Py_IncRef, _PyFrame_BorrowLocals,
+                          PyFrame_GetLocals, PyFrame_GetLocalsReturnsCopy,
+                          PyFrame_GetLocalsCopy, PyFrame_GetLocalsView):
+            capi_func.argtypes = (ctypes.py_object,)
+        for capi_func in (_PyFrame_BorrowLocals, PyFrame_GetLocals,
+                          PyFrame_GetLocalsCopy, PyFrame_GetLocalsView):
+            capi_func.restype = ctypes.py_object
+
+        def get_c_locals(frame):
+            c_locals = _PyFrame_BorrowLocals(frame)
+            Py_IncRef(c_locals) # Make the borrowed reference a real one
+            return c_locals
+
+        # This test covers the retrieval APIs, the behavioural tests are covered
+        # elsewhere using the `frame.f_locals` attribute and the locals() builtin
+
+        # Test querying an optimised frame from an unoptimised scope
+        func_frame = sys._getframe()
+        cls_frame = None
+        def set_cls_frame(f):
+            nonlocal cls_frame
+            cls_frame = 
6D40
f
+        class ExecFrame:
+            c_locals_cache = get_c_locals(func_frame)
+            self.assertIs(get_c_locals(func_frame), c_locals_cache)
+            self.assertTrue(PyFrame_GetLocalsReturnsCopy(func_frame))
+            locals_get = PyFrame_GetLocals(func_frame)
+            self.assertIsInstance(locals_get, dict)
+            self.assertIsNot(locals_get, c_locals_cache)
+            locals_copy = PyFrame_GetLocalsCopy(func_frame)
+            self.assertIsInstance(locals_copy, dict)
+            self.assertIsNot(locals_copy, c_locals_cache)
+            locals_view = PyFrame_GetLocalsView(func_frame)
+            self.assertIsInstance(locals_view, types.MappingProxyType)
+
+            # Keep the class frame alive for the functions below to access
+            set_cls_frame(sys._getframe())
+
+        # Test querying an unoptimised frame from an optimised scope
+        c_locals_cache = get_c_locals(cls_frame)
+        self.assertIs(get_c_locals(cls_frame), c_locals_cache)
+        self.assertFalse(PyFrame_GetLocalsReturnsCopy(cls_frame))
+        locals_get = PyFrame_GetLocals(cls_frame)
+        self.assertIs(locals_get, c_locals_cache)
+        locals_copy = PyFrame_GetLocalsCopy(cls_frame)
+        self.assertIsInstance(locals_copy, dict)
+        self.assertIsNot(locals_copy, c_locals_cache)
+        locals_view = PyFrame_GetLocalsView(cls_frame)
+        self.assertIsInstance(locals_view, types.MappingProxyType)
+
 
 class ReprTest(unittest.TestCase):
     """

@@ -34,6 +34,7 @@ _frame_get_locals_mapping(PyFrameObject *f)
 {
     PyObject *locals = _PyFrame_Specials(f)[FRAME_SPECIALS_LOCALS_OFFSET];
     if (locals == NULL) {
+        printf("Allocating new frame locals cache\n");
         locals = _PyFrame_Specials(f)[FRAME_SPECIALS_LOCALS_OFFSET] = PyDict_New();
     }
     return locals;
@@ -53,16 +54,20 @@ _frame_get_updated_locals(PyFrameObject *f)
 PyObject *
 _PyFrame_BorrowLocals(PyFrameObject *f)
 {
-    // This is called by PyEval_GetLocals(), which has historically returned
-    // a borrowed reference, so this does the same
+    // This frame API supports the PyEval_GetLocals() stable API, which has
+    // historically returned a borrowed reference (so this does the same)
     return _frame_get_updated_locals(f);
 }
 
 PyObject *
 PyFrame_GetLocals(PyFrameObject *f)
 {
-    // This API implements the Python level locals() builtin
+    // This frame API implements the Python level locals() builtin
+    // and supports the PyLocals_Get() stable API
     PyObject *updated_locals = _frame_get_updated_locals(f);
+    if (updated_locals == NULL) {
+        return NULL;
+    }
     PyCodeObject *co = _PyFrame_GetCode(f);
 
     assert(co);
@@ -77,6 +82,26 @@ PyFrame_GetLocals(PyFrameObject *f)
     return updated_locals;
 }
 
+int
+PyFrame_GetLocalsReturnsCopy(PyFrameObject *f)
+{
+    // This frame API supports the stable PyLocals_GetReturnsCopy() API
+    PyCodeObject *co = _PyFrame_GetCode(f);
+    assert(co);
+    return (co->co_flags & CO_OPTIMIZED);
+}
+
+PyObject *
+PyFrame_GetLocalsCopy(PyFrameObject *f)
+{
+    // This frame API supports the stable PyLocals_GetCopy() API
+    PyObject *updated_locals = _frame_get_updated_locals(f);
+    if (updated_locals == NULL) {
+        return NULL;
+    }
+    return PyDict_Copy(updated_locals);
+}
+
 static PyObject *
 frame_getlocals(PyFrameObject *f, void *Py_UNUSED(ignored))
 {
@@ -97,11 +122,25 @@ frame_getlocals(PyFrameObject *f, void *Py_UNUSED(ignored))
     } else {
         // Share a direct locals reference for class and module scopes
         f_locals_attr = _frame_get_updated_locals(f);
+        if (f_locals_attr == NULL) {
+            return NULL;
+        }
         Py_INCREF(f_locals_attr);
     }
     return f_locals_attr;
 }
 
+PyObject *
+PyFrame_GetLocalsView(PyFrameObject *f)
+{
+    // This frame API supports the stable PyLocals_GetView() API
+    PyObject *rw_locals = frame_getlocals(f, NULL);
+    if (rw_locals == NULL) {
+        return NULL;
+    }
+    return PyDictProxy_New(rw_locals);
+}
+
 int
 PyFrame_GetLineNumber(PyFrameObject *f)
 {
@@ -1018,6 +1057,7 @@ PyFrame_FastToLocalsWithError(PyFrameObject *f)
         return -1;
     }
     co = _PyFrame_GetCode(f);
+    assert(co);
     fast = f->f_localsptr;
     for (int i = 0; i < co->co_nlocalsplus; i++) {
         _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i);
@@ -1648,8 +1688,10 @@ static PyObject *
 fastlocalsproxy_richcompare(fastlocalsproxyobject *flp, PyObject *w, int op)
 {
     // Need values, so use the dynamic snapshot on the frame
-    // Ensure it is up to date, as checking is O(n) anyway
-    PyObject *locals = _frame_get_updated_locals(flp->frame);
+    // Assume f_locals snapshot is up to date, as even though the worst
+    // case comparison is O(n) to determine equality, there are O(1) shortcuts
+    // for inequality checks (i.e. different sizes)
+    PyObject *locals = _frame_get_locals_mapping(flp->frame);
     return PyObject_RichCompare(locals, w, op);
 }
 
@@ -1841,7 +1883,7 @@ static PyObject *
 fastlocalsproxy_str(fastlocalsproxyobject *flp)
 {
     // Need values, so use the dynamic snapshot on the frame
-    // Ensure it is up to date, as checking is O(n) anyway
+    // Ensure it is up to date, as displaying everything is O(n) anyway
     PyObject *locals = _frame_get_updated_locals(flp->frame);
     return PyObject_Str(locals);
 }

@@ -5798,6 +5798,46 @@ PyLocals_Get(void)
     return PyFrame_GetLocals(current_frame);
 }
 
+int
+PyLocals_GetReturnsCopy(void)
+{
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyFrameObject *current_frame = tstate->frame;
+    if (current_frame == NULL) {
+        _PyErr_SetString(tstate, PyExc_SystemError, "frame does not exist");
+        return NULL;
+    }
+
+    return PyFrame_GetLocalsReturnsCopy(current_frame);
+}
+
+PyObject *
+PyLocals_GetCopy(void)
+{
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyFrameObject *current_frame = tstate->frame;
+    if (current_frame == NULL) {
+        _PyErr_SetString(tstate, PyExc_SystemError, "frame does not exist");
+        return NULL;
+    }
+
+    return PyFrame_GetLocalsCopy(current_frame);
+}
+
+PyObject *
+PyLocals_GetView(void)
+{
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyFrameObject *current_frame = tstate->frame;
+    if (current_frame == NULL) {
+        _PyErr_SetString(tstate, PyExc_SystemError, "frame does not exist");
+        return NULL;
+    }
+
+    return PyFrame_GetLocalsView(current_frame);
+}
+
+
 PyObject *
 PyEval_GetGlobals(void)
 {