Handle `macro_rules!` tokens consistently across crates#73569

Aaron1011 · 2020-06-21T01:52:23Z

When we serialize a macro_rules! macro, we used a 'lowered' TokenStream for its body, which has all Nonterminals expanded in-place via nt_to_tokenstream. This matters when an 'outer' macro_rules! macro expands to an 'inner' macro_rules! macro - the inner macro may use tokens captured from the 'outer' macro in its definition.

This means that invoking a foreign macro_rules! macro may use a different body TokenStream than when the same macro_rules! macro is invoked in the same crate. This difference is observable by proc-macros invoked by a macro_rules! macro - a None-delimited group will be seen in the same-crate case (inserted when convering Nonterminals to the proc_macro crate's structs), but no None-delimited group in the cross-crate case.

To fix this inconsistency, we now insert None-delimited groups when 'lowering' a Nonterminal macro_rules! body, just as we do in proc_macro_server. Additionally, we no longer print extra spaces for None-delimited groups - as far as pretty-printing is concerned, they don't exist (only their contents do). This ensures that Display output of a TokenStream does not depend on which crate a macro_rules! macro was invoked from.

This PR is necessary in order to patch the solana-genesis-programs for the upcoming hygiene serialization breakage (#72121 (comment)). The solana-genesis-programs crate will need to use a proc macro to re-span certain tokens in a nested macro_rules!, which requires us to consistently use a None-delimited group.

See src/test/ui/proc-macro/nested-macro-rules.rs for an example of the kind of nested macro_rules! affected by this crate.

rust-highfive · 2020-06-21T01:52:27Z

r? @eddyb

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

Aaron1011 · 2020-06-21T01:52:34Z

r? @petrochenkov

Aaron1011 · 2020-06-21T06:15:03Z

I'm not quite sure what the best way to handle pretty-printing is. There seems to be a few options:

Don't insert None-delimited groups, and leave macro_rules! behavior inconsistent across crates. This seems confusing for users, and has real consequences when writing proc-macros (less span information is available). However, it avoids any breakage.
Do what this PR currently does. but special-case stringify! to not insert extra spaces for outer (or maybe all?) Nonterminals. This seems like kind of a hack, and will change the behavior of proc-macros that generate calls to stringify! with custom tokens.
Instead of adding spaces when pretty-printing None-delimited groups, just print nothing. I think this may be the best option: since they will be lost on re-parsing, they only serve as an indication that a metavariable replacement occurred (but without providing any additional information). I think it's almost impossible to write any code which actually uses the behavior, and any code which does would be better served by inspecting the TokenStream directly. However, this will probably result in some amount of breakage due to crates matching exactly on the pretty-printed output.

ratijas · 2020-06-21T14:35:44Z

Hi,

Just throwing my two cents here. I heard about None-delimited groups which you are talking about. In fact, I had troubles with them, and found some bugs in rustc AND proc_macro ecosystem: #71422.

Any chances that this PR fixes some of these issues raised there?

Aaron1011 · 2020-06-21T20:32:40Z

@ratijas: I don't think this PR will affect #71422, since that issue don't seem to be related to nested macro_rules! macros.

Aaron1011 · 2020-06-21T21:00:28Z

I've switched to removing the extra spaces printed for None-delimited groups. This has the nice effect of making re-collecting tokens a no-op in more cases - the pretty-printed output is no longer affected by the replacement of Nonterminals with None-delimited groups. We should probably do a Crater run for this.

Aaron1011 · 2020-06-21T21:00:52Z

@bors try

bors · 2020-06-21T21:01:04Z

⌛ Trying commit 2faaf6dc571385cea3f0ba0094beb1c4e7414b28 with merge 4a152b6bdae9e6d4a77446a19ff11d26b8d2b536...

bors · 2020-06-21T23:53:32Z

☀️ Try build successful - checks-azure
Build commit: 4a152b6bdae9e6d4a77446a19ff11d26b8d2b536 (4a152b6bdae9e6d4a77446a19ff11d26b8d2b536)

petrochenkov · 2020-06-23T17:59:40Z

To fix this inconsistency, we now insert None-delimited groups when 'lowering' a Nonterminal macro_rules! body, just as we do in proc_macro_server.

This is a great catch, I never noticed we forget to wrap tokens into a group during "lowering" nonterminals to HIR.
Serializing them without wrapping is certainly incorrect and may cause mistakes like $e * 3 where $e is 1 * 2 turning into 1 + 2 * 3.

One problem here is that rustc parser is entirely unready to encounter None delimiters and will interpret them as "no parents" anyway - #67062, it shouldn't block this fix though.

src/librustc_ast/attr/mod.rs

src/librustc_ast_pretty/pprust.rs

Aaron1011 · 2020-06-23T20:08:58Z

@petrochenkov: I've added additional test cases

petrochenkov · 2020-06-23T21:38:26Z

@bors try

bors · 2020-06-23T21:38:39Z

⌛ Trying commit b35cb0aeb8e0f095fa9a2506ee438dc4d4c63a07 with merge 433da287daece77315c0cb8c4eb03b037342c115...

petrochenkov · 2020-07-01T16:35:47Z

Seems ok to patch macro_rules like this, it's basically the same thing as the attribute changes from the earlier commits, attributes see the original token stream without NoDelim group flattening as well.

@bors r+
(The PR could use some squashing to eliminate fixup commits though.)

bors · 2020-07-01T16:35:49Z

📌 Commit 4cd31965c91c7eefff880075426e35846e2ec22e has been approved by petrochenkov

When a `macro_rules!` macro expands to another `macro_rules!` macro, we may see `None`-delimited groups in odd places when another crate deserializes the 'inner' macro. This commit 'unwraps' an outer `None`-delimited group to avoid breaking existing code. See rust-lang#73569 (comment) for more details. The proper fix is to handle `None`-delimited groups systematically throughout the parser, but that will require significant work. In the meantime, this hack lets us fix important hygiene bugs in macros

Aaron1011 · 2020-07-01T16:43:51Z

Squashed into more meaningful commits

@bors r=petrochenkov

bors · 2020-07-01T16:43:52Z

📌 Commit 1ded7a5 has been approved by petrochenkov

…petrochenkov Handle `macro_rules!` tokens consistently across crates When we serialize a `macro_rules!` macro, we used a 'lowered' `TokenStream` for its body, which has all `Nonterminal`s expanded in-place via `nt_to_tokenstream`. This matters when an 'outer' `macro_rules!` macro expands to an 'inner' `macro_rules!` macro - the inner macro may use tokens captured from the 'outer' macro in its definition. This means that invoking a foreign `macro_rules!` macro may use a different body `TokenStream` than when the same `macro_rules!` macro is invoked in the same crate. This difference is observable by proc-macros invoked by a `macro_rules!` macro - a `None`-delimited group will be seen in the same-crate case (inserted when convering `Nonterminal`s to the `proc_macro` crate's structs), but no `None`-delimited group in the cross-crate case. To fix this inconsistency, we now insert `None`-delimited groups when 'lowering' a `Nonterminal` `macro_rules!` body, just as we do in `proc_macro_server`. Additionally, we no longer print extra spaces for `None`-delimited groups - as far as pretty-printing is concerned, they don't exist (only their contents do). This ensures that `Display` output of a `TokenStream` does not depend on which crate a `macro_rules!` macro was invoked from. This PR is necessary in order to patch the `solana-genesis-programs` for the upcoming hygiene serialization breakage (rust-lang#72121 (comment)). The `solana-genesis-programs` crate will need to use a proc macro to re-span certain tokens in a nested `macro_rules!`, which requires us to consistently use a `None`-delimited group. See `src/test/ui/proc-macro/nested-macro-rules.rs` for an example of the kind of nested `macro_rules!` affected by this crate.

@ghost

…arth Rollup of 16 pull requests Successful merges: - rust-lang#72569 (Remove legacy InnoSetup GUI installer) - rust-lang#73306 (Don't implement Fn* traits for #[target_feature] functions) - rust-lang#73345 (expand: Stop using nonterminals for passing tokens to attribute and derive macros) - rust-lang#73449 (Provide more information on duplicate lang item error.) - rust-lang#73569 (Handle `macro_rules!` tokens consistently across crates) - rust-lang#73803 (Recover extra trailing angle brackets in struct definition) - rust-lang#73839 (Split and expand nonstandard-style lints unicode unit test.) - rust-lang#73841 (Remove defunct `-Z print-region-graph`) - rust-lang#73848 (Fix markdown rendering in librustc_lexer docs) - rust-lang#73865 (Fix Zulip topic format) - rust-lang#73892 (Clean up E0712 explanation) - rust-lang#73898 (remove duplicate test for rust-lang#61935) - rust-lang#73906 (Add missing backtick in `ty_error_with_message`) - rust-lang#73909 (`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs) - rust-lang#73910 (Rewrite a few manual index loops with while-let) - rust-lang#73929 (Fix comment typo) Failed merges: r? @ghost

When a `macro_rules!` macro expands to another `macro_rules!` macro, we may see `None`-delimited groups in odd places when another crate deserializes the 'inner' macro. This commit 'unwraps' an outer `None`-delimited group to avoid breaking existing code. See rust-lang#73569 (comment) for more details. The proper fix is to handle `None`-delimited groups systematically throughout the parser, but that will require significant work. In the meantime, this hack lets us fix important hygiene bugs in macros

rust-highfive assigned eddyb Jun 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2020

rust-highfive assigned petrochenkov and unassigned eddyb Jun 21, 2020

This comment has been minimized.

Sign in to view

Aaron1011 force-pushed the fix/macro-rules-group branch from c2a6042 to e0cccf5 Compare June 21, 2020 03:12

This comment has been minimized.

Sign in to view

Aaron1011 force-pushed the fix/macro-rules-group branch from e0cccf5 to 2faaf6d Compare June 21, 2020 20:56

Aaron1011 mentioned this pull request Jun 22, 2020

Serialize span hygiene data #72121

Merged

6 tasks

petrochenkov reviewed Jun 23, 2020

View reviewed changes

src/librustc_ast/attr/mod.rs Outdated Show resolved Hide resolved

src/librustc_ast_pretty/pprust.rs Outdated Show resolved Hide resolved

petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2020

Aaron1011 force-pushed the fix/macro-rules-group branch from 2faaf6d to 991caff Compare June 23, 2020 20:08

Aaron1011 force-pushed the fix/macro-rules-group branch from 991caff to f1008d2 Compare June 23, 2020 20:49

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 23, 2020

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 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 Jul 1, 2020

Aaron1011 added 3 commits July 1, 2020 12:42

Insert NoDelim groups around nonterminals when lowering macro_rules

5da0576

Don't print additional spaces when pretty-printing NoDelim groups

c844028

Aaron1011 force-pushed the fix/macro-rules-group branch from 4cd3196 to 1ded7a5 Compare July 1, 2020 16:43

Manishearth mentioned this pull request Jul 1, 2020

Rollup of 16 pull requests #73939

Closed

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 16 pull requests #73944

Closed

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 17 pull requests #73946

Closed

Manishearth mentioned this pull request Jul 2, 2020

Rollup of 16 pull requests #73950

Merged

bors merged commit ce49944 into rust-lang:master Jul 2, 2020

Aaron1011 mentioned this pull request Jul 31, 2020

Re-land PR #72388: Recursively expand TokenKind::Interpolated in probably_equal_for_proc_macro #73084

Merged

cuviper added this to the 1.46 milestone May 2, 2024

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rust-highfive commented Jun 21, 2020

Uh oh!

Aaron1011 commented Jun 21, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

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

Uh oh!

Uh oh!

ratijas commented Jun 21, 2020

Uh oh!

Aaron1011 commented Jun 21, 2020

Uh oh!

Aaron1011 commented Jun 21, 2020

Uh oh!

Aaron1011 commented Jun 21, 2020

Uh oh!

bors commented Jun 21, 2020

Uh oh!

bors commented Jun 21, 2020

Uh oh!

petrochenkov commented Jun 23, 2020

Uh oh!

Uh oh!

Uh oh!

Aaron1011 commented Jun 23, 2020

Uh oh!

petrochenkov commented Jun 23, 2020

Uh oh!

bors commented Jun 23, 2020

Uh oh!

petrochenkov commented Jul 1, 2020

Uh oh!

bors commented Jul 1, 2020

Uh oh!

Aaron1011 commented Jul 1, 2020

Uh oh!

bors commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Aaron1011 commented Jun 21, 2020 •

edited

Loading

Aaron1011 commented Jun 21, 2020 •

edited

Loading