hygiene: Eliminate expansion hierarchy in favor of call-site hierarchy #51457
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 has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Hmm, we need someone interested in hygiene and not on PTO, parental leave or perpetual absence. Hard choice. cc @nrc @jseyfried |
oli-obk
reviewed
Jun 9, 2018
| } | ||
| } | ||
|
|
||
| macro use_field($define_field: item) { |
There was a problem hiding this comment.
Can you also add a test for macro_rules, as they go through different code?
| } | ||
|
|
||
| use_field!(define_field!{}); | ||
|
|
There was a problem hiding this comment.
if define_field took an identifier as an argument, and the user passed "field" as the identifier, would that have compiled before, but would fail now? Or are there other ways this is a breaking change (other than diagnostics)?
I have a feeling there could be very subtle breakage
There was a problem hiding this comment.
Yes, this is a subtle breakage for users of declarative macros 2.0 and their def-site hygiene.
A had hard time coming up even with one behavior-changing test though (but it turned out to fix things, rather than break).
I'll add a test for field passed as an identifier and macro_rules! use_field, but I'm pretty sure the second one is not affected by the change (it doesn't go through the "who is the parent" logic).
petrochenkov
force-pushed
the
hygnoparent
branch
from
26b08a0 to
643391f
Compare
|
Updated. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
petrochenkov
force-pushed
the
hygnoparent
branch
from
643391f to
ef001ae
Compare
oli-obk
reviewed
Jun 11, 2018
| @@ -347,7 +347,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> { | |||
| let derives = derives.entry(invoc.expansion_data.mark).or_insert_with(Vec::new); | |||
|
|
|||
| for path in &traits { | |||
| let mark = Mark::fresh(self.cx.current_expansion.mark); | |||
There was a problem hiding this comment.
I was more worried about this causing breaking changes to macro_rules than breaking the unstable macros 2.0. But I also cannot come up with an example that would break due to this change. Maybe we should do a crater run?
There was a problem hiding this comment.
Yeah, that would be interesting since unstable crates are on crates.io too.
|
Check-only crater run requested! |
petrochenkov
added
S-waiting-on-crater
|
@bors try |
|
⌛ Trying commit ef001ae with merge fbbd1bb74974ed8dd0d394e4dfcd2383462475f6... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-author
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added a commit
that referenced
this pull request
Jun 11, 2018
hygiene: Eliminate expansion hierarchy in favor of call-site hierarchy
"Expansion hierarchy" and "call-site hierarchy" are two slightly different ways to nest macro invocations and answer the question "who is the parent" for a given invocation.
For example, here both hierarchies coincide
```rust
macro inner() { ... }
macro outer() { inner!() }
outer!();
// expansions: root -> outer -> inner
// call-sites: root -> outer -> inner
```
but here they are different
```rust
macro inner() { ... }
macro outer($stuff: stuff) { $stuff }
// expansions: root -> outer -> inner (`inner` is expanded as a part of `outer`'s output)
// call-sites: root -> outer; root -> inner
outer!(inner!())
```
All our talk about hygiene was in terms of "def-site" and "call-site" name resolution so far and the "expansion hierarchy" is an internal detail, but it was actually used in few places in the way affecting resolution results. For example, in the attached test case the structure `S` itself was previously resolved succesfully, but resolution of its field `field` failed due to use of "expansion hierarchy".
This PR eliminates expansion hierarchy from hygiene algorithm and uses call-site hierarchy instead.
This is also a nice simplification of the model.
Instead of growing in *three* dimensions in similar but subtly different ways (def-site, call-site, expansion) hygiene hierarchies now grow in *two* dimensions in similar but subtly different ways (def-site, call-site).
|
☀️ Test successful - status-travis |
kennytm
added
S-waiting-on-crater
|
If crater isn't started yet, then I'll close this. Apparently |
petrochenkov
removed
the
S-waiting-on-crater
|
|
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.
"Expansion hierarchy" and "call-site hierarchy" are two slightly different ways to nest macro invocations and answer the question "who is the parent" for a given invocation.
For example, here both hierarchies coincide
but here they are different
All our talk about hygiene was in terms of "def-site" and "call-site" name resolution so far and the "expansion hierarchy" is an internal detail, but it was actually used in few places in the way affecting resolution results. For example, in the attached test case the structure
Sitself was previously resolved succesfully, but resolution of its fieldfieldfailed due to use of "expansion hierarchy".This PR eliminates expansion hierarchy from hygiene algorithm and uses call-site hierarchy instead.
This is also a nice simplification of the model.
Instead of growing in three dimensions in similar but subtly different ways (def-site, call-site, expansion) hygiene hierarchies now grow in two dimensions in similar but subtly different ways (def-site, call-site).