From 0af02278342bada5c76bded787d4a7736cbdb96a Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 4 Jul 2024 11:08:22 +0100 Subject: [PATCH] fix deprecation warning for trailing optional on `#[setter]` functions (#4304) * fix deprecation warning for trailing optional on `#[setter]` functions * add comment --- newsfragments/4304.fixed.md | 1 + pyo3-macros-backend/src/deprecations.rs | 1 + pyo3-macros-backend/src/method.rs | 19 ++++++- tests/test_getter_setter.rs | 36 +++++++++++++ tests/ui/deprecations.rs | 3 -- tests/ui/deprecations.stderr | 72 +++++++++++-------------- 6 files changed, 88 insertions(+), 44 deletions(-) create mode 100644 newsfragments/4304.fixed.md diff --git a/newsfragments/4304.fixed.md b/newsfragments/4304.fixed.md new file mode 100644 index 00000000000..5f5b5fd8ed0 --- /dev/null +++ b/newsfragments/4304.fixed.md @@ -0,0 +1 @@ +Fix invalid deprecation warning for trailing optional on `#[setter]` function. diff --git a/pyo3-macros-backend/src/deprecations.rs b/pyo3-macros-backend/src/deprecations.rs index 802561a126c..426ca2c0c7d 100644 --- a/pyo3-macros-backend/src/deprecations.rs +++ b/pyo3-macros-backend/src/deprecations.rs @@ -51,6 +51,7 @@ impl<'ctx> ToTokens for Deprecations<'ctx> { pub(crate) fn deprecate_trailing_option_default(spec: &FnSpec<'_>) -> TokenStream { if spec.signature.attribute.is_none() + && spec.tp.signature_attribute_allowed() && spec.signature.arguments.iter().any(|arg| { if let FnArg::Regular(arg) = arg { arg.option_wrapped_type.is_some() diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index cd06a92c0f7..0d00f952f6c 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -228,6 +228,20 @@ impl FnType { } } + pub fn signature_attribute_allowed(&self) -> bool { + match self { + FnType::Fn(_) + | FnType::FnNew + | FnType::FnStatic + | FnType::FnClass(_) + | FnType::FnNewClass(_) + | FnType::FnModule(_) => true, + // Setter, Getter and ClassAttribute all have fixed signatures (either take 0 or 1 + // arguments) so cannot have a `signature = (...)` attribute. + FnType::Getter(_) | FnType::Setter(_) | FnType::ClassAttribute => false, + } + } + pub fn self_arg( &self, cls: Option<&syn::Type>, @@ -1096,15 +1110,18 @@ fn ensure_signatures_on_valid_method( if let Some(signature) = signature { match fn_type { FnType::Getter(_) => { + debug_assert!(!fn_type.signature_attribute_allowed()); bail_spanned!(signature.kw.span() => "`signature` not allowed with `getter`") } FnType::Setter(_) => { + debug_assert!(!fn_type.signature_attribute_allowed()); bail_spanned!(signature.kw.span() => "`signature` not allowed with `setter`") } FnType::ClassAttribute => { + debug_assert!(!fn_type.signature_attribute_allowed()); bail_spanned!(signature.kw.span() => "`signature` not allowed with `classattr`") } - _ => {} + _ => debug_assert!(fn_type.signature_attribute_allowed()), } } if let Some(text_signature) = text_signature { diff --git a/tests/test_getter_setter.rs b/tests/test_getter_setter.rs index e2b8307fd32..e3852fcd29a 100644 --- a/tests/test_getter_setter.rs +++ b/tests/test_getter_setter.rs @@ -280,3 +280,39 @@ fn frozen_py_field_get() { py_run!(py, inst, "assert inst.value == 'value'"); }); } + +#[test] +fn test_optional_setter() { + #[pyclass] + struct SimpleClass { + field: Option, + } + + #[pymethods] + impl SimpleClass { + #[getter] + fn get_field(&self) -> Option { + self.field + } + + #[setter] + fn set_field(&mut self, field: Option) { + self.field = field; + } + } + + Python::with_gil(|py| { + let instance = Py::new(py, SimpleClass { field: None }).unwrap(); + py_run!(py, instance, "assert instance.field is None"); + py_run!( + py, + instance, + "instance.field = 42; assert instance.field == 42" + ); + py_run!( + py, + instance, + "instance.field = None; assert instance.field is None" + ); + }) +} diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index dbd0f8aa462..ff34966e00d 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -39,9 +39,6 @@ impl MyClass { #[setter] fn set_bar_bound(&self, _value: &Bound<'_, PyAny>) {} - #[setter] - fn set_option(&self, _value: Option) {} - fn __eq__(&self, #[pyo3(from_py_with = "extract_gil_ref")] _other: i32) -> bool { true } diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index c09894d86c1..4f1797da838 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -10,42 +10,34 @@ note: the lint level is defined here 1 | #![deny(deprecated)] | ^^^^^^^^^^ -error: use of deprecated constant `MyClass::__pymethod_set_set_option__::SIGNATURE`: this function has implicit defaults for the trailing `Option` arguments - = note: these implicit defaults are being phased out - = help: add `#[pyo3(signature = (_value=None))]` to this function to silence this warning and keep the current behavior - --> tests/ui/deprecations.rs:43:8 - | -43 | fn set_option(&self, _value: Option) {} - | ^^^^^^^^^^ - error: use of deprecated constant `__pyfunction_pyfunction_option_2::SIGNATURE`: this function has implicit defaults for the trailing `Option` arguments = note: these implicit defaults are being phased out = help: add `#[pyo3(signature = (_i, _any=None))]` to this function to silence this warning and keep the current behavior - --> tests/ui/deprecations.rs:132:4 + --> tests/ui/deprecations.rs:129:4 | -132 | fn pyfunction_option_2(_i: u32, _any: Option) {} +129 | fn pyfunction_option_2(_i: u32, _any: Option) {} | ^^^^^^^^^^^^^^^^^^^ error: use of deprecated constant `__pyfunction_pyfunction_option_3::SIGNATURE`: this function has implicit defaults for the trailing `Option` arguments = note: these implicit defaults are being phased out = help: add `#[pyo3(signature = (_i, _any=None, _foo=None))]` to this function to silence this warning and keep the current behavior - --> tests/ui/deprecations.rs:135:4 + --> tests/ui/deprecations.rs:132:4 | -135 | fn pyfunction_option_3(_i: u32, _any: Option, _foo: Option) {} +132 | fn pyfunction_option_3(_i: u32, _any: Option, _foo: Option) {} | ^^^^^^^^^^^^^^^^^^^ error: use of deprecated constant `__pyfunction_pyfunction_option_4::SIGNATURE`: this function has implicit defaults for the trailing `Option` arguments = note: these implicit defaults are being phased out = help: add `#[pyo3(signature = (_i, _any=None, _foo=None))]` to this function to silence this warning and keep the current behavior - --> tests/ui/deprecations.rs:138:4 + --> tests/ui/deprecations.rs:135:4 | -138 | fn pyfunction_option_4( +135 | fn pyfunction_option_4( | ^^^^^^^^^^^^^^^^^^^ error: use of deprecated constant `SimpleEnumWithoutEq::__pyo3__generated____richcmp__::DEPRECATION`: Implicit equality for simple enums is deprecated. Use `#[pyclass(eq, eq_int)` to keep the current behavior. - --> tests/ui/deprecations.rs:200:1 + --> tests/ui/deprecations.rs:197:1 | -200 | #[pyclass] +197 | #[pyclass] | ^^^^^^^^^^ | = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info) @@ -57,9 +49,9 @@ error: use of deprecated struct `pyo3::PyCell`: `PyCell` was merged into `Bound` | ^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:45:44 + --> tests/ui/deprecations.rs:42:44 | -45 | fn __eq__(&self, #[pyo3(from_py_with = "extract_gil_ref")] _other: i32) -> bool { +42 | fn __eq__(&self, #[pyo3(from_py_with = "extract_gil_ref")] _other: i32) -> bool { | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument @@ -93,69 +85,69 @@ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg` | ^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:64:44 + --> tests/ui/deprecations.rs:61:44 | -64 | fn pyfunction_with_module_gil_ref(_module: &PyModule) -> PyResult<&str> { +61 | fn pyfunction_with_module_gil_ref(_module: &PyModule) -> PyResult<&str> { | ^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:74:19 + --> tests/ui/deprecations.rs:71:19 | -74 | fn module_gil_ref(_m: &PyModule) -> PyResult<()> { +71 | fn module_gil_ref(_m: &PyModule) -> PyResult<()> { | ^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:79:57 + --> tests/ui/deprecations.rs:76:57 | -79 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, _m: &PyModule) -> PyResult<()> { +76 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, _m: &PyModule) -> PyResult<()> { | ^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:115:27 + --> tests/ui/deprecations.rs:112:27 | -115 | #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, +112 | #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:121:29 + --> tests/ui/deprecations.rs:118:29 | -121 | fn pyfunction_gil_ref(_any: &PyAny) {} +118 | fn pyfunction_gil_ref(_any: &PyAny) {} | ^ error: use of deprecated method `pyo3::deprecations::OptionGilRefs::>::function_arg`: use `Option<&Bound<'_, T>>` instead for this function argument - --> tests/ui/deprecations.rs:125:36 + --> tests/ui/deprecations.rs:122:36 | -125 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} +122 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} | ^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:150:27 + --> tests/ui/deprecations.rs:147:27 | -150 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] +147 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:160:27 + --> tests/ui/deprecations.rs:157:27 | -160 | #[pyo3(from_py_with = "PyAny::len")] usize, +157 | #[pyo3(from_py_with = "PyAny::len")] usize, | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:166:31 + --> tests/ui/deprecations.rs:163:31 | -166 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), +163 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:173:27 + --> tests/ui/deprecations.rs:170:27 | -173 | #[pyo3(from_py_with = "extract_gil_ref")] +170 | #[pyo3(from_py_with = "extract_gil_ref")] | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::>::is_python`: use `wrap_pyfunction_bound!` instead - --> tests/ui/deprecations.rs:186:13 + --> tests/ui/deprecations.rs:183:13 | -186 | let _ = wrap_pyfunction!(double, py); +183 | let _ = wrap_pyfunction!(double, py); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `wrap_pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)