From dc8032a5ff1c242303d8461a897a45572acf9f91 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Mon, 10 Jan 2022 22:45:32 +0000 Subject: [PATCH] pyfunction: allow required positional after option --- CHANGELOG.md | 2 +- pyo3-macros-backend/src/params.rs | 13 ++------ pytests/pyo3-pytests/src/datetime.rs | 2 +- pytests/pyo3-pytests/tests/test_datetime.py | 8 ++--- tests/test_pyfunction.rs | 36 +++++++++++++++++++++ tests/ui/invalid_pyfunctions.rs | 3 -- tests/ui/invalid_pyfunctions.stderr | 6 ---- tests/ui/invalid_pymethods.rs | 5 --- tests/ui/invalid_pymethods.stderr | 12 ------- 9 files changed, 45 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 209d369b..eba57ef4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `from_instance` -> `from_value` - `into_instance` -> `into_value` - Deprecate `PyType::is_instance`; it is inconsistent with other `is_instance` methods in PyO3. Instead of `typ.is_instance(obj)`, use `obj.is_instance(typ)`. [#2031](https://github.com/PyO3/pyo3/pull/2031) -- Optional parameters of `#[pymethods]` and `#[pyfunction]`s cannot be followed by required parameters, i.e. `fn opt_first(a: Option, b: i32) {}` is not allowed, while `fn opt_last(a:i32, b: Option) {}` is. [#2041](https://github.com/PyO3/pyo3/pull/2041) - `PyErr::new_type` now takes an optional docstring and now returns `PyResult>` rather than a `ffi::PyTypeObject` pointer. - The `create_exception!` macro can now take an optional docstring. This docstring, if supplied, is visible to users (with `.__doc__` and `help()`) and accompanies your error type in your crate's documentation. @@ -62,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix clippy warning `needless-option-as-deref` in code generated by `#[pyfunction]` and `#[pymethods]`. [#2040](https://github.com/PyO3/pyo3/pull/2040) - Fix undefined behavior in `PySlice::indices`. [#2061](https://github.com/PyO3/pyo3/pull/2061) - Use the Rust function path for `wrap_pymodule!` of a `#[pymodule]` with a `#[pyo3(name = "..")]` attribute, not the Python name. [#2081](https://github.com/PyO3/pyo3/pull/2081) +- Fix panic in `#[pyfunction]` generated code when a required argument following an `Option` was not provided. [#2093](https://github.com/PyO3/pyo3/pull/2093) ## [0.15.1] - 2021-11-19 diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 85ee4b48..ef70c41d 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -81,8 +81,6 @@ pub fn impl_arg_params( let mut required_positional_parameters = 0usize; let mut keyword_only_parameters = Vec::new(); - let mut all_positional_required = true; - for arg in spec.args.iter() { if arg.py || is_args(&spec.attrs, arg.name) || is_kwargs(&spec.attrs, arg.name) { continue; @@ -100,19 +98,14 @@ pub fn impl_arg_params( } }); } else { + positional_parameter_names.push(name); + if required { - ensure_spanned!( - all_positional_required, - arg.name.span() => "Required positional parameters cannot come after optional parameters" - ); - required_positional_parameters += 1; - } else { - all_positional_required = false; + required_positional_parameters = positional_parameter_names.len(); } if posonly { positional_only_parameters += 1; } - positional_parameter_names.push(name); } } diff --git a/pytests/pyo3-pytests/src/datetime.rs b/pytests/pyo3-pytests/src/datetime.rs index 7106987e..e833e57a 100644 --- a/pytests/pyo3-pytests/src/datetime.rs +++ b/pytests/pyo3-pytests/src/datetime.rs @@ -51,8 +51,8 @@ fn time_with_fold<'p>( minute: u8, second: u8, microsecond: u32, - fold: bool, tzinfo: Option<&PyTzInfo>, + fold: bool, ) -> PyResult<&'p PyTime> { PyTime::new_with_fold( py, diff --git a/pytests/pyo3-pytests/tests/test_datetime.py b/pytests/pyo3-pytests/tests/test_datetime.py index f9966f63..67d2da16 100644 --- a/pytests/pyo3-pytests/tests/test_datetime.py +++ b/pytests/pyo3-pytests/tests/test_datetime.py @@ -1,12 +1,12 @@ import datetime as pdt import platform -import struct import re +import struct import sys -import pytest import pyo3_pytests.datetime as rdt -from hypothesis import given, example +import pytest +from hypothesis import example, given from hypothesis import strategies as st @@ -139,7 +139,7 @@ def test_time_fold(t): @pytest.mark.xfail(PYPY, reason="Feature not available on PyPy") @pytest.mark.parametrize("fold", [False, True]) def test_time_fold(fold): - t = rdt.time_with_fold(0, 0, 0, 0, fold, None) + t = rdt.time_with_fold(0, 0, 0, 0, None, fold) assert t.fold == fold diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 85bde39b..930fe780 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -295,3 +295,39 @@ fn use_pyfunction() { assert_eq!(f2.call1((42,)).unwrap().extract::().unwrap(), 42); }) } + +#[test] +fn required_argument_after_option() { + #[pyfunction] + pub fn foo(x: Option, y: i32) -> i32 { + y + x.unwrap_or_default() + } + + Python::with_gil(|py| { + let f = wrap_pyfunction!(foo, py).unwrap(); + + // it is an error to call this function with no arguments + py_expect_exception!( + py, + f, + "f()", + PyTypeError, + "foo() missing 2 required positional arguments: 'x' and 'y'" + ); + + // it is an error to call this function with one argument + py_expect_exception!( + py, + f, + "f(None)", + PyTypeError, + "foo() missing 1 required positional argument: 'y'" + ); + + // ok to call with two arguments + py_assert!(py, f, "f(None, 5) == 5"); + + // ok to call with keyword arguments + py_assert!(py, f, "f(x=None, y=5) == 5"); + }) +} diff --git a/tests/ui/invalid_pyfunctions.rs b/tests/ui/invalid_pyfunctions.rs index ab5abd2b..680ca56e 100644 --- a/tests/ui/invalid_pyfunctions.rs +++ b/tests/ui/invalid_pyfunctions.rs @@ -9,7 +9,4 @@ fn impl_trait_function(impl_trait: impl AsRef) {} #[pyfunction] async fn async_function() {} -#[pyfunction] -fn required_arg_after_optional(optional: Option, required: isize) {} - fn main() {} diff --git a/tests/ui/invalid_pyfunctions.stderr b/tests/ui/invalid_pyfunctions.stderr index fdec5afd..00bf963f 100644 --- a/tests/ui/invalid_pyfunctions.stderr +++ b/tests/ui/invalid_pyfunctions.stderr @@ -17,9 +17,3 @@ Additional crates such as `pyo3-asyncio` can be used to integrate async Rust and | 10 | async fn async_function() {} | ^^^^^ - -error: Required positional parameters cannot come after optional parameters - --> tests/ui/invalid_pyfunctions.rs:13:57 - | -13 | fn required_arg_after_optional(optional: Option, required: isize) {} - | ^^^^^^^^ diff --git a/tests/ui/invalid_pymethods.rs b/tests/ui/invalid_pymethods.rs index a46a190c..ed45bac0 100644 --- a/tests/ui/invalid_pymethods.rs +++ b/tests/ui/invalid_pymethods.rs @@ -113,11 +113,6 @@ impl MyClass { fn method_cannot_pass_module(&self, m: &PyModule) {} } -#[pymethods] -impl MyClass { - fn required_arg_after_optional(&self, optional: Option, required: isize) {} -} - #[pymethods] impl MyClass { #[args(has_default = "1")] diff --git a/tests/ui/invalid_pymethods.stderr b/tests/ui/invalid_pymethods.stderr index 2a25508c..3556c272 100644 --- a/tests/ui/invalid_pymethods.stderr +++ b/tests/ui/invalid_pymethods.stderr @@ -101,15 +101,3 @@ error: `pass_module` cannot be used on Python methods | 112 | #[pyo3(pass_module)] | ^^^^^^^^^^^ - -error: Required positional parameters cannot come after optional parameters - --> tests/ui/invalid_pymethods.rs:118:68 - | -118 | fn required_arg_after_optional(&self, optional: Option, required: isize) {} - | ^^^^^^^^ - -error: Required positional parameters cannot come after optional parameters - --> tests/ui/invalid_pymethods.rs:124:63 - | -124 | fn default_arg_before_required(&self, has_default: isize, required: isize) {} - | ^^^^^^^^