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

Add support for wrapping attributes in #[cfg_attr(feature = "pyo3", …)] #2786

Closed
wants to merge 10 commits into from

Conversation

konstin
Copy link
Member

@konstin konstin commented Nov 27, 2022

This adds support for wrapping attributes in #[cfg_attr(feature = "pyo3", ...)], making it possible to build rust libraries with optional python bindings. Check optional_bindings.md for an extensive example.

The main trouble with the PR was that some of the attribute parsing is done with syn::parse::Parse impls, while other parts are done with syn::Meta matching, while syn itself just exposes attribute contents as token stream. Another difficulty is that while #[foo] is a single attribute, in #[cfg_attr(feature = "something", foo, bar)] you can have completely unrelated foo and bar attributes.

For testing, i had to add a pyo3 feature to pyo3 itself. I couldn't find a better solution to get a pyo3 active in the test, but OTOH that feature shouldn't cause any problems on the user side

@birkenfeld
Copy link
Member

Does this fix #780 ? (It was closed by the submitter, not fixed.)

@konstin
Copy link
Member Author

konstin commented Nov 27, 2022

yep!

Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

Looking at the new test file, that is a lot of cfg_attr...

Wild idea: how about a macro that can be applied to a whole module/scope and removes the pyo3 related attributes? It can then be added with cfg_attr(not(feature = "pyo3"), ...)

guide/src/class/optional_bindings.md Outdated Show resolved Hide resolved
if let Some(options) = get_pyo3_options(attr)? {
let mut new_attrs = Vec::new();

for mut attr in attrs.drain(..) {
Copy link
Member

Choose a reason for hiding this comment

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

You can also do

for mut attr in std::mem::take(attrs)

and then push to attrs, saving line 149 and 177.

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 find it easier to read when we have a distinct old-attr-drain and new attr building; iterating and editing/deleting at the same time is unfortunately difficult to write straightforward

newsfragments/2786.added.md Outdated Show resolved Hide resolved
guide/src/class/optional_bindings.md Outdated Show resolved Hide resolved
guide/src/class/optional_bindings.md Show resolved Hide resolved
@konstin
Copy link
Member Author

konstin commented Nov 29, 2022

Wild idea: how about a macro that can be applied to a whole module/scope and removes the pyo3 related attributes? It can then be added with cfg_attr(not(feature = "pyo3"), ...)

I've thought about that too: We could have a stub feature in pyo3 that does nothing but removing the attributes and depends if on anything at all on syn, which would be really cool. This would however mean that by default all dependencies are optional and that build script must not set any linker options while still supporting the existing feature combinations, which is complex. I think it would also make sense to migrate all attributes to the #[pyo3(...)] form before so we can have "self-removing attributes"

@birkenfeld
Copy link
Member

Wild idea: how about a macro that can be applied to a whole module/scope and removes the pyo3 related attributes? It can then be added with cfg_attr(not(feature = "pyo3"), ...)

I've thought about that too: We could have a stub feature in pyo3 that does nothing but removing the attributes and depends if on anything at all on syn, which would be really cool. This would however mean that by default all dependencies are optional and that build script must not set any linker options while still supporting the existing feature combinations, which is complex. I think it would also make sense to migrate all attributes to the #[pyo3(...)] form before so we can have "self-removing attributes"

It would have to be a separate crate pyo3-stub otherwise, but then imports get more complex.

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.

This is very cool, thanks for implementing!

If I understand correctly, this works by assuming that the top-level macro was also wrapped in #[cfg_attr(feature = "pyo3", pyclass)] (for example).

There is some interesting edge cases from this, e.g. I think the following code will work even in crates which don't define a "pyo3" feature:

#[pyclass]
struct Foo;

#[pymethods]
impl Foo {
    #[cfg_attr(feature = "pyo3", new)]
    fn new() -> Self { Self }
}

... because the current assumption seems to be that if the macro is running, the top-level macro has already passed the cfg_attr gate.

Comment on lines +113 to +117
# This makes `#[cfg_attr(feature = "pyo3", pyclass)]` in our own tests work, it has no other function
pyo3 = []
Copy link
Member

Choose a reason for hiding this comment

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

I wonder... should we move this to the pytests crate or something similar, so that we don't need to expose a no-op feature to the users?

To avoid other code changes, you might also just be able to hack it by adding RUSTFLAGS=--cfg "feature=pyo3", which is all cargo really does under the hood I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

so like moving the ui test to pytests/src? The RUSTFLAGS would also do, but imho it'd be nice if cargo test would continue to work.

I've also been thinking about using a different feature name for the tests, but i couldn't come up with a good design for that either, especially for cases such as the guide doc test

Comment on lines 163 to 204
if let Ok(mut meta) = attr.parse_meta() {
if handle_cfg_feature_pyo3(&mut attr, &mut meta, parse_attr)? {
continue;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to check if attr.path.is_ident("cfg_attr") before calling parse_meta, to avoid a lot of unnecessary parsing work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is parsing Meta expensive? This is the only location where we do it in this order, in the other two locations we also need meta for the normal parsing

Copy link
Member

Choose a reason for hiding this comment

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

So I've now written a benchmark in #2794

It's based against main, what I see on my machine is that the cost per attribute (for a very basic attribute) goes from about 250ns to 1us. Most of that overhead is removed again by just adding this check.

So sure, while it's not much, maybe in a bigger codebase with lots of attributes this could eventually add up to a second or so. I'd like us to try to keep macro performance as good as possible, especially when it's so easy to add the check here.

(Generally I've been heading in the direction of avoiding meta parsing and writing more specific parse implementations like the one here, for best efficiency.)

Copy link
Member Author

Choose a reason for hiding this comment

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

sweet, i didn't expect this much rigor! i've now gated both parse with is_ident

Copy link
Member Author

@konstin konstin Dec 1, 2022

Choose a reason for hiding this comment

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

fwiw, i think eventually moving everything into pyo3(...) would also help with perf because then we can skip the non-pyo3 macro parsing completely for attributes that are not ours

@davidhewitt
Copy link
Member

Wild idea: how about a macro that can be applied to a whole module/scope and removes the pyo3 related attributes? It can then be added with cfg_attr(not(feature = "pyo3"), ...)

Also agreed this is a very interesting idea. Note that inner macro attributes are unstable so it would have to go on each pyclass (or a thing containing pyclasses, maybe a whole module after #2367).

Assuming that typically users would set things up to have no pyo3 by default, and then expose a feature which adds a pyo3 dependency, I'm trying to think through the practical usage.

It would have to be a separate crate pyo3-stub otherwise, but then imports get more complex.

I don't think it's so bad; if a PyO3 dependency is disabled then imports will have a #[cfg] anyway for any imported PyO3 items.

With a separate crate, users can probably just have something like

#[cfg_attr(any(test, not(feature = "py")), pyo3_stub::pyo3_stub)]
#[pyclass]
struct Foo;

Or with a stub feature in PyO3, I guess we'd potentially need to make the base PyO3 without features contain absolutely nothing and then enable e.g. c-api, macros-stub, macros as users want? So this would be breaking.

So I'm thinking maybe the separate crate is the way to go? It could still depend on pyo3-macros-backend but without any pyo3-build-config. I'd be quite interested to see how that compares to the approach in this PR!

@konstin
Copy link
Member Author

konstin commented Nov 29, 2022

There is some interesting edge cases from this, e.g. I think the following code will work even in crates which don't define a "pyo3" feature:

#[pyclass]
struct Foo;

#[pymethods]
impl Foo {
    #[cfg_attr(feature = "pyo3", new)]
    fn new() -> Self { Self }
}

... because the current assumption seems to be that if the macro is running, the top-level macro has already passed the cfg_attr gate.

yeah this would work, but tbh i don't think anybody will run into problems with that, feature = "pyo3" is too oddly specific

@davidhewitt
Copy link
Member

On reflection I'm leaning towards merging this (preferably with slight perf tweak as noted above).

Although we agree it introduces a weird edge case, you could argue #[cfg_attr] not working at all is also a weird edge case and definitely more annoying for users!

@konstin
Copy link
Member Author

konstin commented Dec 1, 2022

Although we agree it introduces a weird edge case, you could argue #[cfg_attr] not working at all is also a weird edge case and definitely more annoying for users!

yeah this is really more a hack to get this case to work at all. For a real solution we'd need either cooperation from rustc to tell us which ctr_attr epressions would evaluate to true (or just being able to ask it to preprocess the attributes) or some help from cargo to stub out proc macros, as in "don't even compile the proc macro crate".

@konstin
Copy link
Member Author

konstin commented Dec 1, 2022

BTW this is the reason i wanted to have this feature: https://github.com/konstin/pep440-rs

@mejrs
Copy link
Member

mejrs commented Dec 3, 2022

Although we agree it introduces a weird edge case, you could argue #[cfg_attr] not working at all is also a weird edge case and definitely more annoying for users!

I'm reluctant (not necessarily opposed) to merge this. It's a rust edge case; not a pyo3 edge case, so I'd much rather point people to cfg_eval instead.

@davidhewitt
Copy link
Member

@mejrs I know what you mean, initially I felt similar, I've warmed to it a bit when looking at it through the lens of which edge case is least bad for the user.

Last I saw #[cfg_eval] it was proposed for stabilization but then postponed because it's unclear what extra features the language wants to give to proc macros here.

I wonder, to give us some control (and way to deprecate this when the language can do better), if we should gate this behavior behind an option? E.g. #[pyclass(feature = "pyo3")] or #[pyclass(cfg = (feature = "pyo3"))] ? @mejrs do you think this would be beneficial?

@davidhewitt
Copy link
Member

I wonder, to give us some control (and way to deprecate this when the language can do better), if we should gate this behavior behind an option? E.g. #[pyclass(feature = "pyo3")] or #[pyclass(cfg = (feature = "pyo3"))] ? @mejrs do you think this would be beneficial?

Anyone got any further thoughts on this? I can help push this over the line for 0.18, though I'd like some input on what configuration form folks like. Personally, I think having #[pyclass(feature = "pyo3")] as the way to enable this is best, as it's reasonably simple and gives the users the choice of the feature name.

@joshparallel
Copy link

I wonder, to give us some control (and way to deprecate this when the language can do better), if we should gate this behavior behind an option? E.g. #[pyclass(feature = "pyo3")] or #[pyclass(cfg = (feature = "pyo3"))] ? @mejrs do you think this would be beneficial?

Anyone got any further thoughts on this? I can help push this over the line for 0.18, though I'd like some input on what configuration form folks like. Personally, I think having #[pyclass(feature = "pyo3")] as the way to enable this is best, as it's reasonably simple and gives the users the choice of the feature name.

Does this mean I as a user would have to use the above syntax every time I invoke pyclass? Is there a way to provide control and a deprecation pathway without exposing the extra verbosity to the user at the call site? E.g. if I could just specify the feature in one place in my workspace Cargo.toml that would be really easy for me. Also, would a more self-explanatory feature name be desirable, e.g. "feature_guards"? Sorry if I'm misunderstanding anything here...

@mejrs
Copy link
Member

mejrs commented Dec 28, 2022

I wonder, to give us some control (and way to deprecate this when the language can do better), if we should gate this behavior behind an option? E.g. #[pyclass(feature = "pyo3")] or #[pyclass(cfg = (feature = "pyo3"))] ? @mejrs do you think this would be beneficial?

This assumes people will read the documentation, which I assume they won't. If we do this then few users will discover it. So if we introduce this feature I'd want it to "just work".

@konstin
Copy link
Member Author

konstin commented Jan 1, 2023

This assumes people will read the documentation, which I assume they won't. If we do this then few users will discover it. So if we introduce this feature I'd want it to "just work".

Sorry i'm not following, but which is the solution you'd want?

@davidhewitt
Copy link
Member

If we do this then few users will discover it. So if we introduce this feature I'd want it to "just work".

Sorry i'm not following, but which is the solution you'd want?

I read @mejrs' comment as "don't add either option, and keep this as-is".

Is there a way to provide control and a deprecation pathway without exposing the extra verbosity to the user at the call site?

I suppose it may be the case that if the language eventually supports a better solution we might be able to transparently just use it without any deprecation.

... in which case, should we just merge this as-is? (After rebase and maybe a squash)

@mejrs
Copy link
Member

mejrs commented Jan 2, 2023

Sorry i'm not following, but which is the solution you'd want?

that either:

  • we merge this PR as is, no gating behind an option.
  • we don't merge it

I am in the same boat as you: I also conditionally use pyo3. Which is why I wrote #2796 and #2692 but these are done in a way that is unsurprising and composes well. I'm not sure that's true for this PR; it feels rather hacky.

I personally would prefer to just tell users to write...

impl Foo{
    // Rust methods
}

#[cfg(feature = "pyo3")]
#[pymethods]
impl Foo{
    // Python methods
}

...which may result in some extra boilerplate, but that's not a disaster or anything.

That said, if you are convinced that there are no gotchas or nasty corner cases with it, I'm not opposed to merging this 👍

@davidhewitt
Copy link
Member

I don't think there are nasty gotchas, though I agree with you it is hacky and the sort of thing we may regret later as a result.

I guess worst case if we merge this, we get reports of issues which mean we need to gate it behind the option later on?

@davidhewitt
Copy link
Member

The suggestion to simply recommend a separate pymethods block with a top-level #[cfg] also seems very reasonable to me, I wonder if this would be suitable for you @konstin ?

@davidhewitt davidhewitt modified the milestones: 0.18, 0.19 Jan 20, 2023
@konstin
Copy link
Member Author

konstin commented Jan 30, 2023

(sorry this slipped through my notifications, i was just reminded by konstin/pep440-rs#4)

The suggestion to simply recommend a separate pymethods block with a top-level #[cfg] also seems very reasonable to me, I wonder if this would be suitable for you @konstin ?

I've already been doing that, I need that for the struct itself, mainly https://github.com/konstin/pep440-rs/blob/3148c9016cbc01a9e6116ae8080b10e14e985487/src/version.rs#L247-L284

That said, if you are convinced that there are no gotchas or nasty corner cases with it, I'm not opposed to merging this +1

i'm rather certain that the solution as-is is the best workaround for the lack of stable cfg_eval we can currently get

@ChristopherRabotin
Copy link

Hi there,

I was wondering whether there was any chance this would merge in the coming weeks. I would use it. My use-case is that I want to allow Python users to set some of the fields of a struct (which are Python-compatible) but not touch the other ones.

Thanks

@davidhewitt
Copy link
Member

davidhewitt commented Feb 28, 2023

Sorry, I somehow missed the last reply on this thread from @konstin .

So the understanding we have is that #[pymethods] has a workaround but #[pyclass] currently does not.

The other day I had an idea to add a getters option to #[pyclass] (name to be bikeshed) with the context being that it could support more complex lookups. I was a bit unsure of its power, though I see here it would also solve the problem.

E.g. the code we have today

#[pyclass]
pub struct Foo {
    #[pyo3(get, set, name = "bar")]
    pub baz: usize,
}

could be rewritten using this hypothetical getters (and setters) as

#[pyclass(
    getters = (bar = self.baz),
    setters = (bar = self.baz)
)]
pub struct Foo {
    pub baz: usize,
}

which would scale naturally to #[cfg_attr] without hacks:

#[cfg_attr(
    feature = "pyo3",
    pyclass(
        getters = (bar = self.baz),
        setters = (bar = self.baz)
    )
)]
pub struct Foo {
    pub baz: usize,
}

Perhaps we should explore that idea further?

@ChristopherRabotin
Copy link

ChristopherRabotin commented Mar 3, 2023

I have no preference ;-)

Edit: what is the current workaround for pymethods? I am having an issue using the new for conditional pymethods.

@konstin konstin force-pushed the cfg-feature-pyo3 branch from 25fe971 to 6492f93 Compare March 3, 2023 10:41
@konstin
Copy link
Member Author

konstin commented Mar 3, 2023

we could definitely move all field attributes to into the container attribute, but this will most likely be more cumbersome both in pyo3 itself and on the user side. I still think the current solution is the best compromise we can get as a workaround for the lack of #[cfg_eval].

konstin added a commit to konstin/pep508_rs that referenced this pull request Mar 16, 2023
publish to crates.io is also in there but disabled due to PyO3/pyo3#2786
konstin added a commit to PyO3/pyproject-toml-rs that referenced this pull request Mar 22, 2023
…08 parser

Using crates.io releases is currently blocked on PyO3/pyo3#2786
@davidhewitt davidhewitt removed this from the 0.19 milestone Jun 16, 2023
@ChristopherRabotin
Copy link

@davidhewitt sorry to bump this up again, but I'm about to embark on a new PyO3 development that will also be behind a feature flag. What is the best approach to solve what this PR solves with version 0.20 (or higher)? Thanks

@davidhewitt
Copy link
Member

I think #2786 (comment) is still most applicable.

@xnerhu
Copy link

xnerhu commented Oct 15, 2024

what's the status?

@konstin
Copy link
Member Author

konstin commented Oct 15, 2024

There does not seem to be progress on cfg_eval, but this PR has too many conflicts to still being merged.

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.

7 participants