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

Accept paths in wrap_* macros #1709

Closed
Tracked by #2079
Legend-of-iPhoenix opened this issue Jul 2, 2021 · 11 comments · Fixed by #2081
Closed
Tracked by #2079

Accept paths in wrap_* macros #1709

Legend-of-iPhoenix opened this issue Jul 2, 2021 · 11 comments · Fixed by #2081

Comments

@Legend-of-iPhoenix
Copy link

For example, for a python submodule defined in a separate rust module, the following fails to compile with an error saying that no rules expected the ::.

use pyo3::prelude::*;
use pyo3::{wrap_pymodule};

mod a;

#[pymodule]
fn mymodule(_py: Python, module: &PyModule) -> PyResult<()> {
	module.add_wrapped(wrap_pymodule!(a::submodule))?;

	Ok(())
}

There was a tiny bit of discussion about this over on the matrix channel.

@davidhewitt
Copy link
Member

davidhewitt commented Jul 2, 2021

Fwiw I had previously tried to implement this in the past, but handling the path in a macro_rules! macro was pretty painful.

When we bump MSRV past 1.42 then we can take advantage of proc macros in expression position and reimplement the wrap_ macros in pyo3-macros. This would allow us to drop the dependency on paste too.

Eric-Arellano added a commit to Eric-Arellano/pyo3 that referenced this issue Jul 20, 2021
davidhewitt pushed a commit that referenced this issue Jul 22, 2021
* Rewrite `module.md` for clarity and add tip on code organization

* Add section on how to build the guide + add workaround proposed by David

* Make more clear references to #1709
@davidhewitt
Copy link
Member

blocked on #1782

@aviramha aviramha added this to Nice-to-have in 1.0 Nov 18, 2021
@aviramha aviramha moved this from Nice-to-have to To Do in 1.0 Nov 18, 2021
@davidhewitt
Copy link
Member

This is unblocked once #2004 merges, so we can make this part of 0.16

@davidhewitt davidhewitt added this to the 0.16 milestone Nov 18, 2021
@davidhewitt
Copy link
Member

(If anyone is interested in implementing this, I'm happy to provide some mentoring notes. It should be a reasonably gentle introduction to the proc-macro code.)

@aviramha
Copy link
Member

@davidhewitt Some hints would be nice, I might take it.

@davidhewitt
Copy link
Member

Sure thing. (Maybe assign this issue to yourself if you do start working on it so that others know not to start?)

So at the moment wrap_pyfunction is a macro_rules! macro implemented in src/macros.rs. All it really does is adjust the input ident to refer to a hidden symbol which #[pyfunction] generates.

e.g. wrap_pyfunction!(foo) expands to &|py| __pyo3_get_function_foo(py).

To accept paths, I think the cleanest approach will be to convert it to a proc-macro. So the first step will be to re-implement the existing wrap_pyfunction expansion as a proc-macro instead. The pyo3-macros-backend code has a helper function function_wrapper_ident which you can use to generate the identifier of the hidden symbol instead of paste.

Once that's done, the new proc-macro implementation can be expanded to accept either a path or an ident. If a path is found, just the trailing element should be tweaked.

e.g. wrap_pyfunction!(foo::bar) should expand to &|py| foo::__pyo3_get_function_bar(py).

... once that's working, you'll need to do an equivalent rewrite for wrap_pymodule! and then we can actually drop the paste dependency completely!

@aviramha
Copy link
Member

Okay, I tried a take but I'm getting confused with the part where wrap_pyfunction! can take arguments then it recursively call itself.
This is my current code:

#[proc_macro]
pub fn wrap_pyfunction(input: TokenStream) -> TokenStream {
    let path = parse_macro_input!(input as syn::Path);
    if let Some(ident) = path.get_ident() {
        quote!(&|py| __pyo3_get_function_#ident(py))
            .into()
    } else {
        let mut segments = path.segments.clone();
        let function_segment = segments.pop().unwrap().value();
        quote!(&|py| #segments __pyo3_get_function_#function_segment(py))
            .into()
    }
}

Can I do recursive code also for my function?

@davidhewitt
Copy link
Member

Hmm, to be honest I wouldn't bother with going recursion, instead with two arguments you could just expand wrap_pyfunction!(foo::bar, py) to foo::__pyo3_get_function_bar(py).

To achieve that you'll probably want to have a struct something like this:

#[derive(Clone, Debug, PartialEq)]
pub struct WrapPyFunctionArgs {
    pub path: syn::Path,
    pub py: Option<(Token![,], syn::Expr)>,
}

impl Parse for WrapPyFunctionArgs {
    fn parse(input: ParseStream) -> Result<Self> {
        Ok(WrapPyFunctionArgs {
            path: input.parse()?,
            py: input.parse()?.map(|comma| (comma, input.parse()?),
        })
    }
}

... and then go for the alternative expansion if py expression is present.

I'm not totally sure that that Parse implementation is valid, but hopefully you get the idea.

@davidhewitt
Copy link
Member

@aviramha how did you get on with this? If you've got some code on a branch, want to push it and open a PR and I'll help finish it off? Otherwise, I'm happy to write an implementation for this; it would be nice to drop the paste dependency for 0.16.

@davidhewitt davidhewitt mentioned this issue Dec 27, 2021
10 tasks
@aviramha
Copy link
Member

Sorry I didn't make much progress that is worth a branch..

@davidhewitt
Copy link
Member

No problem; I managed to take a spin at this tonight in #2081!

1.0 automation moved this from To Do to Done Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
1.0
Done
Development

Successfully merging a pull request may close this issue.

3 participants