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

Update ModuleInitializer to handle Bound API #3894

Closed
wants to merge 1 commit into from

Conversation

LilyFoote
Copy link
Contributor

@LilyFoote LilyFoote commented Feb 23, 2024

This allows us to make progress on deprecating Py::as_ref (#3864).

@LilyFoote
Copy link
Contributor Author

I'm stuck with errors in the hygiene tests:

error[E0277]: the trait bound `impl_::pymodule::ModuleInitializer: std::convert::From<for<'a, 'b> fn(marker::Python<'a>, &'b types::module::PyModule) -> std::result::Result<(), err::PyErr> {tests::hygiene::pymodule::my_module}>` is not satisfied
  --> src/tests/hygiene/pymodule.rs:18:4
   |
16 | #[crate::pymodule]
   | ------------------ required by a bound introduced by this call
17 | #[pyo3(crate = "crate")]
18 | fn my_module(_py: crate::Python<'_>, m: &crate::types::PyModule) -> crate::PyResult<()> {
   |    ^^^^^^^^^ the trait `std::convert::From<for<'a, 'b> fn(marker::Python<'a>, &'b types::module::PyModule) -> std::result::Result<(), err::PyErr> {tests::hygiene::pymodule::my_module}>` is not implemented for `impl_::pymodule::ModuleInitializer`
   |
   = help: the following other types implement trait `std::convert::From<T>`:
             <impl_::pymodule::ModuleInitializer as std::convert::From<for<'py, 'a> fn(marker::Python<'py>, &'a instance::Bound<'py, types::module::PyModule>) -> std::result::Result<(), err::PyErr>>>
             <impl_::pymodule::ModuleInitializer as std::convert::From<for<'py, 'a> fn(marker::Python<'py>, &'a types::module::PyModule) -> std::result::Result<(), err::PyErr>>>
   = note: required for `fn(Python<'a>, &PyModule) -> Result<(), PyErr> {my_module}` to implement `std::convert::Into<impl_::pymodule::ModuleInitializer>`
   = note: the full type name has been written to '/home/lily/dev/pyo3/target/debug/deps/pyo3-112773e66b7a548c.long-type-11202974107036457787.txt'

I'm not sure why the right From implementation isn't being found and used.

@@ -147,7 +173,7 @@ mod tests {
ModuleDef::new(
"test_module\0",
"some doc\0",
ModuleInitializer(|_, m| {
ModuleInitializer::GilRef(|_, m| {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also couldn't get .into() to work here, so I suspect there's just a problem with my From impls.

@Icxolu
Copy link
Contributor

Icxolu commented Feb 23, 2024

The problem with the From implementations lies in the subtle difference between function items and function pointers. Arguably the compiler error really is not that great here, but it's that bit here {tests::hygiene::pymodule::my_module}, which shows that the compiler sees this as a function item, which is a unique and unnamable type (similar to closures). To convert it into a function pointer, which is nameable, you have to explicitly cast it using something like as fn(_, _) -> _. I've done something similar in #3828 and #3837 if you want to take a look.

@LilyFoote
Copy link
Contributor Author

Aha!

I found this issue for improving the error message, but there's not been any progress in 4 years: rust-lang/rust#62385.

@LilyFoote
Copy link
Contributor Author

Hmm... I'm still not getting anywhere:

                    const INITIALIZER: impl_::ModuleInitializer = ::std::convert::Into::into(#fnname as fn(_, _) -> _);
error[E0277]: the trait bound `impl_::pymodule::ModuleInitializer: std::convert::From<fn(_, _) -> _>` is not satisfied
  --> src/tests/hygiene/pymodule.rs:16:1
   |
16 | #[crate::pymodule]
   | ^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<fn(_, _) -> _>` is not implemented for `impl_::pymodule::ModuleInitializer`
   |
   = help: the following other types implement trait `std::convert::From<T>`:
             <impl_::pymodule::ModuleInitializer as std::convert::From<for<'py, 'a> fn(marker::Python<'py>, &'a instance::Bound<'py, types::module::PyModule>) -> std::result::Result<(), err::PyErr>>>
             <impl_::pymodule::ModuleInitializer as std::convert::From<for<'py, 'a> fn(marker::Python<'py>, &'a types::module::PyModule) -> std::result::Result<(), err::PyErr>>>
   = note: required for `fn(_, _) -> _` to implement `std::convert::Into<impl_::pymodule::ModuleInitializer>`
   = note: this error originates in the attribute macro `crate::pymodule` (in Nightly builds, run with -Z macro-backtrace for more info)

@Icxolu
Copy link
Contributor

Icxolu commented Feb 24, 2024

Hmm, not sure why it can't figure out the types here. Maybe it has to do with the HRTBs. I assume that it would work if you give it more hints for the types, but there is a more fundamental problem here.

Because this is stored in a const we can't use conversions here, since we don't have const in traits yet. I thinks what we could do instead is to unconditionally change ModuleInitializer to store for<'a, 'py> fn(Python<'py>, &'a Bound<'py, PyModule>) -> PyResult<()> and then generate a wrapper function

fn module_initializer<'py>(py: Python<'py>, m: &Bound<'py, PyModule>) ->PyResult<()> {
    <original initializer>(py, Into::into(BoundWrapper(m)))
}

as part of the macro code. BoundWrapper can than be converted to both &Bound<PyModule> and &PyModule.

@davidhewitt
Copy link
Member

Sorry that I didn't manage to get around to seeing this PR sooner - what @Icxolu proposes above is actually what I happened to try on-stream and still exists as part of the draft in https://github.com/PyO3/pyo3/pull/3744/files#diff-912566621a79aeb2b0463cd8d22a412c3166600d99e1cc7f164aef67492c2429R96-R104

(That PR is very out of date, but it still contains both this and also an implementation of wrap_pyfunction_bound, if someone wants to take inspiration from it!)

@davidhewitt
Copy link
Member

Also it looks like #3815 also makes changes to PyModuleInitializer in the same way too, so maybe best to pause here until that's merged.

@LilyFoote
Copy link
Contributor Author

I did try to see if anyone was tackling this one, but I did miss those PRs. Nevermind, I think it was still valuable for my own learning!

@davidhewitt
Copy link
Member

I think after #3815 merges I'm totally happy if you want to rebase this and draw inspiration from #3744. Today I am just doing "project management" (e.g. catching up with reviews and might categorise good first issues) and hopefully writing docs later too.

@LilyFoote LilyFoote closed this Feb 25, 2024
@LilyFoote LilyFoote deleted the module-initializer-bound branch February 25, 2024 13:52
@LilyFoote
Copy link
Contributor Author

Replaced with #3897.

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.

3 participants