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

Derive TypeFoldable using a proc-macro #66384

Merged
merged 5 commits into from Nov 17, 2019
Merged

Conversation

cjgillot
Copy link
Contributor

A new proc macro is added in librustc_macros.
It is used to derive TypeFoldable inside librustc and librustc_traits.

For now, the macro uses the 'tcx lifetime implicitly, and does not allow for a more robust selection of the adequate lifetime.

The Clone-based TypeFoldable implementations are not migrated.

Closes #65674

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2019
@@ -307,7 +307,8 @@ pub struct ResolvedOpaqueTy<'tcx> {
///
/// Here, we would store the type `T`, the span of the value `x`, and the "scope-span" for
/// the scope that contains `x`.
#[derive(RustcEncodable, RustcDecodable, Clone, Debug, Eq, Hash, HashStable, PartialEq)]
#[derive(RustcEncodable, RustcDecodable, Clone, Debug, Eq, Hash, HashStable, PartialEq,
TypeFoldable)]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have two #[derive(...)] attributes, one for the standard derives and one for the rustc-specific derives?

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM, really nice! cc @rust-lang/compiler Anyone see any reason not to do this?

@eddyb
Copy link
Member

eddyb commented Nov 13, 2019

The Clone-based TypeFoldable implementations are not migrated.

Maybe we could handle those by having #[type_foldable(noop)] on the type, or maybe even on individual fields?

@nikomatsakis
Copy link
Contributor

This is awesome. We should definitely do this.

@cjgillot
Copy link
Contributor Author

I changed the formatting of the derives.

If you don't mind, I would like to keep the Clone-based implementations for another PR.
The implementation is subtler than I thought, and I am not sure it is worth it.

@Zoxc
Copy link
Contributor

Zoxc commented Nov 15, 2019

@cjgillot It's a good idea to keep PR small anyway.

Moving TypeFoldable and Lift to proc macros has been on my todo list. Maybe you want to tackle Lift too while you're on your proc macro spree?

}
};
}

#[macro_export]
macro_rules! EnumTypeFoldableImpl {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still used to implement TypeFoldable for types defined in librustc's dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... maybe we should keep the other macros too then since they could also be used for that.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just use the derives for all librustc* and libsyntax* crates, even if they don't are/depend on librustc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eddyb As in, add a generic Fold trait for librustc dependencies? That seems maybe too complex. I'm not sure there's many uses of this macro left.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, sorry. What I said makes sense for HashStable but not as much this.

@Zoxc
Copy link
Contributor

Zoxc commented Nov 15, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 15, 2019

📌 Commit cb46c35 has been approved by Zoxc

@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 Nov 15, 2019
@bors
Copy link
Contributor

bors commented Nov 17, 2019

⌛ Testing commit cb46c35 with merge d801458...

bors added a commit that referenced this pull request Nov 17, 2019
Derive TypeFoldable using a proc-macro

A new proc macro is added in librustc_macros.
It is used to derive TypeFoldable inside librustc and librustc_traits.

For now, the macro uses the `'tcx` lifetime implicitly, and does not allow for a more robust selection of the adequate lifetime.

The Clone-based TypeFoldable implementations are not migrated.

Closes #65674
@bors
Copy link
Contributor

bors commented Nov 17, 2019

☀️ Test successful - checks-azure
Approved by: Zoxc
Pushing d801458 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2019
@bors bors merged commit cb46c35 into rust-lang:master Nov 17, 2019
@cjgillot cjgillot deleted the typefoldable branch November 18, 2019 08:03
bors added a commit that referenced this pull request Nov 19, 2019
Derive Lift using a proc-macro

Based on #66384

r? @Zoxc
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.

Make TypeFoldable derivable
7 participants