Merge pull request #2093 from davidhewitt/positional-option

pyfunction: allow required positional after option

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<i32>, b: i32) {}` is not allowed, while `fn opt_last(a:i32, b: Option<i32>) {}` is. [#2041](https://github.com/PyO3/pyo3/pull/2041)
 - `PyErr::new_type` now takes an optional docstring and now returns `PyResult<Py<PyType>>` 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
 

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!(
+                required_positional_parameters = positional_parameter_names.len();
-                    all_positional_required,
-                    arg.name.span() => "Required positional parameters cannot come after optional parameters"
-                );
-                required_positional_parameters += 1;
-            } else {
-                all_positional_required = false;
             }
             if posonly {
                 positional_only_parameters += 1;
             }
-            positional_parameter_names.push(name);
         }
     }
 

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,

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
 
 

tests/test_pyfunction.rs

@@ -295,3 +295,39 @@ fn use_pyfunction() {
         assert_eq!(f2.call1((42,)).unwrap().extract::<i32>().unwrap(), 42);
     })
 }
+
+#[test]
+fn required_argument_after_option() {
+    #[pyfunction]
+    pub fn foo(x: Option<i32>, 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");
+    })
+}

tests/ui/invalid_pyfunctions.rs

@@ -9,7 +9,4 @@ fn impl_trait_function(impl_trait: impl AsRef<PyAny>) {}
 #[pyfunction]
 async fn async_function() {}
 
-#[pyfunction]
-fn required_arg_after_optional(optional: Option<isize>, required: isize) {}
-
 fn main() {}

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<isize>, required: isize) {}
-   |                                                         ^^^^^^^^

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<isize>, required: isize) {}
-}
-
 #[pymethods]
 impl MyClass {
     #[args(has_default = "1")]

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<isize>, 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) {}
-    |                                                               ^^^^^^^^