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

nnethercote · 2024-05-16T10:38:44Z

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

r? @petrochenkov

nnethercote · 2024-05-16T10:41:42Z

@bors try @rust-timer queue

…, r=<try> Print `token::Interpolated` with token stream pretty printing. r? `@ghost`

bors · 2024-05-16T10:42:53Z

⌛ Trying commit f7ce1ac with merge 53af78d...

nnethercote · 2024-05-17T06:45:59Z

Full details are in the individual commits.

@dtolnay has agreed to update the tests/ui/macros/stringify.rs test and move the AST pretty printing checks into a new test. That could be done before or after this PR is merged.

A crater run should be done before this merges, to make sure the stringify! changes don't cause problems for existing code.

Performance is unlikely to be affected, but let's confirm that:
@bors try @rust-timer queue

…, 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`

bors · 2024-05-17T06:47:11Z

⌛ Trying commit 9c693ad with merge e07dc21...

compiler/rustc_parse/src/parser/mod.rs

bors · 2024-05-17T08:24:10Z

☀️ Try build successful - checks-actions
Build commit: e07dc21 (e07dc21eff3c4c268d22684a47778eff58db7d17)

dtolnay · 2024-05-17T09:37:54Z

While I can see how this PR is a necessary step toward removing token::Interpolated, I think it could use some better explanation why removing token::Interpolated is a necessary step toward solving #67062 (it's a necessary stepping stone) before we would commit to this change.

It unavoidably changes the output of the stringify! macro, generally for the better.

This is the opposite of what I expect, and I consider stringify! changes to be a casualty of choosing this approach. We'll need to add a separate stringify_expr! macro to core. The AST pretty printer is always going to be more capable at stringifying expressions than the tokenstream pretty printer.

stringify! is used by the assert and dbg family of macros in the ecosystem (although apparently not by std's assert, which is a built-in and continues to use the AST pretty-printer). The prototypical usage is https://docs.rs/failure/0.1.8/failure/macro.ensure.html

#[macro_export]
macro_rules! ensure {
    ($cond:expr) => {
        if let false = $cond {
            return Err($crate::err_msg(format!("{}", stringify!($cond))));
        }
    };
}

Some changes picked up by https://github.com/dtolnay/anyhow's test suite from this PR:

- Error::msg::<<str as ToOwned>::Owned>.t(1) == 2
+ Error ::msg ::<< str as ToOwned >::Owned >.t (1) == 2

- f::<1>() != ()
+ f ::<1 >() != ()

- f::<-1>() != ()
+ f ::<- 1 >() != ()

- E::U::<> > E::U::<u8>
+ E ::U ::<> > E ::U ::<u8 >

- E::U::<u8> > E::U
+ E ::U ::<u8 > > E ::U

- b\"hmm\"[1] == b'c'
+ b\"hmm\" [1] == b'c'

- result? == 2
+ result ? == 2

- f as for<'a> fn() as usize * 0 != 0
+ f as for<'a > fn() as usize * 0 != 0

- &0 as &dyn EqDebug<i32, Assoc = bool> != &0
+ &0 as &dyn EqDebug <i32, Assoc = bool > != &0

- PhantomData as PhantomData<<i32 as ToOwned>::Owned> != PhantomData
+ PhantomData as PhantomData << i32 as ToOwned >::Owned > != PhantomData

- 0 as int!(...) != 0
+ 0 as int !(...) != 0

- 0 as int![...] != 0
+ 0 as int ![...] != 0

- 0 as int! { ... } != 0
+ 0 as int ! { ... } != 0

- if let -1..=1 = 0 { 0 } else { 1 } == 1
+ if let -1 ..=1 = 0 { 0 } else { 1 } == 1

rust-timer · 2024-05-17T09:39:05Z

Finished benchmarking commit (e07dc21): comparison URL.

Overall result: ❌ regressions - 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)	0.3%	[0.2%, 0.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.3%]	3

Max RSS (memory usage)

Results (primary 0.2%, secondary -5.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.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	0.2%	[-0.2%, 0.6%]	2

Cycles

Results (primary -0.3%, secondary 0.2%)

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.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	2.4%	[2.1%, 2.6%]	3
Improvements ✅ (primary)	-1.5%	[-1.6%, -1.4%]	3
Improvements ✅ (secondary)	-1.5%	[-2.4%, -0.9%]	4
All ❌✅ (primary)	-0.3%	[-1.6%, 3.3%]	4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.726s -> 668.949s (-0.12%)
Artifact size: 316.16 MiB -> 315.99 MiB (-0.06%)

nnethercote · 2024-05-17T10:15:48Z

The AST pretty printer is always going to be more capable at stringifying expressions than the tokenstream pretty printer.

We can discuss what "more capable" means, but there are cases visible in the stringify.rs diff where I consider the tokenstream pretty printer to do a better job.

original	AST pp	tokenstream pp	difference
`f :: < u8>( )`	`f::<u8>()`	`f :: < u8>()`	AST pp loses more whitespace
`f(true,)`	`f(true)`	`f(true,)`	AST loses the `,`
`for<> fn()`	`fn()`	`for<> fn()`	AST loses the `for<>`
`pub macro stringify() {}`	`pub macro stringify { () => {} }`	`pub macro stringify() {}`	AST inserts `() => {}`

Some changes picked up by https://github.com/dtolnay/anyhow's test suite from this PR:

How do I run that suite? I tried cargo test test_ensure. I can't reproduce the failures, and the failures you listed are surprising because the tokenstream pretty printer usually produces much nicer output than that.

dtolnay · 2024-05-17T10:24:19Z

How do I run that suite? I tried cargo test test_ensure.

cargo test --test test_ensure. I built rustc from 9c693ad (the current state of this PR) and ran cargo +stage2 test --test test_ensure using dtolnay/anyhow@fecdbbb to expose more of the failures simultaneously.

nnethercote · 2024-05-17T10:43:57Z

#[macro_export]
macro_rules! ensure {
    ($cond:expr) => {
        if let false = $cond {
            return Err($crate::err_msg(format!("{}", stringify!($cond))));
        }
    };
}

AFAICT this is the cfg(doc) version of that macro. The cfg(not(doc)) version is much more complex. I don't have time to look at this today, I'll take a look next week.

dtolnay · 2024-05-17T16:34:12Z

Minimal repro:

macro_rules! repro {
    ($foo:ident $colons:tt $bar:ident) => {
        stringify_expr!($foo $colons $bar)
    };
}

macro_rules! stringify_expr {
    ($expr:expr) => {
        stringify!($expr)
    };
}

fn main() {
    println!("{}", repro!(foo::bar));
}

- foo::bar
+ foo ::bar

nnethercote · 2024-06-10T00:32:38Z

The crater results are good. There are only two genuine failures related to this PR, both in crates with macros that (inappropriately) depend on the output of TokenStream::to_string having a particular form. Neither of these crates are used elsewhere, and I think causing them to no longer compile is entirely acceptable.

nullptr-solutions.reflect

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/nullptr-solutions.reflect/log.txt

The following code relies on visibilities pretty-printing with a space after them, something the token stream pretty printer doesn't do.

https://github.com/nullptr-solutions/reflect/blob/d1be5347f659fd4a8d2c7c93e63c4fbd5b51d318/src/macros.rs#L6-L17

The crate is on GitHub, but not on crates.io. It has a single commit, made two years ago. There is no README. The repo has one star, two people watching it, and zero forks.

Jules-Bertholet.decimalfp-rs

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/Jules-Bertholet.decimalfp-rs/log.txt

The following code relies on negative literals printing without a space after the leading - sign, something that the token stream pretty printer doesn't do when metavar expansion is involved.

https://github.com/Jules-Bertholet/decimalfp-rs/blob/65a952738e35d36a75ed2ac7998e0234704727e5/decimalfp_macros_support/src/lib.rs#L97-L109

The crate is on GitHub, but not on crates.io. It has 10 commits, the most recent a year ago. There is a short README. The repo has zero stars, one person watching it, and zero forks.

C compilation failure?

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/TateKennington.ROpenGL/log.txt

This one appears to be a failure to build some C code.

Out of disk space

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/dyspxkrypt-os.dyspxkrypt-libuefi/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/rootware.lattice_evolution/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/ssanusi.guess_game/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/willmtemple.synth-poc/log.txt

All of these are the familiar "no space left on device". I don't know why these aren't considered spurious errors.

Linker crash?

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/shishitoh.almost-prime-rust/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/tshabatyn.rustlings/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/udsamani.offcengine/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/voxelvortex.RHelloWorld/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/wiomoc.swissbit-tse-rs/log.txt
https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/willemv.tinyrenderer-rust/log.txt

"note: collect2: fatal error: ld terminated with signal 7 [Bus error], core dumped"

Build script failure

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/tantona.rust-asteroids/log.txt

"failed to run custom build command for `raylib-sys v5.0.0" and "failed to copy raylib source"

Don't know what went wrong, but seems unrelated to this PR.

Mystery warning

https://crater-reports.s3.amazonaws.com/pr-125174-1/try%23a2f2c057581611ecb4c650c6c11630fca66cb4a4/gh/thomasebsmith.advent-of-code/log.txt

There is a single repeated warning. For some reason on the second build (with this PR's changes applied) the warning causes compilation to fail. I don't know why. I can't reproduce this failure locally. Whatever it is, it seems unrelated to this PR.

nnethercote · 2024-06-10T00:34:11Z

@petrochenkov: Based on the crater results, I think this is ready for final approval and review.

petrochenkov · 2024-06-11T14:56:17Z

@bors r+

bors · 2024-06-11T14:56:21Z

📌 Commit f7d49fd has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2024-06-11T20:11:24Z

⌛ Testing commit f7d49fd with merge d0227c6...

bors · 2024-06-11T22:19:27Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing d0227c6 to master...

rust-timer · 2024-06-12T00:43:11Z

Finished benchmarking commit (d0227c6): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	0.3%	[0.2%, 0.4%]	9
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.4%]	9

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary 2.1%)

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.994s -> 678.403s (0.21%)
Artifact size: 320.03 MiB -> 320.07 MiB (0.01%)

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour. The original "hack" implementing alternative behaviour was added in rust-lang#73345. The lint was added in rust-lang#83127. The tracking issue is rust-lang#83125. The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

…ghtly Rust The C++ build started failling with nightly rust: rust-lang/rust#125174 changed the output of strignify! to contins more spaces between tokens, which we relied on to perform some type substitution from Rust types to C++ types, resulting in compilation errors: ``` build/api/cpp/generated_include/slint_builtin_structs_internal.h:71:5: error: ‘Option’ does not name a type 71 | Option < core :: ops :: Range < i32 >> replacement_range; | ^~~~~~ build/api/cpp/generated_include/slint_builtin_structs_internal.h:75:14: error: ‘core’ was not declared in this scope 75 | Option < core :: ops :: Range < i32 >> preedit_selection; | ^~~~ ``` Workaround by cleaning whitespace before matching the types.

rylev · 2024-06-20T11:43:33Z

@nnethercote @petrochenkov I didn't see any discussion of the perf regressions seen here. It seems like most libc benchmarks were negatively impacted while no other benchmarks were. I'm not quite sure why that would be.

nnethercote · 2024-06-20T23:33:23Z

I'm a little surprised, I wouldn't have thought pretty-printing was on the performance critical path for anything. The new code could be slightly slower. The fact that it's a single benchmark, and small regressions, makes me not too worried. And the relevant code (nonterminal_to_string) is going to be eliminated as part of #124141 anyway.

@rustbot label: +perf-regression-triaged

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 May 16, 2024

nnethercote marked this pull request as draft May 16, 2024 10:38

nnethercote force-pushed the less-ast-pretty-printing branch from a397a90 to f7ce1ac Compare May 16, 2024 10:40

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 May 16, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2024

Auto merge of rust-lang#125174 - nnethercote:less-ast-pretty-printing…

53af78d

…, r=<try> Print `token::Interpolated` with token stream pretty printing. r? `@ghost`

nnethercote force-pushed the less-ast-pretty-printing branch 2 times, most recently from ca35330 to 9c693ad Compare May 17, 2024 06:41

nnethercote marked this pull request as ready for review May 17, 2024 06:42

rustbot assigned petrochenkov May 17, 2024

This comment has been minimized.

Sign in to view

nnethercote mentioned this pull request May 17, 2024

Remove Nonterminal and TokenKind::Interpolated #124141

Draft

Veykril reviewed May 17, 2024

View reviewed changes

compiler/rustc_parse/src/parser/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 17, 2024

dtolnay mentioned this pull request May 17, 2024

token::Interpolated dtolnay/anyhow#364

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 Jun 7, 2024

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2024

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2024

bors merged commit d0227c6 into rust-lang:master Jun 11, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jun 11, 2024

nnethercote deleted the less-ast-pretty-printing branch June 11, 2024 23:24

dtolnay mentioned this pull request Jun 13, 2024

Update test suite to nightly-2024-06-12 dtolnay/anyhow#378

Merged

ogoffart mentioned this pull request Jun 15, 2024

Fix C++ build breakage caused by changed in stringify! output in ni… slint-ui/slint#5412

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 20, 2024

nnethercote mentioned this pull request Jul 1, 2024

Should tokens passed through macro_rules be able to join? #127123

Open

Nemo157 mentioned this pull request Aug 11, 2024

regression: assertion left == right failed but it depends on library-unstable details #128899

Closed

workingjubilee mentioned this pull request Aug 11, 2024

Subtle change in how ty fragments get stringified #128992

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nnethercote commented May 16, 2024 •

edited

Loading

nnethercote commented May 16, 2024

This comment has been minimized.

bors commented May 16, 2024

nnethercote commented May 17, 2024

This comment has been minimized.

bors commented May 17, 2024

bors commented May 17, 2024

This comment has been minimized.

dtolnay commented May 17, 2024 •

edited

Loading

rust-timer commented May 17, 2024

nnethercote commented May 17, 2024

dtolnay commented May 17, 2024

nnethercote commented May 17, 2024

dtolnay commented May 17, 2024

nnethercote commented Jun 10, 2024

nnethercote commented Jun 10, 2024

petrochenkov commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

rust-timer commented Jun 12, 2024

rylev commented Jun 20, 2024

nnethercote commented Jun 20, 2024 •

edited by dtolnay

Loading

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

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

Conversation

nnethercote commented May 16, 2024 • edited Loading

nnethercote commented May 16, 2024

This comment has been minimized.

bors commented May 16, 2024

nnethercote commented May 17, 2024

This comment has been minimized.

bors commented May 17, 2024

bors commented May 17, 2024

This comment has been minimized.

dtolnay commented May 17, 2024 • edited Loading

rust-timer commented May 17, 2024

Overall result: ❌ regressions - ACTION NEEDED

nnethercote commented May 17, 2024

dtolnay commented May 17, 2024

nnethercote commented May 17, 2024

dtolnay commented May 17, 2024

nnethercote commented Jun 10, 2024

nullptr-solutions.reflect

Jules-Bertholet.decimalfp-rs

C compilation failure?

Out of disk space

Linker crash?

Build script failure

Mystery warning

nnethercote commented Jun 10, 2024

petrochenkov commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

rust-timer commented Jun 12, 2024

Overall result: ❌ regressions - ACTION NEEDED

rylev commented Jun 20, 2024

nnethercote commented Jun 20, 2024 • edited by dtolnay Loading

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

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

nnethercote commented May 16, 2024 •

edited

Loading

dtolnay commented May 17, 2024 •

edited

Loading

nnethercote commented Jun 20, 2024 •

edited by dtolnay

Loading