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] 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)] + | ^^