Remove `Nonterminal` and `TokenKind::Interpolated` #124141

nnethercote · 2024-04-18T23:28:21Z

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

rustbot · 2024-04-18T23:28:25Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

ijackson · 2024-05-10T15:28:11Z

❤️ @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 · 2024-05-10T20:45:22Z

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

nnethercote · 2024-05-10T20:47:31Z

@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 · 2024-05-10T22:40:41Z

@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 · 2024-05-12T21:49:53Z

After this is done TokenKind will become Copy right?

nnethercote · 2024-05-12T22:35:11Z

After this is done TokenKind will become Copy right?

Yes.

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.

…, 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 · 2024-05-17T06:47:27Z

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

bors · 2024-05-18T00:46:59Z

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

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** — 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** — 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`

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** — 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** — 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`

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 · 2024-05-23T20:01:24Z

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 · 2024-05-23T20:02:23Z

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

petrochenkov · 2024-05-23T20:03:04Z

Blocked on #125174.
@rustbot blocked

rustbot · 2024-09-10T23:31:10Z

Could not assign reviewer from: petrochenkov.
User(s) petrochenkov are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

…Kind-Interpolated, r=<try> Remove `Nonterminal` and `TokenKind::Interpolated` A third attempt at this; the first attempt was rust-lang#96724 and the second was rust-lang#114647. r? `@ghost`

bors · 2024-09-10T23:32:20Z

⌛ Trying commit 74e2b10 with merge 4882bfc...

bors · 2024-09-11T01:28:03Z

☀️ Try build successful - checks-actions
Build commit: 4882bfc (4882bfc6682c66afefb9f4457b7700b9fb4fc577)

nnethercote · 2024-09-11T02:28:11Z

@craterbot check

craterbot · 2024-09-11T02:28:20Z

👌 Experiment pr-124141 created and queued.
🤖 Automatically detected try build 4882bfc
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

rust-timer · 2024-09-11T03:47:29Z

Finished benchmarking commit (4882bfc): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[0.3%, 5.9%]	29
Regressions ❌ (secondary)	6.5%	[0.3%, 20.4%]	21
Improvements ✅ (primary)	-0.4%	[-0.7%, -0.2%]	33
Improvements ✅ (secondary)	-0.5%	[-1.1%, -0.2%]	32
All ❌✅ (primary)	0.9%	[-0.7%, 5.9%]	62

Max RSS (memory usage)

Results (primary -2.5%, secondary 6.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	8.1%	[2.5%, 17.9%]	15
Improvements ✅ (primary)	-2.5%	[-2.9%, -2.2%]	2
Improvements ✅ (secondary)	-1.9%	[-2.2%, -1.1%]	4
All ❌✅ (primary)	-2.5%	[-2.9%, -2.2%]	2

Cycles

Results (primary 3.0%, secondary 7.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.0%	[0.9%, 6.6%]	31
Regressions ❌ (secondary)	7.0%	[1.2%, 24.2%]	36
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[0.9%, 6.6%]	31

Binary size

Results (primary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	8

Bootstrap: 756.373s -> 775.217s (2.49%)
Artifact size: 341.27 MiB -> 341.12 MiB (-0.04%)

[CRATER] Crater rollup This is a crater rollup of: * rust-lang#124141 * rust-lang#130285 * rust-lang#130367 **What is a crater rollup?** It's simply a (manually set-up) crater job that is run on all of the containing PRs together, and then we can set the crates list for each of these jobs to *just* the list of failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just waste time to build without being affected by the union of all of these changes. After this is finished, I will adjust all of the jobs to use only the list of failed crates. That should significantly speed up these jobs from taking like ~6 days to taking ~2. See the last time I did this: rust-lang#129660. Given that rust-lang#130285 is running in build-and-test mode, let's run all of them in build-and-test mode.

craterbot · 2024-09-16T07:49:07Z

🚧 Experiment pr-124141 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Merge `PatParam`/`PatWithOr`, and `Expr`/`Expr2021`, for a few reasons. - It's conceptually nice, because the two pattern kinds and the two expression kinds are very similar. - With expressions in particular, there are several places where both expression kinds get the same treatment. - It removes one unreachable match arm. - Most importantly, for rust-lang#124141 I will need to introduce a new type `MetaVarKind` that is very similar to `NonterminalKind`, but records a couple of extra fields for expression metavars. It's nicer to have a single `MetaVarKind::Expr` expression variant to hold those extra fields instead of duplicating them across two variants `MetaVarKind::{Expr,Expr2021}`. And then it makes sense for patterns to be treated the same way, and for `NonterminalKind` to also be treated the same way. I also clarified the comments, because I have long found them a little hard to understand.

…r=compiler-errors Rework pattern and expression nonterminal kinds. Some tweaks to `NonterminalKind` that will assist with rust-lang#124141. Details in the individual commits. r? compiler-errors cc ```@eholk```

craterbot · 2024-09-18T20:14:30Z

🎉 Experiment pr-124141 is completed!
📊 191 regressed and 1 fixed (511940 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

nnethercote · 2024-09-19T00:04:06Z

Ugh, lots of failures to investigate.

It currently doesn't handle the three-char tokens `>>=` and `<<=` correctly. These can be broken twice, resulting in three individual tokens. This is a latent bug that currently doesn't cause any problems, but does cause problems for rust-lang#124141, because that PR increases the usage of lazy token streams.

nnethercote · 2024-09-19T09:56:03Z

I have found and fixed two sources of failures.

All(?) the ICE failures are fixed by Fix break_last_token. #130551, which was caused by mishandling of >>= tokens in lazy_static! calls like this (simplified):
```
lazy_static! {
    static ref ABC: Vec<Vec<u64>>= vec![];
}
```
where the >>= wasn't being split and captured correctly, which was causing the type to fail to re-parse later on, because the second > was missing.
There are multiple proc macros failing because they can't handle nested invisible delimiter groups. The fix for that is straightforward -- in mk_delimited strip any outer invisible delimiters before adding new invisible delimiters. I will incorporate that fix into this PR. I'm not sure how many of the failures it will fix.

It currently doesn't handle the three-char tokens `>>=` and `<<=` correctly. These can be broken twice, resulting in three individual tokens. This is a latent bug that currently doesn't cause any problems, but does cause problems for rust-lang#124141, because that PR increases the usage of lazy token streams.

petrochenkov · 2024-09-20T16:33:20Z

Blocked on #130551.
@rustbot blocked

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

nnethercote marked this pull request as draft April 18, 2024 23:28

nnethercote mentioned this pull request Apr 18, 2024

Start removing Nonterminal #114647

Closed

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 self-assigned this Apr 28, 2024

nnethercote force-pushed the rm-Nonterminal-and-TokenKind-Interpolated branch from c133e16 to 7aef5db Compare May 16, 2024 10:50

This comment has been minimized.

Sign in to view

nnethercote mentioned this pull request May 17, 2024

Print token::Interpolated with token stream pretty printing. #125174

Merged

dtolnay mentioned this pull request May 18, 2024

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

Merged

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

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 10, 2024

This comment has been minimized.

Sign in to view

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 11, 2024

compiler-errors mentioned this pull request Sep 14, 2024

[CRATER] Crater rollup #130373

Closed

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 18, 2024

nnethercote mentioned this pull request Sep 19, 2024

Fix break_last_token. #130551

Open

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `Nonterminal` and `TokenKind::Interpolated` #124141

Remove `Nonterminal` and `TokenKind::Interpolated` #124141

nnethercote commented Apr 18, 2024 •

edited

Loading

rustbot commented Apr 18, 2024

ijackson commented May 10, 2024

nnethercote commented May 10, 2024

nnethercote commented May 10, 2024

ijackson commented May 10, 2024

dev-ardi commented May 12, 2024

nnethercote commented May 12, 2024

This comment has been minimized.

nnethercote commented May 17, 2024 •

edited

Loading

bors commented May 18, 2024

petrochenkov commented May 23, 2024 •

edited

Loading

petrochenkov commented May 23, 2024

petrochenkov commented May 23, 2024

rustbot commented Sep 10, 2024

This comment has been minimized.

bors commented Sep 10, 2024

bors commented Sep 11, 2024

This comment has been minimized.

nnethercote commented Sep 11, 2024

craterbot commented Sep 11, 2024

rust-timer commented Sep 11, 2024

craterbot commented Sep 16, 2024

craterbot commented Sep 18, 2024

nnethercote commented Sep 19, 2024

nnethercote commented Sep 19, 2024

petrochenkov commented Sep 20, 2024

Remove Nonterminal and TokenKind::Interpolated #124141

Are you sure you want to change the base?

Remove Nonterminal and TokenKind::Interpolated #124141

Conversation

nnethercote commented Apr 18, 2024 • edited Loading

rustbot commented Apr 18, 2024

ijackson commented May 10, 2024

nnethercote commented May 10, 2024

nnethercote commented May 10, 2024

ijackson commented May 10, 2024

dev-ardi commented May 12, 2024

nnethercote commented May 12, 2024

This comment has been minimized.

nnethercote commented May 17, 2024 • edited Loading

bors commented May 18, 2024

petrochenkov commented May 23, 2024 • edited Loading

petrochenkov commented May 23, 2024

petrochenkov commented May 23, 2024

rustbot commented Sep 10, 2024

This comment has been minimized.

bors commented Sep 10, 2024

bors commented Sep 11, 2024

This comment has been minimized.

nnethercote commented Sep 11, 2024

craterbot commented Sep 11, 2024

rust-timer commented Sep 11, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

craterbot commented Sep 16, 2024

craterbot commented Sep 18, 2024

nnethercote commented Sep 19, 2024

nnethercote commented Sep 19, 2024

petrochenkov commented Sep 20, 2024

Remove `Nonterminal` and `TokenKind::Interpolated` #124141

Remove `Nonterminal` and `TokenKind::Interpolated` #124141

nnethercote commented Apr 18, 2024 •

edited

Loading

nnethercote commented May 17, 2024 •

edited

Loading

petrochenkov commented May 23, 2024 •

edited

Loading