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

Remove Nonterminal and TokenKind::Interpolated #124141

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Apr 18, 2024

A third attempt at this; the first attempt was #96724 and the second was #114647.

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@nnethercote nnethercote marked this pull request as draft April 18, 2024 23:28
@nnethercote nnethercote force-pushed the rm-Nonterminal-and-TokenKind-Interpolated branch 2 times, most recently from 42a623a to c133e16 Compare April 23, 2024 03:51
@petrochenkov petrochenkov self-assigned this Apr 28, 2024
@ijackson
Copy link
Contributor

❤️ @nnethercote for working on this. Thank you! I'm not sure if there's a way for me to help, as someone who doesn't really know much about the compiler innards, but please LMK if you think of something.

@nnethercote
Copy link
Contributor Author

@ijackson: thanks! I'm curious why you are interested in this change, given that it's a compiler internals rearrangement?

@nnethercote
Copy link
Contributor Author

@ijackson: Oh, I see, you are interested in #67062 being fixed. Unfortunately my current thoughts are that this PR alone won't be enough to fix that issue, though it's a necessary stepping stone.

@ijackson
Copy link
Contributor

@ijackson: Oh, I see, you are interested in #67062 being fixed. Unfortunately my current thoughts are that this PR alone won't be enough to fix that issue, though it's a necessary stepping stone.

Right. It seems ... quite nontrivial. So, thanks.

@dev-ardi
Copy link
Contributor

After this is done TokenKind will become Copy right?

@nnethercote
Copy link
Contributor Author

After this is done TokenKind will become Copy right?

Yes.

nnethercote added a commit to nnethercote/rust that referenced this pull request May 16, 2024
Instead of using AST pretty printing.

This is a step towards removing `token::Interpolated`, which will
eventually (in rust-lang#124141) be replaced with a token stream within invisible
delimiters.

This changes (improves) the output of the `stringify!` macro in some
cases. This is allowed. As the `stringify!` docs say: "Note that the
expanded results of the input tokens may change in the future. You
should be careful if you rely on the output."

Test changes:

- tests/ui/macros/stringify.rs: this used to test both token stream
  pretty printing and AST pretty printing via different ways of invoking
  of `stringify!` (i.e. `$expr` vs `$tt`). But those two different
  invocations now give the same result, which is a nice consistency
  improvement. This removes the need for the `c2!` macro.

- tests/ui/macros/trace_faulty_macros.rs: there is some sub-optimal
  spacing in the printing of `A { a : a, b : 0, c : _, .. }`, which will
  be fixed in the next commit. The spacing of `1+1` improves -- it now
  matches the formatting in the source code.

- tests/ui/proc-macro/*: minor improvements where small differences
  between `INPUT (DISPLAY)` output and `DEEP-RE-COLLECTED (DISPLAY)`
  output disappear.
@nnethercote nnethercote force-pushed the rm-Nonterminal-and-TokenKind-Interpolated branch from c133e16 to 7aef5db Compare May 16, 2024 10:50
@rust-log-analyzer

This comment has been minimized.

nnethercote added a commit to nnethercote/rust that referenced this pull request May 17, 2024
Instead of using AST pretty printing.

This is a step towards removing `token::Interpolated`, which will
eventually (in rust-lang#124141) be replaced with a token stream within invisible
delimiters.

This changes (improves) the output of the `stringify!` macro in some
cases. This is allowed. As the `stringify!` docs say: "Note that the
expanded results of the input tokens may change in the future. You
should be careful if you rely on the output."

Test changes:

- tests/ui/macros/stringify.rs: this used to test both token stream
  pretty printing and AST pretty printing via different ways of invoking
  of `stringify!` (i.e. `$expr` vs `$tt`). But those two different
  invocations now give the same result, which is a nice consistency
  improvement. This removes the need for the `c2!` macro.

- tests/ui/macros/trace_faulty_macros.rs: there is some sub-optimal
  spacing in the printing of `A { a : a, b : 0, c : _, .. }`, which will
  be fixed in the next commit. The spacing of `1+1` improves -- it now
  matches the formatting in the source code.

- tests/ui/proc-macro/*: minor improvements where small differences
  between `INPUT (DISPLAY)` output and `DEEP-RE-COLLECTED (DISPLAY)`
  output disappear.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 17, 2024
…, r=<try>

Print `token::Interpolated` with token stream pretty printing.

This is a step towards removing `token::Interpolated` (rust-lang#124141). It unavoidably changes the output of the `stringify!` macro, generally for the better.

r? `@petrochenkov`
@nnethercote
Copy link
Contributor Author

nnethercote commented May 17, 2024

#125174 carves off a piece of this PR so it can be merged separately.

@bors
Copy link
Contributor

bors commented May 18, 2024

☔ The latest upstream changes (presumably #123865) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request May 18, 2024
Add tests for `-Zunpretty=expanded` ported from stringify's tests

This PR adds a new set of tests for the AST pretty-printer.

Previously, pretty-printer edge cases were tested by way of `stringify!` in [tests/ui/macros/stringify.rs](https://github.com/rust-lang/rust/blob/1.78.0/tests/ui/macros/stringify.rs), such as the tests added by rust-lang@419b269 and rust-lang@527e2ea.

Those tests will no longer provide effective coverage of the AST pretty-printer after rust-lang#124141. `Nonterminal` and `TokenKind::Interpolated` are being removed, and a consequence is that `stringify!` will perform token stream pretty printing, instead of AST pretty printing, in all of the `stringify!` cases including $:expr and all other interpolations.

This PR adds 2 new ui tests with `compile-flags: -Zunpretty=expanded`:

- **tests/ui/unpretty/expanded-exhaustive.rs** &mdash; this test aims for exhaustive coverage of all the variants of `ExprKind`, `ItemKind`, `PatKind`, `StmtKind`, `TyKind`, and `VisibilityKind`. Some parts could use being fleshed out further, but the current state is roughly on par with what exists in the old stringify-based tests.

- **tests/ui/unpretty/expanded-interpolation.rs** &mdash; this test covers tricky macro metavariable edge cases that require the AST pretty printer to synthesize parentheses in order for the printed code to be valid Rust syntax.

r? `@nnethercote`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2024
Rollup merge of rust-lang#125236 - dtolnay:expandtest, r=nnethercote

Add tests for `-Zunpretty=expanded` ported from stringify's tests

This PR adds a new set of tests for the AST pretty-printer.

Previously, pretty-printer edge cases were tested by way of `stringify!` in [tests/ui/macros/stringify.rs](https://github.com/rust-lang/rust/blob/1.78.0/tests/ui/macros/stringify.rs), such as the tests added by rust-lang@419b269 and rust-lang@527e2ea.

Those tests will no longer provide effective coverage of the AST pretty-printer after rust-lang#124141. `Nonterminal` and `TokenKind::Interpolated` are being removed, and a consequence is that `stringify!` will perform token stream pretty printing, instead of AST pretty printing, in all of the `stringify!` cases including $:expr and all other interpolations.

This PR adds 2 new ui tests with `compile-flags: -Zunpretty=expanded`:

- **tests/ui/unpretty/expanded-exhaustive.rs** &mdash; this test aims for exhaustive coverage of all the variants of `ExprKind`, `ItemKind`, `PatKind`, `StmtKind`, `TyKind`, and `VisibilityKind`. Some parts could use being fleshed out further, but the current state is roughly on par with what exists in the old stringify-based tests.

- **tests/ui/unpretty/expanded-interpolation.rs** &mdash; this test covers tricky macro metavariable edge cases that require the AST pretty printer to synthesize parentheses in order for the printed code to be valid Rust syntax.

r? `@nnethercote`
nnethercote added a commit to nnethercote/rust that referenced this pull request May 20, 2024
Instead of using AST pretty printing.

This is a step towards removing `token::Interpolated`, which will
eventually (in rust-lang#124141) be replaced with a token stream within invisible
delimiters.

This changes (improves) the output of the `stringify!` macro in some
cases. This is allowed. As the `stringify!` docs say: "Note that the
expanded results of the input tokens may change in the future. You
should be careful if you rely on the output."

Test changes:

- tests/ui/macros/stringify.rs: this used to test both token stream
  pretty printing and AST pretty printing via different ways of invoking
  of `stringify!` (i.e. `$expr` vs `$tt`). But those two different
  invocations now give the same result, which is a nice consistency
  improvement. This removes the need for all the `c2*` macros. The AST
  pretty printer now has more thorough testing thanks to rust-lang#125236.

- tests/ui/proc-macro/*: minor improvements where small differences
  between `INPUT (DISPLAY)` output and `DEEP-RE-COLLECTED (DISPLAY)`
  output disappear.
@petrochenkov
Copy link
Contributor

petrochenkov commented May 23, 2024

It's great to see that enum InvisibleOrigin allows to migrate the parser to delimited groups relatively simply, with just the maybe_whole to maybe_reparse_metavar_seq replacement.

Of course it prevents a lot of interesting stuff like reparsing expr as pat and similar, like it would work in a purely token-based model, but all that can be carefully introduced later, when it's possible to do backward compatibly.

@petrochenkov
Copy link
Contributor

How hard would it be to get this to a perf run?
(With or without the NtExpr/NtLiteral stuff.)

@petrochenkov
Copy link
Contributor

Blocked on #125174.
@rustbot blocked

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2024
@bors
Copy link
Contributor

bors commented Nov 10, 2024

☔ The latest upstream changes (presumably #132831) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote added a commit to nnethercote/rust that referenced this pull request Nov 13, 2024
It was added in rust-lang#130349, but it's not used meaningfully, and causes
difficulties for Nonterminal removal in rust-lang#124141.
@nnethercote nnethercote force-pushed the rm-Nonterminal-and-TokenKind-Interpolated branch from 9dfb022 to 313dbcd Compare November 13, 2024 04:20
nnethercote added a commit to nnethercote/rust that referenced this pull request Nov 18, 2024
It was added in rust-lang#130349, but it's not used meaningfully, and causes
difficulties for Nonterminal removal in rust-lang#124141.
nnethercote added a commit to nnethercote/rust that referenced this pull request Nov 20, 2024
It was added in rust-lang#130349, but it's not used meaningfully, and causes
difficulties for Nonterminal removal in rust-lang#124141.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2024
…petrochenkov

rust-lang#124141 preliminaries

Preliminary changes required to start removing `Nonterminal` (rust-lang#124141).

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2024
…petrochenkov

rust-lang#124141 preliminaries

Preliminary changes required to start removing `Nonterminal` (rust-lang#124141).

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2024
…petrochenkov

rust-lang#124141 preliminaries

Preliminary changes required to start removing `Nonterminal` (rust-lang#124141).

r? `@petrochenkov`
icmccorm pushed a commit to borrow-sanitizer/rust that referenced this pull request Nov 22, 2024
It was added in rust-lang#130349, but it's not used meaningfully, and causes
difficulties for Nonterminal removal in rust-lang#124141.
We now use invisible delimiters for expanded `vis` fragments, instead of
`Token::Interpolated`.
Notes about tests:

- tests/ui/parser/macro/trait-object-macro-matcher.rs: the syntax error
  is duplicated, because it occurs now when parsing the decl macro
  input, and also when parsing the expanded decl macro. But this won't
  show up for normal users due to error de-duplication.

- tests/ui/associated-consts/issue-93835.rs: ditto.

- The changes to metavariable descriptions in this PR's earlier commits
  are now visible in error message for several tests.
The one notable test change is `tests/ui/macros/trace_faulty_macros.rs`.
This commit removes the complicated `Interpolated` handling in
`expected_expression_found` that results in a longer error message. But
I think the new, shorter message is actually an improvement.

The original complaint was in rust-lang#71039, when the error message started
with "error: expected expression, found `1 + 1`". That was confusing
because `1 + 1` is an expression. Other than that, the reporter said
"the whole error message is not too bad if you ignore the first line".

Subsequently, extra complexity and wording was added to the error
message. But I don't think the extra wording actually helps all that
much. In particular, it still says of the `1+1` that "this is expected
to be expression". This repeats the problem from the original complaint!

This commit removes the extra complexity, reverting to a simpler error
message. This is primarily because the traversal is a pain without
`Interpolated` tokens. Nonetheless, I think the error message is
*improved*. It now starts with "expected expression, found `pat`
metavariable", which is much clearer and the real problem. It also
doesn't say anything specific about `1+1`, which is good, because the
`1+1` isn't really relevant to the error -- it's the `$e:pat` that's
important.
This involves replacing `nt_pretty_printing_compatibility_hack` with
`stream_pretty_printing_compatibility_hack`.

The handling of statements in `transcribe` is slightly different to
other nonterminal kinds, due to the lack of `from_ast` implementation
for empty statements.

Notable test changes:
- `tests/ui/proc-macro/expand-to-derive.rs`: the diff looks large but
  the only difference is the insertion of a single invisible-delimited
  group around a metavar.
This time when converting them to proc-macro `Group` form.
Note: there was an existing code path involving `Interpolated` in
`MetaItem::from_tokens` that was dead. This commit transfers that to the
new form, but puts an `unreachable!` call inside it.
Notes about tests:
- tests/ui/rfcs/rfc-2294-if-let-guard/feature-gate.rs: some messages are
  now duplicated due to repeated parsing.

- tests/ui/rfcs/rfc-2497-if-let-chains/disallowed-positions.rs: ditto.

- `tests/ui/proc-macro/macro-rules-derive-cfg.rs`: the diff looks large
  but the only difference is the insertion of a single
  invisible-delimited group around a metavar.
`NtBlock` is the last remaining variant of `Nonterminal`, so once it is
gone then `Nonterminal` can be removed as well.
They are no longer needed.

This does slightly worsen the error message for a single test, but that
test contains code that is so badly broken that I'm not worried about
it.
@nnethercote nnethercote force-pushed the rm-Nonterminal-and-TokenKind-Interpolated branch from 313dbcd to 2479f6c Compare November 25, 2024 05:39
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#17 exporting to docker image format
#17 sending tarball 31.1s done
#17 DONE 36.8s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 4.01s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
   Compiling core v0.0.0 (/checkout/library/core)
error: no rules expected `literal` metavariable
   --> /checkout/library/stdarch/crates/std_detect/src/detect/macros.rs:54:91
    |
51  | /         macro_rules! $macro_name {
53  | |                 ($feature_lit) => {
53  | |                 ($feature_lit) => {
54  | |                     $crate::detect_feature!($feature, $feature_lit $(, without cfg check: $feature_cfg_check)? $(: $($target_feature_lit)...
    | |                                                                                           ^^^^^^^^^^^^^^^^^^ 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"));
    |
note: while trying to match keyword `true`
   --> /checkout/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
error: no rules expected `literal` metavariable
   --> /checkout/library/stdarch/crates/std_detect/src/detect/macros.rs:54:91
    |
    |
51  | /         macro_rules! $macro_name {
53  | |                 ($feature_lit) => {
53  | |                 ($feature_lit) => {
54  | |                     $crate::detect_feature!($feature, $feature_lit $(, without cfg check: $feature_cfg_check)? $(: $($target_feature_lit)...
    | |                                                                                           ^^^^^^^^^^^^^^^^^^ no rules expected this token in macro call
88  | |             };
89  | |         }
    | |_________- in this expansion of `is_x86_feature_detected!`
    |
    |
   ::: std/tests/run-time-detect.rs:144:27
    |
144 |       println!("mmx: {:?}", is_x86_feature_detected!("mmx"));
    |
note: while trying to match keyword `true`
   --> /checkout/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
error: could not compile `std` (test "run-time-detect") due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:26:54
  local time: Mon Nov 25 06:10:25 UTC 2024

nnethercote added a commit to nnethercote/stdarch that referenced this pull request Nov 25, 2024
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

This CI failure:

error: no rules expected `literal` metavariable
   --> /checkout/library/stdarch/crates/std_detect/src/detect/macros.rs:54:91
    |
51  | /         macro_rules! $macro_name {
53  | |                 ($feature_lit) => {
53  | |                 ($feature_lit) => {
54  | |                     $crate::detect_feature!($feature, $feature_lit $(, without cfg check: $feature_cfg_check)? $(: $($target_feature_lit)...
    | |                                                                                           ^^^^^^^^^^^^^^^^^^ no rules expected this token in macro call
88  | |             };
89  | |         }
    | |_________- in this expansion of `is_x86_feature_detected!`

is due to a new issue in stdarch. There is some code in stdarch that the current compiler accepts but shouldn't, and this PR causes it to fail to compile. Details in rust-lang/stdarch#1680.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
Remove `NtVis` and `NtTy`

The next part of rust-lang#124141. The first actual remove of `Nonterminal` variants. `NtVis` is a simple case that doesn't get much use, but `NtTy` is more complex.

r? `@petrochenkov`
nnethercote added a commit to nnethercote/stdarch that referenced this pull request Nov 30, 2024
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`.
github-merge-queue bot pushed a commit to rust-lang/stdarch that referenced this pull request Nov 30, 2024
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants