Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move special treatment of derive(Copy, PartialEq, Eq) from expansion infrastructure to elsewhere #63248

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Aug 3, 2019

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, something like that would also work.
PartialEq produces impl Structural, rustc_mir somehow checks for T: Structural + Eq.

Copy link
Contributor

@Centril Centril Aug 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc also @varkor

This could be useful for const generics (#60286), e.g. fn foo<T: Structural, const A: T>();. (But that would require #[derive(PartialEq, Eq)] -> Structural, not just PartialEq.)

@petrochenkov
Copy link
Contributor Author

r? @matthewjasper
eddyb is temporarily away and has too many assigned PRs already

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2019

📌 Commit 2a9b752 has been approved by matthewjasper

@bors 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 Aug 4, 2019
@bors
Copy link
Contributor

bors commented Aug 5, 2019

⌛ Testing commit 2a9b752 with merge e1d7e4a...

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
@bors
Copy link
Contributor

bors commented Aug 5, 2019

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing e1d7e4a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2019
@bors bors merged commit 2a9b752 into rust-lang:master Aug 5, 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
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants