pyclass: deprecate gc option

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

guide/src/class.md

@@ -967,7 +967,6 @@ impl pyo3::IntoPy<PyObject> 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<MyClass>;

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

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)
     }

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<syn::LitStr>,
     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::<Self>::new();
+                    #deprecations;
                     static INTRINSIC_ITEMS: PyClassItems = PyClassItems {
                         methods: &[#(#default_methods),*],
                         slots: &[#(#default_slots),* #(#freelist_slots),*],

pyo3-macros/src/lib.rs

@@ -86,12 +86,11 @@ pub fn pyproto(_: TokenStream, input: TokenStream) -> TokenStream {
 /// |  Parameter  |  Description |
 /// | :-  | :- |
 /// | <span style="white-space: pre">`name = "python_name"`</span> | Sets the name that Python sees this class as. Defaults to the name of the Rust struct. |
-/// | <span style="white-space: pre">`freelist = N`</span> |  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].|
+/// | <span style="white-space: pre">`freelist = N`</span> |  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]. |
 /// | <span style="white-space: pre">`extends = BaseType`</span>  | 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.|
 /// | <span style="white-space: pre">`module = "module_name"`</span> |  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;

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: () = ();

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;
 

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<ffi::Py_ssize_t>,
     weaklist_offset: Option<ffi::Py_ssize_t>,
     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;
     }

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,
 }

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,
 }

tests/ui/deprecations.rs

@@ -11,6 +11,9 @@ impl DeprecatedCall {
     fn deprecated_call(&self) {}
 }
 
+#[pyclass(gc)]
+struct DeprecatedGc;
+
 fn main() {
 
 }

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