move away from the FromPyObject blanket for PyClass + Clone (#5488)

* move `FromPyObject` pyclass blanket into macro

* add opt-out option

* add tests

* add newsfragments

* fix typo

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

---------

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Author: Icxolu

guide/pyclass-parameters.md

@@ -21,6 +21,7 @@
 | `rename_all = "renaming_rule"` | Applies renaming rules to every getters and setters of a struct, or every variants of an enum. Possible values are: "camelCase", "kebab-case", "lowercase", "PascalCase", "SCREAMING-KEBAB-CASE", "SCREAMING_SNAKE_CASE", "snake_case", "UPPERCASE". |
 | `sequence` |  Inform PyO3 that this class is a [`Sequence`][params-sequence], and so leave its C-API mapping length slot empty. |
 | `set_all` | Generates setters for all fields of the pyclass. |
+| `skip_from_py_object` | Prevents this PyClass from participating in the `FromPyObject: PyClass + Clone` blanket implementation. This allows a custom `FromPyObject` impl, even if `self` is `Clone`. |
 | `str` | Implements `__str__` using the `Display` implementation of the underlying Rust datatype or by passing an optional format string `str="<format string>"`. *Note: The optional format string is only allowed for structs.  `name` and `rename_all` are incompatible with the optional format string.  Additional details can be found in the discussion on this [PR](https://github.com/PyO3/pyo3/pull/4233).* |
 | `subclass` | Allows other Python classes and `#[pyclass]` to inherit from this class. Enums cannot be subclassed. |
 | `unsendable` | Required if your struct is not [`Send`][params-3]. Rather than using `unsendable`, consider implementing your struct in a thread-safe way by e.g. substituting [`Rc`][params-4] with [`Arc`][params-5]. By using `unsendable`, your class will panic when accessed by another thread. Also note the Python's GC is multi-threaded and while unsendable classes will not be traversed on foreign threads to avoid UB, this can lead to memory leaks. |

guide/src/conversions/traits.md

@@ -535,6 +535,29 @@ struct RustyStruct {
 # }
 ```
 
+#### ⚠ Phase-Out of `FromPyObject` blanket implementation for cloneable PyClasses ⚠
+Historically PyO3 has provided a blanket implementation for `#[pyclass]` types that also implement `Clone`, to allow extraction of such types by value. Over time this has turned out problematic for a few reasons, the major one being the prevention of custom conversions by downstream crates if their type is `Clone`. Over the next few releases the blanket implementation is gradually phased out, and eventually replaced by an opt-in option. As a first step of this migration a new `skip_from_py_object` option for `#[pyclass]` was introduced, to opt-out of the blanket implementation and allow downstream users to provide their own implementation:
+```rust
+# #![allow(dead_code)]
+# use pyo3::prelude::*;
+
+#[pyclass(skip_from_py_object)] // opt-out of the PyO3 FromPyObject blanket
+#[derive(Clone)]
+struct Number(i32);
+
+impl<'py> FromPyObject<'_, 'py> for Number {
+    type Error = PyErr;
+
+    fn extract(obj: pyo3::Borrowed<'_, 'py, pyo3::PyAny>) -> Result<Self, Self::Error> {
+        if let Ok(obj) = obj.cast::<Self>() { // first try extraction via class object
+            Ok(obj.borrow().clone())
+        } else {
+            obj.extract::<i32>().map(Self) // otherwise try integer directly
+        }
+    }
+}
+```
+
 ### `IntoPyObject`
 The [`IntoPyObject`] trait defines the to-python conversion for a Rust type. All types in PyO3 implement this trait,
 as does a `#[pyclass]` which doesn't use `extends`.

newsfragments/5488.added.md

@@ -0,0 +1 @@
+add `skip_from_py_object` pyclass option, to opt-out of the `FromPyObject: PyClass + Clone` blanket impl

pyo3-macros-backend/src/attributes.rs

@@ -53,6 +53,7 @@ pub mod kw {
     syn::custom_keyword!(warn);
     syn::custom_keyword!(message);
     syn::custom_keyword!(category);
+    syn::custom_keyword!(skip_from_py_object);
 }
 
 fn take_int(read: &mut &str, tracker: &mut usize) -> String {

pyo3-macros-backend/src/pyclass.rs

@@ -90,6 +90,7 @@ pub struct PyClassPyO3Options {
     pub unsendable: Option<kw::unsendable>,
     pub weakref: Option<kw::weakref>,
     pub generic: Option<kw::generic>,
+    pub skip_from_py_object: Option<kw::skip_from_py_object>,
 }
 
 pub enum PyClassPyO3Option {
@@ -115,6 +116,7 @@ pub enum PyClassPyO3Option {
     Unsendable(kw::unsendable),
     Weakref(kw::weakref),
     Generic(kw::generic),
+    SkipFromPyObject(kw::skip_from_py_object),
 }
 
 impl Parse for PyClassPyO3Option {
@@ -164,6 +166,8 @@ impl Parse for PyClassPyO3Option {
             input.parse().map(PyClassPyO3Option::Weakref)
         } else if lookahead.peek(attributes::kw::generic) {
             input.parse().map(PyClassPyO3Option::Generic)
+        } else if lookahead.peek(attributes::kw::skip_from_py_object) {
+            input.parse().map(PyClassPyO3Option::SkipFromPyObject)
         } else {
             Err(lookahead.error())
         }
@@ -243,6 +247,9 @@ impl PyClassPyO3Options {
                 set_option!(weakref);
             }
             PyClassPyO3Option::Generic(generic) => set_option!(generic),
+            PyClassPyO3Option::SkipFromPyObject(skip_from_py_object) => {
+                set_option!(skip_from_py_object)
+            }
         }
         Ok(())
     }
@@ -2497,7 +2504,15 @@ impl<'a> PyClassImplsBuilder<'a> {
             quote! {}
         };
 
+        let extract_pyclass_with_clone = if self.attr.options.skip_from_py_object.is_none() {
+            quote!( impl #pyo3_path::impl_::pyclass::ExtractPyClassWithClone for #cls {} )
+        } else {
+            TokenStream::new()
+        };
+
         Ok(quote! {
+            #extract_pyclass_with_clone
+
             #assertions
 
             #pyclass_base_type_impl

src/conversion.rs

@@ -1,5 +1,6 @@
 //! Defines conversions between Rust and Python types.
 use crate::err::PyResult;
+use crate::impl_::pyclass::ExtractPyClassWithClone;
 #[cfg(feature = "experimental-inspect")]
 use crate::inspect::types::TypeInfo;
 use crate::pyclass::boolean_struct::False;
@@ -529,7 +530,7 @@ impl<'py, T> FromPyObjectOwned<'py> for T where T: for<'a> FromPyObject<'a, 'py>
 
 impl<'a, 'py, T> FromPyObject<'a, 'py> for T
 where
-    T: PyClass + Clone,
+    T: PyClass + Clone + ExtractPyClassWithClone,
 {
     type Error = PyClassGuardError<'a, 'py>;
 
@@ -601,3 +602,38 @@ impl<'py> IntoPyObject<'py> for () {
 /// })
 /// ```
 mod test_no_clone {}
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    #[cfg(feature = "macros")]
+    fn test_pyclass_skip_from_py_object() {
+        use crate::{types::PyAnyMethods, FromPyObject, IntoPyObject, PyErr, Python};
+
+        #[crate::pyclass(crate = "crate", skip_from_py_object)]
+        #[derive(Clone)]
+        struct Foo(i32);
+
+        impl<'py> FromPyObject<'_, 'py> for Foo {
+            type Error = PyErr;
+
+            fn extract(obj: crate::Borrowed<'_, 'py, crate::PyAny>) -> Result<Self, Self::Error> {
+                if let Ok(obj) = obj.cast::<Self>() {
+                    Ok(obj.borrow().clone())
+                } else {
+                    obj.extract::<i32>().map(Self)
+                }
+            }
+        }
+        Python::attach(|py| {
+            let foo1 = 42i32.into_pyobject(py)?;
+            assert_eq!(foo1.extract::<Foo>()?.0, 42);
+
+            let foo2 = Foo(0).into_pyobject(py)?;
+            assert_eq!(foo2.extract::<Foo>()?.0, 0);
+
+            Ok::<_, PyErr>(())
+        })
+        .unwrap();
+    }
+}

src/impl_/pyclass.rs

@@ -1426,6 +1426,8 @@ impl<const IMPLEMENTS_INTOPYOBJECT: bool> ConvertField<false, IMPLEMENTS_INTOPYO
     }
 }
 
+pub trait ExtractPyClassWithClone {}
+
 #[cfg(test)]
 #[cfg(feature = "macros")]
 mod tests {

tests/ui/invalid_pyclass_args.stderr

@@ -1,4 +1,4 @@
-error: expected one of: `crate`, `dict`, `eq`, `eq_int`, `extends`, `freelist`, `frozen`, `get_all`, `hash`, `immutable_type`, `mapping`, `module`, `name`, `ord`, `rename_all`, `sequence`, `set_all`, `str`, `subclass`, `unsendable`, `weakref`, `generic`
+error: expected one of: `crate`, `dict`, `eq`, `eq_int`, `extends`, `freelist`, `frozen`, `get_all`, `hash`, `immutable_type`, `mapping`, `module`, `name`, `ord`, `rename_all`, `sequence`, `set_all`, `str`, `subclass`, `unsendable`, `weakref`, `generic`, `skip_from_py_object`
  --> tests/ui/invalid_pyclass_args.rs:4:11
   |
 4 | #[pyclass(extend=pyo3::types::PyDict)]
@@ -46,7 +46,7 @@ error: expected string literal
 25 | #[pyclass(module = my_module)]
    |                    ^^^^^^^^^
 
-error: expected one of: `crate`, `dict`, `eq`, `eq_int`, `extends`, `freelist`, `frozen`, `get_all`, `hash`, `immutable_type`, `mapping`, `module`, `name`, `ord`, `rename_all`, `sequence`, `set_all`, `str`, `subclass`, `unsendable`, `weakref`, `generic`
+error: expected one of: `crate`, `dict`, `eq`, `eq_int`, `extends`, `freelist`, `frozen`, `get_all`, `hash`, `immutable_type`, `mapping`, `module`, `name`, `ord`, `rename_all`, `sequence`, `set_all`, `str`, `subclass`, `unsendable`, `weakref`, `generic`, `skip_from_py_object`
   --> tests/ui/invalid_pyclass_args.rs:28:11
    |
 28 | #[pyclass(weakrev)]

tests/ui/invalid_pyfunction_argument.rs

@@ -6,4 +6,13 @@ fn invalid_pyfunction_argument(arg: AtomicPtr<()>) {
     let _ = arg;
 }
 
+#[pyclass(skip_from_py_object)]
+#[derive(Clone)]
+struct Foo;
+
+#[pyfunction]
+fn skip_from_py_object_without_custom_from_py_object(arg: Foo) {
+    let _ = arg;
+}
+
 fn main() {}

tests/ui/invalid_pyfunction_argument.stderr

@@ -6,6 +6,26 @@ error[E0277]: `AtomicPtr<()>` cannot be used as a Python function argument
   |
   = note: implement `FromPyObject` to enable using `AtomicPtr<()>` as a function argument
   = note: `Python<'py>` is also a valid argument type to pass the Python token into `#[pyfunction]`s and `#[pymethods]`
+  = help: the trait `PyClass` is implemented for `Foo`
+  = note: required for `AtomicPtr<()>` to implement `FromPyObject<'_, '_>`
+  = note: required for `AtomicPtr<()>` to implement `PyFunctionArgument<'_, '_, '_, true>`
+note: required by a bound in `extract_argument`
+ --> src/impl_/extract_argument.rs
+  |
+  | pub fn extract_argument<'a, 'holder, 'py, T, const IMPLEMENTS_FROMPYOBJECT: bool>(
+  |        ---------------- required by a bound in this function
+...
+  |     T: PyFunctionArgument<'a, 'holder, 'py, IMPLEMENTS_FROMPYOBJECT>,
+  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `extract_argument`
+
+error[E0277]: `AtomicPtr<()>` cannot be used as a Python function argument
+ --> tests/ui/invalid_pyfunction_argument.rs:5:37
+  |
+5 | fn invalid_pyfunction_argument(arg: AtomicPtr<()>) {
+  |                                     ^^^^^^^^^ the trait `Clone` is not implemented for `AtomicPtr<()>`
+  |
+  = note: implement `FromPyObject` to enable using `AtomicPtr<()>` as a function argument
+  = note: `Python<'py>` is also a valid argument type to pass the Python token into `#[pyfunction]`s and `#[pymethods]`
   = help: the following other types implement trait `PyFunctionArgument<'a, 'holder, 'py, IMPLEMENTS_FROMPYOBJECT>`:
             `&'a pyo3::Bound<'py, T>` implements `PyFunctionArgument<'a, '_, 'py, false>`
             `&'holder T` implements `PyFunctionArgument<'a, 'holder, '_, false>`
@@ -26,7 +46,7 @@ error[E0277]: `AtomicPtr<()>` cannot be used as a Python function argument
  --> tests/ui/invalid_pyfunction_argument.rs:5:37
   |
 5 | fn invalid_pyfunction_argument(arg: AtomicPtr<()>) {
-  |                                     ^^^^^^^^^ the trait `Clone` is not implemented for `AtomicPtr<()>`
+  |                                     ^^^^^^^^^ the trait `ExtractPyClassWithClone` is not implemented for `AtomicPtr<()>`
   |
   = note: implement `FromPyObject` to enable using `AtomicPtr<()>` as a function argument
   = note: `Python<'py>` is also a valid argument type to pass the Python token into `#[pyfunction]`s and `#[pymethods]`
@@ -45,3 +65,27 @@ note: required by a bound in `extract_argument`
 ...
   |     T: PyFunctionArgument<'a, 'holder, 'py, IMPLEMENTS_FROMPYOBJECT>,
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `extract_argument`
+
+error[E0277]: `Foo` cannot be used as a Python function argument
+  --> tests/ui/invalid_pyfunction_argument.rs:14:59
+   |
+14 | fn skip_from_py_object_without_custom_from_py_object(arg: Foo) {
+   |                                                           ^^^ the trait `ExtractPyClassWithClone` is not implemented for `Foo`
+   |
+   = note: implement `FromPyObject` to enable using `Foo` as a function argument
+   = note: `Python<'py>` is also a valid argument type to pass the Python token into `#[pyfunction]`s and `#[pymethods]`
+   = help: the following other types implement trait `PyFunctionArgument<'a, 'holder, 'py, IMPLEMENTS_FROMPYOBJECT>`:
+             `&'a pyo3::Bound<'py, T>` implements `PyFunctionArgument<'a, '_, 'py, false>`
+             `&'holder T` implements `PyFunctionArgument<'a, 'holder, '_, false>`
+             `&'holder mut T` implements `PyFunctionArgument<'a, 'holder, '_, false>`
+             `Option<T>` implements `PyFunctionArgument<'a, 'holder, 'py, false>`
+   = note: required for `Foo` to implement `FromPyObject<'_, '_>`
+   = note: required for `Foo` to implement `PyFunctionArgument<'_, '_, '_, true>`
+note: required by a bound in `extract_argument`
+  --> src/impl_/extract_argument.rs
+   |
+   | pub fn extract_argument<'a, 'holder, 'py, T, const IMPLEMENTS_FROMPYOBJECT: bool>(
+   |        ---------------- required by a bound in this function
+...
+   |     T: PyFunctionArgument<'a, 'holder, 'py, IMPLEMENTS_FROMPYOBJECT>,
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `extract_argument`