RFC: remove default of None for trailing optional arguments

[davidhewitt]

In #2934 I've documented a longstanding PyO3 behaviour to automatically add a default of None to all trailing Option<T> arguments.

I think that behaviour is a small footgun, as I don't think first-time users necessarily expect this. I see two downsides:

• Users accidentally allow default of None for these arguments when they wanted them to be required
• Users reordering function arguments can easily remove the default without noticing.

We currently have the deprecation from #2703 which we can upgrade to an error in 0.20. This helps the situation, as it makes the refactoring in the second bullet fail nosily.

I'd be keen to remove the trailing default, with a deprecation warning for functions using this behaviour which don't currently have a #[pyo3(signature = (...))] annotation. I think it's a simpler API for users if PyO3 never adds an implicit default, and removes the need for the error on the refactoring case (which is purely a guard rail to avoid the current footgun).

What do others think? I'd be keen to hear from folks who think the conveniences of the current behaviour (i.e. fewer macro annotations) outweigh the drawbacks listed above. (@mejrs - in #2193 I got the impression that you might hold this view.)

[birkenfeld]

IMO the current mode (with the warning elevated to error) is fine. With arguments that can be None, it's pretty typical that they also have a default of None.

[davidhewitt]

@ritchie46 - after reading #2925 I'd be interested in your comment as a user - you were either unaware of this default, or would prefer the change proposed here?

[davidhewitt]

#2932 sounds like another case of a user confused by this (maybe just because it wasn't documented).

[mejrs]

I think it's a simpler API for users if PyO3 never adds an implicit default

I'm generally against doing implicit things, so this sounds good.

I've been mulling about how to communicate this behavior. The current api heavily relies on assuming that users read the documentation. I've been guilty of this myself - I have at times just used Option without really thinking about what behavior I actually wanted. What about just requiring the signature attribute if there is an Option anywhere in the function signature?

[birkenfeld]

I'm generally against doing implicit things, so this sounds good.

Although "implicit" can mean a lot of different things.

In this case, you could argue that any case of not using pyo3(signature) is implicit, and pyo3(signature) should be mandatory.

[ritchie46]

@ritchie46 - after reading #2925 I'd be interested in your comment as a user - you were either unaware of this default, or would prefer the change proposed here?

I was unaware of the defaults. I think having a well documented default makes sense. It can remove some boilerplate for the default case.

[davidhewitt]

In this case, you could argue that any case of not using pyo3(signature) is implicit, and pyo3(signature) should be mandatory.

Definitely true, though I'd make the argument that it's reasonable to assume that not using pyo3(signature) would be the same as pyo3(signature = (x, y, z)), i.e. the trivial signature with no special modifications.

Because of the rule here which I propose we deprecate, at the moment the implicit signature is more like pyo3(signature = (x, y, z=None)), with the number of =None varying depending on how many trailing Option<T> arguments there are.

I was unaware of the defaults. I think having a well documented default makes sense. It can remove some boilerplate for the default case.

Thanks. Good to know.

---

From a user perspective, for some function like

#[pyfunction]
fn my_function(arg1: Option<String>, arg2: Option<String>) {

depending on the default we pick here, one of the following forms will always be necessary:

// to take required `Option<String>` arguments, this is currently necessary
#[pyfunction(signature = (arg1, arg2))]
fn my_function(arg1: Option<String>, arg2: Option<String>) {

// to make optional arguments, this would be necessary if the proposal here were accepted
#[pyfunction(signature = (arg1=None, arg2=None))]
fn my_function(arg1: Option<String>, arg2: Option<String>) {

The question is which of these would users be happier writing?

Personally, I think it's more frustrating as a user to have to write the form #[pyfunction(signature = (arg1, arg2))]. That looks and feels like a no-op to me which forces me to repeat myself. Writing the latter form #[pyfunction(signature = (arg1=None, arg2=None))], by contrast, is making the defaults of None very explicit, which is not otherwise obvious from the Rust function definition, so I think helps readability even if it's a little boilerplatey.

What about just requiring the signature attribute if there is an Option anywhere in the function signature?

I think to action the change here, we would need to do exactly that for a couple of versions (maybe as a deprecation warning). In the long term, I'd prefer to not have to write the "no-op" signature (e.g. #[pyfunction(signature = (arg1, arg2))]).

[clbarnes]

Add me to the "confused user" list (#3735 ). Rust does not have a concept of optional arguments, just arguments of type Option. Python has the concept of optional arguments and arguments of type Optional, and crucially, those are different things. The current behaviour conflates the two by trying to pre-empt user behaviour in a way I don't find particularly convincing. It's true that sometimes nullable arguments are null by default, but there are plenty of cases where they're not, and IMO the default behaviour should be to rely on an explicit distinction between what is optional and what is nullable, rather than having to add extra information to tell pyo3 code to behave like rust code and python code (python does not add an implicit =None to arg: Optional either).

My particular case is that

# python
def my_fn(a: Optional[int], b: int): ...

has the obvious equivalent of

// pyo3
pub fn my_fn(a: Option<i64>, b: i64) -> ...

but the latter is a compile error.

[davidhewitt]

This was shipped in 0.22; I will add to 0.24 milestone as the next step (making a hard error temporarily) will come then.

[ChristopherRabotin]

Jumping into this RFC to warn of a regression this behavior change would cause for libraries that are used similarly in Rust and Python.

One example is my code, ANISE, where several of the functions are available in both languages. Let's take the example of the translate function whose final argument is an Option<_> type.

Py03 version 0.22 warns the following:

warning: use of deprecated constant `ephemerides::translations::<impl almanac::Almanac>::__pymethod_translate__::SIGNATURE`: this function has implicit defaults for the trailing `Option<T>` arguments 
         = note: these implicit defaults are being phased out 
         = help: add `#[pyo3(signature = (target_frame, observer_frame, epoch, ab_corr=None))]` to this function to silence this warning and keep the current behavior
  --> anise/src/ephemerides/translations.rs:51:12
   |
51 |     pub fn translate(
   |            ^^^^^^^^^

Adding a pyo3(signature..._) attribute to this function in the Rust code without a cfg_attr leads to a compilation error (similar to #1003):

error: cannot find attribute `pyo3` in this scope
  --> anise/src/ephemerides/translations.rs:49:7
   |
49 |     #[pyo3(signature = (target_frame, observer_frame, epoch, ab_corr=None))]
   |       ^^^^

with

#[pyo3(signature = (target_frame, observer_frame, epoch, ab_corr=None))]
    pub fn translate(
// ...

Attempting to add a cfg_attr helps the Rust crate build, but fails in the Python build with maturin develop:

error: cannot find attribute `pyo3` in this scope
  --> anise/src/ephemerides/translations.rs:50:35
   |
50 |     #[cfg_attr(feature ="python", pyo3(signature = (target_frame, observer_frame, epoch, ab_corr=None)))]
   |                                   ^^^^
   |
   = note: `pyo3` is in scope, but it is a crate, not an attribute

with

#[cfg_attr(feature ="python", pyo3(signature = (target_frame, observer_frame, epoch, ab_corr=None)))]
    pub fn translate(

Note that the ANISE crate and Python package are separate crates but part of the same cargo workspace.

[davidhewitt]

Thanks @ChristopherRabotin for raising the concern.

A couple of thoughts here:

• pyo3(signature) is necessary for a bunch of functionality (e.g. accepting *args, **kwargs) so it is not isolated to this case. The comment in Add support for wrapping attributes in #[cfg_attr(feature = "pyo3", …)] #2786 (comment) provides a solution to handle #[cfg_attr] which should work for your case, even if verbose.
• We could potentially add an option to #[pymethods], e.g. #[pymethods(require_trailing_options)] which brings forward the final behaviour and drops the requirement to have #[pyo3(signature)] over the migration window. This of course assumes that you want the trailing Option arguments to be required.