Type error when using (None) as the default value in function signature

[GoldsteinE]

Bug Description

This part in pyo3 macros:

pyo3/pyo3-macros-backend/src/params.rs

Lines 192 to 201 in 8f4a26a

	// Option<T> arguments have special treatment: the default should be specified _without_ the
	// Some() wrapper. Maybe this should be changed in future?!
	if arg.optional.is_some() {
	default = Some(match &default {
	Some(expression) if expression.to_string() != "None" => {
	quote!(::std::option::Option::Some(#expression))
	}
	_ => quote!(::std::option::Option::None),
	})
	}

special-cases the ident None when it’s used as the default value for a parameter. Unfortunately, it doesn’t work for any other expression evaluating to None, e.g. (None).

This line looks like it also should be fixed:

pyo3/pyo3-macros-backend/src/pyfunction/signature.rs

Line 483 in 8f4a26a

}) if path.is_ident("None") => {

Steps to Reproduce

Try to compile this code:

#[allow(unused_parens)] // sic: unused parens are the point
#[pyfunction]
#[pyo3(signature = (x = (None)))]
fn takes_x(x: Option<u8>) {
    println!("{x:?}");
}

Your operating system and version

NixOS

Your Python version (python --version)

Python 3.10

Your Rust version (rustc --version)

rustc 1.74.0-nightly (e3abbd499 2023-09-06)

Your PyO3 version

0.19.2

How did you install python? Did you use a virtualenv?

System repositories

Additional Info

Using arbitrary expressions evaluating to None may seem like a strange thing to want, but I’m autogenerating Python bindings and my defaults are something like None.convert_to_type_that_impls_IntoPy(). I’d rather not special-case None here.

[GoldsteinE]

Unfortunately, I’m not sure how to fix this without removing the special treatment of Option<_> completely. IMO Some-wrapping is kinda counterintuitive, especially considering this problem, but I get why it could be desired.

[davidhewitt]

I have mixed feelings about the Some-wrapping. Given that this is the API boundary which is supposed to look like Python, I think it does make the user experience a little cuter.

I agree that this edge case is a bit horrible though, and in general I don't like implementations which rely on recognising tokens at text. For example, if you put Option under a different name the Some-wrapping doesn't work, because we can't detect type equivalency.

I suggest two ways forward:

• You could rely on the fact we detect known name Option and use an alias in your generated code to circumvent the feature.
• We could explore a way to reimplement this functionality using traits, which might be able to support some-wrapping automatically. (Or try blocks on nightly?)

[GoldsteinE]

I’m currently just forwarding this if to my generating code, like

if default_value_s == "None" {
    quote!(#name = None)
} else {
    quote!(#name = #default_value.convert())
}

Aliasing Option<_> is another possible workaround. I’m not too happy about either of them though.

Some trait-based solution would be interesting. Maybe some approach using deref specialization to use wrapping trait if available or fall back to identity function? I’ll try to write an example later today.

[GoldsteinE]

@davidhewitt This looks like a proper solution:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8d3d90a25757bf9def5c2875e32102fe

I probably won’t have time in the near future to actually implement this though.

[GoldsteinE]

Note that it could trivially be abstracted over the wrapper type:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f28eff9301f625b888d353e7faff271e

[davidhewitt]

Thanks! Having been successfully nerd-sniped I managed to reduce the trait to a relatively simple form to solve the exact problem we needed. #3461

[GoldsteinE]

Thanks for the fix! It looks like it would solve the problem.

As a sidenote though, deref-based version doesn't rely on textually detecting Option<_>, doing a proper type comparison. As far as I can see, version in the PR still detects Option<_> textually. It's a much lesser concern than detecting None textually though.

[davidhewitt]

Agreed, unfortunately we also detect text Option<_> for #2935 - which one day maybe we might kill, but until then there wasn't really any benefit in trying to remove that detection here.

The deref-based version you shared ran into other complexities - e.g. we needed to specify the type annotation in the macro, but we need to strip all lifetimes from the type annotation first because we're moving it from a function signature into the caller. Possibly a future refinement.