Allow passing expr metavariable as cfg predicate rust-lang/rust#146961

View all comments

This PR allows expanding expr metavariables inside the configuration predicates of cfg and cfg_attr invocations.
For example, the following code will now compile:

macro_rules! mac {
    ($e:expr) => {
        #[cfg_attr($e, inline)]
        #[cfg($e)]
        fn func() {}

        #[cfg(not($e))]
        fn func() {
            panic!()
        }
    }
}


mac!(any(unix, feature = "foo"));

There is currently no macro_rules fragment specifier that can represent all valid cfg predicates. meta comes closest, but excludes true and false. By fixing that, this change makes it easier to write declarative macros that parse cfg or cfg_attr invocations, for example #146281.

@rustbot label T-lang needs-fcp A-attributes A-cfg A-macros

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Just as an update, after #124141 we effectively no longer have nonterminal tokens in the grammar (*).
At parsing time a pasted expr metavariable is not some NtExpr token, it's just a token stream in invisible parentheses.
If that token stream from expr parses as a cfg predicate then it can be used as a cfg predicate, if it parses as a type or pattern, then it can be used as a pattern as well.
And on the other hand, if a token stream from some other matcher like ty or pat parses as a cfg predicate, then it can be used as a cfg predicate.
In general, the matcher kind is only relevant during macro LHS matching, not during RHS parsing (**).

I hoped that some time after #124141 either @nnethercote or me relax these rules in many cases, when there are no backward compatibility concerns at least, and pass the change through lang team, but it didn't happen yet.
(In particular, tokens from any matchers should be parse-able in expr, ty and pat positions, there should be no compatibility issues there.)

A number of current hacks like accepting impl $ty for Type, or what this PR suggests, are just special cases of that strategy.

@Jules-Bertholet You could very well start the same process, just from a different point and accept any token streams that look like cfg predicates as cfg predicates, from any matchers, not just expr.

(*) Or we technically have, but only for compatibility and to avoid extending the language without the lang team process.
(**) Again, unless there are backward compatibility issues.

As for the current implementation, the new expr-accepting logic is used in parse_meta_item and the new attr parsing infra, so it seems more than just cfg predicates specifically, but I'm not sure what else exactly is affected.

This seems reasonable to me. It allows all :expr fragments here, and not all of those will subsequently parse, but that seems okay.

@rfcbot merge

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot reviewed

@rfcbot reviewed

🔔 This is now entering its final comment period, as per the review above. 🔔

As for the current implementation, the new expr-accepting logic is used in parse_meta_item and the new attr parsing infra, so it seems more than just cfg predicates specifically, but I'm not sure what else exactly is affected.

Happy to see a more general version of this, if we can.

@rfcbot reviewed

I am in favor of making the "special tokens" and invisible delimiters less visible to the user.

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Procedural note, this has been marked as S-waiting-on-t-lang but technically should be S-waiting-on-review.

I reviewed the code and think this would be good to merge, unless @petrochenkov has any opinions.

View changes since this review

As for the current implementation, the new expr-accepting logic is used in parse_meta_item and the new attr parsing infra, so it seems more than just cfg predicates specifically, but I'm not sure what else exactly is affected.

During the first review it was not clear to me what this PR actually implements ^^^.
But I'll look again.

As for the current implementation, the new expr-accepting logic is used in parse_meta_item and the new attr parsing infra, so it seems more than just cfg predicates specifically, but I'm not sure what else exactly is affected.

This concern still stands.
This PR doesn't just support expr in cfg predicates like the PR description says, it supports it in wide range of contexts using parse_meta_item from the regular parsing infra or parse_attr_item from the Dutch parsing infra.
From the lang team comments above it's not clear whether it's what was approved or not.

The PR either need to be changed to implement what it describes (support for expr in cfg predicates), or document what it actually implements instead and ensure the lang team is ok with it.
@rustbot author

Reminder, once the PR becomes ready for a review, use @rustbot ready.

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rustbot ready

This should now be confined to affecting cfg predicates only.

r? @JonathanBrouwer
Could you double check this? The new attribute parsing infra is impenetrable.
The expr fragments are now supposed to parsed in cfg predicates, and only in cfg predicates.

(Why does it all converge to parse_attr_item? cfg predicates are not attr items at all...)

@rustbot ready

@bors r+ rollup

View changes since this review

📌 Commit ab36d50 has been approved by JonathanBrouwer

It is now in the queue for this repository.