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

Stabilize built-in attribute macro #[cfg_eval] #87221

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

Built-in attribute macro #[cfg_eval] is used for eagerly expanding all #[cfg] and #[cfg_attr] attributes in its input ("fully configuring" the input).
The effect is identical to effect of #[derive(Foo, Bar)] which also fully configures its input before passing it to macros Foo and Bar, except that cfg_eval is supported on any code fragments supporting macro attributes, unlike derive.

Example:

#[cfg_eval]
#[my_attr] // Receives `struct S {}` as input, the field is configured away by `#[cfg_eval]`
struct S {
    #[cfg(FALSE)]
    field: u8,
}

The main alternative to this attribute is to automatically fully configure items before expanding any attributes.
That alternative was implemented as an experiment in #85073.
Crater showed that there are indeed attributes that want to do some custom cfg processing (#85073 (comment)) which are broken by this change, so we need some kind opt-out from eager configuration, but it's not clear how that opt-out should look like.
I suggest considering switching to this alternative for the next edition (~Rust 2024), but leave it alone for now.

This PR is somewhat related to #87220, but I don't think they are blocking each other.

Closes #82679
r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2021
@petrochenkov petrochenkov added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2021
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 17, 2021
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting. Sorry for the delay in processing this.

The general sentiment in the meeting was that we were uncertain about putting #[cfg_eval] at the use sites of attributes or similar. It seemed to everyone in the meeting that it shouldn't be the responsibility of the caller of #[my_attr] to know that #[my_attr] can't handle unevaluated cfgs; it should be the responsibility of #[my_attr].

Whether as an opt-in (for current editions) or an opt-out (for a future edition that defaults to it), either way we felt like this should be tied to the proc macro and not to the call site.

We know that people want eager evaluation for macros, but it'd be helpful to understand what the use case is for determining this at the call site.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 7, 2021
@petrochenkov
Copy link
Contributor Author

#[cfg_eval] at call site is necessary if we want the "straightforward left to right expansion" scheme as a goal, with #[cfg]s and #[cfg_attr]s expanded more or less like any other macro attributes.

#[cfg(foo)] // expanded first
#[bar] // expanded second
#[cfg(baz)] // expanded third
struct S {
    #[xyz] // expanded fourth
    #[cfg(abc)] // expanded fifth
    field: u8,
}

In that case there's no language way to expand attributes out-of-order, we can only ask the leftmost attribute to process its input and output the processed token stream. If we want deep cfg-expansion as processing, then #[cfg_eval] is the leftmost attribute which we are asking to do it. Other attributes can perform other tasks, deep cfg-expansion is not built into the language, it's just some specific macro attribute's logic.

If we are ok with #[cfg]s and #[cfg_attr] being very special and not like other active attributes, and pulling more logic (deep cfg-expansion) into the core language, then I have nothing against doubling down on their eager expansion (#85073) and having some control over it at macro definition time.

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 8, 2021
@nikomatsakis
Copy link
Contributor

I am nervous about the piecemeal stabilization of changes here. It feels a bit directionless to me. I am wondering whether we could start an initiative to draft up and document the considerations, as well as the overall goal we are reaching for. For example, latent in @petrochenkov's comment seems to be an assumption that "outer-to-inner" processing is what we want-- this may be true, it makes some sense to me, but I'd like to see that evaluated against a set of use cases or something.

@nikomatsakis
Copy link
Contributor

Put another way, I think that @petrochenkov and others likely have put quite a bit of thought into what they'd like to see, but I feel like we haven't captured those thoughts in a way that I can readily read and understand.

@jonasbb
Copy link
Contributor

jonasbb commented Sep 8, 2021

Whether as an opt-in (for current editions) or an opt-out (for a future edition that defaults to it), either way we felt like this should be tied to the proc macro and not to the call site.

How will this work with two macros, one which cares about cfg values and the other one which needs them evaluated. Let's assume the proc-macros do not alter the tokenstream and simply return the input. So right now, the order of the attributes would not matter.

#[no_cfg]
#[cares_about_cfg]
struct S {}

// or

#[cares_about_cfg]
#[no_cfg]
struct S {}

In the first case, will the cares_about_cfg see any cfg values or will the no_cfg basically act like a cfg_eval. In the latter case, it would be a breaking change to add cfg_eval to the proc-macro and it might also be surprising to see the cfg attributes stripped.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 8, 2021
@petrochenkov
Copy link
Contributor Author

@nikomatsakis

I am wondering whether we could start an initiative to draft up and document the considerations, as well as the overall goal we are reaching for. For example, latent in @petrochenkov's comment seems to be an assumption that "outer-to-inner" processing is what we want-- this may be true, it makes some sense to me, but I'd like to see that evaluated against a set of use cases or something.

There's some additional context to this, but not too much.

Multiple attributes on the same target were always resolved and expanded in left-to-right order in general, but there were a few exceptions.

In the last couple of years I've been working on eliminating such exceptions, starting from hacks like #63248, and finishing with turning derive into a regular macro attribute #79078.

Right now the only remaining attributes that are expanded out-of-order are #[cfg] and #[cfg_attr].
I tried to expand them left-to-right in #83354, but it's a change that is breaking enough, so we cannot do it immediately.
I've been intending to add a lint like "these attributes are written in one order, but expanded in another" to make progress.
Fixing such lint usually only requires reordering attributes, but in some cases it may require #[cfg_eval], e.g.

// `cfg_attr` is evaluated first, and written second so the lint is reported
// however, we cannot reorder the attributes because `my_trait_helper` is not in scope before `#[derive(MyTrait)]`
// but we can use `cfg_eval` to both evaluate `cfg_attr` and keep relative order of `MyTrait` and `my_trait_helper`
#[derive(MyTrait)]
#[cfg_attr(my_feature, my_trait_helper)]
struct S;

Left-to-right expansion makes everything simpler starting from mental model, and ending with compiler implementation, including token collection for attribute macros.
But if cfg and cfg_attr stay an exception in this regard, then it's not a tragedy either.

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2021
@petrochenkov
Copy link
Contributor Author

Note that #[cfg_eval] as stabilized here is not a core language feature, but a procedural macro that could potentially be user-written if proc macro API were sufficiently rich to have access to values of predicates like target_os = "foo" etc.
So it's somewhat orthogonal to other core language changes like enabling eager evaluation of cfg(_attr)s by default, in the worst case this macro may become a no-op in the future.

@nikomatsakis
Copy link
Contributor

@petrochenkov

This example took me a second to understand. Let me read it back, cause I guess I get it now:

#[derive(MyTrait)]
#[cfg_attr(my_feature, my_trait_helper)]
struct S;

The problem here is that derive excepts to be a "fully configured" input, right? But if we move that #[cfg_attr] earlier in the list, then the fully configured version would be

#[my_trait_helper]
#[derive(MyTrait)]
struct S;

and that is broken. So we put #[cfg_eval] first instead. Interesting. Subtle! Now I have to re-read your message to try and see what I think in light of this understanding =)

@nikomatsakis
Copy link
Contributor

@petrochenkov

a procedural macro that could potentially be user-written if proc macro API were sufficiently rich to have access to values of predicates

Alternatively, it seems like a limited form of "eager expansion", right? i.e., if the proc macro API had some way to say "expand attributes matching some filter that you see in this list", then you could implement it this as a procedural macro.

@nikomatsakis
Copy link
Contributor

I ask because: I agree that left-to-right is desirable; but I'm also not super keen on having to write #[cfg_eval] in the example you gave. I feel like I might prefer if #[derive] had the option to run configuration on its input (which other procedural macros might want to take advantage of, too). I'm trying to decide if that would be easier or harder to understand.

I've traditionally been resistant to eager expansion, but it does seem kind of inevitable that we'll have attributes that do want to do something like it.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Sep 18, 2021

@nikomatsakis

The problem here is that derive excepts to be a "fully configured" input, right?

No, derive does the cfg-evaluation for MyTrait by itself (#87220), that's not the problem.
In the world with cfg_attr being expanded in-order

#[derive(MyTrait)]
#[cfg_attr(my_feature, my_trait_helper)]
struct S;

is perfectly ok.

The problem is purely with the lint, in the world with cfg_attr being expanded out-of-order.
If the lint is sufficiently smart, then it would ideally not fire on this case, but would still fire on cases where reordering is the right thing to do, like

#[test]
#[cfg(FALSE)] // WARNING: `cfg` is written after `test`, but expanded before `test`, please reorder
fn foo() {}

I guess #[cfg_eval] is not strictly necessary in the derive helper case, because

#[allow(out_of_order_expansion)]
#[derive(MyTrait)]
#[cfg_attr(my_feature, my_trait_helper)]
struct S;

would work too, since it's indeed a false positive, and reordering is not the right thing to do here.

@petrochenkov
Copy link
Contributor Author

Alternatively, it seems like a limited form of "eager expansion", right?

Eager expansion in general should be able to resolve paths, and deal with the "path cannot be resolved yet" situations.
Cfg expansion doesn't need to do anything like that, it's just some token stream processing that any stable proc macros can do today, only predicate values are missing.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Sep 21, 2021

@jonasbb

In the first case, will the cares_about_cfg see any cfg values or will the no_cfg basically act like a cfg_eval. In the latter case, it would be a breaking change to add cfg_eval to the proc-macro and it might also be surprising to see the cfg attributes stripped.

So, I think for a proc macro that wants to mostly add code rather than to modify the item it would be ideal to be able to use both the configured version of the input for adding new code and the original version of it for re-emitting.
That's what #[derive] does with #87220 - use configured, re-emit unconfigured.

This functionality can neither be provided by #[cfg_eval] nor by auto-configuration, it likely needs a proc macro api like fn configure_this(TokenStream, MacroPositionKind) -> TokenStream (with MacroPositionKind being the main problem for stabilization).

@jonasbb
Copy link
Contributor

jonasbb commented Sep 21, 2021

That's what #[derive] does with #87220 - use configured, re-emit unconfigured.

That seems useful for the example I gave, and only by the description I gave, a derive macro likely works better. I don't know of any specific pair of proc-macros which would behave like this.

I mainly wanted to add to this point by @joshtriplett, by showing that some situations might still arise where the caller needs to know if the proc-macro evaluates cfg-attributes.

It seemed to everyone in the meeting that it shouldn't be the responsibility of the caller of #[my_attr] to know that #[my_attr] can't handle unevaluated cfgs; it should be the responsibility of #[my_attr].

@mejrs mejrs mentioned this pull request Oct 11, 2021
@nikomatsakis
Copy link
Contributor

We discussed this in our @rust-lang/lang meeting on 2021-10-12. @petrochenkov we wanted to clarify something and make sure we understand it.

Based on your OP, it seems like you think the right future is one where "cfg happens by default", before any procedural macros run. But unfortunately before we could do that switch, there would be a need to have some kind of opt-out?

I'm not quite sure, then, why we are adding a opt-in, and I'm not quite sure where the idea of a lint that encourages reordering things fits in here, especially since that reordering is not (ultimately) expected to matter.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 19, 2021
@joshtriplett joshtriplett added I-lang-nominated Nominated for discussion during a lang team meeting. and removed I-nominated I-lang-nominated Nominated for discussion during a lang team meeting. labels Oct 20, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 14, 2021
@camelid camelid added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2021
@petrochenkov
Copy link
Contributor Author

it seems like you think the right future is one where "cfg happens by default", before any procedural macros run

I'm not sure in this.
After discussion above the right solution seems to be different for different macros:

  • If the macro is cfg-agnostic, then it's better to not configure the input just to avoid extra work.
  • If the macro needs cfgs expanded, and mostly adds code rather than changes it (example: derive), then it needs to re-emit the unconfigured version, and produce some code based on the configured version. So a new proc macro API for configuring token streams is required.
  • If the macro needs cfgs expanded, and changes its input, then it only needs the configured version, and "cfg by default" will work.
  • Finally, if the macro needs the original unconfigured input, then some way for observing the original strream is required if "cfg
    by default" is enabled.

So 3 cases against 1 for not doing the configuration by default, or at least providing a way to observe the unconfigured version of the input together with the configured one.

I'm not quite sure, then, why we are adding a opt-in

Because it will be useful for at least 3 years, regardless of whether we switch the defaults later or not.

@petrochenkov
Copy link
Contributor Author

I'm going to close this for now and first land the refactoring parts of #83354, and implement the lint discussed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for built-in attribute macro #[cfg_eval]
9 participants