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

Macro definitions should be regular HIR items #87406

Closed
cjgillot opened this issue Jul 23, 2021 · 13 comments · Fixed by #88019
Closed

Macro definitions should be regular HIR items #87406

cjgillot opened this issue Jul 23, 2021 · 13 comments · Fixed by #88019
Assignees
Labels
A-HIR Area: The high-level intermediate representation (HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-decl_macro `#![feature(decl_macro)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cjgillot
Copy link
Contributor

Macro definitions are currently handled separately from other item kinds in HIR, either as MacroDef nodes inside hir::Crate::exported_macros or as using the hir::Crate::non_exported_macro_attrs.

They should be transformed into additional variants in the hir::ItemKind enum, and iterated over like any other item.

Instructions:

  • introduce two additional variants hir::ItemKind::ExportedMacro { ast: ast::MacroDef } and hir::ItemKind::NonExportedMacro;
  • move the macro case in rustc_ast_lowering::item::LoweringContext::lower_item into lower_item_kind;
  • remove hir::OwnerNode::{MacroDef, NonExportedMacro} variants.

I recomment waiting on #83723 and #87234 to be merged.

Original idea by @petrochenkov in #83723 (comment)
Should help with #73754

@cjgillot cjgillot added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jul 23, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 23, 2021

See also #77862, #87257, #79396 (comment).

@inquisitivecrystal
Copy link
Contributor

ExportedMacro (or indeed, exported_macros) is a fairly misleading name, since this includes non exported MBE 2.0 macros. I brought this up in #86968, but I haven't opened a follow-up discussion (partly because of a lack of bandwidth, and partly because it seemed like the relevant code needed to be rewritten at some point).

Incidentally, how difficult do you think this task will be?

@inquisitivecrystal inquisitivecrystal added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-cleanup Category: PRs that clean code up or issues documenting cleanup. F-decl_macro `#![feature(decl_macro)]` F-pub_macro_rules `#![feature(pub_macro_rules)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 24, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 24, 2021

@inquisitivecrystal my understanding without looking into it much is that basically everything that touches exported_macros today will have to be rewritten. So at least E-medium, possibly E-hard. But @petrochenkov would have a better idea than me.

@inquisitivecrystal
Copy link
Contributor

Alright. I'm currently looking at whether this might be within my ability to implement. I don't like saying I'll do something until I I've checked that I have some chance of figuring it out, but the E-mentor helps a bit with that.

@inquisitivecrystal
Copy link
Contributor

I'm going to give this a go! There's no way of knowing for sure if I can do it until I actually try it out, and I can't do that properly until the PRs mentioned in the initial post are merged.

@inquisitivecrystal inquisitivecrystal self-assigned this Jul 24, 2021
@cjgillot
Copy link
Contributor Author

@inquisitivecrystal : #87234 contains a perf regression which may very well be caused by this bug. I think you can go ahead with the implementation, I will rebase it on top of your work.
923ae28c0d60720a76782803ff478e49b099746c introduces the hir::OwnerNode::NonExportedMacro variant I mention in the top post. This commit will point the right place to modify, but you should not need to cherry-pick it.
Don't hesitate to contact me on zulip if you need more precisions.

@inquisitivecrystal
Copy link
Contributor

I'm on it!

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Jul 28, 2021

I'm making progress on this.

I'm going step by step, and I'm on step two, which is migrating the way rustc_ast_lowering encodes macros to hir::Item. I've just gotten to the point where the standard library builds again. Getting this far has taken... a while. I'm not sure exactly, but it wouldn't surprise me if if you told me I'd spent ten hours on everything put together. (I actually started before my last post.) I'm not very experienced, which is probably why it's taking so long. For reference, there was one bug that took a few hours to track down on its own.

My next subgoal is making the tests pass again. I currently have nine failing tests. As far as I can tell, there are at least two distinct bugs that are causing tests to fail, and possibly more. After I fix those, I've got to move onto the future steps. Here's what the checklist looks like:

  • Add macros to hir::ItemKind
  • Change over to the new macro representation
  • Remove macros fromOwnerNode
  • Lift #[export_macro] macros to the top level
  • Change rustdoc to the new system
  • Add tests
  • General code cleanup

I'll update that as I go. I may slow down a bit, as I'm not really sure I can keep up this intensity. :)

@jyn514
Copy link
Member

jyn514 commented Jul 28, 2021

I'll update that as I go. I may slow down a bit, as I'm not really sure I can keep up this intensity. :)

No worries, slow progress is better than no progress :) thank you for working on this!

@inquisitivecrystal
Copy link
Contributor

I'm still working on this. Lifting the macros to top level is surprisingly complicated, but I'm making headway.

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Aug 4, 2021

After a fair bit of work, the standard library now builds again, even with the macros being lifted to the top level. However, I do still need to make the tests pass again. I'm also definitely going to need to spend a while cleaning my code up. It's horribly messy.

@inquisitivecrystal
Copy link
Contributor

I had to take a break for a few days, but I'm back at it. I'm hoping I'll be able to have a PR out for people to comment on soonish.

@inquisitivecrystal
Copy link
Contributor

The PR is up! It's #88019.

@inquisitivecrystal inquisitivecrystal added A-HIR Area: The high-level intermediate representation (HIR) and removed F-pub_macro_rules `#![feature(pub_macro_rules)]` labels Aug 14, 2021
@bors bors closed this as completed in 05cccdc Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-HIR Area: The high-level intermediate representation (HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-decl_macro `#![feature(decl_macro)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants