Print token::Interpolated with token stream pretty printing.

[nnethercote]

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]

@bors try @rust-timer queue

[bors]

⌛ Trying commit f7ce1ac with merge 53af78d...

[nnethercote]

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

[bors]

⌛ Trying commit 9c693ad with merge e07dc21...

[bors]

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

[dtolnay]

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]

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]

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]

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]

#[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]

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]

@craterbot check

[craterbot]

👌 Experiment pr-125174 created and queued.
🤖 Automatically detected try build e07dc21
⚠️ Try build based on commit 9c693ad, but latest commit is 33d6e6f. Did you forget to make a new try build?
🔍 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

[craterbot]

🚧 Experiment pr-125174 is now running

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

[craterbot]

🎉 Experiment pr-125174 is completed!
📊 192 regressed and 4 fixed (464004 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

[bors]

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

[dtolnay]

michaelzoech/vkinfo: this one needs to be fixed.

error: proc-macro derive panicked
  --> src/vk.rs:62:14
   |
62 |     #[derive(Serialize)]
   |              ^^^^^^^^^
   |
   = help: message: called `Result::unwrap()` on an `Err` value: "failed to parse derive input: \"pubstruct InstanceCreateFlags { bits: u32, }\""

I skimmed a few of the rest and they all seemed like they might be the same issue.

[dtolnay]

Here is the issue: https://github.com/bitflags/bitflags/blob/1.0.1/src/lib.rs#L312

This is something I observed in anyhow also, with the new printer.
https://github.com/dtolnay/anyhow/blob/e6c730e8ec334eb3d7c6513940fd2dd6516ef7b6/src/ensure.rs#L750
I had to inserted spaces before ) and } to get the output I wanted, where I don't think I should have to.

IMO a punct immediately preceding a close delimiter with no space should be treated as Alone anyway, and the printer should never print space inside of (​) or [​], and always print space inside of {​}.

[petrochenkov]

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.

I don't think removing nonterminals is strictly required for addressing #67062.
It just greatly helps with testing by flooding the parser with Invisible-delimited groups, so if something goes wrong we notice it very soon.

I do feel that characterizing nonterminals as "legacy" or "slated for removal" in comments is premature.
They are not just used to implement some functionality that is (hopefully) can be implemented using invisible delimited groups, they are also an optimization allowing to avoid reparsing same pieces of code again and again when passing them through multiple nested macro calls (including recursive cases).
I'm not sure we'll be able to land the removal without introducing some kind of "TokenStream -> AST" cache, even if functionally invisible delimited groups work without too much breakage.

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

I'd expect large pieces of code like items (impls with nested functions, with large bodies with blocks) to be botched by token pretty-printing to a larger degree than expressions.
I'd actually expect expressions to do pretty well (unless they contain blocks with statements), if the decl macro issues are addressed.
Maybe if enum Spacing keeps some line break information it will help printing larger pieces of code better (but I'm not sure it it's worth doing).

For decl macros we have to be careful, because whitespace after tokens in rules doesn't necessarily match what you want in the final output.

From what I remember in decl macro output we just give up and print spaces (possibly prettified by space_between)?
We probably should do something better there given that the decl macro case causes majority of the problems for dtolnay.

[nnethercote]

I've looked at the crater results. There are two things going on.

Spacing

When I first created this PR on May 16 I had a commit "Add Spacing to mbe::TokenTree::Token". This was buggy, and could cause some pairs of tokens to be concatenated, like consecutive identifiers, leading to invalid output such as pubstruct. @dtolnay, this was the cause of some of the extra spaces you had to insert into ensure!. Once I realized this I removed the buggy commit, and made some other changes, and force-pushed on May 20. Only after doing that did I trigger the crater run.

But somehow that buggy commit ended up in the crater run: 3b977bb. The buggy commit doesn't show up in the PR's commits list. But that explains a lot of the failures such as nullptr-solutions.reflect, michaelzoech.vkinfo, and AlcaYezz.pong_rust. I was able to reproduce these failures locally with the buggy commit applied, and then they compiled successfully with the buggy commit removed.

So I think this cause of failures is solved, though I don't understand why the crater run included the buggy commit.

rental

rustc has a "pretty_printing_compatibility_hack" that causes slightly different token-handling behaviour when compiling versions of the rental crate prior to 0.5.6:

rust/compiler/rustc_expand/src/proc_macro.rs

Lines 123 to 133 in 78dd504

	let hack = crate::base::ann_pretty_printing_compatibility_hack(&item, &ecx.sess);
	let input = if hack {
	let nt = match item {
	Annotatable::Item(item) => token::NtItem(item),
	Annotatable::Stmt(stmt) => token::NtStmt(stmt),
	_ => unreachable!(),
	};
	TokenStream::token_alone(token::Interpolated(Lrc::new(nt)), DUMMY_SP)
	} else {
	item.to_tokens()
	};

The hack forces a matching item or statement to be put into an Interpolated token so that it gets printed with the AST pretty-printer, as opposed to the normal behaviour of converting it to a token stream that is then printed with the token stream pretty printer. This is done because the AST pretty-printer inserts an extra comma that rental relies on.

So this PR perfectly undoes the hack, because Interpolated tokens are now printed with the tokenstream pretty printer!
This accounts for all(?) of the other failures. Indeed, there was a previous attempt to remove the hack in #93275 and the crater run in that PR has a lot of failure overlap with the crater run in this PR.

So I think the best way to fix these failures is to finally remove the hack. It has been four years since it was added. I think that's better than trying to adapt the hack to work with the tokenstream pretty printer.

@petrochenkov, what do you think?

[dtolnay]

+1 from me for removing that hack targeting 4-year-old rental versions.

I don't understand why the crater run included the buggy commit.

Crater doesn't compile rustc. It uses an already-compiled rustc that got built by bors in the most recent "bors try".

In this PR there was a "bors try" in #125174 (comment). According to the bors response in #125174 (comment), the head of the PR at the time was commit 9c693ad. There was a force push from that commit to 33d6e6f right below #125174 (comment), followed by "craterbot check" in #125174 (comment). Craterbot uses the most recent try build, and says so in #125174 (comment): "🤖 Automatically detected try build e07dc21 ⚠️ Try build based on commit 9c693ad, but latest commit is 33d6e6f. Did you forget to make a new try build?"

[nnethercote]

Aha. Old try build, makes sense.

About rental: I have commented in #106060. The discussion there indicated people are being extremely cautious about even scaling back the hack, let alone removing it. Hmm. I've made a short argument, other more eloquent arguments in favour of removal would be appreciated if people have them...

[petrochenkov]

I'm also fine with removing the hack.

[nnethercote]

#125596 converts the hack to a hard error, which is enough to unblock this PR.