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

Fix the features macro. #1680

Merged
merged 1 commit into from
Nov 30, 2024
Merged

Conversation

nnethercote
Copy link
Contributor

The first rule of the features macro looks like this:

macro_rules! features {
    (
      @TARGET: $target:ident;
      @CFG: $cfg:meta;
      @MACRO_NAME: $macro_name:ident;
      @MACRO_ATTRS: $(#[$macro_attrs:meta])*
      $(@BIND_FEATURE_NAME: $bind_feature:tt; $feature_impl:tt; $(#[$deprecate_attr:meta];)?)*
      $(@NO_RUNTIME_DETECTION: $nort_feature:tt; )*
      $(@FEATURE: #[$stability_attr:meta] $feature:ident: $feature_lit:tt;
          $(without cfg check: $feature_cfg_check:literal;)?
          $(implied by target_features: [$($target_feature_lit:tt),*];)?
          $(#[$feature_comment:meta])*)*
    ) => {

Notice all the tt specifiers. They are used because they are forwarded to another macro. Only ident, lifetime, and tt specifiers can be forwarded this way.

But there is an exception: $feature_lit:tt, which was added recently. In theory it should cause an error like this:

error: no rules expected `literal` metavariable
   --> /home/njn/dev/rust3/library/stdarch/crates/std_detect/src/detect/macros.rs:54:91
    |
51  | /         macro_rules! $macro_name {
52  | |             $(
53  | |                 ($feature_lit) => {
54  | |                     $crate::detect_feature!($feature, $feature_lit $(, without cfg check: $feature_cfg_check)? ...
    | |                                                                                           ^^^^^^^^^^^^^^^^^^ no rules expected this token in macro call
...   |
88  | |             };
89  | |         }
    | |_________- in this expansion of `is_x86_feature_detected!`
    |
   ::: std/tests/run-time-detect.rs:145:27
    |
145 |       println!("tsc: {:?}", is_x86_feature_detected!("tsc"));
    |                             ------------------------------- in this macro invocation
    |
note: while trying to match keyword `true`
   --> /home/njn/dev/rust3/library/stdarch/crates/std_detect/src/detect/macros.rs:12:55
    |
12  |     ($feature:tt, $feature_lit:tt, without cfg check: true) => {
    |                                                       ^^^^
    = note: captured metavariables except for `:tt`, `:ident` and `:lifetime` cannot be compared to other tokens
    = note: see <https://doc.rust-lang.org/nightly/reference/macros-by-example.html#forwarding-a-matched-fragment> for more information

(The URL at the end of the error has more details about this forwarding limitation.)

In practice it doesn't cause this error. I'm not sure why, but the existing macro implementation in rustc is far from perfect, so it's believable that it does the wrong thing here.

Why does this matter? Because rust-lang/rust#124141 is modifying the macro implementation, and when that PR is applied the error does occur. (It's one of several cases I have found where the existing compiler accepts code it shouldn't, but #124141 causes that code to be rejected.)

Fortunately the fix is simple: replace the literal specifier with tt.

@rustbot
Copy link
Collaborator

rustbot commented Nov 25, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

@nnethercote
Copy link
Contributor Author

You can see the problem in action on the rustc CI at rust-lang/rust#124141 (comment)

@Amanieu
Copy link
Member

Amanieu commented Nov 29, 2024

CI should be fixed now, can you rebase?

The first rule of the `features` macro looks like this:
```
macro_rules! features {
    (
      @target: $target:ident;
      @cfg: $cfg:meta;
      @MACRO_NAME: $macro_name:ident;
      @MACRO_ATTRS: $(#[$macro_attrs:meta])*
      $(@BIND_FEATURE_NAME: $bind_feature:tt; $feature_impl:tt; $(#[$deprecate_attr:meta];)?)*
      $(@NO_RUNTIME_DETECTION: $nort_feature:tt; )*
      $(@feature: #[$stability_attr:meta] $feature:ident: $feature_lit:tt;
          $(without cfg check: $feature_cfg_check:literal;)?
          $(implied by target_features: [$($target_feature_lit:tt),*];)?
          $(#[$feature_comment:meta])*)*
    ) => {
```
Notice all the `tt` specifiers. They are used because they are forwarded
to another macro. Only `ident`, `lifetime`, and `tt` specifiers can be
forwarded this way.

But there is an exception: `$feature_lit:tt`, which was added recently.
In theory it should cause an error like this:
```
error: no rules expected `literal` metavariable
   --> /home/njn/dev/rust3/library/stdarch/crates/std_detect/src/detect/macros.rs:54:91
    |
51  | /         macro_rules! $macro_name {
52  | |             $(
53  | |                 ($feature_lit) => {
54  | |                     $crate::detect_feature!($feature, $feature_lit $(, without cfg check: $feature_cfg_check)? ...
    | |                                                                                           ^^^^^^^^^^^^^^^^^^ no rules expected this token in macro call
...   |
88  | |             };
89  | |         }
    | |_________- in this expansion of `is_x86_feature_detected!`
    |
   ::: std/tests/run-time-detect.rs:145:27
    |
145 |       println!("tsc: {:?}", is_x86_feature_detected!("tsc"));
    |                             ------------------------------- in this macro invocation
    |
note: while trying to match keyword `true`
   --> /home/njn/dev/rust3/library/stdarch/crates/std_detect/src/detect/macros.rs:12:55
    |
12  |     ($feature:tt, $feature_lit:tt, without cfg check: true) => {
    |                                                       ^^^^
    = note: captured metavariables except for `:tt`, `:ident` and `:lifetime` cannot be compared to other tokens
    = note: see <https://doc.rust-lang.org/nightly/reference/macros-by-example.html#forwarding-a-matched-fragment> for more information
```
(The URL at the end of the error has more details about this forwarding
limitation.)

In practice it doesn't cause this error. I'm not sure why, but the
existing macro implementation in rustc is far from perfect, so it's
believable that it does the wrong thing here.

Why does this matter? Because rust-lang/rust#124141
is modifying the macro implementation, and when that PR is applied the
error *does* occur. (It's one of several cases I have found where the
existing compiler accepts code it shouldn't, but #124141 causes that
code to be rejected.)

Fortunately the fix is simple: replace the `literal` specifier with `tt`.
@nnethercote
Copy link
Contributor Author

I rebased.

@Amanieu Amanieu added this pull request to the merge queue Nov 30, 2024
Merged via the queue into rust-lang:master with commit b86e49b Nov 30, 2024
54 checks passed
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.

4 participants