From 676295b8deb23e869665d268821b37f9e7f13438 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 10 Feb 2022 21:28:42 +0000 Subject: [PATCH 1/2] pymethods: support gc protocol --- CHANGELOG.md | 1 + guide/src/class/protocols.md | 3 +- pyo3-macros-backend/src/pyclass.rs | 21 --- pyo3-macros-backend/src/pymethod.rs | 208 ++++++++++++++++------------ src/callback.rs | 15 ++ src/class/gc.rs | 16 +++ tests/test_gc.rs | 14 +- 7 files changed, 157 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e466334397..507d3d4f5de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `wrap_pyfunction!` can now wrap a `#[pyfunction]` which is implemented in a different Rust module or crate. [#2091](https://github.com/PyO3/pyo3/pull/2091) - Add `PyAny::contains` method (`in` operator for `PyAny`). [#2115](https://github.com/PyO3/pyo3/pull/2115) - Add `PyMapping::contains` method (`in` operator for `PyMapping`). [#2133](https://github.com/PyO3/pyo3/pull/2133) +- Add garbage collection magic methods `__traverse__` and `__clear__` to `#[pymethods]`. [#2159](https://github.com/PyO3/pyo3/pull/2159) ### Changed diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md index d0d570decc4..61a452ceda8 100644 --- a/guide/src/class/protocols.md +++ b/guide/src/class/protocols.md @@ -175,7 +175,8 @@ given signatures should be interpreted as follows: #### Garbage Collector Integration -TODO; see [#1884](https://github.com/PyO3/pyo3/issues/1884) + - `__traverse__(, visit: pyo3::class::gc::PyVisit) -> Result<(), pyo3::class::gc::PyTraverseError>` + - `__clear__() -> ()` ### `#[pyproto]` traits diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 74f8b9452d8..9d909b3d542 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -757,7 +757,6 @@ impl<'a> PyClassImplsBuilder<'a> { self.impl_into_py(), self.impl_pyclassimpl(), self.impl_freelist(), - self.impl_gc(), ] .into_iter() .collect() @@ -981,26 +980,6 @@ impl<'a> PyClassImplsBuilder<'a> { Vec::new() } } - - /// Enforce at compile time that PyGCProtocol is implemented - fn impl_gc(&self) -> TokenStream { - let cls = self.cls; - let attr = self.attr; - if attr.is_gc { - let closure_name = format!("__assertion_closure_{}", cls); - let closure_token = syn::Ident::new(&closure_name, Span::call_site()); - quote! { - fn #closure_token() { - use _pyo3::class; - - fn _assert_implements_protocol<'p, T: _pyo3::class::PyGCProtocol<'p>>() {} - _assert_implements_protocol::<#cls>(); - } - } - } else { - quote! {} - } - } } fn define_inventory_class(inventory_class_name: &syn::Ident) -> TokenStream { diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 4ec87956c61..297f206b370 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -36,14 +36,86 @@ enum PyMethodKind { impl PyMethodKind { fn from_name(name: &str) -> Self { - if let Some(slot_def) = pyproto(name) { - PyMethodKind::Proto(PyMethodProtoKind::Slot(slot_def)) - } else if name == "__call__" { - PyMethodKind::Proto(PyMethodProtoKind::Call) - } else if let Some(slot_fragment_def) = pyproto_fragment(name) { - PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(slot_fragment_def)) - } else { - PyMethodKind::Fn + match name { + // Protocol implemented through slots + "__getattr__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__GETATTR__)), + "__str__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__STR__)), + "__repr__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__REPR__)), + "__hash__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__HASH__)), + "__richcmp__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__RICHCMP__)), + "__get__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__GET__)), + "__iter__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__ITER__)), + "__next__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__NEXT__)), + "__await__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__AWAIT__)), + "__aiter__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__AITER__)), + "__anext__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__ANEXT__)), + "__len__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__LEN__)), + "__contains__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__CONTAINS__)), + "__getitem__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__GETITEM__)), + "__pos__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__POS__)), + "__neg__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__NEG__)), + "__abs__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__ABS__)), + "__invert__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__INVERT__)), + "__index__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__INDEX__)), + "__int__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__INT__)), + "__float__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__FLOAT__)), + "__bool__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__BOOL__)), + "__iadd__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IADD__)), + "__isub__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__ISUB__)), + "__imul__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IMUL__)), + "__imatmul__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IMATMUL__)), + "__itruediv__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__ITRUEDIV__)), + "__ifloordiv__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IFLOORDIV__)), + "__imod__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IMOD__)), + "__ipow__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IPOW__)), + "__ilshift__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__ILSHIFT__)), + "__irshift__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IRSHIFT__)), + "__iand__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IAND__)), + "__ixor__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IXOR__)), + "__ior__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__IOR__)), + "__getbuffer__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__GETBUFFER__)), + "__releasebuffer__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__RELEASEBUFFER__)), + "__clear__" => PyMethodKind::Proto(PyMethodProtoKind::Slot(&__CLEAR__)), + // Protocols implemented through traits + "__setattr__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__SETATTR__)), + "__delattr__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__DELATTR__)), + "__set__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__SET__)), + "__delete__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__DELETE__)), + "__setitem__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__SETITEM__)), + "__delitem__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__DELITEM__)), + "__add__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__ADD__)), + "__radd__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RADD__)), + "__sub__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__SUB__)), + "__rsub__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RSUB__)), + "__mul__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__MUL__)), + "__rmul__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RMUL__)), + "__matmul__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__MATMUL__)), + "__rmatmul__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RMATMUL__)), + "__floordiv__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__FLOORDIV__)), + "__rfloordiv__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RFLOORDIV__)), + "__truediv__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__TRUEDIV__)), + "__rtruediv__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RTRUEDIV__)), + "__divmod__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__DIVMOD__)), + "__rdivmod__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RDIVMOD__)), + "__mod__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__MOD__)), + "__rmod__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RMOD__)), + "__lshift__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__LSHIFT__)), + "__rlshift__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RLSHIFT__)), + "__rshift__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RSHIFT__)), + "__rrshift__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RRSHIFT__)), + "__and__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__AND__)), + "__rand__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RAND__)), + "__xor__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__XOR__)), + "__rxor__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RXOR__)), + "__or__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__OR__)), + "__ror__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__ROR__)), + "__pow__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__POW__)), + "__rpow__" => PyMethodKind::Proto(PyMethodProtoKind::SlotFragment(&__RPOW__)), + // Some tricky protocols which don't fit the pattern of the rest + "__call__" => PyMethodKind::Proto(PyMethodProtoKind::Call), + "__traverse__" => PyMethodKind::Proto(PyMethodProtoKind::Traverse), + // Not a proto + _ => PyMethodKind::Fn, } } } @@ -51,6 +123,7 @@ impl PyMethodKind { enum PyMethodProtoKind { Slot(&'static SlotDef), Call, + Traverse, SlotFragment(&'static SlotFragmentDef), } @@ -108,6 +181,9 @@ pub fn gen_py_method( PyMethodProtoKind::Call => { GeneratedPyMethod::Proto(impl_call_slot(cls, method.spec)?) } + PyMethodProtoKind::Traverse => { + GeneratedPyMethod::Proto(impl_traverse_slot(cls, method.spec)?) + } PyMethodProtoKind::SlotFragment(slot_fragment_def) => { let proto = slot_fragment_def.generate_pyproto_fragment(cls, spec)?; GeneratedPyMethod::SlotTraitImpl(method.method_name, proto) @@ -220,6 +296,36 @@ fn impl_call_slot(cls: &syn::Type, mut spec: FnSpec) -> Result { }}) } +fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec) -> Result { + let ident = spec.name; + Ok(quote! {{ + pub unsafe extern "C" fn __wrap_( + slf: *mut _pyo3::ffi::PyObject, + visit: _pyo3::ffi::visitproc, + arg: *mut ::std::os::raw::c_void, + ) -> ::std::os::raw::c_int + { + let pool = _pyo3::GILPool::new(); + let py = pool.python(); + _pyo3::callback::abort_on_traverse_panic(::std::panic::catch_unwind(move || { + let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf); + + let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py); + let borrow = slf.try_borrow(); + if let ::std::result::Result::Ok(borrow) = borrow { + _pyo3::class::gc::unwrap_traverse_result(borrow.#ident(visit)) + } else { + 0 + } + })) + } + _pyo3::ffi::PyType_Slot { + slot: _pyo3::ffi::Py_tp_traverse, + pfunc: __wrap_ as _pyo3::ffi::traverseproc as _ + } + }}) +} + fn impl_py_class_attribute(cls: &syn::Type, spec: &FnSpec) -> TokenStream { let name = &spec.name; let deprecations = &spec.deprecations; @@ -567,49 +673,9 @@ const __RELEASEBUFFER__: SlotDef = SlotDef::new("Py_bf_releasebuffer", "releaseb .arguments(&[Ty::PyBuffer]) .ret_ty(Ty::Void) .require_unsafe(); - -fn pyproto(method_name: &str) -> Option<&'static SlotDef> { - match method_name { - "__getattr__" => Some(&__GETATTR__), - "__str__" => Some(&__STR__), - "__repr__" => Some(&__REPR__), - "__hash__" => Some(&__HASH__), - "__richcmp__" => Some(&__RICHCMP__), - "__get__" => Some(&__GET__), - "__iter__" => Some(&__ITER__), - "__next__" => Some(&__NEXT__), - "__await__" => Some(&__AWAIT__), - "__aiter__" => Some(&__AITER__), - "__anext__" => Some(&__ANEXT__), - "__len__" => Some(&__LEN__), - "__contains__" => Some(&__CONTAINS__), - "__getitem__" => Some(&__GETITEM__), - "__pos__" => Some(&__POS__), - "__neg__" => Some(&__NEG__), - "__abs__" => Some(&__ABS__), - "__invert__" => Some(&__INVERT__), - "__index__" => Some(&__INDEX__), - "__int__" => Some(&__INT__), - "__float__" => Some(&__FLOAT__), - "__bool__" => Some(&__BOOL__), - "__iadd__" => Some(&__IADD__), - "__isub__" => Some(&__ISUB__), - "__imul__" => Some(&__IMUL__), - "__imatmul__" => Some(&__IMATMUL__), - "__itruediv__" => Some(&__ITRUEDIV__), - "__ifloordiv__" => Some(&__IFLOORDIV__), - "__imod__" => Some(&__IMOD__), - "__ipow__" => Some(&__IPOW__), - "__ilshift__" => Some(&__ILSHIFT__), - "__irshift__" => Some(&__IRSHIFT__), - "__iand__" => Some(&__IAND__), - "__ixor__" => Some(&__IXOR__), - "__ior__" => Some(&__IOR__), - "__getbuffer__" => Some(&__GETBUFFER__), - "__releasebuffer__" => Some(&__RELEASEBUFFER__), - _ => None, - } -} +const __CLEAR__: SlotDef = SlotDef::new("Py_tp_clear", "inquiry") + .arguments(&[]) + .ret_ty(Ty::Int); #[derive(Clone, Copy)] enum Ty { @@ -1045,46 +1111,6 @@ const __RPOW__: SlotFragmentDef = SlotFragmentDef::new("__rpow__", &[Ty::Object, .extract_error_mode(ExtractErrorMode::NotImplemented) .ret_ty(Ty::Object); -fn pyproto_fragment(method_name: &str) -> Option<&'static SlotFragmentDef> { - match method_name { - "__setattr__" => Some(&__SETATTR__), - "__delattr__" => Some(&__DELATTR__), - "__set__" => Some(&__SET__), - "__delete__" => Some(&__DELETE__), - "__setitem__" => Some(&__SETITEM__), - "__delitem__" => Some(&__DELITEM__), - "__add__" => Some(&__ADD__), - "__radd__" => Some(&__RADD__), - "__sub__" => Some(&__SUB__), - "__rsub__" => Some(&__RSUB__), - "__mul__" => Some(&__MUL__), - "__rmul__" => Some(&__RMUL__), - "__matmul__" => Some(&__MATMUL__), - "__rmatmul__" => Some(&__RMATMUL__), - "__floordiv__" => Some(&__FLOORDIV__), - "__rfloordiv__" => Some(&__RFLOORDIV__), - "__truediv__" => Some(&__TRUEDIV__), - "__rtruediv__" => Some(&__RTRUEDIV__), - "__divmod__" => Some(&__DIVMOD__), - "__rdivmod__" => Some(&__RDIVMOD__), - "__mod__" => Some(&__MOD__), - "__rmod__" => Some(&__RMOD__), - "__lshift__" => Some(&__LSHIFT__), - "__rlshift__" => Some(&__RLSHIFT__), - "__rshift__" => Some(&__RSHIFT__), - "__rrshift__" => Some(&__RRSHIFT__), - "__and__" => Some(&__AND__), - "__rand__" => Some(&__RAND__), - "__xor__" => Some(&__XOR__), - "__rxor__" => Some(&__RXOR__), - "__or__" => Some(&__OR__), - "__ror__" => Some(&__ROR__), - "__pow__" => Some(&__POW__), - "__rpow__" => Some(&__RPOW__), - _ => None, - } -} - fn extract_proto_arguments( cls: &syn::Type, py: &syn::Ident, diff --git a/src/callback.rs b/src/callback.rs index 4b54de81fc5..e32def03364 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -266,3 +266,18 @@ where R::ERR_VALUE }) } + +/// Aborts if panic has occurred. Used inside `__traverse__` implementations, where panicking is not possible. +#[doc(hidden)] +#[inline] +pub fn abort_on_traverse_panic( + panic_result: Result>, +) -> c_int { + match panic_result { + Ok(traverse_result) => traverse_result, + Err(_payload) => { + eprintln!("FATAL: panic inside __traverse__ handler; aborting."); + ::std::process::abort() + } + } +} diff --git a/src/class/gc.rs b/src/class/gc.rs index 2641a2b3f21..81e761fe0a8 100644 --- a/src/class/gc.rs +++ b/src/class/gc.rs @@ -83,4 +83,20 @@ impl<'p> PyVisit<'p> { Err(PyTraverseError(r)) } } + + /// Creates the PyVisit from the arguments to tp_traverse + #[doc(hidden)] + pub unsafe fn from_raw(visit: ffi::visitproc, arg: *mut c_void, _py: Python<'p>) -> Self { + Self { visit, arg, _py } + } +} + +/// Unwraps the result of __traverse__ for tp_traverse +#[doc(hidden)] +#[inline] +pub fn unwrap_traverse_result(result: Result<(), PyTraverseError>) -> c_int { + match result { + Ok(()) => 0, + Err(PyTraverseError(value)) => value, + } } diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 92854473211..d137d04b278 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -1,7 +1,5 @@ #![cfg(feature = "macros")] -#![cfg(feature = "pyproto")] // FIXME: #[pymethods] to support gc protocol -use pyo3::class::PyGCProtocol; use pyo3::class::PyTraverseError; use pyo3::class::PyVisit; use pyo3::prelude::*; @@ -90,8 +88,8 @@ struct GcIntegration { dropped: TestDropCall, } -#[pyproto] -impl PyGCProtocol for GcIntegration { +#[pymethods] +impl GcIntegration { fn __traverse__(&self, visit: PyVisit) -> Result<(), PyTraverseError> { visit.call(&self.self_ref) } @@ -133,8 +131,8 @@ fn gc_integration() { #[pyclass(gc)] struct GcIntegration2 {} -#[pyproto] -impl PyGCProtocol for GcIntegration2 { +#[pymethods] +impl GcIntegration2 { fn __traverse__(&self, _visit: PyVisit) -> Result<(), PyTraverseError> { Ok(()) } @@ -230,8 +228,8 @@ impl TraversableClass { } } -#[pyproto] -impl PyGCProtocol for TraversableClass { +#[pymethods] +impl TraversableClass { fn __clear__(&mut self) {} fn __traverse__(&self, _visit: PyVisit) -> Result<(), PyTraverseError> { self.traversed.store(true, Ordering::Relaxed); From 79123b396c4755320e35fc3761c504ffe1fd1da6 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Fri, 11 Feb 2022 08:31:06 +0000 Subject: [PATCH 2/2] pyclass: deprecate gc option --- CHANGELOG.md | 1 + guide/src/class.md | 1 - guide/src/class/protocols.md | 13 ------ pyo3-macros-backend/src/deprecations.rs | 2 + pyo3-macros-backend/src/pyclass.rs | 17 +++---- pyo3-macros/src/lib.rs | 12 +++-- src/impl_/deprecations.rs | 6 +++ src/impl_/pyclass.rs | 3 -- src/pyclass.rs | 59 +++++++++++++++---------- tests/test_gc.rs | 4 +- tests/test_gc_pyproto.rs | 4 +- tests/ui/deprecations.rs | 3 ++ tests/ui/deprecations.stderr | 6 +++ 13 files changed, 71 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 507d3d4f5de..5c2cb6eebe2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `PyTZInfo_CheckExact` - `PyDateTime_FromTimestamp` - `PyDate_FromTimestamp` +- Deprecate the `gc` option for `pyclass` (e.g. `#[pyclass(gc)]`). Just implement a `__traverse__` `#[pymethod]`. [#2159](https://github.com/PyO3/pyo3/pull/2159) - The `ml_meth` field of `PyMethodDef` is now represented by the `PyMethodDefPointer` union [2166](https://github.com/PyO3/pyo3/pull/2166) ### Removed diff --git a/guide/src/class.md b/guide/src/class.md index 819b3aea782..08f8c3df77b 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -967,7 +967,6 @@ impl pyo3::IntoPy for MyClass { impl pyo3::impl_::pyclass::PyClassImpl for MyClass { const DOC: &'static str = "Class for demonstration\u{0}"; - const IS_GC: bool = false; const IS_BASETYPE: bool = false; const IS_SUBCLASS: bool = false; type Layout = PyCell; diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md index 61a452ceda8..e45662878fe 100644 --- a/guide/src/class/protocols.md +++ b/guide/src/class/protocols.md @@ -445,19 +445,6 @@ impl PyGCProtocol for ClassWithGCSupport { } ``` -Special protocol trait implementations have to be annotated with the `#[pyproto]` attribute. - -It is also possible to enable GC for custom classes using the `gc` parameter of the `pyclass` attribute. -i.e. `#[pyclass(gc)]`. In that case instances of custom class participate in Python garbage -collection, and it is possible to track them with `gc` module methods. When using the `gc` parameter, -it is *required* to implement the `PyGCProtocol` trait, failure to do so will result in an error -at compile time: - -```compile_fail -#[pyclass(gc)] -struct GCTracked {} // Fails because it does not implement PyGCProtocol -``` - #### Iterator Types Iterators can be defined using the diff --git a/pyo3-macros-backend/src/deprecations.rs b/pyo3-macros-backend/src/deprecations.rs index 3d5fe878510..ab164a027f3 100644 --- a/pyo3-macros-backend/src/deprecations.rs +++ b/pyo3-macros-backend/src/deprecations.rs @@ -3,12 +3,14 @@ use quote::{quote_spanned, ToTokens}; pub enum Deprecation { CallAttribute, + PyClassGcOption, } impl Deprecation { fn ident(&self, span: Span) -> syn::Ident { let string = match self { Deprecation::CallAttribute => "CALL_ATTRIBUTE", + Deprecation::PyClassGcOption => "PYCLASS_GC_OPTION", }; syn::Ident::new(string, span) } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 9d909b3d542..ff0085a8fc3 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -3,7 +3,7 @@ use crate::attributes::{ self, take_pyo3_options, CrateAttribute, NameAttribute, TextSignatureAttribute, }; -use crate::deprecations::Deprecations; +use crate::deprecations::{Deprecation, Deprecations}; use crate::konst::{ConstAttributes, ConstSpec}; use crate::pyimpl::{gen_default_items, gen_py_const, PyClassMethodsType}; use crate::pymethod::{impl_py_getter_def, impl_py_setter_def, PropertyType}; @@ -29,12 +29,12 @@ pub struct PyClassArgs { pub base: syn::TypePath, pub has_dict: bool, pub has_weaklist: bool, - pub is_gc: bool, pub is_basetype: bool, pub has_extends: bool, pub has_unsendable: bool, pub module: Option, pub class_kind: PyClassKind, + pub deprecations: Deprecations, } impl PyClassArgs { @@ -63,11 +63,11 @@ impl PyClassArgs { base: parse_quote! { _pyo3::PyAny }, has_dict: false, has_weaklist: false, - is_gc: false, is_basetype: false, has_extends: false, has_unsendable: false, class_kind, + deprecations: Deprecations::new(), } } @@ -158,9 +158,9 @@ impl PyClassArgs { fn add_path(&mut self, exp: &syn::ExprPath) -> syn::Result<()> { let flag = exp.path.segments.first().unwrap().ident.to_string(); match flag.as_str() { - "gc" => { - self.is_gc = true; - } + "gc" => self + .deprecations + .push(Deprecation::PyClassGcOption, exp.span()), "weakref" => { self.has_weaklist = true; } @@ -825,7 +825,6 @@ impl<'a> PyClassImplsBuilder<'a> { fn impl_pyclassimpl(&self) -> TokenStream { let cls = self.cls; let doc = self.doc.as_ref().map_or(quote! {"\0"}, |doc| quote! {#doc}); - let is_gc = self.attr.is_gc; let is_basetype = self.attr.is_basetype; let base = &self.attr.base; let is_subclass = self.attr.has_extends; @@ -903,10 +902,11 @@ impl<'a> PyClassImplsBuilder<'a> { let default_slots = &self.default_slots; let freelist_slots = self.freelist_slots(); + let deprecations = &self.attr.deprecations; + quote! { impl _pyo3::impl_::pyclass::PyClassImpl for #cls { const DOC: &'static str = #doc; - const IS_GC: bool = #is_gc; const IS_BASETYPE: bool = #is_basetype; const IS_SUBCLASS: bool = #is_subclass; @@ -918,6 +918,7 @@ impl<'a> PyClassImplsBuilder<'a> { fn for_all_items(visitor: &mut dyn ::std::ops::FnMut(& _pyo3::impl_::pyclass::PyClassItems)) { use _pyo3::impl_::pyclass::*; let collector = PyClassImplCollector::::new(); + #deprecations; static INTRINSIC_ITEMS: PyClassItems = PyClassItems { methods: &[#(#default_methods),*], slots: &[#(#default_slots),* #(#freelist_slots),*], diff --git a/pyo3-macros/src/lib.rs b/pyo3-macros/src/lib.rs index f14e8170d9e..0673805345f 100644 --- a/pyo3-macros/src/lib.rs +++ b/pyo3-macros/src/lib.rs @@ -86,12 +86,11 @@ pub fn pyproto(_: TokenStream, input: TokenStream) -> TokenStream { /// | Parameter | Description | /// | :- | :- | /// | `name = "python_name"` | Sets the name that Python sees this class as. Defaults to the name of the Rust struct. | -/// | `freelist = N` | Implements a [free list][10] of size N. This can improve performance for types that are often created and deleted in quick succession. Profile your code to see whether `freelist` is right for you. | -/// | `gc` | Participate in Python's [garbage collection][5]. Required if your type contains references to other Python objects. If you don't (or incorrectly) implement this, contained Python objects may be hidden from Python's garbage collector and you may leak memory. Note that leaking memory, while undesirable, [is safe behavior][7].| +/// | `freelist = N` | Implements a [free list][9] of size N. This can improve performance for types that are often created and deleted in quick succession. Profile your code to see whether `freelist` is right for you. | /// | `weakref` | Allows this class to be [weakly referenceable][6]. | /// | `extends = BaseType` | Use a custom baseclass. Defaults to [`PyAny`][4] | /// | `subclass` | Allows other Python classes and `#[pyclass]` to inherit from this class. Enums cannot be subclassed. | -/// | `unsendable` | Required if your struct is not [`Send`][3]. Rather than using `unsendable`, consider implementing your struct in a threadsafe way by e.g. substituting [`Rc`][8] with [`Arc`][9]. By using `unsendable`, your class will panic when accessed by another thread.| +/// | `unsendable` | Required if your struct is not [`Send`][3]. Rather than using `unsendable`, consider implementing your struct in a threadsafe way by e.g. substituting [`Rc`][7] with [`Arc`][8]. By using `unsendable`, your class will panic when accessed by another thread.| /// | `module = "module_name"` | Python code will see the class as being defined in this module. Defaults to `builtins`. | /// /// For more on creating Python classes, @@ -103,10 +102,9 @@ pub fn pyproto(_: TokenStream, input: TokenStream) -> TokenStream { /// [4]: ../prelude/struct.PyAny.html /// [5]: https://pyo3.rs/latest/class/protocols.html#garbage-collector-integration /// [6]: https://docs.python.org/3/library/weakref.html -/// [7]: https://doc.rust-lang.org/nomicon/leaking.html -/// [8]: std::rc::Rc -/// [9]: std::sync::Arc -/// [10]: https://en.wikipedia.org/wiki/Free_list +/// [7]: std::rc::Rc +/// [8]: std::sync::Arc +/// [9]: https://en.wikipedia.org/wiki/Free_list #[proc_macro_attribute] pub fn pyclass(attr: TokenStream, input: TokenStream) -> TokenStream { use syn::Item; diff --git a/src/impl_/deprecations.rs b/src/impl_/deprecations.rs index c0faa0cf4ed..72c8ff994e2 100644 --- a/src/impl_/deprecations.rs +++ b/src/impl_/deprecations.rs @@ -2,3 +2,9 @@ #[deprecated(since = "0.15.0", note = "use `fn __call__` instead of `#[call]`")] pub const CALL_ATTRIBUTE: () = (); + +#[deprecated( + since = "0.16.0", + note = "implement a `__traverse__` `#[pymethod]` instead of using `gc` option" +)] +pub const PYCLASS_GC_OPTION: () = (); diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 7b1c8ef0e8c..abf7fb26f11 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -149,9 +149,6 @@ pub trait PyClassImpl: Sized { /// Class doc string const DOC: &'static str = "\0"; - /// #[pyclass(gc)] - const IS_GC: bool = false; - /// #[pyclass(subclass)] const IS_BASETYPE: bool = false; diff --git a/src/pyclass.rs b/src/pyclass.rs index db93239f704..a327729574c 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -1,6 +1,7 @@ //! `PyClass` and related traits. use crate::{ callback::IntoPyCallbackOutput, + exceptions::PyTypeError, ffi, impl_::pyclass::{ assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc, PyClassDict, @@ -53,7 +54,6 @@ where T::dict_offset(), T::weaklist_offset(), &T::for_all_items, - T::IS_GC, T::IS_BASETYPE, ) } { @@ -74,7 +74,6 @@ unsafe fn create_type_object_impl( dict_offset: Option, weaklist_offset: Option, for_all_items: &dyn Fn(&mut dyn FnMut(&PyClassItems)), - is_gc: bool, is_basetype: bool, ) -> PyResult<*mut ffi::PyTypeObject> { let mut slots = Vec::new(); @@ -117,7 +116,8 @@ unsafe fn create_type_object_impl( let mut has_new = false; let mut has_getitem = false; let mut has_setitem = false; - let mut has_gc_methods = false; + let mut has_traverse = false; + let mut has_clear = false; // Before Python 3.9, need to patch in buffer methods manually (they don't work in slots) #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] @@ -125,20 +125,23 @@ unsafe fn create_type_object_impl( for_all_items(&mut |items| { for slot in items.slots { - has_new |= slot.slot == ffi::Py_tp_new; - has_getitem |= slot.slot == ffi::Py_mp_subscript; - has_setitem |= slot.slot == ffi::Py_mp_ass_subscript; - has_gc_methods |= slot.slot == ffi::Py_tp_clear || slot.slot == ffi::Py_tp_traverse; - #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] - if slot.slot == ffi::Py_bf_getbuffer { - // Safety: slot.pfunc is a valid function pointer - buffer_procs.bf_getbuffer = Some(std::mem::transmute(slot.pfunc)); - } - - #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] - if slot.slot == ffi::Py_bf_releasebuffer { - // Safety: slot.pfunc is a valid function pointer - buffer_procs.bf_releasebuffer = Some(std::mem::transmute(slot.pfunc)); + match slot.slot { + ffi::Py_tp_new => has_new = true, + ffi::Py_mp_subscript => has_getitem = true, + ffi::Py_mp_ass_subscript => has_setitem = true, + ffi::Py_tp_traverse => has_traverse = true, + ffi::Py_tp_clear => has_clear = true, + #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] + ffi::Py_bf_getbuffer => { + // Safety: slot.pfunc is a valid function pointer + buffer_procs.bf_getbuffer = Some(std::mem::transmute(slot.pfunc)); + } + #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))] + ffi::Py_bf_releasebuffer => { + // Safety: slot.pfunc is a valid function pointer + buffer_procs.bf_releasebuffer = Some(std::mem::transmute(slot.pfunc)); + } + _ => {} } } slots.extend_from_slice(items.slots); @@ -170,6 +173,13 @@ unsafe fn create_type_object_impl( push_slot(&mut slots, ffi::Py_tp_new, no_constructor_defined as _); } + if has_clear && !has_traverse { + return Err(PyTypeError::new_err(format!( + "`#[pyclass]` {} implements __clear__ without __traverse__", + name + ))); + } + // Add empty sentinel at the end push_slot(&mut slots, 0, ptr::null_mut()); @@ -177,7 +187,7 @@ unsafe fn create_type_object_impl( name: py_class_qualified_name(module_name, name)?, basicsize: basicsize as c_int, itemsize: 0, - flags: py_class_flags(has_gc_methods, is_gc, is_basetype), + flags: py_class_flags(has_traverse, is_basetype), slots: slots.as_mut_ptr(), }; @@ -287,12 +297,13 @@ fn py_class_qualified_name(module_name: Option<&str>, class_name: &str) -> PyRes .into_raw()) } -fn py_class_flags(has_gc_methods: bool, is_gc: bool, is_basetype: bool) -> c_uint { - let mut flags = if has_gc_methods || is_gc { - ffi::Py_TPFLAGS_DEFAULT | ffi::Py_TPFLAGS_HAVE_GC - } else { - ffi::Py_TPFLAGS_DEFAULT - }; +fn py_class_flags(is_gc: bool, is_basetype: bool) -> c_uint { + let mut flags = ffi::Py_TPFLAGS_DEFAULT; + + if is_gc { + flags |= ffi::Py_TPFLAGS_HAVE_GC; + } + if is_basetype { flags |= ffi::Py_TPFLAGS_BASETYPE; } diff --git a/tests/test_gc.rs b/tests/test_gc.rs index d137d04b278..839517b9618 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -128,7 +128,7 @@ fn gc_integration() { assert!(drop_called.load(Ordering::Relaxed)); } -#[pyclass(gc)] +#[pyclass] struct GcIntegration2 {} #[pymethods] @@ -215,7 +215,7 @@ fn inheritance_with_new_methods_with_drop() { assert!(drop_called2.load(Ordering::Relaxed)); } -#[pyclass(gc)] +#[pyclass] struct TraversableClass { traversed: AtomicBool, } diff --git a/tests/test_gc_pyproto.rs b/tests/test_gc_pyproto.rs index cf6793b78bc..bf0911d80a2 100644 --- a/tests/test_gc_pyproto.rs +++ b/tests/test_gc_pyproto.rs @@ -130,7 +130,7 @@ fn gc_integration() { assert!(drop_called.load(Ordering::Relaxed)); } -#[pyclass(gc)] +#[pyclass] struct GcIntegration2 {} #[pyproto] @@ -217,7 +217,7 @@ fn inheritance_with_new_methods_with_drop() { assert!(drop_called2.load(Ordering::Relaxed)); } -#[pyclass(gc)] +#[pyclass] struct TraversableClass { traversed: AtomicBool, } diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index 5ad70f9c99f..7ad3f98ea62 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -11,6 +11,9 @@ impl DeprecatedCall { fn deprecated_call(&self) {} } +#[pyclass(gc)] +struct DeprecatedGc; + fn main() { } diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index d71c1927a35..c5d6aba3e97 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -9,3 +9,9 @@ note: the lint level is defined here | 1 | #![deny(deprecated)] | ^^^^^^^^^^ + +error: use of deprecated constant `pyo3::impl_::deprecations::PYCLASS_GC_OPTION`: implement a `__traverse__` `#[pymethod]` instead of using `gc` option + --> tests/ui/deprecations.rs:14:11 + | +14 | #[pyclass(gc)] + | ^^