Disallow positional arguments after optional arguments

[juniperparsnips]

Thank you for contributing to pyo3!

Please consider adding the following to your pull request:

• an entry in CHANGELOG.md
• docs to all new functions and / or detail in the guide
• tests for all new or changed functions

Be aware the CI pipeline will check your pull request for the following:

• Rust tests (Just cargo test or make test if you need to test examples)
• Rust lints (make clippy)
• Rust formatting (cargo fmt)
• Python formatting (black . --check. You can install black with pip install black)
• Compatibility with all supported Python versions for all examples. This uses tox; you can do run it using make test_py.

You can run a similar set of checks as the CI pipeline using make test.

When a rust binding has an optional parameter followed by a required parameter it may cause a hard to follow panic in python.
To reproduce:

use pyo3::prelude::*;

#[pyclass]
struct Foo {
    #[pyo3(get, set)]
    pub a: i32
}

#[pymethods]
impl Foo {
    #[new]
    fn new() -> Self {
        Self { a: 0 }
    }

    fn opt_first(&mut self, a: Option<i32>, b: i32) {
        self.a = a.unwrap_or(b)
    }
}

#[pymodule]
fn pyo3test(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_class::<Foo>()?;
    Ok(())
}

>>> import pyo3test
>>> foo = pyo3test.Foo()
>>> foo.opt_first()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Foo.opt_first() missing 1 required positional argument: 'a'
>>> foo.opt_first(1)
thread '<unnamed>' panicked at 'Failed to extract required method argument', src/lib.rs:24:48
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
pyo3_runtime.PanicException: Failed to extract required method argument
>>> foo.opt_first(1, 2)
>>> foo.a
1
>>> foo.opt_first(None, 1)
>>> foo.a
1
>>>

While the python type hinting states that there is only one required positional argument, the function behaves as if there are two required arguments, and even if one were optional, it would be 'a', not 'b'. Additionally, this behavior leads to illegal python, as def opt_first(a: Optional[int]=None, b: int) triggers the "non-default argument follows default argument" syntax error.

Reworking the proc-macro so that an Option followed by a required positional parameter must be provided would make the behavior of Options inconsistent, so we should explicitly disallow required parameters that follow optional parameters.

Both of the following now fail at compile time rather than runtime,

#[pymethods]
impl Foo {
    fn opt_first(&mut self, a: Option<i32>, b: i32) {
        self.a = a.unwrap_or(b)
    }

    #[args(a = "1")]
    fn default_first_provided(&mut self, a: i32, b: i32) {
        self.a = a + b;
    }
}

While these still compile and work as expected,

#[pymethods]
impl Foo {
    #[args(b = "5")]
    fn default_last_provided(&mut self, a: Option<i32>, b: i32) {
        self.a = a.unwrap_or(b)
    }

    fn opt_last(&mut self, a:i32, b: Option<i32>) {
        self.a = b.unwrap_or(a)
    }
}

[birkenfeld]

Principle looks correct to me.

[birkenfeld]

Test failures need rebase on #2040 I think.

[juniperparsnips]

Rebased. Looks like a need a maintainer to approve running the workflow though.

[mejrs]

Don't worry about the CI failure, the main branch is currently broken. I'll try to fix it, unless @davidhewitt beats me to it.

[mejrs]

Can you rebase again? Apologies for the inconvenience 🙏🏻

[davidhewitt]

Thanks, this is a good catch 👍

This definitely needs a CHANGELOG entry. I'd suggest putting in the Changed section as it's technically breaking as well as bugfix.

... and sorry I broke CI. Thanks @mejrs for fixing.

[juniperparsnips]

Of course! I just rebased and added to the change log.

[davidhewitt]

LGTM, thanks again!