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

Recursively expand TokenKind::Interpolated in probably_equal_for_proc_macro #72388

Merged
merged 4 commits into from
May 24, 2020

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 20, 2020

Fixes #68430

When comparing the captured and re-parsed TokenStream for a TokenKind::Interpolated, we currently treat any nested TokenKind::Interpolated tokens as unequal. If a TokenKind::Interpolated token shows up in the captured TokenStream due to a macro_rules! expansion, we will throw away the captured TokenStream, losing span information.

This PR recursively invokes nt_to_tokenstream on nested TokenKind::Interpolated tokens, effectively flattening the stream into a sequence of non-interpolated tokens. This allows it to compare equal with the re-parsed stream, allowing us to keep the original captured TokenStream (with span information).

This requires all of the probably_equal_for_proc_macro methods to be moved from librustc_ast to librustc_parse so that they can call nt_to_tokenstream.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2020
@petrochenkov petrochenkov self-assigned this May 20, 2020
@davidtwco davidtwco removed their assignment May 21, 2020
@petrochenkov
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 21, 2020

⌛ Trying commit e3d3b4d2f56d8f6c3aaba1fc10d79d74a1bf6296 with merge 7a63e056b52f40554fc1c032eac566fa6b26d9ce...

@petrochenkov
Copy link
Contributor

LGTM, but need to check perf.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2020
@bors
Copy link
Contributor

bors commented May 21, 2020

☀️ Try build successful - checks-azure
Build commit: 7a63e056b52f40554fc1c032eac566fa6b26d9ce (7a63e056b52f40554fc1c032eac566fa6b26d9ce)

@rust-timer
Copy link
Collaborator

Queued 7a63e056b52f40554fc1c032eac566fa6b26d9ce with parent 06c9fef, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 7a63e056b52f40554fc1c032eac566fa6b26d9ce, comparison URL.

@Aaron1011
Copy link
Member Author

@petrochenkov: It looks like perf is unchanged

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2020

📌 Commit e3d3b4d2f56d8f6c3aaba1fc10d79d74a1bf6296 has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 22, 2020
@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 22, 2020
@bors

This comment has been minimized.

@bors 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 May 22, 2020
@Aaron1011 Aaron1011 force-pushed the fix/deep-tokenstream-equality branch from e3d3b4d to 4d260dd Compare May 22, 2020 19:05
@Aaron1011 Aaron1011 force-pushed the fix/deep-tokenstream-equality branch from 4d260dd to 5685e4d Compare May 22, 2020 19:13
rrbutani added a commit to rrbutani/wasm-bindgen that referenced this pull request May 26, 2020
rrbutani added a commit to rrbutani/wasm-bindgen that referenced this pull request May 26, 2020
alexcrichton pushed a commit to rustwasm/wasm-bindgen that referenced this pull request May 26, 2020
* Handle the possibility that the class name is in its own Group

rust-lang/rust#72388 makes this happen

* Handle Groups in more places

* fmt!

* Add some functions to handle Groups

As suggested by @alexcrichton [here](https://gist.github.com/alexcrichton/3c93ab2547d45d9caa3b72309cd4262b).
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 30, 2020
…, r=petrochenkov

Revert recursive `TokenKind::Interpolated` expansion for now

The crater run rust-lang#72622 revealed many root regressions, at least one of which is going to take some time to fix.

For now, let's revert rust-lang#72388 to allow the 709 affected crates to continue building on the latest nightly.
Aaron1011 added a commit to Aaron1011/syn that referenced this pull request May 31, 2020
When rust-lang/rust#72388 re-lands, we may accumulate several 'layers'
of `None`-delimited groups. This commit ensures that we 'unwrap' all of
the layers, allowing consumers to avoid needing to handle these cases.
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Aug 22, 2020
Fixes rust-lang#68430

This is a re-attempt of PR rust-lang#72388, which was previously reverted due to
a large number of breakages. All of the known breakages should now be
patched upstream.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2020
…d, r=petrochenkov

Re-land PR rust-lang#72388:  Recursively expand `TokenKind::Interpolated` in `probably_equal_for_proc_macro`

PR rust-lang#72388 allowed us to preserve the original `TokenStream` in more cases during proc-macro expansion, but had to be reverted due to a large number of regressions (See rust-lang#72545 and rust-lang#72622). These regressions fell into two categories

1. Missing handling for `Group`s with `Delimiter::None`, which are inserted during `macro_rules!` expansion (but are lost during stringification and re-parsing). A large number of these regressions were due to `syn` and `proc-macro-hack`, but several crates needed changes to their own proc-macro code.
2. Legitimate hygiene issues that were previously being masked by stringification. Some of these were relatively benign (e.g. [a compiliation error](paritytech/parity-scale-codec#210) caused by misusing `quote_spanned!`). However, two crates had intentionally written unhygenic `macro_rules!` macros, which were able to access identifiers that were not passed as arguments (see rust-lang#72622 (comment)).

All but one of the Crater regressions have now been fixed upstream (see https://hackmd.io/ItrXWRaSSquVwoJATPx3PQ?both). The remaining crate (which has a PR pending at sammhicks/face-generator#1) is not on `crates.io`, and is a Yew application that seems unlikely to have any reverse dependencies.

As @petrochenkov mentioned in rust-lang#72545 (comment), not re-landing PR rust-lang#72388 allows more crates to write unhygenic `macro_rules!` macros, which will eventually stop compiling. Since there is only one Crater regression remaining, since additional crates could write unhygenic `macro_rules!` macros in the time it takes that PR to be merged.
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.

Rustc doesn't report line numbers for type errors within proc macro async fn within macro_rules macro
6 participants