Move special treatment of derive(Copy, PartialEq, Eq) from expansion infrastructure to elsewhere
#63248
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
…n infrastructure to elsewhere
rust-highfive
added
the
S-waiting-on-review
Centril
reviewed
Aug 3, 2019
| /// Derive macros cannot modify the item themselves and have to store the markers in the global | ||
| /// context, so they attach the markers to derive container IDs using this resolver table. | ||
| /// FIXME: Find a way for `PartialEq` and `Eq` to emulate `#[structural_match]` | ||
| /// by marking the produced impls rather than the original items. |
There was a problem hiding this comment.
I would think the better way to do this would be a perma unstable trait:
#[lang = "structural_match"]
unsafe trait Structural {}for which implementations are generated.
There was a problem hiding this comment.
Yeah, something like that would also work.
PartialEq produces impl Structural, rustc_mir somehow checks for T: Structural + Eq.
|
r? @matthewjasper |
|
@bors r+ |
|
📌 Commit 2a9b752 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 5, 2019
Move special treatment of `derive(Copy, PartialEq, Eq)` from expansion infrastructure to elsewhere As described in #62086 (comment). Reminder: - `derive(PartialEq, Eq)` makes the type it applied to a "structural match" type, so constants of this type can be used in patterns (and const generics in the future). - `derive(Copy)` notifies other derives that the type it applied to implements `Copy`, so `derive(Clone)` can generate optimized code and other derives can generate code working with `packed` types and types with `rustc_layout_scalar_valid_range` attributes. First, the special behavior is now enabled after properly resolving the derives, rather than after textually comparing them with `"Copy"`, `"PartialEq"` and `"Eq"` in `fn add_derived_markers`. The markers are no longer kept as attributes in AST since derives cannot modify items and previously did it through hacks in the expansion infra. Instead, the markers are now kept in a "global context" available from all the necessary places, namely - resolver. For `derive(PartialEq, Eq)` the markers are created by the derive macros themselves and then consumed during HIR lowering to add the `#[structural_match]` attribute in HIR. This is still a hack, but now it's a hack local to two specific macros rather than affecting the whole expansion infra. Ideally we should find the way to put `#[structural_match]` on the impls rather than on the original item, and then consume it in `rustc_mir`, then no hacks in expansion and lowering will be required. (I'll make an issue about this for someone else to solve, after this PR lands.) The marker for `derive(Copy)` cannot be emitted by the `Copy` macro itself because we need to know it *before* the `Copy` macro is expanded for expanding other macros. So we have to do it in resolve and block expansion of any derives in a `derive(...)` container until we know for sure whether this container has `Copy` in it or not. Nasty stuff. r? @eddyb or @matthewjasper
|
☀️ Test successful - checks-azure |
This was referenced Aug 24, 2019
bors
added a commit
that referenced
this pull request
Aug 26, 2019
resolve: Block expansion of a derive container until all its derives are resolved So, it turns out there's one more reason to block expansion of a `#[derive]` container until all the derives inside it are resolved, beside `Copy` (#63248). The set of derive helper attributes registered by derives in the container also has to be known before the derives themselves are expanded, otherwise it may be too late (see #63468 (comment) and the `#[stable_hasher]`-related test failures in #63468). So, we stop our attempts to unblock the container earlier, as soon as the `Copy` status is known, and just block until all its derives are resolved. After all the derives are resolved we immediately go and process their helper attributes in the item, without delaying it until expansion of the individual derives. Unblocks #63468 r? @matthewjasper (as a reviewer of #63248) cc @c410-f3r
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 29, 2019
resolve: Block expansion of a derive container until all its derives are resolved So, it turns out there's one more reason to block expansion of a `#[derive]` container until all the derives inside it are resolved, beside `Copy` (rust-lang#63248). The set of derive helper attributes registered by derives in the container also has to be known before the derives themselves are expanded, otherwise it may be too late (see rust-lang#63468 (comment) and the `#[stable_hasher]`-related test failures in rust-lang#63468). So, we stop our attempts to unblock the container earlier, as soon as the `Copy` status is known, and just block until all its derives are resolved. After all the derives are resolved we immediately go and process their helper attributes in the item, without delaying it until expansion of the individual derives. Unblocks rust-lang#63468 r? @matthewjasper (as a reviewer of rust-lang#63248) cc @c410-f3r
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.
As described in #62086 (comment).
Reminder:
derive(PartialEq, Eq)makes the type it applied to a "structural match" type, so constants of this type can be used in patterns (and const generics in the future).derive(Copy)notifies other derives that the type it applied to implementsCopy, soderive(Clone)can generate optimized code and other derives can generate code working withpackedtypes and types withrustc_layout_scalar_valid_rangeattributes.First, the special behavior is now enabled after properly resolving the derives, rather than after textually comparing them with
"Copy","PartialEq"and"Eq"infn add_derived_markers.The markers are no longer kept as attributes in AST since derives cannot modify items and previously did it through hacks in the expansion infra.
Instead, the markers are now kept in a "global context" available from all the necessary places, namely - resolver.
For
derive(PartialEq, Eq)the markers are created by the derive macros themselves and then consumed during HIR lowering to add the#[structural_match]attribute in HIR.This is still a hack, but now it's a hack local to two specific macros rather than affecting the whole expansion infra.
Ideally we should find the way to put
#[structural_match]on the impls rather than on the original item, and then consume it inrustc_mir, then no hacks in expansion and lowering will be required.(I'll make an issue about this for someone else to solve, after this PR lands.)
The marker for
derive(Copy)cannot be emitted by theCopymacro itself because we need to know it before theCopymacro is expanded for expanding other macros.So we have to do it in resolve and block expansion of any derives in a
derive(...)container until we know for sure whether this container hasCopyin it or not.Nasty stuff.
r? @eddyb or @matthewjasper