Skip to content

Make def_key and HIR parenting consistent. #82891

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

Merged
merged 1 commit into from
Mar 13, 2021
Merged

Make def_key and HIR parenting consistent. #82891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions compiler/rustc_ast_lowering/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ enum ImplTraitContext<'b, 'a> {
/// equivalent to a fresh universal parameter like `fn foo<T: Debug>(x: T)`.
///
/// Newly generated parameters should be inserted into the given `Vec`.
Universal(&'b mut Vec<hir::GenericParam<'a>>),
Universal(&'b mut Vec<hir::GenericParam<'a>>, LocalDefId),

/// Treat `impl Trait` as shorthand for a new opaque type.
/// Example: `fn foo() -> impl Debug`, where `impl Debug` is conceptually
Expand Down Expand Up @@ -278,7 +278,7 @@ impl<'a> ImplTraitContext<'_, 'a> {
fn reborrow<'this>(&'this mut self) -> ImplTraitContext<'this, 'a> {
use self::ImplTraitContext::*;
match self {
Universal(params) => Universal(params),
Universal(params, parent) => Universal(params, *parent),
ReturnPositionOpaqueTy { fn_def_id, origin } => {
ReturnPositionOpaqueTy { fn_def_id: *fn_def_id, origin: *origin }
}
Expand Down Expand Up @@ -475,25 +475,18 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}

impl MiscCollector<'_, '_, '_> {
fn allocate_use_tree_hir_id_counters(&mut self, tree: &UseTree, owner: LocalDefId) {
fn allocate_use_tree_hir_id_counters(&mut self, tree: &UseTree) {
match tree.kind {
UseTreeKind::Simple(_, id1, id2) => {
for &id in &[id1, id2] {
self.lctx.resolver.create_def(
owner,
id,
DefPathData::Misc,
ExpnId::root(),
tree.prefix.span,
);
self.lctx.allocate_hir_id_counter(id);
}
}
UseTreeKind::Glob => (),
UseTreeKind::Nested(ref trees) => {
for &(ref use_tree, id) in trees {
let hir_id = self.lctx.allocate_hir_id_counter(id);
self.allocate_use_tree_hir_id_counters(use_tree, hir_id.owner);
self.lctx.allocate_hir_id_counter(id);
self.allocate_use_tree_hir_id_counters(use_tree);
}
}
}
Expand All @@ -502,7 +495,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {

impl<'tcx> Visitor<'tcx> for MiscCollector<'tcx, '_, '_> {
fn visit_item(&mut self, item: &'tcx Item) {
let hir_id = self.lctx.allocate_hir_id_counter(item.id);
self.lctx.allocate_hir_id_counter(item.id);

match item.kind {
ItemKind::Struct(_, ref generics)
Expand All @@ -521,7 +514,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
self.lctx.type_def_lifetime_params.insert(def_id.to_def_id(), count);
}
ItemKind::Use(ref use_tree) => {
self.allocate_use_tree_hir_id_counters(use_tree, hir_id.owner);
self.allocate_use_tree_hir_id_counters(use_tree);
}
_ => {}
}
Expand Down Expand Up @@ -939,8 +932,13 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
// `lifetimes_to_define`. If we swapped the order of these two,
// in-band-lifetimes introduced by generics or where-clauses
// wouldn't have been added yet.
let generics =
this.lower_generics_mut(generics, ImplTraitContext::Universal(&mut params));
let generics = this.lower_generics_mut(
generics,
ImplTraitContext::Universal(
&mut params,
this.current_hir_id_owner.last().unwrap().0,
),
);
let res = f(this, &mut params);
(params, (generics, res))
})
Expand Down Expand Up @@ -1145,6 +1143,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}
AssocTyConstraintKind::Bound { ref bounds } => {
let mut capturable_lifetimes;
let mut parent_def_id = self.current_hir_id_owner.last().unwrap().0;
// Piggy-back on the `impl Trait` context to figure out the correct behavior.
let (desugar_to_impl_trait, itctx) = match itctx {
// We are in the return position:
Expand All @@ -1164,7 +1163,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
// so desugar to
//
// fn foo(x: dyn Iterator<Item = impl Debug>)
ImplTraitContext::Universal(..) if self.is_in_dyn_type => (true, itctx),
ImplTraitContext::Universal(_, parent) if self.is_in_dyn_type => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If parenting depends on flags like is_in_dyn_type unavailable in def collector, how is it possible to make it consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific lowering is not handled inside the def collector. The generic parameter's DefId is created line 1199, and its HIR parent is the item's generic parameter list.

parent_def_id = parent;
(true, itctx)
}

// In `type Foo = dyn Iterator<Item: Debug>` we desugar to
// `type Foo = dyn Iterator<Item = impl Debug>` but we have to override the
Expand Down Expand Up @@ -1198,7 +1200,6 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
// constructing the HIR for `impl bounds...` and then lowering that.

let impl_trait_node_id = self.resolver.next_node_id();
let parent_def_id = self.current_hir_id_owner.last().unwrap().0;
self.resolver.create_def(
parent_def_id,
impl_trait_node_id,
Expand Down Expand Up @@ -1451,7 +1452,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
|this| this.lower_param_bounds(bounds, nested_itctx),
)
}
ImplTraitContext::Universal(in_band_ty_params) => {
ImplTraitContext::Universal(in_band_ty_params, parent_def_id) => {
// Add a definition for the in-band `Param`.
let def_id = self.resolver.local_def_id(def_node_id);

Expand All @@ -1460,7 +1461,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
let hir_bounds = self.with_hir_id_owner(def_node_id, |this| {
this.lower_param_bounds(
bounds,
ImplTraitContext::Universal(in_band_ty_params),
ImplTraitContext::Universal(in_band_ty_params, parent_def_id),
)
});
// Set the name to `impl Bound1 + Bound2`.
Expand Down Expand Up @@ -1891,7 +1892,13 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
}
this.arena.alloc_from_iter(inputs.iter().map(|param| {
if let Some((_, ibty)) = &mut in_band_ty_params {
this.lower_ty_direct(&param.ty, ImplTraitContext::Universal(ibty))
this.lower_ty_direct(
&param.ty,
ImplTraitContext::Universal(
ibty,
this.current_hir_id_owner.last().unwrap().0,
),
)
} else {
this.lower_ty_direct(&param.ty, ImplTraitContext::disallowed())
}
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_middle/src/hir/map/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,18 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {

data.signature =
Some(self.arena.alloc(Owner { parent: entry.parent, node: entry.node }));

let dk_parent = self.definitions.def_key(id.owner).parent;
if let Some(dk_parent) = dk_parent {
let dk_parent = LocalDefId { local_def_index: dk_parent };
let dk_parent = self.definitions.local_def_id_to_hir_id(dk_parent);
if dk_parent.owner != entry.parent.owner {
panic!(
"Different parents for {:?} => dk_parent={:?} actual={:?}",
id.owner, dk_parent, entry.parent,
)
}
}
} else {
assert_eq!(entry.parent.owner, id.owner);
insert_vec_map(
Expand Down
89 changes: 71 additions & 18 deletions compiler/rustc_resolve/src/def_collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::Resolver;
use crate::{ImplTraitContext, Resolver};
use rustc_ast::visit::{self, FnKind};
use rustc_ast::walk_list;
use rustc_ast::*;
Expand All @@ -16,14 +16,15 @@ crate fn collect_definitions(
fragment: &AstFragment,
expansion: ExpnId,
) {
let parent_def = resolver.invocation_parents[&expansion];
fragment.visit_with(&mut DefCollector { resolver, parent_def, expansion });
let (parent_def, impl_trait_context) = resolver.invocation_parents[&expansion];
fragment.visit_with(&mut DefCollector { resolver, parent_def, expansion, impl_trait_context });
}

/// Creates `DefId`s for nodes in the AST.
struct DefCollector<'a, 'b> {
resolver: &'a mut Resolver<'b>,
parent_def: LocalDefId,
impl_trait_context: ImplTraitContext,
expansion: ExpnId,
}

Expand All @@ -40,6 +41,16 @@ impl<'a, 'b> DefCollector<'a, 'b> {
self.parent_def = orig_parent_def;
}

fn with_impl_trait<F: FnOnce(&mut Self)>(
&mut self,
impl_trait_context: ImplTraitContext,
f: F,
) {
let orig_itc = std::mem::replace(&mut self.impl_trait_context, impl_trait_context);
f(self);
self.impl_trait_context = orig_itc;
}

fn collect_field(&mut self, field: &'a StructField, index: Option<usize>) {
let index = |this: &Self| {
index.unwrap_or_else(|| {
Expand All @@ -60,8 +71,9 @@ impl<'a, 'b> DefCollector<'a, 'b> {
}

fn visit_macro_invoc(&mut self, id: NodeId) {
let id = id.placeholder_to_expn_id();
let old_parent =
self.resolver.invocation_parents.insert(id.placeholder_to_expn_id(), self.parent_def);
self.resolver.invocation_parents.insert(id, (self.parent_def, self.impl_trait_context));
assert!(old_parent.is_none(), "parent `LocalDefId` is reset for an invocation");
}
}
Expand Down Expand Up @@ -103,29 +115,37 @@ impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> {
let def = self.create_def(i.id, def_data, i.span);

self.with_parent(def, |this| {
match i.kind {
ItemKind::Struct(ref struct_def, _) | ItemKind::Union(ref struct_def, _) => {
// If this is a unit or tuple-like struct, register the constructor.
if let Some(ctor_hir_id) = struct_def.ctor_id() {
this.create_def(ctor_hir_id, DefPathData::Ctor, i.span);
this.with_impl_trait(ImplTraitContext::Existential, |this| {
match i.kind {
ItemKind::Struct(ref struct_def, _) | ItemKind::Union(ref struct_def, _) => {
// If this is a unit or tuple-like struct, register the constructor.
if let Some(ctor_hir_id) = struct_def.ctor_id() {
this.create_def(ctor_hir_id, DefPathData::Ctor, i.span);
}
}
_ => {}
}
_ => {}
}
visit::walk_item(this, i);
visit::walk_item(this, i);
})
});
}

fn visit_fn(&mut self, fn_kind: FnKind<'a>, span: Span, _: NodeId) {
if let FnKind::Fn(_, _, sig, _, body) = fn_kind {
if let Async::Yes { closure_id, return_impl_trait_id, .. } = sig.header.asyncness {
self.create_def(return_impl_trait_id, DefPathData::ImplTrait, span);
let return_impl_trait_id =
self.create_def(return_impl_trait_id, DefPathData::ImplTrait, span);

// For async functions, we need to create their inner defs inside of a
// closure to match their desugared representation. Besides that,
// we must mirror everything that `visit::walk_fn` below does.
self.visit_fn_header(&sig.header);
visit::walk_fn_decl(self, &sig.decl);
for param in &sig.decl.inputs {
self.visit_param(param);
}
self.with_parent(return_impl_trait_id, |this| {
this.visit_fn_ret_ty(&sig.decl.output)
});
let closure_def = self.create_def(closure_id, DefPathData::ClosureExpr, span);
self.with_parent(closure_def, |this| walk_list!(this, visit_block, body));
return;
Expand All @@ -137,6 +157,14 @@ impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> {

fn visit_use_tree(&mut self, use_tree: &'a UseTree, id: NodeId, _nested: bool) {
self.create_def(id, DefPathData::Misc, use_tree.span);
match use_tree.kind {
UseTreeKind::Simple(_, id1, id2) => {
self.create_def(id1, DefPathData::Misc, use_tree.prefix.span);
self.create_def(id2, DefPathData::Misc, use_tree.prefix.span);
}
UseTreeKind::Glob => (),
UseTreeKind::Nested(..) => {}
}
visit::walk_use_tree(self, use_tree, id);
}

Expand Down Expand Up @@ -191,7 +219,15 @@ impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> {
};
self.create_def(param.id, def_path_data, param.ident.span);

visit::walk_generic_param(self, param);
// impl-Trait can happen inside generic parameters, like
// ```
// fn foo<U: Iterator<Item = impl Clone>>() {}
// ```
//
// In that case, the impl-trait is lowered as an additional generic parameter.
self.with_impl_trait(ImplTraitContext::Universal(self.parent_def), |this| {
visit::walk_generic_param(this, param)
});
}

fn visit_assoc_item(&mut self, i: &'a AssocItem, ctxt: visit::AssocCtxt) {
Expand Down Expand Up @@ -244,8 +280,19 @@ impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> {
match ty.kind {
TyKind::MacCall(..) => self.visit_macro_invoc(ty.id),
TyKind::ImplTrait(node_id, _) => {
let parent_def = self.create_def(node_id, DefPathData::ImplTrait, ty.span);
self.with_parent(parent_def, |this| visit::walk_ty(this, ty));
let parent_def = match self.impl_trait_context {
ImplTraitContext::Universal(item_def) => self.resolver.create_def(
item_def,
node_id,
DefPathData::ImplTrait,
self.expansion,
ty.span,
),
ImplTraitContext::Existential => {
self.create_def(node_id, DefPathData::ImplTrait, ty.span)
}
};
self.with_parent(parent_def, |this| visit::walk_ty(this, ty))
}
_ => visit::walk_ty(self, ty),
}
Expand Down Expand Up @@ -275,7 +322,13 @@ impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> {
}

fn visit_param(&mut self, p: &'a Param) {
if p.is_placeholder { self.visit_macro_invoc(p.id) } else { visit::walk_param(self, p) }
if p.is_placeholder {
self.visit_macro_invoc(p.id)
} else {
self.with_impl_trait(ImplTraitContext::Universal(self.parent_def), |this| {
visit::walk_param(this, p)
})
}
}

// This method is called only when we are visiting an individual field
Expand Down
13 changes: 10 additions & 3 deletions compiler/rustc_resolve/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,12 @@ impl<'a> ParentScope<'a> {
}
}

#[derive(Copy, Debug, Clone)]
enum ImplTraitContext {
Existential,
Universal(LocalDefId),
}

#[derive(Eq)]
struct BindingError {
name: Symbol,
Expand Down Expand Up @@ -989,8 +995,9 @@ pub struct Resolver<'a> {
/// Indices of unnamed struct or variant fields with unresolved attributes.
placeholder_field_indices: FxHashMap<NodeId, usize>,
/// When collecting definitions from an AST fragment produced by a macro invocation `ExpnId`
/// we know what parent node that fragment should be attached to thanks to this table.
invocation_parents: FxHashMap<ExpnId, LocalDefId>,
/// we know what parent node that fragment should be attached to thanks to this table,
/// and how the `impl Trait` fragments were introduced.
invocation_parents: FxHashMap<ExpnId, (LocalDefId, ImplTraitContext)>,

next_disambiguator: FxHashMap<(LocalDefId, DefPathData), u32>,
/// Some way to know that we are in a *trait* impl in `visit_assoc_item`.
Expand Down Expand Up @@ -1205,7 +1212,7 @@ impl<'a> Resolver<'a> {
node_id_to_def_id.insert(CRATE_NODE_ID, root);

let mut invocation_parents = FxHashMap::default();
invocation_parents.insert(ExpnId::root(), root);
invocation_parents.insert(ExpnId::root(), (root, ImplTraitContext::Existential));

let mut extern_prelude: FxHashMap<Ident, ExternPreludeEntry<'_>> = session
.opts
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_resolve/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ impl<'a> ResolverExpand for Resolver<'a> {
// nearest closing item - we should try to return the closest parent of the ExpnId
self.invocation_parents
.get(&expn_id)
.map_or(ast::CRATE_NODE_ID, |id| self.def_id_to_node_id[*id])
.map_or(ast::CRATE_NODE_ID, |id| self.def_id_to_node_id[id.0])
}

fn has_derive_copy(&self, expn_id: ExpnId) -> bool {
Expand Down
3 changes: 0 additions & 3 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,6 @@
59| 1| used_only_from_this_lib_crate_generic_function(some_vec);
60| 1| used_only_from_this_lib_crate_generic_function("used ONLY from library used_crate.rs");
61| 1|}
------------------
| Unexecuted instantiation: used_crate::use_this_lib_crate
------------------
62| |
63| |// FIXME(#79651): `used_from_bin_crate_and_lib_crate_generic_function()` is covered and executed
64| |// `2` times, but the coverage output also shows (at the bottom of the coverage report):
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/consts/miri_unleashed/tls.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ error[E0080]: could not evaluate static initializer
--> $DIR/tls.rs:12:25
|
LL | unsafe { let _val = A; }
| ^ cannot access thread local static (DefId(0:4 ~ tls[317d]::A))
| ^ cannot access thread local static (DefId(0:6 ~ tls[317d]::A))

error[E0080]: could not evaluate static initializer
--> $DIR/tls.rs:19:26
|
LL | unsafe { let _val = &A; }
| ^ cannot access thread local static (DefId(0:4 ~ tls[317d]::A))
| ^ cannot access thread local static (DefId(0:6 ~ tls[317d]::A))

warning: skipping const checks
|
Expand Down
Loading