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.

Sign up for GitHub

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

Jump to bottom

Support #[macro_reexport]ing custom derives #37542

Merged
merged 12 commits into from
Nov 10, 2016
Merged

Support #[macro_reexport]ing custom derives #37542

merged 12 commits into from
Nov 10, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Nov 2, 2016 •
edited
Loading

This is gated behind #![feature(macro_reexport)] and #![feature(proc_macro)].
r? @nrc

@jseyfried jseyfried force-pushed the custom_derive_reexports branch from bf9164a to 596a773 Compare November 2, 2016 22:18
@jseyfried
Copy link
Contributor Author

jseyfried commented Nov 2, 2016 •
edited
Loading

cc #35896 #35900
cc @alexcrichton

@jseyfried jseyfried force-pushed the custom_derive_reexports branch 2 times, most recently from ae64f33 to 39b65bd Compare November 3, 2016 04:54
@alexcrichton
Copy link
Member

Nice!

@jseyfried jseyfried force-pushed the custom_derive_reexports branch from 39b65bd to 7012100 Compare November 5, 2016 21:08
nrc
nrc suggested changes Nov 7, 2016
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

I'm not clear on what is going on here - why do we want macro_reexport and custom derive to work together? I would like macro_reexport to disappear entirely and I don't know of any motivation for it to work with custom derive. I assume this is a step towards something else, but I'm not sure what

src/test/compile-fail-fulldeps/proc-macro/cannot-link.rs
extern crate derive_a;
//~^ ERROR: crates of the `proc-macro` crate type cannot be linked at runtime

fn main() {}
Copy link
Member

Choose a reason for hiding this comment

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

This test should still be valid, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a good reason to disallow it? For example, we will want to support:

extern crate derive_a;
use derive_a::A;
#[derive(A)] struct Foo;

Copy link
Member

Choose a reason for hiding this comment

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

I would like to maintain a strict phase distinction between macro and non-macro crates (we intend to relax this eventually, but I don't want it to creep in). I don't want the macro_use to be optional, and until we actually support useing derives, it seems that we should require it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is still a phase distinction -- only the macros are available from extern crate derive_a and derive_a is not linked.

until we actually support useing derives

I was going to support this behind a feature gate in a PR tomorrow...

I don't want the macro_use to be optional

Why not? Without #[macro_use] or the ability to use derives, extern crate derive_a; just defines an empty module derive_a. Maybe a warning would be appropriate, but I don't see a reason to disallow it.

Copy link
Member

Choose a reason for hiding this comment

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

OK, if useing derives will work soon, then lets allow it. As long as the non-macro_use case has sensible semantics, then I see no reason to block it.

src/test/compile-fail-fulldeps/proc-macro/shadow.rs
@@ -15,6 +15,6 @@
#[macro_use]
extern crate derive_a;
#[macro_use]
extern crate derive_a; //~ ERROR `derive_a` has already been defined
extern crate derive_a; //~ ERROR `derive_a` has already been imported

fn main() {}
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a test that uses macro_reexport with a custom derive?

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, I added a test.

src/librustdoc/visit_ast.rs
let def = self.cx.sess().cstore.load_macro(def_id, self.cx.sess());
let def = match self.cx.sess().cstore.load_macro(def_id, self.cx.sess()) {
LoadedMacro::MacroRules(macro_rules) => macro_rules,
LoadedMacro::ProcMacro(..) => continue, // FIXME
Copy link
Member

Choose a reason for hiding this comment

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

This FIXME needs more explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jseyfried
Copy link
Contributor Author

jseyfried commented Nov 7, 2016 •
edited
Loading

@nrc The main purpose of this PR is to change the metadata to support reexporting custom derives.
Once we support this, I see no reason to specifically disallow reexporting custom derives (it would actually be a couple of extra lines).

#[macro_reexport] is feature gated and I think we should deprecate it once we can pub use macros behind a feature gate (writing that PR now). Until #[macro_reexport] is removed, though, I think it should behave as consistently as possible on different kinds of macros.

@nrc
Copy link
Member

nrc commented Nov 7, 2016 •
edited by jseyfried
Loading

Until #[macro_reexport] is removed, though, I think it should behave as consistently as possible on different kinds of macros.

OK, that makes sense.

@bors
Copy link
Contributor

bors commented Nov 7, 2016

☔ The latest upstream changes (presumably #37506) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the custom_derive_reexports branch 3 times, most recently from e4d0070 to 652ec19 Compare November 8, 2016 01:09
@nrc
Copy link
Member

nrc commented Nov 8, 2016

r = me, but a test seems to be broken

@jseyfried jseyfried force-pushed the custom_derive_reexports branch from 652ec19 to 8d44bd5 Compare November 8, 2016 06:15
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Nov 8, 2016

📌 Commit 8d44bd5 has been approved by nrc

@bors
Copy link
Contributor

bors commented Nov 8, 2016

⌛ Testing commit 8d44bd5 with merge 9723bd2...

@bors
Copy link
Contributor

bors commented Nov 8, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@jseyfried jseyfried force-pushed the custom_derive_reexports branch from 8d44bd5 to 5173173 Compare November 8, 2016 22:30
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Nov 8, 2016

📌 Commit 5173173 has been approved by nrc

@eddyb eddyb mentioned this pull request Nov 9, 2016
Merged
bors added a commit that referenced this pull request Nov 9, 2016
@bors
Auto merge of #37670 - eddyb:rollup, r=eddyb
da2ce22 
Rollup of 15 pull requests

- Successful merges: #36868, #37134, #37229, #37250, #37370, #37428, #37432, #37472, #37524, #37614, #37622, #37627, #37636, #37644, #37654
- Failed merges: #37463, #37542, #37645
@bors
Copy link
Contributor

bors commented Nov 9, 2016

🔒 Merge conflict

@bors
Copy link
Contributor

bors commented Nov 9, 2016

☔ The latest upstream changes (presumably #37670) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the custom_derive_reexports branch 2 times, most recently from c5d28e8 to 76d15f4 Compare November 10, 2016 11:18
@jseyfried jseyfried force-pushed the custom_derive_reexports branch from 76d15f4 to f35eff2 Compare November 10, 2016 11:19
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Nov 10, 2016

📌 Commit f35eff2 has been approved by nrc

@bors
Copy link
Contributor

bors commented Nov 10, 2016

⌛ Testing commit f35eff2 with merge 187d989...

bors added a commit that referenced this pull request Nov 10, 2016
@bors
Auto merge of #37542 - jseyfried:custom_derive_reexports, r=nrc
187d989 
Support `#[macro_reexport]`ing custom derives

This is gated behind `#![feature(macro_reexport)]` and `#![feature(proc_macro)]`.
r? @nrc
@bors
Copy link
Contributor

bors commented Nov 10, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: nrc
Pushing 187d989 to master...

@bors bors merged commit f35eff2 into rust-lang:master Nov 10, 2016
@bors bors mentioned this pull request Nov 10, 2016
Merged
@jseyfried jseyfried deleted the custom_derive_reexports branch November 15, 2016 23:03
This was referenced Nov 15, 2016
Closed
Closed
@jseyfried jseyfried mentioned this pull request Feb 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrc nrc nrc approved these changes

Assignees

@nrc nrc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jseyfried @alexcrichton @nrc @bors