resolve: Properly integrate derives and macro_rules scopes
#63667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
petrochenkov
force-pushed
the
deriveholders
branch
from
0bd5e23 to
1064d41
Compare
|
@bors r+ |
|
📌 Commit 1064d41 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 17, 2019
…ewjasper resolve: Properly integrate derives and `macro_rules` scopes So, ```rust #[derive(A, B)] struct S; m!(); ``` turns into something like ```rust struct S; A_placeholder!( struct S; ); B_placeholder!( struct S; ); m!(); ``` during expansion. And for `m!()` its "`macro_rules` scope" (aka "legacy scope") should point to the `B_placeholder` call rather than to the derive container `#[derive(A, B)]`. `fn build_reduced_graph` now makes sure the legacy scope points to the right thing. (It's still a mystery for me why this worked before rust-lang#63535.) Unfortunately, placeholders from derives are currently treated separately from placeholders from other macros and need to be passed as `extra_placeholders` rather than a part of the AST fragment. That's fixable, but I wanted to keep this PR more minimal to close the regression faster. Fixes rust-lang#63651 r? @matthewjasper
bors
added a commit
that referenced
this pull request
Aug 17, 2019
Rollup of 5 pull requests Successful merges: - #62451 (Add APIs for uninitialized Box, Rc, and Arc. (Plus get_mut_unchecked)) - #63487 (Remove meaningless comments in src/test) - #63657 (Crank up invalid value lint) - #63667 (resolve: Properly integrate derives and `macro_rules` scopes) - #63669 (fix typos in mir/interpret) Failed merges: r? @ghost
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Oct 11, 2019
…hewjasper expand: Simplify expansion of derives And make it more uniform with other macros. This is done by merging placeholders for future derives' outputs into the derive container's output fragment early (addressing FIXMEs from rust-lang#63667). Also, macros with names starting with `_` are no longer reported as unused, in accordance with the usual behavior of `unused` lints. r? @matthewjasper or @mark-i-m
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 13, 2019
…hewjasper expand: Simplify expansion of derives And make it more uniform with other macros. This is done by merging placeholders for future derives' outputs into the derive container's output fragment early (addressing FIXMEs from rust-lang#63667). Also, macros with names starting with `_` are no longer reported as unused, in accordance with the usual behavior of `unused` lints. r? @matthewjasper or @mark-i-m
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 14, 2019
…hewjasper expand: Simplify expansion of derives And make it more uniform with other macros. This is done by merging placeholders for future derives' outputs into the derive container's output fragment early (addressing FIXMEs from rust-lang#63667). Also, macros with names starting with `_` are no longer reported as unused, in accordance with the usual behavior of `unused` lints. r? @matthewjasper or @mark-i-m
Centril
added a commit
to Centril/rust
that referenced
this pull request
Oct 19, 2019
…hewjasper expand: Simplify expansion of derives And make it more uniform with other macros. This is done by merging placeholders for future derives' outputs into the derive container's output fragment early (addressing FIXMEs from rust-lang#63667). Also, macros with names starting with `_` are no longer reported as unused, in accordance with the usual behavior of `unused` lints. r? @matthewjasper or @mark-i-m
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So,
turns into something like
during expansion.
And for
m!()its "macro_rulesscope" (aka "legacy scope") should point to theB_placeholdercall rather than to the derive container#[derive(A, B)].fn build_reduced_graphnow makes sure the legacy scope points to the right thing.(It's still a mystery for me why this worked before #63535.)
Unfortunately, placeholders from derives are currently treated separately from placeholders from other macros and need to be passed as
extra_placeholdersrather than a part of the AST fragment.That's fixable, but I wanted to keep this PR more minimal to close the regression faster.
Fixes #63651
r? @matthewjasper