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 proper module parent during resolution #77984

Merged
merged 1 commit into from
Oct 25, 2020

Conversation

Aaron1011
Copy link
Member

Fixes #75982

The direct parent of a module may not be a module
(e.g. const _: () = { #[path = "foo.rs"] mod foo; };).

To find the parent of a module for purposes of resolution, we need to
walk up the tree until we hit a module or a crate root.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Oct 15, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

This same code is copied in rustdoc:

// The immediate parent might not always be a module.
// Find the first parent which is.
loop {
if let Some(parent) = self.cx.tcx.parent(current) {
if self.cx.tcx.def_kind(parent) == DefKind::Mod {
break Some(parent);
}
current = parent;
} else {
debug!(
"{:?} has no parent (kind={:?}, original was {:?})",
current,
self.cx.tcx.def_kind(current),
item.def_id
);
break None;
}
}

Maybe it's worth making this a query or method on tcx?

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 15, 2020
@Aaron1011
Copy link
Member Author

@jyn514: It's not quite the same code - we don't have a tcx available in the resolver, so we need to go through the cstore methods directly. I think it's better to keep this separate, since going through a tcx is better when a tcx is available.

@petrochenkov petrochenkov self-assigned this Oct 15, 2020
@davidtwco davidtwco removed their assignment Oct 16, 2020
@petrochenkov
Copy link
Contributor

So, the sad part here is that the Module in question is not actually a mod item, but any "module in resolve sense" aka "a container for named items", that includes blocks.

If

const _: () = {
    #[macro_export]
    macro_rules! my_macro {
        () => {};
    }
};

was written in the current crate, then get_module would indeed returned the constant's block (it would have ModuleKind::Block).

We don't track blocks for other crates, so we simply cannot produce a correct answer here.
This situation is specific to #[macro_export] macros which have unique ability to be visible to other crates while being written in very internal locations.

So, returning the closest mod item is not really correct, but at least it doesn't ICEs.
Thankfully, returning an incorrect parent module in this case shouldn't have any observable effect in theory (but it would be a different story if my_macro could use def-site hygiene).

@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 Oct 19, 2020
@Aaron1011 Aaron1011 force-pushed the fix/macro-mod-weird-parent branch 2 times, most recently from 0567261 to c07f8f8 Compare October 24, 2020 16:30
@Aaron1011 Aaron1011 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 Oct 24, 2020
@petrochenkov
Copy link
Contributor

r=me with the test changes #77984 (comment)

@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 Oct 24, 2020
Fixes rust-lang#75982

The direct parent of a module may not be a module
(e.g. `const _: () =  { #[path = "foo.rs"] mod foo; };`).

To find the parent of a module for purposes of resolution, we need to
walk up the tree until we hit a module or a crate root.
@Aaron1011
Copy link
Member Author

@petrochenkov: Your suggested test caught a bug in my implementation. We also need to perform the parent-walking in macro_def_scope (not just in get_module), since a macro_rules! might occur directly inside a const item.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 24, 2020

📌 Commit 283053a 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 Oct 24, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 25, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77984 (Compute proper module parent during resolution)
 - rust-lang#78085 (MIR validation should check `SwitchInt` values are valid for the type)
 - rust-lang#78208 (replace `#[allow_internal_unstable]` with `#[rustc_allow_const_fn_unstable]` for `const fn`s)
 - rust-lang#78209 (Update `compiler_builtins` to 0.1.36)
 - rust-lang#78276 (Bump backtrace-rs to enable Mach-O support on iOS.)
 - rust-lang#78320 (Link to cargo's `build-std` feature instead of `xargo` in custom target docs)
 - rust-lang#78322 (BTreeMap: stop mistaking node::MIN_LEN for a node level constraint)
 - rust-lang#78326 (Split out statement attributes changes from rust-lang#78306)

Failed merges:

r? `@ghost`
@bors bors merged commit 569d29d into rust-lang:master Oct 25, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: Expected module, found DefId
7 participants