pyclass: refactor initialization, call native type dealloc

PyO3 · Jun 5, 2021 · 0b3cf7a · 0b3cf7a
1 parent daa731d
commit 0b3cf7a
Show file tree

Hide file tree

Showing 15 changed files with 485 additions and 380 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -65,6 +65,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Remove `__doc__` from module's `__all__`. [#1509](https://github.com/PyO3/pyo3/pull/1509)
 - Remove `PYO3_CROSS_INCLUDE_DIR` environment variable and the associated C header parsing functionality. [#1521](https://github.com/PyO3/pyo3/pull/1521)
 - Remove `raw_pycfunction!` macro. [#1619](https://github.com/PyO3/pyo3/pull/1619)
+- Remove `PyClassAlloc` trait. [#1657](https://github.com/PyO3/pyo3/pull/1657)
 
 ### Fixed
 - Remove FFI definition `PyCFunction_ClearFreeList` for Python 3.9 and later. [#1425](https://github.com/PyO3/pyo3/pull/1425)
@@ -80,6 +81,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fix unneccessary rebuilds when cycling between `cargo check` and `cargo clippy` in a Python virtualenv. [#1557](https://github.com/PyO3/pyo3/pull/1557)
 - Fix segfault when dereferencing `ffi::PyDateTimeAPI` without the GIL. [#1563](https://github.com/PyO3/pyo3/pull/1563)
 - Fix memory leak when converting to u128 and i128. [#1638](https://github.com/PyO3/pyo3/pull/1638)
+- Fix `#[pyclass(extends=PyDict)]` leaking the dict contents on drop. [#1657](https://github.com/PyO3/pyo3/pull/1657)
 
 ## [0.13.2] - 2021-02-12
 ### Packaging

diff --git a/guide/src/class.md b/guide/src/class.md
@@ -720,8 +720,6 @@ struct MyClass {
     debug: bool,
 }
 
-impl pyo3::pyclass::PyClassAlloc for MyClass {}
-
 unsafe impl pyo3::PyTypeInfo for MyClass {
     type AsRefTarget = PyCell<Self>;
 
@@ -774,6 +772,16 @@ impl pyo3::class::impl_::PyClassImpl for MyClass {
         let collector = PyClassImplCollector::<Self>::new();
         collector.new_impl()
     }
+    fn get_alloc() -> Option<pyo3::ffi::allocfunc> {
+        use pyo3::class::impl_::*;
+        let collector = PyClassImplCollector::<Self>::new();
+        collector.alloc_impl()
+    }
+    fn get_free() -> Option<pyo3::ffi::freefunc> {
+        use pyo3::class::impl_::*;
+        let collector = PyClassImplCollector::<Self>::new();
+        collector.free_impl()
+    }
     fn get_call() -> Option<pyo3::ffi::PyCFunctionWithKeywords> {
         use pyo3::class::impl_::*;
         let collector = PyClassImplCollector::<Self>::new();

diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs
@@ -311,29 +311,37 @@ fn impl_class(
 ) -> syn::Result<TokenStream> {
     let cls_name = get_class_python_name(cls, attr).to_string();
 
-    let alloc = {
-        if let Some(freelist) = &attr.freelist {
+    let alloc = attr.freelist.as_ref().map(|freelist| {
             quote! {
-                impl pyo3::freelist::PyClassWithFreeList for #cls {
+                impl pyo3::class::impl_::PyClassWithFreeList for #cls {
                     #[inline]
-                    fn get_free_list(_py: pyo3::Python) -> &mut pyo3::freelist::FreeList<*mut pyo3::ffi::PyObject> {
-                        static mut FREELIST: *mut pyo3::freelist::FreeList<*mut pyo3::ffi::PyObject> = 0 as *mut _;
+                    fn get_free_list(_py: pyo3::Python) -> &mut pyo3::impl_::freelist::FreeList<*mut pyo3::ffi::PyObject> {
+                        static mut FREELIST: *mut pyo3::impl_::freelist::FreeList<*mut pyo3::ffi::PyObject> = 0 as *mut _;
                         unsafe {
                             if FREELIST.is_null() {
                                 FREELIST = Box::into_raw(Box::new(
-                                    pyo3::freelist::FreeList::with_capacity(#freelist)));
+                                    pyo3::impl_::freelist::FreeList::with_capacity(#freelist)));
                             }
                             &mut *FREELIST
                         }
                     }
                 }
+
+                impl pyo3::class::impl_::PyClassAllocImpl<#cls> for pyo3::class::impl_::PyClassImplCollector<#cls> {
+                    #[inline]
+                    fn alloc_impl(self) -> Option<pyo3::ffi::allocfunc> {
+                        Some(pyo3::class::impl_::alloc_with_freelist::<#cls>)
+                    }
+                }
+
+                impl pyo3::class::impl_::PyClassFreeImpl<#cls> for pyo3::class::impl_::PyClassImplCollector<#cls> {
+                    #[inline]
+                    fn free_impl(self) -> Option<pyo3::ffi::freefunc> {
+                        Some(pyo3::class::impl_::free_with_freelist::<#cls>)
+                    }
+                }
             }
-        } else {
-            quote! {
-                impl pyo3::pyclass::PyClassAlloc for #cls {}
-            }
-        }
-    };
+        });
 
     let descriptors = impl_descriptors(cls, field_options)?;
 
@@ -482,6 +490,16 @@ fn impl_class(
                 let collector = PyClassImplCollector::<Self>::new();
                 collector.new_impl()
             }
+            fn get_alloc() -> Option<pyo3::ffi::allocfunc> {
+                use pyo3::class::impl_::*;
+                let collector = PyClassImplCollector::<Self>::new();
+                collector.alloc_impl()
+            }
+            fn get_free() -> Option<pyo3::ffi::freefunc> {
+                use pyo3::class::impl_::*;
+                let collector = PyClassImplCollector::<Self>::new();
+                collector.free_impl()
+            }
             fn get_call() -> Option<pyo3::ffi::PyCFunctionWithKeywords> {
                 use pyo3::class::impl_::*;
                 let collector = PyClassImplCollector::<Self>::new();

diff --git a/src/class/impl_.rs b/src/class/impl_.rs
@@ -2,12 +2,13 @@
 
 use crate::{
     ffi,
+    impl_::freelist::FreeList,
     pycell::PyCellLayout,
     pyclass_init::PyObjectInit,
     type_object::{PyLayout, PyTypeObject},
-    PyClass, PyMethodDefType, PyNativeType, PyTypeInfo,
+    PyCell, PyClass, PyMethodDefType, PyNativeType, PyTypeInfo, Python,
 };
-use std::{marker::PhantomData, thread};
+use std::{marker::PhantomData, os::raw::c_void, thread};
 
 /// This type is used as a "dummy" type on which dtolnay specializations are
 /// applied to apply implementations from `#[pymethods]` & `#[pyproto]`
@@ -72,6 +73,12 @@ pub trait PyClassImpl: Sized {
     fn get_call() -> Option<ffi::PyCFunctionWithKeywords> {
         None
     }
+    fn get_alloc() -> Option<ffi::allocfunc> {
+        None
+    }
+    fn get_free() -> Option<ffi::freefunc> {
+        None
+    }
     fn for_each_proto_slot(_visitor: &mut dyn FnMut(&[ffi::PyType_Slot])) {}
     fn get_buffer() -> Option<&'static PyBufferProcs> {
         None
@@ -100,6 +107,104 @@ impl<T> PyClassCallImpl<T> for &'_ PyClassImplCollector<T> {
     }
 }
 
+pub trait PyClassAllocImpl<T> {
+    fn alloc_impl(self) -> Option<ffi::allocfunc>;
+}
+
+impl<T> PyClassAllocImpl<T> for &'_ PyClassImplCollector<T> {
+    fn alloc_impl(self) -> Option<ffi::allocfunc> {
+        None
+    }
+}
+
+pub trait PyClassFreeImpl<T> {
+    fn free_impl(self) -> Option<ffi::freefunc>;
+}
+
+impl<T> PyClassFreeImpl<T> for &'_ PyClassImplCollector<T> {
+    fn free_impl(self) -> Option<ffi::freefunc> {
+        None
+    }
+}
+
+/// Implements a freelist.
+///
+/// Do not implement this trait manually. Instead, use `#[pyclass(freelist = N)]`
+/// on a Rust struct to implement it.
+pub trait PyClassWithFreeList: PyClass {
+    fn get_free_list(py: Python) -> &mut FreeList<*mut ffi::PyObject>;
+}
+
+/// Implementation of tp_alloc for `freelist` classes.
+pub unsafe extern "C" fn alloc_with_freelist<T: PyClassWithFreeList>(
+    subtype: *mut ffi::PyTypeObject,
+    nitems: ffi::Py_ssize_t,
+) -> *mut ffi::PyObject {
+    let py = Python::assume_gil_acquired();
+
+    #[cfg(not(Py_3_8))]
+    bpo_35810_workaround(py, subtype);
+
+    let self_type = T::type_object_raw(py);
+    // If this type is a variable type or the subtype is not equal to this type, we cannot use the
+    // freelist
+    if nitems == 0 && subtype == self_type {
+        if let Some(obj) = T::get_free_list(py).pop() {
+            ffi::PyObject_Init(obj, subtype);
+            return obj as _;
+        }
+    }
+
+    ffi::PyType_GenericAlloc(subtype, nitems)
+}
+
+/// Implementation of tp_free for `freelist` classes.
+#[allow(clippy::collapsible_if)] // for if cfg!
+pub unsafe extern "C" fn free_with_freelist<T: PyClassWithFreeList>(obj: *mut c_void) {
+    let obj = obj as *mut ffi::PyObject;
+    debug_assert_eq!(
+        T::type_object_raw(Python::assume_gil_acquired()),
+        ffi::Py_TYPE(obj)
+    );
+    if let Some(obj) = T::get_free_list(Python::assume_gil_acquired()).insert(obj) {
+        let ty = ffi::Py_TYPE(obj);
+
+        // Deduce appropriate inverse of PyType_GenericAlloc
+        let free = if ffi::PyType_IS_GC(ty) != 0 {
+            ffi::PyObject_GC_Del
+        } else {
+            ffi::PyObject_Free
+        };
+        free(obj as *mut c_void);
+
+        if cfg!(Py_3_8) {
+            if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 {
+                ffi::Py_DECREF(ty as *mut ffi::PyObject);
+            }
+        }
+    }
+}
+
+/// Workaround for Python issue 35810; no longer necessary in Python 3.8
+#[inline]
+#[cfg(not(Py_3_8))]
+unsafe fn bpo_35810_workaround(_py: Python, ty: *mut ffi::PyTypeObject) {
+    #[cfg(Py_LIMITED_API)]
+    {
+        // Must check version at runtime for abi3 wheels - they could run against a higher version
+        // than the build config suggests.
+        use crate::once_cell::GILOnceCell;
+        static IS_PYTHON_3_8: GILOnceCell<bool> = GILOnceCell::new();
+
+        if *IS_PYTHON_3_8.get_or_init(_py, || _py.version_info() >= (3, 8)) {
+            // No fix needed - the wheel is running on a sufficiently new interpreter.
+            return;
+        }
+    }
+
+    ffi::Py_INCREF(ty as *mut ffi::PyObject);
+}
+
 // General methods implementation: either dtolnay specialization trait or inventory if
 // multiple-pymethods feature is enabled.
 
@@ -216,6 +321,7 @@ pub struct ThreadCheckerStub<T: Send>(PhantomData<T>);
 
 impl<T: Send> PyClassThreadChecker<T> for ThreadCheckerStub<T> {
     fn ensure(&self) {}
+    #[inline]
     fn new() -> Self {
         ThreadCheckerStub(PhantomData)
     }
@@ -224,6 +330,7 @@ impl<T: Send> PyClassThreadChecker<T> for ThreadCheckerStub<T> {
 
 impl<T: PyNativeType> PyClassThreadChecker<T> for ThreadCheckerStub<crate::PyObject> {
     fn ensure(&self) {}
+    #[inline]
     fn new() -> Self {
         ThreadCheckerStub(PhantomData)
     }
@@ -279,8 +386,30 @@ pub trait PyClassBaseType: Sized {
 impl<T: PyClass> PyClassBaseType for T {
     type Dict = T::Dict;
     type WeakRef = T::WeakRef;
-    type LayoutAsBase = crate::pycell::PyCellInner<T>;
+    type LayoutAsBase = crate::pycell::PyCell<T>;
     type BaseNativeType = T::BaseNativeType;
     type ThreadChecker = T::ThreadChecker;
     type Initializer = crate::pyclass_init::PyClassInitializer<Self>;
 }
+
+// Default new implementation
+
+pub(crate) unsafe extern "C" fn fallback_new(
+    _subtype: *mut ffi::PyTypeObject,
+    _args: *mut ffi::PyObject,
+    _kwds: *mut ffi::PyObject,
+) -> *mut ffi::PyObject {
+    crate::callback_body!(py, {
+        Err::<(), _>(crate::exceptions::PyTypeError::new_err(
+            "No constructor defined",
+        ))
+    })
+}
+
+pub(crate) unsafe extern "C" fn tp_dealloc<T: PyClass>(obj: *mut ffi::PyObject) {
+    crate::callback_body!(py, {
+        // Safety: Python will only call tp_dealloc when no references to the object remain.
+        let cell: &mut PyCell<T> = &mut *(obj as *mut _);
+        cell.tp_dealloc(py);
+    })
+}
diff --git a/src/ffi/datetime.rs b/src/ffi/datetime.rs
@@ -650,7 +650,6 @@ mod tests {
     fn test_date_fromtimestamp() {
         Python::with_gil(|py| {
             let args: Py<PyAny> = (100,).into_py(py);
-            dbg!(args.as_ref(py));
             unsafe { PyDateTime_IMPORT() };
             let dt: &PyAny = unsafe { py.from_owned_ptr(PyDate_FromTimestamp(args.as_ptr())) };
             py_run!(