Merge #2739

2739: error when `#[pyo3(signature = ())]` used on invalid methods r=davidhewitt a=davidhewitt

A follow-up to #2702 to reject some invalid applications of `#[pyo3(signature = (...))]` attribute, specifically on magic methods and getters / setters / class attributes.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

guide/src/class/protocols.md

@@ -11,8 +11,8 @@ The magic methods handled by PyO3 are very similar to the standard Python ones o
  - Magic methods for the buffer protocol
 
 When PyO3 handles a magic method, a couple of changes apply compared to other `#[pymethods]`:
- - The `#[pyo3(text_signature = "...")]` attribute is not allowed
- - The signature is restricted to match the magic method
+ - The Rust function signature is restricted to match the magic method.
+ - The `#[pyo3(signature = (...)]` and `#[pyo3(text_signature = "...")]` attributes are not allowed.
 
 The following sections list of all magic methods PyO3 currently handles.  The
 given signatures should be interpreted as follows:

pyo3-macros-backend/src/method.rs

@@ -5,8 +5,8 @@ use std::borrow::Cow;
 use crate::attributes::TextSignatureAttribute;
 use crate::deprecations::{Deprecation, Deprecations};
 use crate::params::impl_arg_params;
-use crate::pyfunction::PyFunctionOptions;
 use crate::pyfunction::{DeprecatedArgs, FunctionSignature, PyFunctionArgPyO3Attributes};
+use crate::pyfunction::{PyFunctionOptions, SignatureAttribute};
 use crate::utils::{self, PythonDoc};
 use proc_macro2::{Span, TokenStream};
 use quote::ToTokens;
@@ -282,7 +282,7 @@ impl<'a> FnSpec<'a> {
 
         let (fn_type, skip_first_arg, fixed_convention) =
             Self::parse_fn_type(sig, fn_type_attr, &mut python_name)?;
-        Self::ensure_text_signature_on_valid_method(&fn_type, text_signature.as_ref())?;
+        ensure_signatures_on_valid_method(&fn_type, signature.as_ref(), text_signature.as_ref())?;
 
         let name = &sig.ident;
         let ty = get_return_info(&sig.output);
@@ -341,26 +341,6 @@ impl<'a> FnSpec<'a> {
         syn::LitStr::new(&format!("{}\0", self.python_name), self.python_name.span())
     }
 
-    fn ensure_text_signature_on_valid_method(
-        fn_type: &FnType,
-        text_signature: Option<&TextSignatureAttribute>,
-    ) -> syn::Result<()> {
-        if let Some(text_signature) = text_signature {
-            match &fn_type {
-                FnType::FnNew => bail_spanned!(
-                    text_signature.kw.span() =>
-                    "text_signature not allowed on __new__; if you want to add a signature on \
-                     __new__, put it on the struct definition instead"
-                ),
-                FnType::Getter(_) | FnType::Setter(_) | FnType::ClassAttribute => bail_spanned!(
-                    text_signature.kw.span() => "text_signature not allowed with this method type"
-                ),
-                _ => {}
-            }
-        }
-        Ok(())
-    }
-
     fn parse_fn_type(
         sig: &syn::Signature,
         fn_type_attr: Option<MethodTypeAttribute>,
@@ -747,3 +727,44 @@ const IMPL_TRAIT_ERR: &str = "Python functions cannot have `impl Trait` argument
 const RECEIVER_BY_VALUE_ERR: &str =
     "Python objects are shared, so 'self' cannot be moved out of the Python interpreter.
 Try `&self`, `&mut self, `slf: PyRef<'_, Self>` or `slf: PyRefMut<'_, Self>`.";
+
+fn ensure_signatures_on_valid_method(
+    fn_type: &FnType,
+    signature: Option<&SignatureAttribute>,
+    text_signature: Option<&TextSignatureAttribute>,
+) -> syn::Result<()> {
+    if let Some(signature) = signature {
+        match fn_type {
+            FnType::Getter(_) => {
+                bail_spanned!(signature.kw.span() => "`signature` not allowed with `getter`")
+            }
+            FnType::Setter(_) => {
+                bail_spanned!(signature.kw.span() => "`signature` not allowed with `setter`")
+            }
+            FnType::ClassAttribute => {
+                bail_spanned!(signature.kw.span() => "`signature` not allowed with `classattr`")
+            }
+            _ => {}
+        }
+    }
+    if let Some(text_signature) = text_signature {
+        match fn_type {
+            FnType::FnNew => bail_spanned!(
+                text_signature.kw.span() =>
+                "`text_signature` not allowed on `__new__`; if you want to add a signature on \
+                 `__new__`, put it on the struct definition instead"
+            ),
+            FnType::Getter(_) => {
+                bail_spanned!(text_signature.kw.span() => "`text_signature` not allowed with `getter`")
+            }
+            FnType::Setter(_) => {
+                bail_spanned!(text_signature.kw.span() => "`text_signature` not allowed with `setter`")
+            }
+            FnType::ClassAttribute => {
+                bail_spanned!(text_signature.kw.span() => "`text_signature` not allowed with `classattr`")
+            }
+            _ => {}
+        }
+    }
+    Ok(())
+}

pyo3-macros-backend/src/pyfunction/signature.rs

@@ -223,6 +223,7 @@ impl PythonSignature {
 pub struct FunctionSignature<'a> {
     pub arguments: Vec<FnArg<'a>>,
     pub python_signature: PythonSignature,
+    pub attribute: Option<SignatureAttribute>,
 }
 
 pub enum ParseState {
@@ -371,7 +372,7 @@ impl<'a> FunctionSignature<'a> {
             ),
         };
 
-        for item in attribute.value.items {
+        for item in &attribute.value.items {
             match item {
                 SignatureItem::Argument(arg) => {
                     let fn_arg = next_argument_checked(&arg.ident)?;
@@ -381,8 +382,8 @@ impl<'a> FunctionSignature<'a> {
                         arg.eq_and_default.is_none(),
                         arg.span(),
                     )?;
-                    if let Some((_, default)) = arg.eq_and_default {
-                        fn_arg.default = Some(default);
+                    if let Some((_, default)) = &arg.eq_and_default {
+                        fn_arg.default = Some(default.clone());
                     }
                 }
                 SignatureItem::VarargsSep(sep) => parse_state.finish_pos_args(sep.span())?,
@@ -411,6 +412,7 @@ impl<'a> FunctionSignature<'a> {
         Ok(FunctionSignature {
             arguments,
             python_signature,
+            attribute: Some(attribute),
         })
     }
 
@@ -515,6 +517,7 @@ impl<'a> FunctionSignature<'a> {
                 keyword_only_parameters,
                 accepts_kwargs,
             },
+            attribute: None,
         })
     }
 
@@ -550,6 +553,7 @@ impl<'a> FunctionSignature<'a> {
         Self {
             arguments,
             python_signature,
+            attribute: None,
         }
     }
 }

pyo3-macros-backend/src/pymethod.rs

@@ -196,7 +196,7 @@ pub fn gen_py_method(
             GeneratedPyMethod::Method(impl_py_class_attribute(cls, spec)?)
         }
         (PyMethodKind::Proto(proto_kind), _) => {
-            ensure_no_forbidden_protocol_attributes(spec, &method.method_name)?;
+            ensure_no_forbidden_protocol_attributes(&proto_kind, spec, &method.method_name)?;
             match proto_kind {
                 PyMethodProtoKind::Slot(slot_def) => {
                     let slot = slot_def.generate_type_slot(cls, spec, &method.method_name)?;
@@ -206,7 +206,7 @@ pub fn gen_py_method(
                     GeneratedPyMethod::Proto(impl_call_slot(cls, method.spec)?)
                 }
                 PyMethodProtoKind::Traverse => {
-                    GeneratedPyMethod::Proto(impl_traverse_slot(cls, method.spec))
+                    GeneratedPyMethod::Proto(impl_traverse_slot(cls, &spec.name))
                 }
                 PyMethodProtoKind::SlotFragment(slot_fragment_def) => {
                     let proto = slot_fragment_def.generate_pyproto_fragment(cls, spec)?;
@@ -263,11 +263,18 @@ fn ensure_function_options_valid(options: &PyFunctionOptions) -> syn::Result<()>
 }
 
 fn ensure_no_forbidden_protocol_attributes(
+    proto_kind: &PyMethodProtoKind,
     spec: &FnSpec<'_>,
     method_name: &str,
 ) -> syn::Result<()> {
+    if let Some(signature) = &spec.signature.attribute {
+        // __call__ is allowed to have a signature, but nothing else is.
+        if !matches!(proto_kind, PyMethodProtoKind::Call) {
+            bail_spanned!(signature.kw.span() => format!("`signature` cannot be used with magic method `{}`", method_name));
+        }
+    }
     if let Some(text_signature) = &spec.text_signature {
-        bail_spanned!(text_signature.kw.span() => format!("`text_signature` cannot be used with `{}`", method_name));
+        bail_spanned!(text_signature.kw.span() => format!("`text_signature` cannot be used with magic method `{}`", method_name));
     }
     Ok(())
 }
@@ -360,8 +367,7 @@ fn impl_call_slot(cls: &syn::Type, mut spec: FnSpec<'_>) -> Result<MethodAndSlot
     })
 }
 
-fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec<'_>) -> MethodAndSlotDef {
-    let ident = spec.name;
+fn impl_traverse_slot(cls: &syn::Type, rust_fn_ident: &syn::Ident) -> MethodAndSlotDef {
     let associated_method = quote! {
         pub unsafe extern "C" fn __pymethod_traverse__(
             slf: *mut _pyo3::ffi::PyObject,
@@ -377,7 +383,7 @@ fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec<'_>) -> MethodAndSlotDef {
             let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
             let borrow = slf.try_borrow();
             let retval = if let ::std::result::Result::Ok(borrow) = borrow {
-                _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit))
+                _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#rust_fn_ident(visit))
             } else {
                 0
             };

tests/test_compile_error.rs

@@ -35,6 +35,7 @@ fn _test_compile_errors() {
     t.compile_fail("tests/ui/invalid_macro_args.rs");
     t.compile_fail("tests/ui/invalid_need_module_arg_position.rs");
     t.compile_fail("tests/ui/invalid_property_args.rs");
+    t.compile_fail("tests/ui/invalid_proto_pymethods.rs");
     t.compile_fail("tests/ui/invalid_pyclass_args.rs");
     t.compile_fail("tests/ui/invalid_pyclass_enum.rs");
     t.compile_fail("tests/ui/invalid_pyclass_item.rs");
@@ -44,8 +45,6 @@ fn _test_compile_errors() {
     t.compile_fail("tests/ui/invalid_pymethod_names.rs");
     t.compile_fail("tests/ui/invalid_pymodule_args.rs");
     t.compile_fail("tests/ui/reject_generics.rs");
-    t.compile_fail("tests/ui/invalid_pymethod_proto_args.rs");
-    t.compile_fail("tests/ui/invalid_pymethod_proto_args_py.rs");
 
     tests_rust_1_49(&t);
     tests_rust_1_56(&t);

tests/ui/invalid_proto_pymethods.rs

@@ -0,0 +1,51 @@
+//! Check that some magic methods edge cases error as expected.
+//!
+//! For convenience use #[pyo3(name = "__some_dunder__")] to create the methods,
+//! so that the function names can describe the edge case to be rejected.
+
+use pyo3::prelude::*;
+
+#[pyclass]
+struct MyClass {}
+
+//
+// Argument counts
+//
+
+#[pymethods]
+impl MyClass {
+    #[pyo3(name = "__truediv__")]
+    fn truediv_expects_one_argument(&self) -> PyResult<()> {
+        Ok(())
+    }
+}
+
+#[pymethods]
+impl MyClass {
+    #[pyo3(name = "__truediv__")]
+    fn truediv_expects_one_argument_py(&self, _py: Python<'_>) -> PyResult<()> {
+        Ok(())
+    }
+}
+
+//
+// Forbidden attributes
+//
+
+#[pymethods]
+impl MyClass {
+    #[pyo3(name = "__bool__", signature = ())]
+    fn signature_is_forbidden(&self) -> bool {
+        true
+    }
+}
+
+#[pymethods]
+impl MyClass {
+    #[pyo3(name = "__bool__", text_signature = "")]
+    fn text_signature_is_forbidden(&self) -> bool {
+        true
+    }
+}
+
+fn main() {}

tests/ui/invalid_proto_pymethods.stderr

@@ -0,0 +1,23 @@
+error: Expected 1 arguments, got 0
+  --> tests/ui/invalid_proto_pymethods.rs:18:8
+   |
+18 |     fn truediv_expects_one_argument(&self) -> PyResult<()> {
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: Expected 1 arguments, got 0
+  --> tests/ui/invalid_proto_pymethods.rs:26:8
+   |
+26 |     fn truediv_expects_one_argument_py(&self, _py: Python<'_>) -> PyResult<()> {
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: `signature` cannot be used with magic method `__bool__`
+  --> tests/ui/invalid_proto_pymethods.rs:37:31
+   |
+37 |     #[pyo3(name = "__bool__", signature = ())]
+   |                               ^^^^^^^^^
+
+error: `text_signature` cannot be used with magic method `__bool__`
+  --> tests/ui/invalid_proto_pymethods.rs:45:31
+   |
+45 |     #[pyo3(name = "__bool__", text_signature = "")]
+   |                               ^^^^^^^^^^^^^^