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

pyfunction: use extract_argument with holder to avoid extractext #2503

Merged
merged 1 commit into from
Jul 17, 2022

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jul 12, 2022

Motivated by #2487, this is hopefully a better long-term solution than patching to avoid upsetting clippy.

This is a reworking of the pyfunction macro implementations to have some smarter internal type inference for extracting reference values. At the moment there is an ExtractExt trait and a lot of generated macro code dancing with derefs which make clippy quite unhappy.

This PR replaces ExtractExt with a PyFunctionArgument trait which avoids the need for deref juggling by taking a &mut holder argument used as temporary storage. For T: FromPyObject types the holder is just () (and should hopefully be optimised out), and for &T / &mut T for #[pyclass] types the holder is Option<PyRef<T>> and Option<PyRefMut<T>> (Option because if the wrong argument is passed from Python then the holder never gets populated).

I was careful to check both benchmarks and generated lines of code, and I got no noticeable difference to either. The net result is more friendly on type inference, leads to simpler macro code, and avoids clippy trouble.

To have a practical example of what this change results as - for a function which looks like below:

#[pyclass]
struct Foo;

#[pyfunction]
fn foo(_value: &Foo) { }

The relevant generated code has changed from:

let _tmp: <&Foo as _pyo3::derive_utils::ExtractExt<'_>>::Target =
    _pyo3::impl_::extract_argument::extract_argument(
        _pyo3::impl_::extract_argument::unwrap_required_argument(output[0usize]),
        "_value",
    )?;
#[allow(clippy::needless_option_as_deref)]
let arg0 = &*_tmp;
foo(arg0);

to:

foo(
    _pyo3::impl_::extract_argument::extract_argument(
        _pyo3::impl_::extract_argument::unwrap_required_argument(output[0usize]),
        &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
        "_value",
    )?
);

@davidhewitt davidhewitt requested a review from mejrs July 12, 2022 21:43
@davidhewitt
Copy link
Member Author

For the "bad import" case described in #2487, this PR appears to also cleanly solve that:

// missing PathBuf import
use pyo3::prelude::*;

#[pyfunction]
fn foo(_path: Option<PathBuf>) { }

gives an error exactly as one would hope:

error[E0412]: cannot find type `PathBuf` in this scope
 --> src/lib.rs:6:22
  |
6 | fn foo(_path: Option<PathBuf>) { }
  |                      ^^^^^^^ not found in this scope
  |
help: consider importing this struct
  |
3 | use std::path::PathBuf;
  |

@davidhewitt davidhewitt force-pushed the extract_argument_holder branch 3 times, most recently from 67fd2be to ba59ee9 Compare July 13, 2022 20:41
@davidhewitt davidhewitt force-pushed the extract_argument_holder branch from ba59ee9 to 209c942 Compare July 14, 2022 07:42
@davidhewitt
Copy link
Member Author

I'm sufficiently convinced that this is better than the ExtractExt implementation that I will proceed to merge now.

@davidhewitt davidhewitt merged commit fa19f32 into PyO3:main Jul 17, 2022
@davidhewitt davidhewitt deleted the extract_argument_holder branch July 17, 2022 06:14
@mejrs
Copy link
Member

mejrs commented Jul 19, 2022

Thanks :)

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