Replace pretty-print/compare/retokenize hack with targeted workarounds#79472

Aaron1011 · 2020-11-27T17:34:27Z

The 'pretty-print/compare/retokenize' hack is used to try to avoid passing an outdated TokenStream to a proc-macro when the underlying AST is modified in some way (e.g. cfg-stripping before derives). Unfortunately, retokenizing throws away spans (including hygiene information), which causes issues of its own. Every improvement to the accuracy of the pretty-print/retokenize comparison has resulted in non-trivial ecosystem breakage due to hygiene changes. In extreme cases, users deliberately wrote unhygienic macro_rules! macros (likely because they did not realize that the compiler's behavior was a bug).

Additionaly, the comparison between the original and pretty-printed/retoknized token streams comes at a non-trivial runtime cost, as shown by #79338

This PR removes the pretty-print/compare/retokenize logic from nt_to_tokenstream. We only discard the original TokenStream under two circumstances:

Inner attributes are used (detected by examining the AST)
cfg/cfg_attr processing modifies the AST. This is detected by making the visitor update a flag when it performs a modification, instead of trying to detect the modification after-the-fact. Note that a 'matching' cfg (e.g. #[cfg(not(FALSE)]) does not actually get removed from the AST, allowing us to preserve the original TokenStream.

In all other cases, we preserve the original TokenStream.

This could use a bit of refactoring/renaming - opening for a Crater run.

r? @ghost

petrochenkov · 2020-11-27T17:52:35Z

Note that a 'matching' cfg (e.g. #[cfg(not(FALSE)]) does not actually get removed from the AST

This is a bug though - #79341 (comment).
(We cannot turn cfg/cfg_attr into regular attribute macros like derive, mostly due to restrictions on expanded code, but we can make them behave very closely to them. That includes removing itself during expansion, which is also an optimization preventing cfg predicates from being evaluated multiple times.)

Aaron1011 · 2020-11-27T17:55:15Z

This is a bug though - #79341 (comment).

I see. When we start removing 'active' `#[cfg]' attributes, it should be very straightforward to change this code to unconditionally set the 'modified' flag.

Aaron1011 · 2020-11-27T18:02:53Z

@bors try

bors · 2020-11-27T18:03:04Z

⌛ Trying commit be337a0c8fe6a88dc0eb3877a4b4afdc4c32600b with merge 9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c...

bors · 2020-11-27T18:45:34Z

☀️ Try build successful - checks-actions
Build commit: 9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c (9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c)

Aaron1011 · 2020-11-27T18:51:47Z

@craterbot check

craterbot · 2020-11-27T18:51:57Z

👌 Experiment pr-79472 created and queued.
🤖 Automatically detected try build 9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c
⚠️ Try build based on commit c922857, but latest commit is be337a0c8fe6a88dc0eb3877a4b4afdc4c32600b. 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

Aaron1011 · 2020-11-28T21:18:23Z

@rust-timer queue

rust-timer · 2020-11-28T21:18:24Z

Awaiting bors try build completion

Aaron1011 · 2020-11-28T21:18:45Z

@rust-timer queue 9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c

rust-timer · 2020-11-28T21:18:46Z

Awaiting bors try build completion

Mark-Simulacrum · 2020-11-28T21:20:27Z

@rust-timer build 9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c

rust-timer · 2020-11-28T21:20:29Z

Queued 9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c with parent c922857, future comparison URL.

rust-timer · 2020-11-28T22:57:12Z

Finished benchmarking try commit (9b49d8c6c904f63eb486fe51b14bbe0aa2feda4c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

craterbot · 2020-12-08T10:28:48Z

🚧 Experiment pr-79472 is now running

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

craterbot · 2020-12-11T01:25:15Z

🚨 Experiment pr-79472 has encountered an error: some threads returned an error
🛠️ If the error is fixed use the retry command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

emilyalbini · 2020-12-11T12:31:14Z

@craterbot retry

craterbot · 2020-12-11T12:31:17Z

🛠️ Experiment pr-79472 queued again.

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

petrochenkov · 2020-12-11T12:53:20Z

r? @petrochenkov

craterbot · 2020-12-11T13:04:26Z

🚧 Experiment pr-79472 is now running

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

craterbot · 2020-12-12T06:09:11Z

🎉 Experiment pr-79472 is completed!
📊 21 regressed and 26 fixed (132150 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

petrochenkov · 2020-12-12T18:53:03Z

Waiting on author to triage the regressions.

Aaron1011 · 2020-12-29T14:37:23Z

@bors r=petrochenkov

bors · 2020-12-29T14:37:24Z

📌 Commit 530a629 has been approved by petrochenkov

bors · 2020-12-30T02:22:23Z

⌛ Testing commit 530a629 with merge 044dcb533fd8268957c1be93a77f8b1d58dc1924...

bors · 2020-12-30T06:37:24Z

💥 Test timed out

Aaron1011 · 2020-12-30T07:04:44Z

@bors retry

bors · 2020-12-30T07:05:00Z

⌛ Testing commit 530a629 with merge b9c403b...

bors · 2020-12-30T09:51:35Z

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

rustc_parse: Better spans for synthesized token streams I think using the nonterminal span for synthesizing its tokens is a better approximation than using `DUMMY_SP` or the attribute span like rust-lang#79472 did in `expand.rs`. r? `@Aaron1011`

It's no longer necessary after rust-lang#79472

rustc_ast_pretty: Remove `PrintState::insert_extra_parens` It's no longer necessary after rust-lang#79472. r? `@Aaron1011`

Pkgsrc changes: * Adjust patches, re-compute line offsets, fix capitalization. * Remove i686/FreeBSD support, no longer provided upstream. * Bump bootstraps to 1.49.0. * Change USE_TOOLS from bsdtar to gtar. * Reduce diffs to pkgsrc-wip package patches. * Allow rust.BUILD_TARGET to override automatic choice of target. * Add an i586/NetBSD (pentium) bootstrap variant (needs testing), not yet added as bootstrap since 1.49 doesn't have that variant. Upstream changes: Version 1.50.0 (2021-02-11) ============================ Language ----------------------- - [You can now use `const` values for `x` in `[x; N]` array expressions.][79270] This has been technically possible since 1.38.0, as it was unintentionally stabilized. - [Assignments to `ManuallyDrop<T>` union fields are now considered safe.][78068] Compiler ----------------------- - [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi` target.][78142] - [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484] - [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries ----------------------- - [`proc_macro::Punct` now implements `PartialEq<char>`.][78636] - [`ops::{Index, IndexMut}` are now implemented for fixed sized arrays of any length.][74989] - [On Unix platforms, the `std::fs::File` type now has a "niche" of `-1`.][74699] This value cannot be a valid file descriptor, and now means `Option<File>` takes up the same amount of space as `File`. Stabilized APIs --------------- - [`bool::then`] - [`btree_map::Entry::or_insert_with_key`] - [`f32::clamp`] - [`f64::clamp`] - [`hash_map::Entry::or_insert_with_key`] - [`Ord::clamp`] - [`RefCell::take`] - [`slice::fill`] - [`UnsafeCell::get_mut`] The following previously stable methods are now `const`. - [`IpAddr::is_ipv4`] - [`IpAddr::is_ipv6`] - [`Layout::size`] - [`Layout::align`] - [`Layout::from_size_align`] - `pow` for all integer types. - `checked_pow` for all integer types. - `saturating_pow` for all integer types. - `wrapping_pow` for all integer types. - `next_power_of_two` for all unsigned integer types. - `checked_power_of_two` for all unsigned integer types. Cargo ----------------------- - [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976] This option sets a wrapper to execute instead of `rustc`, for workspace members only. - [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire contents of that directory for changes.][cargo/8973] - [Added the `--workspace` flag to the `cargo update` command.][cargo/8725] Misc ---- - [The search results tab and the help button are focusable with keyboard in rustdoc.][79896] - [Running tests will now print the total time taken to execute.][75752] Compatibility Notes ------------------- - [The `compare_and_swap` method on atomics has been deprecated.][79261] It's recommended to use the `compare_exchange` and `compare_exchange_weak` methods instead. - [Changes in how `TokenStream`s are checked have fixed some cases where you could write unhygenic `macro_rules!` macros.][79472] - [`#![test]` as an inner attribute is now considered unstable like other inner macro attributes, and reports an error by default through the `soft_unstable` lint.][79003] - [Overriding a `forbid` lint at the same level that it was set is now a hard error.][78864] - [Dropped support for all cloudabi targets.][78439] - [You can no longer intercept `panic!` calls by supplying your own macro.][78343] It's recommended to use the `#[panic_handler]` attribute to provide your own implementation. - [Semi-colons after item statements (e.g. `struct Foo {};`) now produce a warning.][78296] [74989]: rust-lang/rust#74989 [79261]: rust-lang/rust#79261 [79896]: rust-lang/rust#79896 [79484]: rust-lang/rust#79484 [79472]: rust-lang/rust#79472 [79270]: rust-lang/rust#79270 [79003]: rust-lang/rust#79003 [78864]: rust-lang/rust#78864 [78636]: rust-lang/rust#78636 [78439]: rust-lang/rust#78439 [78343]: rust-lang/rust#78343 [78296]: rust-lang/rust#78296 [78068]: rust-lang/rust#78068 [75752]: rust-lang/rust#75752 [74699]: rust-lang/rust#74699 [78142]: rust-lang/rust#78142 [77484]: rust-lang/rust#77484 [cargo/8976]: rust-lang/cargo#8976 [cargo/8973]: rust-lang/cargo#8973 [cargo/8725]: rust-lang/cargo#8725 [`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4 [`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6 [`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align [`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align [`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size [`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp [`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take [`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut [`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then [`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key [`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp [`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp [`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key [`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill

Aaron1011 force-pushed the new-remove-pretty-print-hack branch from 2adb43b to be337a0 Compare November 27, 2020 18:02

craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 27, 2020

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2020

craterbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2020

rust-highfive assigned petrochenkov Dec 11, 2020

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 Dec 12, 2020

Aaron1011 force-pushed the new-remove-pretty-print-hack branch from 517aa4a to 530a629 Compare December 29, 2020 14:37

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 29, 2020

Aaron1011 added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 29, 2020

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

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 Dec 30, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2020

bors merged commit b9c403b into rust-lang:master Dec 30, 2020

rustbot added this to the 1.51.0 milestone Dec 30, 2020

bors mentioned this pull request Dec 30, 2020

[EXPERIMENT] Panic when the pretty-print/reparse check fails #79996

Closed

petrochenkov mentioned this pull request Jan 7, 2021

rustc_parse: Better spans for synthesized token streams #80784

Merged

petrochenkov mentioned this pull request Jan 7, 2021

rustc_ast_pretty: Remove PrintState::insert_extra_parens #80785

Merged

petrochenkov added a commit to petrochenkov/rust that referenced this pull request Jan 7, 2021

rustc_ast_pretty: Remove PrintState::insert_extra_parens

f915e3e

It's no longer necessary after rust-lang#79472

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 8, 2021

Rollup merge of rust-lang#80785 - petrochenkov:prettyparen, r=Aaron1011

f6c58af

rustc_ast_pretty: Remove `PrintState::insert_extra_parens` It's no longer necessary after rust-lang#79472. r? `@Aaron1011`

This was referenced Jan 12, 2021

Replace the pretty-print/retokenize hack with a direct comparsion of AST nodes #75820

Closed

Compiler loses location information before calling macros (sometimes) #43081

Closed

ghost mentioned this pull request Jan 30, 2021

ICE (Missing tokens for nt) on nightly with stmt_expr_attributes and proc_macro_hygiene #81555

Closed

Uh oh!

Conversation

Aaron1011 commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Nov 27, 2020

Uh oh!

Aaron1011 commented Nov 27, 2020

Uh oh!

Aaron1011 commented Nov 27, 2020

Uh oh!

bors commented Nov 27, 2020

Uh oh!

bors commented Nov 27, 2020

Uh oh!

Aaron1011 commented Nov 27, 2020

Uh oh!

craterbot commented Nov 27, 2020

Uh oh!

Aaron1011 commented Nov 28, 2020

Uh oh!

rust-timer commented Nov 28, 2020

Uh oh!

Aaron1011 commented Nov 28, 2020

Uh oh!

rust-timer commented Nov 28, 2020

Uh oh!

Mark-Simulacrum commented Nov 28, 2020

Uh oh!

rust-timer commented Nov 28, 2020

Uh oh!

rust-timer commented Nov 28, 2020

Uh oh!

craterbot commented Dec 8, 2020

Uh oh!

craterbot commented Dec 11, 2020

Uh oh!

emilyalbini commented Dec 11, 2020

Uh oh!

craterbot commented Dec 11, 2020

Uh oh!

petrochenkov commented Dec 11, 2020

Uh oh!

craterbot commented Dec 11, 2020

Uh oh!

craterbot commented Dec 12, 2020

Uh oh!

petrochenkov commented Dec 12, 2020

Uh oh!

Aaron1011 commented Dec 29, 2020

Uh oh!

bors commented Dec 29, 2020

Uh oh!

bors commented Dec 30, 2020

Uh oh!

bors commented Dec 30, 2020

Uh oh!

Aaron1011 commented Dec 30, 2020

Uh oh!

bors commented Dec 30, 2020

Uh oh!

bors commented Dec 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Aaron1011 commented Nov 27, 2020 •

edited

Loading