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

inventory & multiple-pymethods #2210

Closed
davidhewitt opened this issue Mar 5, 2022 · 16 comments · Fixed by #2492
Closed

inventory & multiple-pymethods #2210

davidhewitt opened this issue Mar 5, 2022 · 16 comments · Fixed by #2492

Comments

@davidhewitt
Copy link
Member

davidhewitt commented Mar 5, 2022

The inventory crate has been archived. This brings into question the future of the multiple-pymethods feature, which currently depends on inventory.

As far as I know, the crate was archived because it relies on platform-dependent linker behaviour, and also has unstable behaviour on newer Rust versions (dtolnay/inventory#32).

Deprecating this feature is the simplest path, however it is also a decent ergonomic improvement. In my day job I maintain a PyO3 codebase which exports a large number of #[pyclass] structs. To avoid repetition we have macros to write standard magic methods, e.g.

#[pyclass]
struct Foo { }

// Methods common to many types
basic_magic_methods!(Foo);
getstate_setstate!(Foo);

// Methods unique to this class
#[pymethods]
impl Foo {
    // ...
}

These helper macros basically extend to additional #[pymethods] blocks. Successfully migrating this codebase away from multiple-pymethods would require coming up with a suitable alternative pattern.


As an alternative to deprecating the feature, I've been wondering whether to experiment with bringing a minimalist in-crate implementation of linkme - the #[distributed_slice] concept should be enough to build #[pymethods] here.

In the long-term, it would be interesting to see if we can garner support for such a primitive in the language itself. See https://internals.rust-lang.org/t/from-life-before-main-to-common-life-in-main/16006

If a #[distributed_slice] implementation was landed in nightly Rust, we could then switch the multiple-pymethods feature to be nightly-only, with the hope that it would one day be available on stable.

@mejrs
Copy link
Member

mejrs commented Mar 5, 2022

Does linkme not have similar problems to inventory? It is also archived.

@adamreichold
Copy link
Member

If a #[distributed_slice] implementation was landed in nightly Rust, we could then switch the multiple-pymethods feature to be nightly-only, with the hope that it would one day be available on stable.

I think that would be the best long-term solution. Especially since the underlying issue not specific to PyO3.

@davidhewitt
Copy link
Member Author

Does linkme not have similar problems to inventory? It is also archived.

It does, however not fully understanding the exact issue that's causing the problem (and the fact that I've not seen any reports on PyO3 of multiple-pymethods not working), I am unsure whether the issues which plague the general solution affect the specific use case here.

@pravic
Copy link

pravic commented Mar 6, 2022

To avoid repetition we have macros to write standard magic methods, e.g.

Not as well-organized as the original, but this is viable too:

// Methods unique to this class
#[pymethods]
impl Foo {
    // Methods common to many types
    basic_magic_methods!();
    getstate_setstate!();

    // ...
}

@pravic
Copy link

pravic commented Mar 6, 2022

It does, however not fully understanding the exact issue that's causing the problem

See rust-lang/rust#47384:

Statics that are marked no_mangle and/or used only get to the linker if they are in a reachable module of a reachable crate. The static itself does not need to be used in code, only some function in the same module.

So, probably that's why you have no errors (yet?) because your #[pymethods] are reachable?

@davidhewitt
Copy link
Member Author

To avoid repetition we have macros to write standard magic methods, e.g.

Not as well-organized as the original, but this is viable too:

// Methods unique to this class
#[pymethods]
impl Foo {
    // Methods common to many types
    basic_magic_methods!();
    getstate_setstate!();

    // ...
}

I don't think this works - the #[pymethods] macro will not see the result of expanding basic_magic_methods! etc. So the methods included in that macro will not be exposed to Python.

@pravic
Copy link

pravic commented Mar 6, 2022

I don't think this works - the #[pymethods] macro will not see the result of expanding basic_magic_methods!

This makes me curious: what's the priority of procedural macros and macro_rules?

@davidhewitt
Copy link
Member Author

AFAIK it's always outermost macro gets expanded first, then inner ones.

@prehner
Copy link
Contributor

prehner commented Mar 9, 2022

Similar to @davidhewitt's case, we use macros to generate python methods. The macros mimic Rust traits, that are defined in a core crate and are implemented in more specific crates. In every specific crate Python classes are defined, that call the macro to automatically implement the functionalities given by the Rust traits. This structure relies heavily on the multiple-pymethods feature. Therefore, deprecating the feature would have quite a detrimental effect on our crates.

@Igosuki
Copy link

Igosuki commented Mar 15, 2022

Interested to see how you solve this as I'm both a user of pyo3 and inventory ...

@CLOVIS-AI
Copy link
Contributor

CLOVIS-AI commented Jun 16, 2022

I haven't looked at how multiple-pymethods is implemented, but #[distributed_slice] would be enough to implement the inspection / pyi generation for multiple #[pymethods] block, and would actually make the code simpler even for a single # [pymethods].

@birkenfeld
Copy link
Member

@pravic
Copy link

pravic commented Jul 1, 2022

For Rust 1.62+, though. But it's good news nonetheless!

@birkenfeld
Copy link
Member

Yep. Since Debian 12 may freeze in 1/2023, we have a good chance of a new enough toolchain making it in there.

@davidhewitt
Copy link
Member Author

Given that there's a possible soundness concern, I'd argue that in PyO3 0.17 we should actually update inventory to 0.3 and so require that multiple-pymethods uses Rust 1.62.

@birkenfeld
Copy link
Member

Fair enough.

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 a pull request may close this issue.

8 participants