Merge pull request #995 from scalexm/recursive-attr

Re-enable recursive class attributes

Author: kngwyu

CHANGELOG.md

@@ -31,7 +31,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Removed
 - Remove `ManagedPyRef` (unused, and needs specialization) [#930](https://github.com/PyO3/pyo3/pull/930)
-- Disable `#[classattr]` where the class attribute is the same type as the class. (This may be re-enabled in the future; the previous implemenation was unsound.) [#975](https://github.com/PyO3/pyo3/pull/975)
 
 ### Fixed
 - Fix passing explicit `None` to `Option<T>` argument `#[pyfunction]` with a default value. [#936](https://github.com/PyO3/pyo3/pull/936)

guide/src/class.md

@@ -565,11 +565,6 @@ impl MyClass {
 }
 ```
 
-Note that defining a class attribute of the same type as the class will make the class unusable.
-Attempting to use the class will cause a panic reading `Recursive evaluation of type_object`.
-As an alternative, if having the attribute on instances is acceptable, create a `#[getter]` which
-uses a `GILOnceCell` to cache the attribute value. Or add the attribute to a module instead.
-
 ## Callable objects
 
 To specify a custom `__call__` method for a custom class, the method needs to be annotated with

src/once_cell.rs

@@ -58,6 +58,10 @@ impl<T> GILOnceCell<T> {
     ///     calling `f()`. Even when this happens `GILOnceCell` guarantees that only **one** write
     ///     to the cell ever occurs - other threads will simply discard the value they compute and
     ///     return the result of the first complete computation.
+    ///  3) if f() does not release the GIL and does not panic, it is guaranteed to be called
+    ///     exactly once, even if multiple threads attempt to call `get_or_init`
+    ///  4) if f() can panic but still does not release the GIL, it may be called multiple times,
+    ///     but it is guaranteed that f() will never be called concurrently
     pub fn get_or_init<F>(&self, py: Python, f: F) -> &T
     where
         F: FnOnce() -> T,

src/pyclass.rs

@@ -1,10 +1,10 @@
 //! `PyClass` trait
 use crate::class::methods::{PyClassAttributeDef, PyMethodDefType, PyMethods};
 use crate::class::proto_methods::PyProtoMethods;
-use crate::conversion::{AsPyPointer, FromPyPointer, IntoPyPointer, ToPyObject};
+use crate::conversion::{AsPyPointer, FromPyPointer};
 use crate::pyclass_slots::{PyClassDict, PyClassWeakRef};
 use crate::type_object::{type_flags, PyLayout};
-use crate::types::{PyAny, PyDict};
+use crate::types::PyAny;
 use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python};
 use std::ffi::CString;
 use std::os::raw::c_void;
@@ -188,23 +188,14 @@ where
     // buffer protocol
     type_object.tp_as_buffer = T::buffer_methods().map_or_else(ptr::null_mut, |p| p.as_ptr());
 
-    let (new, call, mut methods, attrs) = py_class_method_defs::<T>();
+    let (new, call, mut methods) = py_class_method_defs::<T>();
 
     // normal methods
     if !methods.is_empty() {
         methods.push(ffi::PyMethodDef_INIT);
         type_object.tp_methods = Box::into_raw(methods.into_boxed_slice()) as _;
     }
 
-    // class attributes
-    if !attrs.is_empty() {
-        let dict = PyDict::new(py);
-        for attr in attrs {
-            dict.set_item(attr.name, (attr.meth)(py))?;
-        }
-        type_object.tp_dict = dict.to_object(py).into_ptr();
-    }
-
     // __new__ method
     type_object.tp_new = new;
     // __call__ method
@@ -248,14 +239,19 @@ fn py_class_flags<T: PyTypeInfo>(type_object: &mut ffi::PyTypeObject) {
     }
 }
 
+pub(crate) fn py_class_attributes<T: PyMethods>() -> impl Iterator<Item = PyClassAttributeDef> {
+    T::py_methods().into_iter().filter_map(|def| match def {
+        PyMethodDefType::ClassAttribute(attr) => Some(*attr),
+        _ => None,
+    })
+}
+
 fn py_class_method_defs<T: PyMethods>() -> (
     Option<ffi::newfunc>,
     Option<ffi::PyCFunctionWithKeywords>,
     Vec<ffi::PyMethodDef>,
-    Vec<PyClassAttributeDef>,
 ) {
     let mut defs = Vec::new();
-    let mut attrs = Vec::new();
     let mut call = None;
     let mut new = None;
 
@@ -278,14 +274,11 @@ fn py_class_method_defs<T: PyMethods>() -> (
             | PyMethodDefType::Static(ref def) => {
                 defs.push(def.as_method_def());
             }
-            PyMethodDefType::ClassAttribute(def) => {
-                attrs.push(def);
-            }
             _ => (),
         }
     }
 
-    (new, call, defs, attrs)
+    (new, call, defs)
 }
 
 fn py_class_properties<T: PyMethods>() -> Vec<ffi::PyGetSetDef> {

src/type_object.rs

@@ -1,11 +1,12 @@
 // Copyright (c) 2017-present PyO3 Project and Contributors
 //! Python type object information
 
+use crate::conversion::IntoPyPointer;
 use crate::once_cell::GILOnceCell;
-use crate::pyclass::{initialize_type_object, PyClass};
+use crate::pyclass::{initialize_type_object, py_class_attributes, PyClass};
 use crate::pyclass_init::PyObjectInit;
 use crate::types::{PyAny, PyType};
-use crate::{ffi, AsPyPointer, PyNativeType, Python};
+use crate::{ffi, AsPyPointer, PyErr, PyNativeType, PyObject, PyResult, Python};
 use parking_lot::{const_mutex, Mutex};
 use std::thread::{self, ThreadId};
 
@@ -139,51 +140,108 @@ where
 pub struct LazyStaticType {
     // Boxed because Python expects the type object to have a stable address.
     value: GILOnceCell<*mut ffi::PyTypeObject>,
-    // Threads which have begun initialization. Used for reentrant initialization detection.
+    // Threads which have begun initialization of the `tp_dict`. Used for
+    // reentrant initialization detection.
     initializing_threads: Mutex<Vec<ThreadId>>,
+    tp_dict_filled: GILOnceCell<PyResult<()>>,
 }
 
 impl LazyStaticType {
     pub const fn new() -> Self {
         LazyStaticType {
             value: GILOnceCell::new(),
             initializing_threads: const_mutex(Vec::new()),
+            tp_dict_filled: GILOnceCell::new(),
         }
     }
 
     pub fn get_or_init<T: PyClass>(&self, py: Python) -> *mut ffi::PyTypeObject {
-        *self.value.get_or_init(py, || {
-            {
-                // Code evaluated at class init time, such as class attributes, might lead to
-                // recursive initalization of the type object if the class attribute is the same
-                // type as the class.
-                //
-                // That could lead to all sorts of unsafety such as using incomplete type objects
-                // to initialize class instances, so recursive initialization is prevented.
-                let thread_id = thread::current().id();
-                let mut threads = self.initializing_threads.lock();
-                if threads.contains(&thread_id) {
-                    panic!("Recursive initialization of type_object for {}", T::NAME);
-                } else {
-                    threads.push(thread_id)
-                }
-            }
-
-            // Okay, not recursive initialization - can proceed safely.
+        let type_object = *self.value.get_or_init(py, || {
             let mut type_object = Box::new(ffi::PyTypeObject_INIT);
-
             initialize_type_object::<T>(py, T::MODULE, type_object.as_mut()).unwrap_or_else(|e| {
                 e.print(py);
                 panic!("An error occurred while initializing class {}", T::NAME)
             });
+            Box::into_raw(type_object)
+        });
+
+        // We might want to fill the `tp_dict` with python instances of `T`
+        // itself. In order to do so, we must first initialize the type object
+        // with an empty `tp_dict`: now we can create instances of `T`.
+        //
+        // Then we fill the `tp_dict`. Multiple threads may try to fill it at
+        // the same time, but only one of them will succeed.
+        //
+        // More importantly, if a thread is performing initialization of the
+        // `tp_dict`, it can still request the type object through `get_or_init`,
+        // but the `tp_dict` may appear empty of course.
+
+        if self.tp_dict_filled.get(py).is_some() {
+            // `tp_dict` is already filled: ok.
+            return type_object;
+        }
+
+        {
+            let thread_id = thread::current().id();
+            let mut threads = self.initializing_threads.lock();
+            if threads.contains(&thread_id) {
+                // Reentrant call: just return the type object, even if the
+                // `tp_dict` is not filled yet.
+                return type_object;
+            }
+            threads.push(thread_id);
+        }
+
+        // Pre-compute the class attribute objects: this can temporarily
+        // release the GIL since we're calling into arbitrary user code. It
+        // means that another thread can continue the initialization in the
+        // meantime: at worst, we'll just make a useless computation.
+        let mut items = vec![];
+        for attr in py_class_attributes::<T>() {
+            items.push((attr.name, (attr.meth)(py)));
+        }
+
+        // Now we hold the GIL and we can assume it won't be released until we
+        // return from the function.
+        let result = self.tp_dict_filled.get_or_init(py, move || {
+            let tp_dict = unsafe { (*type_object).tp_dict };
+            let result = initialize_tp_dict(py, tp_dict, items);
+            // See discussion on #982 for why we need this.
+            unsafe { ffi::PyType_Modified(type_object) };
 
             // Initialization successfully complete, can clear the thread list.
-            // (No futher calls to get_or_init() will try to init, on any thread.)
+            // (No further calls to get_or_init() will try to init, on any thread.)
             *self.initializing_threads.lock() = Vec::new();
+            result
+        });
 
-            Box::into_raw(type_object)
-        })
+        if let Err(err) = result {
+            err.clone_ref(py).print(py);
+            panic!("An error occured while initializing `{}.__dict__`", T::NAME);
+        }
+
+        type_object
+    }
+}
+
+fn initialize_tp_dict(
+    py: Python,
+    tp_dict: *mut ffi::PyObject,
+    items: Vec<(&'static str, PyObject)>,
+) -> PyResult<()> {
+    use std::ffi::CString;
+
+    // We hold the GIL: the dictionary update can be considered atomic from
+    // the POV of other threads.
+    for (key, val) in items {
+        let ret = unsafe {
+            ffi::PyDict_SetItemString(tp_dict, CString::new(key)?.as_ptr(), val.into_ptr())
+        };
+        if ret < 0 {
+            return Err(PyErr::fetch(py));
+        }
     }
+    Ok(())
 }
 
 // This is necessary for making static `LazyStaticType`s

tests/test_class_attributes.rs

@@ -34,6 +34,11 @@ impl Foo {
     fn bar() -> Bar {
         Bar { x: 2 }
     }
+
+    #[classattr]
+    fn foo() -> Foo {
+        Foo { x: 1 }
+    }
 }
 
 #[test]
@@ -54,23 +59,22 @@ fn class_attributes_are_immutable() {
     py_expect_exception!(py, foo_obj, "foo_obj.a = 6", TypeError);
 }
 
-#[pyclass]
-struct SelfClassAttribute {
-    #[pyo3(get)]
-    x: i32,
-}
-
 #[pymethods]
-impl SelfClassAttribute {
+impl Bar {
     #[classattr]
-    const SELF: SelfClassAttribute = SelfClassAttribute { x: 1 };
+    fn foo() -> Foo {
+        Foo { x: 3 }
+    }
 }
 
 #[test]
-#[should_panic(expected = "Recursive initialization of type_object for SelfClassAttribute")]
 fn recursive_class_attributes() {
     let gil = Python::acquire_gil();
     let py = gil.python();
 
-    py.get_type::<SelfClassAttribute>();
+    let foo_obj = py.get_type::<Foo>();
+    let bar_obj = py.get_type::<Bar>();
+    py_assert!(py, foo_obj, "foo_obj.foo.x == 1");
+    py_assert!(py, foo_obj, "foo_obj.bar.x == 2");
+    py_assert!(py, bar_obj, "bar_obj.foo.x == 3");
 }