call PyObject_GC_Untrack before deallocating

.github/workflows/ci.yml

@@ -363,6 +363,49 @@ jobs:
       - name: Test
         run: nox -s test-emscripten
 
+  test-debug:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: Swatinem/rust-cache@v2
+        with:
+          key: cargo-test-debug
+        continue-on-error: true
+      - uses: dtolnay/rust-toolchain@stable
+        with:
+          components: rust-src
+      - name: Install python3 standalone debug build with nox
+        run: |
+          PBS_RELEASE="20230826"
+          PBS_PYTHON_VERSION="3.11.5"
+          PBS_ARCHIVE="cpython-${PBS_PYTHON_VERSION}+${PBS_RELEASE}-x86_64-unknown-linux-gnu-debug-full.tar.zst"
+          wget "https://github.com/indygreg/python-build-standalone/releases/download/${PBS_RELEASE}/${PBS_ARCHIVE}"
+          tar -I zstd -xf "${PBS_ARCHIVE}"
+          ls -l $(pwd)/python/install/bin
+          ls -l $(pwd)/python/install/lib
+          echo PATH=$(pwd)/python/install/bin:$PATH >> $GITHUB_ENV
+          echo LD_LIBRARY_PATH=$(pwd)/python/install/lib:$LD_LIBRARY_PATH >> $GITHUB_ENV
+          echo PYTHONHOME=$(pwd)/python/install >> $GITHUB_ENV
+          echo PYO3_PYTHON=$(pwd)/python/install/bin/python3 >> $GITHUB_ENV
+      - run: python3 -m sysconfig
+      - run: python3 -m pip install --upgrade pip && pip install nox
+      - run: |
+          PYO3_CONFIG_FILE=$(mktemp)
+          cat > $PYO3_CONFIG_FILE << EOF
+          implementation=CPython
+          version=3.11
+          shared=true
+          abi3=false
+          lib_name=python3.11d
+          lib_dir=${{ github.workspace }}/python/install/lib
+          executable=${{ github.workspace }}/python/install/bin/python3
+          pointer_width=64
+          build_flags=Py_DEBUG,Py_REF_DEBUG
+          suppress_build_script_link_lines=false
+          EOF
+          echo PYO3_CONFIG_FILE=$PYO3_CONFIG_FILE >> $GITHUB_ENV
+      - run: python3 -m nox -s test
+
   conclusion:
     needs:
       - fmt

guide/src/class.md

@@ -1136,7 +1136,6 @@ impl pyo3::IntoPy<PyObject> for MyClass {
 impl pyo3::impl_::pyclass::PyClassImpl for MyClass {
     const IS_BASETYPE: bool = false;
     const IS_SUBCLASS: bool = false;
-    type Layout = PyCell<MyClass>;
     type BaseType = PyAny;
     type ThreadChecker = pyo3::impl_::pyclass::SendablePyClass<MyClass>;
     type PyClassMutability = <<pyo3::PyAny as pyo3::impl_::pyclass::PyClassBaseType>::PyClassMutability as pyo3::impl_::pycell::PyClassMutability>::MutableChild;

newsfragments/3404.fixed.md

@@ -0,0 +1 @@
+Fix `ResourceWarning` and crashes related to GC when running with debug builds of CPython.

noxfile.py

@@ -475,14 +475,14 @@ def set_minimal_package_versions(session: nox.Session):
     min_pkg_versions = {
         "rust_decimal": "1.26.1",
         "csv": "1.1.6",
+        "indexmap": "1.9.3",
         "hashbrown": "0.12.3",
         "log": "0.4.17",
         "once_cell": "1.17.2",
         "rayon": "1.6.1",
         "rayon-core": "1.10.2",
         "regex": "1.7.3",
         "proptest": "1.0.0",
-        "indexmap": "1.9.3",
         "chrono": "0.4.25",
     }
 

pyo3-ffi/src/objimpl.rs

@@ -7,6 +7,7 @@ use crate::pyport::Py_ssize_t;
 extern "C" {
     #[cfg_attr(PyPy, link_name = "PyPyObject_Malloc")]
     pub fn PyObject_Malloc(size: size_t) -> *mut c_void;
+    #[cfg_attr(PyPy, link_name = "PyPyObject_Calloc")]
     pub fn PyObject_Calloc(nelem: size_t, elsize: size_t) -> *mut c_void;
     #[cfg_attr(PyPy, link_name = "PyPyObject_Realloc")]
     pub fn PyObject_Realloc(ptr: *mut c_void, new_size: size_t) -> *mut c_void;
@@ -72,7 +73,9 @@ extern "C" {
     pub fn _PyObject_GC_New(arg1: *mut PyTypeObject) -> *mut PyObject;
     #[cfg_attr(PyPy, link_name = "_PyPyObject_GC_NewVar")]
     pub fn _PyObject_GC_NewVar(arg1: *mut PyTypeObject, arg2: Py_ssize_t) -> *mut PyVarObject;
+    #[cfg(not(PyPy))]
     pub fn PyObject_GC_Track(arg1: *mut c_void);
+    #[cfg(not(PyPy))]
     pub fn PyObject_GC_UnTrack(arg1: *mut c_void);
     #[cfg_attr(PyPy, link_name = "PyPyObject_GC_Del")]
     pub fn PyObject_GC_Del(arg1: *mut c_void);

pyo3-macros-backend/src/pyclass.rs

@@ -1029,7 +1029,6 @@ impl<'a> PyClassImplsBuilder<'a> {
                 const IS_MAPPING: bool = #is_mapping;
                 const IS_SEQUENCE: bool = #is_sequence;
 
-                type Layout = _pyo3::PyCell<Self>;
                 type BaseType = #base;
                 type ThreadChecker = #thread_checker;
                 #inventory

src/impl_/freelist.rs

@@ -55,7 +55,7 @@ impl<T> FreeList<T> {
         }
     }
 
-    /// Inserts a value into the list. Returns `None` if the `FreeList` is full.
+    /// Inserts a value into the list. Returns `Some(val)` if the `FreeList` is full.
     pub fn insert(&mut self, val: T) -> Option<T> {
         let next = self.split + 1;
         if next < self.capacity {

src/impl_/pyclass.rs

@@ -6,7 +6,6 @@ use crate::{
     internal_tricks::extract_c_string,
     pycell::PyCellLayout,
     pyclass_init::PyObjectInit,
-    type_object::PyLayout,
     Py, PyAny, PyCell, PyClass, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python,
 };
 use std::{
@@ -153,9 +152,6 @@ pub trait PyClassImpl: Sized + 'static {
     /// #[pyclass(sequence)]
     const IS_SEQUENCE: bool = false;
 
-    /// Layout
-    type Layout: PyLayout<Self>;
-
     /// Base class
     type BaseType: PyTypeInfo + PyClassBaseType;
 
@@ -1098,9 +1094,18 @@ impl<T: PyClass> PyClassBaseType for T {
     type PyClassMutability = T::PyClassMutability;
 }
 
-/// Implementation of tp_dealloc for all pyclasses
+/// Implementation of tp_dealloc for pyclasses without gc
 pub(crate) unsafe extern "C" fn tp_dealloc<T: PyClass>(obj: *mut ffi::PyObject) {
-    crate::impl_::trampoline::dealloc(obj, T::Layout::tp_dealloc)
+    crate::impl_::trampoline::dealloc(obj, PyCell::<T>::tp_dealloc)
+}
+
+/// Implementation of tp_dealloc for pyclasses with gc
+pub(crate) unsafe extern "C" fn tp_dealloc_with_gc<T: PyClass>(obj: *mut ffi::PyObject) {
+    #[cfg(not(PyPy))]
+    {
+        ffi::PyObject_GC_UnTrack(obj.cast());
+    }
+    crate::impl_::trampoline::dealloc(obj, PyCell::<T>::tp_dealloc)
 }
 
 pub(crate) unsafe extern "C" fn get_sequence_item_from_mapping(

src/pycell.rs

@@ -204,7 +204,6 @@ use crate::type_object::{PyLayout, PySizedLayout};
 use crate::types::PyAny;
 use crate::{
     conversion::{AsPyPointer, FromPyPointer, ToPyObject},
-    ffi::PyBaseObject_Type,
     type_object::get_tp_free,
     PyTypeInfo,
 };
@@ -967,15 +966,23 @@ where
 {
     fn ensure_threadsafe(&self) {}
     unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject) {
+        let type_obj = T::type_object_raw(py);
         // For `#[pyclass]` types which inherit from PyAny, we can just call tp_free
-        if T::type_object_raw(py) == &mut PyBaseObject_Type {
+        if type_obj == std::ptr::addr_of_mut!(ffi::PyBaseObject_Type) {
             return get_tp_free(ffi::Py_TYPE(slf))(slf as _);
         }
 
         // More complex native types (e.g. `extends=PyDict`) require calling the base's dealloc.
         #[cfg(not(Py_LIMITED_API))]
         {
-            if let Some(dealloc) = (*T::type_object_raw(py)).tp_dealloc {
+            if let Some(dealloc) = (*type_obj).tp_dealloc {
+                // Before CPython 3.11 BaseException_dealloc would use Py_GC_UNTRACK which
+                // assumes the exception is currently GC tracked, so we have to re-track
+                // before calling the dealloc so that it can safely call Py_GC_UNTRACK.
+                #[cfg(not(any(Py_3_11, PyPy)))]
+                if ffi::PyType_FastSubclass(type_obj, ffi::Py_TPFLAGS_BASE_EXC_SUBCLASS) == 1 {
+                    ffi::PyObject_GC_Track(slf.cast());
+                }
                 dealloc(slf as _);
             } else {
                 get_tp_free(ffi::Py_TYPE(slf))(slf as _);

src/pycell/impl_.rs

@@ -174,8 +174,7 @@ impl<T: PyClassImpl<PyClassMutability = Self>> GetBorrowChecker<T> for Immutable
 impl<T: PyClassImpl<PyClassMutability = Self>, M: PyClassMutability> GetBorrowChecker<T>
     for ExtendsMutableAncestor<M>
 where
-    T::BaseType: PyClassImpl<Layout = PyCell<T::BaseType>>
-        + PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
+    T::BaseType: PyClassImpl + PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
     <T::BaseType as PyClassImpl>::PyClassMutability: PyClassMutability<Checker = BorrowChecker>,
 {
     fn borrow_checker(cell: &PyCell<T>) -> &BorrowChecker {
@@ -188,7 +187,6 @@ where
 mod tests {
     use super::*;
 
-    use crate::impl_::pyclass::{PyClassBaseType, PyClassImpl};
     use crate::prelude::*;
     use crate::pyclass::boolean_struct::{False, True};
     use crate::PyClass;
@@ -239,25 +237,11 @@ mod tests {
     fn assert_immutable<T: PyClass<Frozen = True, PyClassMutability = ImmutableClass>>() {}
     fn assert_mutable_with_mutable_ancestor<
         T: PyClass<Frozen = False, PyClassMutability = ExtendsMutableAncestor<MutableClass>>,
-    >()
-    // These horrible bounds are necessary for Rust 1.48 but not newer versions
-    where
-        <T as PyClassImpl>::BaseType: PyClassImpl<Layout = PyCell<T::BaseType>>,
-        <<T as PyClassImpl>::BaseType as PyClassImpl>::PyClassMutability:
-            PyClassMutability<Checker = BorrowChecker>,
-        <T as PyClassImpl>::BaseType: PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
-    {
+    >() {
     }
     fn assert_immutable_with_mutable_ancestor<
         T: PyClass<Frozen = True, PyClassMutability = ExtendsMutableAncestor<ImmutableClass>>,
-    >()
-    // These horrible bounds are necessary for Rust 1.48 but not newer versions
-    where
-        <T as PyClassImpl>::BaseType: PyClassImpl<Layout = PyCell<T::BaseType>>,
-        <<T as PyClassImpl>::BaseType as PyClassImpl>::PyClassMutability:
-            PyClassMutability<Checker = BorrowChecker>,
-        <T as PyClassImpl>::BaseType: PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
-    {
+    >() {
     }
 
     #[test]

src/pyclass.rs

@@ -15,9 +15,7 @@ pub use self::gc::{PyTraverseError, PyVisit};
 ///
 /// The `#[pyclass]` attribute implements this trait for your Rust struct -
 /// you shouldn't implement this trait directly.
-pub trait PyClass:
-    PyTypeInfo<AsRefTarget = PyCell<Self>> + PyClassImpl<Layout = PyCell<Self>>
-{
+pub trait PyClass: PyTypeInfo<AsRefTarget = PyCell<Self>> + PyClassImpl {
     /// Whether the pyclass is frozen.
     ///
     /// This can be enabled via `#[pyclass(frozen)]`.

src/pyclass/create_type_object.rs

@@ -1,16 +1,18 @@
+use pyo3_ffi::PyType_IS_GC;
+
 use crate::{
     exceptions::PyTypeError,
     ffi,
     impl_::pyclass::{
         assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc,
-        PyClassItemsIter,
+        tp_dealloc_with_gc, PyClassItemsIter,
     },
     impl_::{
         pymethods::{get_doc, get_name, Getter, Setter},
         trampoline::trampoline,
     },
     types::PyType,
-    Py, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python,
+    Py, PyCell, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python,
 };
 use std::{
     borrow::Cow,
@@ -32,22 +34,37 @@ where
     T: PyClass,
 {
     unsafe {
-        PyTypeBuilder::default()
-            .type_doc(T::doc(py)?)
-            .offsets(T::dict_offset(), T::weaklist_offset())
-            .slot(ffi::Py_tp_base, T::BaseType::type_object_raw(py))
-            .slot(ffi::Py_tp_dealloc, tp_dealloc::<T> as *mut c_void)
-            .set_is_basetype(T::IS_BASETYPE)
-            .set_is_mapping(T::IS_MAPPING)
-            .set_is_sequence(T::IS_SEQUENCE)
-            .class_items(T::items_iter())
-            .build(py, T::NAME, T::MODULE, std::mem::size_of::<T::Layout>())
+        PyTypeBuilder {
+            slots: Vec::new(),
+            method_defs: Vec::new(),
+            getset_builders: HashMap::new(),
+            cleanup: Vec::new(),
+            tp_base: T::BaseType::type_object_raw(py),
+            tp_dealloc: tp_dealloc::<T>,
+            tp_dealloc_with_gc: tp_dealloc_with_gc::<T>,
+            is_mapping: T::IS_MAPPING,
+            is_sequence: T::IS_SEQUENCE,
+            has_new: false,
+            has_dealloc: false,
+            has_getitem: false,
+            has_setitem: false,
+            has_traverse: false,
+            has_clear: false,
+            has_dict: false,
+            class_flags: 0,
+            #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))]
+            buffer_procs: Default::default(),
+        }
+        .type_doc(T::doc(py)?)
+        .offsets(T::dict_offset(), T::weaklist_offset())
+        .set_is_basetype(T::IS_BASETYPE)
+        .class_items(T::items_iter())
+        .build(py, T::NAME, T::MODULE, std::mem::size_of::<PyCell<T>>())
     }
 }
 
 type PyTypeBuilderCleanup = Box<dyn Fn(&PyTypeBuilder, *mut ffi::PyTypeObject)>;
 
-#[derive(Default)]
 struct PyTypeBuilder {
     slots: Vec<ffi::PyType_Slot>,
     method_defs: Vec<ffi::PyMethodDef>,
@@ -56,6 +73,9 @@ struct PyTypeBuilder {
     /// PyType_FromSpec API for... there's no reason this should work,
     /// except for that it does and we have tests.
     cleanup: Vec<PyTypeBuilderCleanup>,
+    tp_base: *mut ffi::PyTypeObject,
+    tp_dealloc: ffi::destructor,
+    tp_dealloc_with_gc: ffi::destructor,
     is_mapping: bool,
     is_sequence: bool,
     has_new: bool,
@@ -114,13 +134,6 @@ impl PyTypeBuilder {
         }
     }
 
-    /// # Safety
-    /// The given pointer must be of the correct type for the given slot
-    unsafe fn slot<T>(mut self, slot: c_int, pfunc: *mut T) -> Self {
-        self.push_slot(slot, pfunc);
-        self
-    }
-
     fn pymethod_def(&mut self, def: &PyMethodDefType) {
         match def {
             PyMethodDefType::Getter(getter) => {
@@ -220,16 +233,6 @@ impl PyTypeBuilder {
         self
     }
 
-    fn set_is_mapping(mut self, is_mapping: bool) -> Self {
-        self.is_mapping = is_mapping;
-        self
-    }
-
-    fn set_is_sequence(mut self, is_sequence: bool) -> Self {
-        self.is_sequence = is_sequence;
-        self
-    }
-
     /// # Safety
     /// All slots in the PyClassItemsIter should be correct
     unsafe fn class_items(mut self, iter: PyClassItemsIter) -> Self {
@@ -342,15 +345,19 @@ impl PyTypeBuilder {
 
         let getset_destructors = self.finalize_methods_and_properties()?;
 
+        unsafe { self.push_slot(ffi::Py_tp_base, self.tp_base) }
+
         if !self.has_new {
             // Safety: This is the correct slot type for Py_tp_new
             unsafe { self.push_slot(ffi::Py_tp_new, no_constructor_defined as *mut c_void) }
         }
 
-        assert!(
-            self.has_dealloc,
-            "PyTypeBuilder requires you to specify slot ffi::Py_tp_dealloc"
-        );
+        let tp_dealloc = if self.has_traverse || unsafe { PyType_IS_GC(self.tp_base) == 1 } {
+            self.tp_dealloc_with_gc
+        } else {
+            self.tp_dealloc
+        };
+        unsafe { self.push_slot(ffi::Py_tp_dealloc, tp_dealloc as *mut c_void) }
 
         if self.has_clear && !self.has_traverse {
             return Err(PyTypeError::new_err(format!(