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

Compute parent module when collecting hir::MacroDef. #80415

Merged
merged 3 commits into from
Jan 6, 2021

Conversation

cjgillot
Copy link
Contributor

Fixes #77828.

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2020
@jyn514 jyn514 added A-hir Area: The high-level intermediate representation (HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Dec 27, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 27, 2020

This is awesome, but I don't know enough about HIR to review I think. Is there a way to test this somehow? Maybe you could see if #80414 now generates documentation at core/marker/macro.Copy.html instead of core/macro.Copy.html ? See https://rustc-dev-guide.rust-lang.org/rustdoc-internals.html#dotting-is-and-crossing-ts for how to write rustdoc tests.

@cjgillot
Copy link
Contributor Author

I can't get such a rustdoc test passing.

@jyn514
Copy link
Member

jyn514 commented Dec 27, 2020

Ok, then it looks like there were two separate bugs for #74355, one in rustc and one in rustdoc.

r? @petrochenkov maybe?

@rust-highfive rust-highfive assigned petrochenkov and unassigned jyn514 Dec 27, 2020
@danielhenrymantilla
Copy link
Contributor

There is still some effort needed to fix #74355, but the changes in this PR are great since they will fix the hack I have used with my fix attempt in #77862

@petrochenkov
Copy link
Contributor

What tcx.hir().parent_module(hir_id) and tcx.parent(def_id) show for #[macro_export] macro_rules! ... items now?

If they are consistent (looks like they should be), then this this PR is good.
However, rustdoc may start showing #[macro_export] macro_rules! ... in inner modules rather than in the crate root, in that case it will need to be fixed.

@cjgillot Could you check both these cases? If they behave as expected, then r=me.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2020
Comment on lines 7 to 8
() => [];
($a:tt) => ();
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this?

@cjgillot
Copy link
Contributor Author

cjgillot commented Jan 4, 2021

@petrochenkov, I am not sure how to address your review. How do you recommend I test the consistency?

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2021
@petrochenkov
Copy link
Contributor

@cjgillot
In any way, e.g. printf from the compiler.
I don't suggest to put this check into test suite, just look at the IDs and make sure that they are consistent.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2021
@cjgillot
Copy link
Contributor Author

cjgillot commented Jan 5, 2021

@petrochenkov, ok thanks.

I tested adding the following in the check_attr HIR visitor, and tested a file with both top-level and module macro exports. It passes.

use rustc_middle::ty::DefIdTree;
let def_id = self.tcx.hir().local_def_id(macro_def.hir_id);
assert_eq!(
    Some(self.tcx.parent_module(macro_def.hir_id).to_def_id()),
    self.tcx.parent(def_id.to_def_id())
);

Should be good to go.

@jyn514
Copy link
Member

jyn514 commented Jan 5, 2021

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jan 5, 2021

📌 Commit 4fb1236 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 5, 2021
@bors
Copy link
Contributor

bors commented Jan 6, 2021

⌛ Testing commit 4fb1236 with merge 41601ef...

@bors
Copy link
Contributor

bors commented Jan 6, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 41601ef to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 6, 2021
@bors bors merged commit 41601ef into rust-lang:master Jan 6, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 6, 2021
@cjgillot cjgillot deleted the issue-77828 branch January 6, 2021 17:11
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, ..) 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.

HIR and TyCtxt disagree about the parents of decl macros
7 participants