From bc724fe78ffdd6408a7151a715860aca6e06d6b4 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 10 Aug 2026 11:22:56 +0200 Subject: [PATCH] fix `tp_clear` of subclass not being installed properly --- src/pyclass/create_type_object.rs | 34 +++++++++++--------- tests/test_gc.rs | 52 +++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 14 deletions(-) diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index f02f158879d..5d68945e2b7 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -3,6 +3,7 @@ use crate::exceptions::PyAttributeError; use crate::impl_::pymethods::{Deleter, PyDeleterDef}; +use crate::internal::get_slot::{get_slot, TP_CLEAR}; use crate::platform::prelude::*; use crate::platform::HashMap; #[cfg(not(Py_3_10))] @@ -391,11 +392,6 @@ impl PyTypeBuilder { self } - fn base_is_gc(&self) -> bool { - // SAFETY: `self.tp_base` is a valid pointer to a `PyTypeObject` - unsafe { ffi::PyType_IS_GC(self.tp_base) == 1 } - } - fn build( mut self, py: Python<'_>, @@ -427,15 +423,25 @@ impl PyTypeBuilder { unsafe { self.push_slot(ffi::Py_tp_traverse, self.tp_traverse as *mut c_void) } // We may need to install a `tp_clear` if this type didn't define one: - if !self.has_clear && - // - if this type is `#[pyclass(dict)]`, to support clearing the dict, or - self.dict_offset.is_some() || - // - if the base is a gc type, to support clearing the base's fields. - self.base_is_gc() - { - let synthesized_clear = self.synthesized_clear; - // Safety: This is the correct slot type for Py_tp_clear - unsafe { self.push_slot(ffi::Py_tp_clear, synthesized_clear as *mut c_void) } + if !self.has_clear { + if self.dict_offset.is_some() { + // (1) if this type is `#[pyclass(dict)]`, to support clearing the dict + // SAFETY: This is the correct slot type for Py_tp_clear + unsafe { self.push_slot(ffi::Py_tp_clear, self.synthesized_clear as *mut c_void) } + } else if let Some(base_clear) = + // SAFETY: `self.tp_base` is known to be a valid pointer to a PyTypeObject + unsafe { get_slot(self.tp_base, TP_CLEAR) } + { + // (2) if the base has a tp_clear, to support clearing the base's fields. + // + // (CPython only inherits the base clear in subclasses if the subclass + // doesn't define any of `Py_TPFLAGS_HAVE_GC`, `tp_traverse`, or `tp_clear` + // itself, but `#[pyclass]` types always have GC and `tp_traverse` set + // so this inheritance doesn't happen automatically.) + + // SAFETY: This is the correct slot type for Py_tp_clear + unsafe { self.push_slot(ffi::Py_tp_clear, base_clear as *mut c_void) } + } } // For sequences, implement sq_length instead of mp_length diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 8ed2080f146..005df14fde6 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -1062,3 +1062,55 @@ fn dict_cycle_collected_with_traverse_and_clear() { check.assert_drops_with_gc(ptr); } + +#[test] +fn test_subclass_clear() { + // An incorrect PyO3 implementation would prevent subclass `__clear__` + // from ever being installed, thus causing this cycle test to leak. + + #[pyclass(subclass)] + struct Base { + _guard: DropGuard, + } + + #[pyclass(extends = Base)] + struct SubClear { + field: Option>, + } + + #[pymethods] + impl SubClear { + fn __traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> { + visit.call(&self.field) + } + + fn __clear__(&mut self) { + self.field = None; + } + } + + let (guard, check) = drop_check(); + + let ptr = Python::attach(|py| { + let base = Base { _guard: guard }; + let obj = Bound::new( + py, + PyClassInitializer::from(base).add_subclass(SubClear { field: None }), + ) + .unwrap(); + obj.borrow_mut().field = Some(obj.clone().into_any().unbind()); + + check.assert_not_dropped(); + let ptr = obj.as_ptr(); + drop(obj); + #[cfg(not(Py_GIL_DISABLED))] + { + // other thread might have caused GC on free-threaded build + check.assert_not_dropped(); + } + + ptr + }); + + check.assert_drops_with_gc(ptr); +}