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

Fix IntoPyCallbackOutput paper cuts #2326

Merged
merged 9 commits into from May 9, 2022
Merged

Fix IntoPyCallbackOutput paper cuts #2326

merged 9 commits into from May 9, 2022

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Apr 23, 2022

struct Blah;

#[pyfunction]
fn blah() -> Blah{
    Blah
}

Current error:

error[E0277]: the trait bound `Blah: IntoPyCallbackOutput<_>` is not satisfied
   --> src\lib.rs:5:1
    |
5   | #[pyfunction]
    | ^^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<_>` is not implemented for `Blah`
    |
note: required by a bound in `pyo3::callback::convert`
   --> C:\Users\bruno\rust\pyo3\src\callback.rs:182:8
    |
182 |     T: IntoPyCallbackOutput<U>,
    |        ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `pyo3::callback::convert`
    = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.

Error with this PR:

error[E0599]: the method `into_result` exists for struct `Blah`, but its trait bounds were not satisfied
   --> src\lib.rs:5:1
    |
3   | struct Blah;
    | ------------
    | |
    | method `into_result` not found for this
    | doesn't satisfy `Blah: IntoPy<Py<PyAny>>`
    | doesn't satisfy `Blah: IntoResult<Result<Blah, PyErr>>`
4   |
5   | #[pyfunction]
    | ^^^^^^^^^^^^^ method cannot be called on `Blah` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `Blah: IntoPy<Py<PyAny>>`
            which is required by `Blah: IntoResult<Result<Blah, PyErr>>`
            `&Blah: IntoPy<Py<PyAny>>`
            which is required by `&Blah: IntoResult<Result<&Blah, PyErr>>`
            `&mut Blah: IntoPy<Py<PyAny>>`
            which is required by `&mut Blah: IntoResult<Result<&mut Blah, PyErr>>`
note: the following trait must be implemented
   --> C:\Users\bruno\rust\pyo3\src\conversion.rs:182:1
    |
182 | / pub trait IntoPy<T>: Sized {
183 | |     /// Performs the conversion.
184 | |     fn into_py(self, py: Python<'_>) -> T;
185 | | }
    | |_^
    = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0599`.

Todo:

  • Changelog
  • Tests
  • Some ui tests

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.

Thanks!

First commit message - "Make arrays of pyclasses easier to sue" - oops :)

src/callback.rs Outdated Show resolved Hide resolved
src/callback.rs Outdated Show resolved Hide resolved
src/callback.rs Outdated Show resolved Hide resolved
src/callback.rs Outdated Show resolved Hide resolved
src/conversions/array.rs Show resolved Hide resolved
@mejrs mejrs marked this pull request as draft April 28, 2022 21:48
@mejrs mejrs force-pushed the callback branch 2 times, most recently from 7e54611 to 7477842 Compare May 1, 2022 09:07
@mejrs mejrs marked this pull request as ready for review May 1, 2022 20:49
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.

Really nice! Just a few final thoughts, and I think it would be better to change from IntoPyResult<R> -> IntoPyResult<T>. After that, I think this is good to merge 👍

src/callback.rs Outdated Show resolved Hide resolved
src/callback.rs Outdated Show resolved Hide resolved
src/callback.rs Outdated Show resolved Hide resolved
src/conversions/array.rs Show resolved Hide resolved
src/conversions/array.rs Show resolved Hide resolved
tests/ui/invalid_result_conversion.stderr Outdated Show resolved Hide resolved
Comment on lines 486 to 489
if false {
use _pyo3::impl_::ghost::IntoPyResult;
ret.into_py_result();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer this approach over what I did before. This won't actually call into_py_result if an user accidentally writes it for their type.

Comment on lines 1 to 19
/// If it does nothing, was it ever really there? 👻
///
/// This is code that is just type checked to e.g. create better compile errors,
/// but that never affects anything at runtime,
use crate::{IntoPy, PyErr, PyObject};

#[allow(clippy::wrong_self_convention)]
pub trait IntoPyResult<T> {
fn into_py_result(&mut self) {}
}

impl<T> IntoPyResult<T> for T where T: IntoPy<PyObject> {}

impl<T, E> IntoPyResult<T> for Result<T, E>
where
T: IntoPy<PyObject>,
E: Into<PyErr>,
{
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer this approach of code that doesn't actually do anything 😉

Comment on lines +27 to +38
error[E0277]: the trait bound `Blah: IntoPyCallbackOutput<_>` is not satisfied
--> tests/ui/missing_intopy.rs:3:1
|
3 | #[pyo3::pyfunction]
| ^^^^^^^^^^^^^^^^^^^ the trait `IntoPyCallbackOutput<_>` is not implemented for `Blah`
|
note: required by a bound in `pyo3::callback::convert`
--> src/callback.rs
|
| T: IntoPyCallbackOutput<U>,
| ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `pyo3::callback::convert`
= note: this error originates in the attribute macro `pyo3::pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)
Copy link
Member Author

Choose a reason for hiding this comment

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

It's unfortunate this IntoPyCallbackOutput part is back - but overall I think it's worth it. The first part of the error is very very good.

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.

Sneaky! Nicely done 👍

/// but that never affects anything at runtime,
use crate::{IntoPy, PyErr, PyObject};

#[allow(clippy::wrong_self_convention)]
Copy link
Member

Choose a reason for hiding this comment

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

Is this lint actually necessary? I wonder if it's a hangover from experimentation.

Copy link
Member Author

@mejrs mejrs May 8, 2022

Choose a reason for hiding this comment

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

Yes, that hits the methods called into_* usually take self by value lint. The reason it takes &mut self is to prevent a cascade like this:

           which is required by `Blah: IntoResult<Result<Blah, PyErr>>`
            which is required by `Blah: IntoPyResult<Result<Blah, PyErr>>`
            `&Blah: IntoPy<Py<PyAny>>`
            which is required by `&Blah: IntoResult<Result<&Blah, PyErr>>`
            which is required by `&Blah: IntoPyResult<Result<&Blah, PyErr>>`
            `&mut Blah: IntoPy<Py<PyAny>>`
            which is required by `&mut Blah: IntoResult<Result<&mut Blah, PyErr>>`
            which is required by `&mut Blah: IntoPyResult<Result<&mut Blah, PyErr>>`

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see! Do you think we should name the method differently then? E.g. into_py_result_hint or assert_into_py_result?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure 👍

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.

Thanks, LGTM! To squash out the various interim commits I'm gonna squash-merge this one 👍

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.

IntoPyCallbackOutput<_> is not implimented for arrays of pyclass
2 participants