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

Do not remove tokens before AST json serialization #78610

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

petrochenkov
Copy link
Contributor

TokenStripper is error-prone and introduces one more use of MutVisitor.
It's much simpler to treat serialization as just one more place that wants lazy token stream to turn into a real token stream.
Also, no code is better than more code, in general.
r? @Aaron1011

(I also merged tests for TokenStripper ICEs into one.)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2020
@Aaron1011
Copy link
Member

I wanted to avoid making this serializable, to guarantee that we properly 'lower' a TokenStream when serializing a macro_rules! definition. However, I agree that the use of MutVisitor isn't ideal.

@petrochenkov
Copy link
Contributor Author

It doesn't matter much when exactly we force the real token stream creation?
We could delay it until metadata encoding (serializing a macro_rules! definition) and that wouldn't be a problem, I think.

Lazy token streams just never survive until that time because all of them are in nonterminals where you have to do other things forcing token creation, like print-reparse checking, anyway.

@Aaron1011
Copy link
Member

The LazyTokenStream can be coverted into a TokenStream multiple times, though. Outside of AST JSON serializition, we should never be attempting to serialize a LazyTokenStream - with this PR, we'll get a (small) performance hit instead of an ICE.

I don't think there's a particularly clean way to only panic if we're encoding the LazyTokenStream for something other than AST JSON. If you can think of one, I think it would be a nice sanity check. However, I don't feel very strongly about this, so r=me otherwise.

@petrochenkov
Copy link
Contributor Author

I think removing the sanity check is a lesser evil than keeping some global variables to check whether ast-json is enabled, or avoiding it with TokenStripper.

Besides, #77271 removes some visitor APIs related to tokens including those used by TokenStripper.

@bors r=Aaron1011 rollup

@bors
Copy link
Contributor

bors commented Nov 1, 2020

📌 Commit 6b63e9b has been approved by Aaron1011

@bors 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 Nov 1, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 1, 2020
Do not remove tokens before AST json serialization

`TokenStripper` is error-prone and introduces one more use of `MutVisitor`.
It's much simpler to treat serialization as just one more place that wants lazy token stream to turn into a real token stream.
Also, no code is better than more code, in general.
r? @Aaron1011

(I also merged tests for `TokenStripper` ICEs into one.)
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#78606 (Clarify handling of final line ending in str::lines())
 - rust-lang#78610 (Do not remove tokens before AST json serialization)
 - rust-lang#78620 (Trivial fixes to bitwise operator documentation)
 - rust-lang#78627 (Point out that total_cmp is no strict superset of partial comparison)
 - rust-lang#78637 (Add fetch_update methods to AtomicBool and AtomicPtr)

Failed merges:

r? `@ghost`
@bors bors merged commit 61305d5 into rust-lang:master Nov 2, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 2, 2020
@petrochenkov petrochenkov deleted the nostriptok branch February 22, 2025 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants