Remove `TreeAndSpacing`. #99887

nnethercote · 2022-07-29T05:09:22Z

A TokenStream contains a Lrc<Vec<(TokenTree, Spacing)>>. But this is
not quite right. Spacing makes sense for TokenTree::Token, but does
not make sense for TokenTree::Delimited, because a
TokenTree::Delimited cannot be joined with another TokenTree.

This commit fixes this problem, by adding Spacing to TokenTree::Token,
changing TokenStream to contain a Lrc<Vec<TokenTree>>, and removing the
TreeAndSpacing typedef.

The commit removes these two impls:

impl From<TokenTree> for TokenStream
impl From<TokenTree> for TreeAndSpacing

These were useful, but also resulted in code with many .into() calls
that was hard to read, particularly for anyone not highly familiar with
the relevant types. This commit makes some other changes to compensate:

TokenTree::token() becomes TokenTree::token_{alone,joint}().
TokenStream::token_{alone,joint}() are added.
TokenStream::delimited is added.

This results in things like this:

TokenTree::token(token::Semi, stmt.span).into()

changing to this:

TokenStream::token_alone(token::Semi, stmt.span)

This makes the type of the result, and its spacing, clearer.

These changes also simplifies Cursor and CursorRef, because they no longer
need to distinguish between next and next_with_spacing.

r? @petrochenkov

rustbot · 2022-07-29T05:09:25Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

A `TokenStream` contains a `Lrc<Vec<(TokenTree, Spacing)>>`. But this is not quite right. `Spacing` makes sense for `TokenTree::Token`, but does not make sense for `TokenTree::Delimited`, because a `TokenTree::Delimited` cannot be joined with another `TokenTree`. This commit fixes this problem, by adding `Spacing` to `TokenTree::Token`, changing `TokenStream` to contain a `Lrc<Vec<TokenTree>>`, and removing the `TreeAndSpacing` typedef. The commit removes these two impls: - `impl From<TokenTree> for TokenStream` - `impl From<TokenTree> for TreeAndSpacing` These were useful, but also resulted in code with many `.into()` calls that was hard to read, particularly for anyone not highly familiar with the relevant types. This commit makes some other changes to compensate: - `TokenTree::token()` becomes `TokenTree::token_{alone,joint}()`. - `TokenStream::token_{alone,joint}()` are added. - `TokenStream::delimited` is added. This results in things like this: ```rust TokenTree::token(token::Semi, stmt.span).into() ``` changing to this: ```rust TokenStream::token_alone(token::Semi, stmt.span) ``` This makes the type of the result, and its spacing, clearer. These changes also simplifies `Cursor` and `CursorRef`, because they no longer need to distinguish between `next` and `next_with_spacing`.

petrochenkov · 2022-07-29T14:55:35Z

I think this is a step to the right direction.

I wanted to make one more step though and add the jointness flag right to rustc_ast::token::Token.
Then it would be always produced from lexer and preserved later, and "censored" (erased for non-punctuation tokens) only at proc macro API (#97340 (comment), #76285). Then it would also be usable, for example, for better token pretty-printing.

petrochenkov · 2022-07-29T14:55:46Z

@bors r+

bors · 2022-07-29T14:55:48Z

📌 Commit 332dffb has been approved by petrochenkov

It is now in the queue for this repository.

nnethercote · 2022-07-29T19:25:35Z

Thank you for the fast review!

I wanted to make one more step though and add the jointness flag right to rustc_ast::token::Token.

I had thought about this, but I decided to see if moving it into TokenTree::Token was acceptable first :)

The jointness wouldn't need to be on every token, just the punctuation tokens, right? Would it be every punctuation token? Ones like < and = and . obviously would need it, but what about ones like , and ; that aren't prefixes of other operators? I'm not sure because (a) a proc macro can easily produce a Comma token that is marked as joint, but (b) the compiler won't actually do any joining with it. Perhaps a joint Comma produced by one proc macro could then become input to another proc macro that might treat it differently to a non-joint Comma?

nnethercote · 2022-07-29T19:27:22Z

I don't expect this to affect performance, but just to be sure:

@bors try @rust-timer queue

bors · 2022-07-29T19:27:23Z

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

rust-timer · 2022-07-29T19:27:23Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

nnethercote · 2022-07-29T19:27:38Z

@bors r-

nnethercote · 2022-07-29T19:28:10Z

@bors try

bors · 2022-07-29T19:28:18Z

⌛ Trying commit 332dffb with merge 765888ed01bd9f231a2c38d345efe4e65a2621d0...

bors · 2022-07-29T21:22:44Z

☀️ Try build successful - checks-actions
Build commit: 765888ed01bd9f231a2c38d345efe4e65a2621d0 (765888ed01bd9f231a2c38d345efe4e65a2621d0)

rust-timer · 2022-07-29T21:22:46Z

Queued 765888ed01bd9f231a2c38d345efe4e65a2621d0 with parent 5dda74a, future comparison URL.

rust-timer · 2022-07-30T00:59:03Z

Finished benchmarking commit (765888ed01bd9f231a2c38d345efe4e65a2621d0): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	0.4%	0.4%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.3%	-0.4%	6
Improvements 🎉 (secondary)	-0.5%	-1.1%	19
All 😿🎉 (primary)	-0.2%	0.4%	7

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.7%	2.7%	12
Regressions 😿 (secondary)	3.6%	4.7%	8
Improvements 🎉 (primary)	-1.4%	-4.1%	8
Improvements 🎉 (secondary)	-2.4%	-3.9%	12
All 😿🎉 (primary)	0.5%	-4.1%	20

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-4.5%	-4.5%	1
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-07-30T01:16:05Z

There's a single regressing result and 25 improved results. The changes are all very small. This is fine.

@rustbot label: +perf-regression-triaged

nnethercote · 2022-07-30T01:16:23Z

@bors r=petrochenkov

bors · 2022-07-30T01:16:26Z

📌 Commit 332dffb has been approved by petrochenkov

It is now in the queue for this repository.

petrochenkov · 2022-07-30T10:02:08Z

The jointness wouldn't need to be on every token, just the punctuation tokens, right?

That's a detail of proc macro API, internally in the compiler I'd like to just preserve this information for all tokens.

bors · 2022-07-30T14:50:09Z

⌛ Testing commit 332dffb with merge 1202bba...

bors · 2022-07-30T17:30:44Z

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

rust-timer · 2022-07-30T21:19:01Z

Finished benchmarking commit (1202bba): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	0.3%	0.3%	1
Regressions 😿 (secondary)	0.2%	0.2%	1
Improvements 🎉 (primary)	-0.3%	-0.4%	5
Improvements 🎉 (secondary)	-0.4%	-0.9%	16
All 😿🎉 (primary)	-0.2%	-0.4%	6

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.4%	2.6%	4
Regressions 😿 (secondary)	3.2%	4.9%	12
Improvements 🎉 (primary)	-1.1%	-1.4%	6
Improvements 🎉 (secondary)	-3.1%	-6.3%	11
All 😿🎉 (primary)	-0.1%	2.6%	10

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	2.6%	2.6%	1
Regressions 😿 (secondary)	10.3%	10.3%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.6%	2.6%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-07-30T23:30:34Z

Much like above: there are 2 regressing results and 21 improved results. The changes are all very small. This is fine.

@rustbot label: +perf-regression-triaged

…ochenkov Remove `TreeAndSpacing`. A `TokenStream` contains a `Lrc<Vec<(TokenTree, Spacing)>>`. But this is not quite right. `Spacing` makes sense for `TokenTree::Token`, but does not make sense for `TokenTree::Delimited`, because a `TokenTree::Delimited` cannot be joined with another `TokenTree`. This commit fixes this problem, by adding `Spacing` to `TokenTree::Token`, changing `TokenStream` to contain a `Lrc<Vec<TokenTree>>`, and removing the `TreeAndSpacing` typedef. The commit removes these two impls: - `impl From<TokenTree> for TokenStream` - `impl From<TokenTree> for TreeAndSpacing` These were useful, but also resulted in code with many `.into()` calls that was hard to read, particularly for anyone not highly familiar with the relevant types. This commit makes some other changes to compensate: - `TokenTree::token()` becomes `TokenTree::token_{alone,joint}()`. - `TokenStream::token_{alone,joint}()` are added. - `TokenStream::delimited` is added. This results in things like this: ```rust TokenTree::token(token::Semi, stmt.span).into() ``` changing to this: ```rust TokenStream::token_alone(token::Semi, stmt.span) ``` This makes the type of the result, and its spacing, clearer. These changes also simplifies `Cursor` and `CursorRef`, because they no longer need to distinguish between `next` and `next_with_spacing`. r? `@petrochenkov`

And into `AttrAnnotatedTokenTree::Token`. PR rust-lang#99887 did the same thing for `TokenStream`.

rust-highfive assigned petrochenkov Jul 29, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2022

This comment has been minimized.

Sign in to view

nnethercote force-pushed the rm-TreeAndSpacing branch from 1810421 to 332dffb Compare July 29, 2022 05:52

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 Jul 29, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 29, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2022

rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 30, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 30, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2022

bors merged commit 1202bba into rust-lang:master Jul 30, 2022

rustbot added this to the 1.64.0 milestone Jul 30, 2022

bors mentioned this pull request Jul 30, 2022

[do not merge] Tweak TokenStream's PartialEq and HashStable impls #97182

Closed

nnethercote deleted the rm-TreeAndSpacing branch July 30, 2022 20:16

nnethercote added a commit to nnethercote/rust that referenced this pull request Sep 9, 2022

Move Spacing out of AttrAnnotatedTokenStream.

890e759

And into `AttrAnnotatedTokenTree::Token`. PR rust-lang#99887 did the same thing for `TokenStream`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `TreeAndSpacing`. #99887

Remove `TreeAndSpacing`. #99887

nnethercote commented Jul 29, 2022

rustbot commented Jul 29, 2022

This comment has been minimized.

petrochenkov commented Jul 29, 2022

petrochenkov commented Jul 29, 2022

bors commented Jul 29, 2022

nnethercote commented Jul 29, 2022

nnethercote commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

nnethercote commented Jul 29, 2022

nnethercote commented Jul 29, 2022

bors commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

rust-timer commented Jul 30, 2022

nnethercote commented Jul 30, 2022

nnethercote commented Jul 30, 2022

bors commented Jul 30, 2022

petrochenkov commented Jul 30, 2022

bors commented Jul 30, 2022

bors commented Jul 30, 2022

rust-timer commented Jul 30, 2022

nnethercote commented Jul 30, 2022

Remove TreeAndSpacing. #99887

Remove TreeAndSpacing. #99887

Conversation

nnethercote commented Jul 29, 2022

rustbot commented Jul 29, 2022

This comment has been minimized.

petrochenkov commented Jul 29, 2022

petrochenkov commented Jul 29, 2022

bors commented Jul 29, 2022

nnethercote commented Jul 29, 2022

nnethercote commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

nnethercote commented Jul 29, 2022

nnethercote commented Jul 29, 2022

bors commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

rust-timer commented Jul 30, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

nnethercote commented Jul 30, 2022

nnethercote commented Jul 30, 2022

bors commented Jul 30, 2022

petrochenkov commented Jul 30, 2022

bors commented Jul 30, 2022

bors commented Jul 30, 2022

rust-timer commented Jul 30, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

nnethercote commented Jul 30, 2022

Remove `TreeAndSpacing`. #99887

Remove `TreeAndSpacing`. #99887