Skip to content

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

Merged
bors merged 1 commit intorust-lang:masterfrom
Aaron1011:new-remove-pretty-print-hack
Dec 30, 2020
Merged

Replace pretty-print/compare/retokenize hack with targeted workarounds#79472
bors merged 1 commit intorust-lang:masterfrom
Aaron1011:new-remove-pretty-print-hack

Conversation

@Aaron1011
Copy link
Contributor

@Aaron1011 Aaron1011 commented Nov 27, 2020

Based on #78296
cc #43081

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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. 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.

9 participants