Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate implicit default for trailing optional arguments #4078

Merged
merged 4 commits into from
May 10, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Apr 13, 2024

This deprecates the implicit default for trailing optional (Option<...>) arguments.

Motivation

Loosely summarized from #2935,

  • The current behavior is unexpected from both Rust and Python.
  • Rust does not have a concept of default values for arguments, so it's surprising that pyo3 just adds one without explicit request
  • Python does have default values for argument, but they have to be set explicitly.
    def fun_1(a, b):
        ...
    def fun_2(a=None, b=None):
        ...
    
    fun_1() # <- This is errors instead of using `None` as default
    fun_2() # <- This is fine

This makes porting Python functions to Rust more surprising that it needs to be, especially if an Option<...> argument is not in trailing position.

Expecting functions without #[pyo3(signature=(...)] to have a trivial signature (without defaults or other modifiers) is more intuitive. To even know about the current behavior, users have to carefully read the docs.

Migration

This PR adds a deprecation warning to Option<...> arguments on functions that do not currently have a
#[pyo3(signature=(...)] attribute and a big warning in the guide. This should push users to explicitly spell out their desired behavior during the migration period. After that we should probably make this a hard error for one release cycle, before we allow Option<...> arguments without #[pyo3(signature=(...)] again, (but without special handling).

Xref: #2935, #3735


Stacked on top of #4033, so I leave this as a draft for now.

@Icxolu Icxolu marked this pull request as ready for review April 14, 2024 15:58
Copy link

codspeed-hq bot commented Apr 14, 2024

CodSpeed Performance Report

Merging #4078 will not alter performance

Comparing Icxolu:fix-2935 (6cf6532) with main (104328c)

🎉 Hooray! codspeed-rust just leveled up to 2.6.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 69 untouched benchmarks

@davidhewitt
Copy link
Member

This gets a big 👍 from me! I do occasionally find the trailing optional arguments a convenient default, but just as often I've had to deliberately disable them. Having the special case also increases cognitive overhead of working out the Python signature.

Anyone got strong feelings that we shouldn't be doing this?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given there doesn't seem to be much resistance, shall we move ahead with this for 0.22? I think the simplification of this edge case is a worthwhile improvement!

Just one idea regarding the deprecation message, which might be interesting to play with...

guide/src/function/signature.md Outdated Show resolved Hide resolved
Comment on lines 13 to 16
error: use of deprecated function `pyo3::deprecations::deprecate_implicit_option`: Implicit default for trailing optional arguments is phased out. Add an explicit `#[pyo3(signature = (...))]` attribute a to silence this warning. In a future pyo3 version `Option<..>` arguments will be treated the same as any other argument.
--> tests/ui/deprecations.rs:119:39
|
119 | fn pyfunction_option_2(_i: u32, _any: Option<i32>) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be awesome if the macro could emit the right signature here instead of (...).

I wonder, what happens if we made the macro deprecation expand to something like:

#[deprecated(note = "this function has implicit defaults for the trailing `Option<T>` argument `_any`. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any = None))]` to this function")]
const PYFUNCTION_OPTION_2_SIGNATURE: () = ();


// immediately use the deprecated symbol to trigger the deprecation warning
const _: () = PYFUNCTION_OPTION_2_SIGNATURE;

I think if we attached that to the span of the whole function, rather than just an individual argument, it'd be super clear to users how to upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be really nice! I'll give it a try

@Icxolu
Copy link
Contributor Author

Icxolu commented May 9, 2024

I don't really have a clue why #![allow(deprecated)] makes this trigger as dead_code. Should I just add #[allow(dead_code)] to the generated const? Or is there a better way?

Comment on lines +13 to +35
error: use of deprecated constant `MyClass::__pymethod_set_set_option__::SIGNATURE`: This function has implicit defaults for the trailing `Option<T>` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_value=None)]` to this function to silence this warning and keep the current behavior
--> tests/ui/deprecations.rs:43:8
|
43 | fn set_option(&self, _value: Option<i32>) {}
| ^^^^^^^^^^

error: use of deprecated constant `__pyfunction_pyfunction_option_2::SIGNATURE`: This function has implicit defaults for the trailing `Option<T>` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any=None)]` to this function to silence this warning and keep the current behavior
--> tests/ui/deprecations.rs:132:4
|
132 | fn pyfunction_option_2(_i: u32, _any: Option<i32>) {}
| ^^^^^^^^^^^^^^^^^^^

error: use of deprecated constant `__pyfunction_pyfunction_option_3::SIGNATURE`: This function has implicit defaults for the trailing `Option<T>` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any=None, _foo=None)]` to this function to silence this warning and keep the current behavior
--> tests/ui/deprecations.rs:135:4
|
135 | fn pyfunction_option_3(_i: u32, _any: Option<i32>, _foo: Option<String>) {}
| ^^^^^^^^^^^^^^^^^^^

error: use of deprecated constant `__pyfunction_pyfunction_option_4::SIGNATURE`: This function has implicit defaults for the trailing `Option<T>` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any=None, _foo=None)]` to this function to silence this warning and keep the current behavior
--> tests/ui/deprecations.rs:138:4
|
138 | fn pyfunction_option_4(
| ^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great 👍 💯

pyo3-macros-backend/src/deprecations.rs Show resolved Hide resolved
@davidhewitt
Copy link
Member

Think it might be unrelated to the #![allow] and just the compiler finding the const isn't ending up in the binary.

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

So I guess just add #[allow(dead_code)]? It is still having the effect of producing an awesome message 🚀

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a final thought here - we should document this in the migration guide, I think.

@Icxolu
Copy link
Contributor Author

Icxolu commented May 9, 2024

Good idea!

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@Icxolu Icxolu enabled auto-merge May 10, 2024 10:18
@Icxolu Icxolu added this pull request to the merge queue May 10, 2024
Merged via the queue into PyO3:main with commit aef0a05 May 10, 2024
43 checks passed
@Icxolu Icxolu deleted the fix-2935 branch May 10, 2024 11:06
emgeee added a commit to probably-nothing-labs/datafusion-python that referenced this pull request Sep 10, 2024
Implicit defaults for trailing optional arguments have been deprecated
in pyo3 v0.22.0 PyO3/pyo3#4078
emgeee added a commit to probably-nothing-labs/datafusion-python that referenced this pull request Sep 10, 2024
Implicit defaults for trailing optional arguments have been deprecated
in pyo3 v0.22.0 PyO3/pyo3#4078
timsaucer added a commit to apache/datafusion-python that referenced this pull request Sep 17, 2024
* update dependencies

* update get_logical_plan signature

* remove row_number() function

row_number was converted to a UDF in datafusion v42 apache/datafusion#12030
This specific functionality needs to be added back in.

* remove unneeded dependency

* fix pyo3 warnings

Implicit defaults for trailing optional arguments have been deprecated
in pyo3 v0.22.0 PyO3/pyo3#4078

* update object_store dependency

* change PyExpr -> PySortExpr

* comment out key.extract::<&PyTuple>() condition statement

* change more instances of PyExpr > PySortExpr

* update function signatures to use _bound versions

* remove clone

* Working through some of the sort requirement changes

* remove unused import

* expr.display_name is deprecated, used format!() + schema_name() instead

* expr.canonical_name() is deprecated, use format!() expr instead

* remove comment

* fix tuple extraction in dataframe.__getitem__()

* remove unneeded import

* Add docstring comments to SortExpr python class

* change extract() to downcast()

Co-authored-by: Michael J Ward <[email protected]>

* deprecate Expr::display_name

Ref: apache/datafusion#11797

* fix lint errors

* update datafusion commit hash

* fix type in cargo file for arrow features

* upgrade to datafusion 42

* cleanup

---------

Co-authored-by: Tim Saucer <[email protected]>
Co-authored-by: Michael J Ward <[email protected]>
Co-authored-by: Michael-J-Ward <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants