Skip to content

Fix incremental bugs in the HIR map #69015

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/librustc/hir/map/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,15 @@ fn alloc_hir_dep_nodes(
dep_graph: &DepGraph,
hcx: &mut StableHashingContext<'_>,
def_path_hash: DefPathHash,
parent: HirId,
item_like: impl for<'a> HashStable<StableHashingContext<'a>>,
hir_body_nodes: &mut Vec<(DefPathHash, Fingerprint)>,
) -> (DepNodeIndex, DepNodeIndex) {
let sig = dep_graph
.input_task(
def_path_hash.to_dep_node(DepKind::Hir),
&mut *hcx,
HirItemLike { item_like: &item_like, hash_bodies: false },
HirItemLike { item_like: &(parent, &item_like), hash_bodies: false },
)
.1;
let (full, hash) = input_dep_node_and_hash(
Expand Down Expand Up @@ -146,6 +147,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
dep_graph,
&mut hcx,
root_mod_def_path_hash,
CRATE_HIR_ID,
(module, attrs, span),
&mut hir_body_nodes,
)
Expand Down Expand Up @@ -316,6 +318,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
self.dep_graph,
&mut self.hcx,
def_path_hash,
self.parent_node,
item_like,
&mut self.hir_body_nodes,
);
Expand Down
23 changes: 17 additions & 6 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ impl<'hir> Iterator for ParentHirIterator<'_, 'hir> {
}

self.current_id = parent_id;
if let Some(entry) = self.map.find_entry(parent_id) {
if let Some(entry) = self.map.find_and_read_entry(parent_id) {
return Some((parent_id, entry.node));
}
// If this `HirId` doesn't have an `Entry`, skip it and look for its `parent_id`.
Expand Down Expand Up @@ -381,6 +381,12 @@ impl<'hir> Map<'hir> {
self.lookup(id).cloned()
}

fn find_and_read_entry(&self, id: HirId) -> Option<Entry<'hir>> {
let entry = self.find_entry(id);
entry.map(|e| self.dep_graph.read_index(e.dep_node));
entry
}

pub fn item(&self, id: HirId) -> &'hir Item<'hir> {
self.read(id);

Expand Down Expand Up @@ -414,15 +420,15 @@ impl<'hir> Map<'hir> {
}

pub fn fn_decl_by_hir_id(&self, hir_id: HirId) -> Option<&'hir FnDecl<'hir>> {
if let Some(entry) = self.find_entry(hir_id) {
if let Some(entry) = self.find_and_read_entry(hir_id) {
entry.fn_decl()
} else {
bug!("no entry for hir_id `{}`", hir_id)
}
}

pub fn fn_sig_by_hir_id(&self, hir_id: HirId) -> Option<&'hir FnSig<'hir>> {
if let Some(entry) = self.find_entry(hir_id) {
if let Some(entry) = self.find_and_read_entry(hir_id) {
entry.fn_sig()
} else {
bug!("no entry for hir_id `{}`", hir_id)
Expand Down Expand Up @@ -612,7 +618,12 @@ impl<'hir> Map<'hir> {
if self.dep_graph.is_fully_enabled() {
let hir_id_owner = hir_id.owner;
let def_path_hash = self.definitions.def_path_hash(hir_id_owner);
self.dep_graph.read(def_path_hash.to_dep_node(DepKind::HirBody));
let kind = if hir_id.local_id == ItemLocalId::from_u32_const(0) {
DepKind::Hir
} else {
DepKind::HirBody
};
self.dep_graph.read(def_path_hash.to_dep_node(kind));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this logic is supposed to do exactly (before or after the change)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is supposed to add a read to the node which reveals the parent of the HirId.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I understand the original thinking now: The fingerprint of HirBody contains the information of the entire item (including the "signature" parts) so adding a dependency to HirBody is the conservative choice.

But as you discovered the parent is not hashed into any of the two dep-nodes, so we are clearly missing dependencies here.

I assume that the if hir_id.local_id == ItemLocalId::from_u32_const(0) check is based on the assumption that the parent of any node except the item root is part of the same HIR item and thus the HirBody fingerprint contains the appropriate information? If that's the case it is far from obvious and a clarifying comment will be much appreciated by anybody trying to understand this code.

}

self.find_entry(hir_id).and_then(|x| x.parent_node()).unwrap_or(hir_id)
Expand Down Expand Up @@ -654,7 +665,7 @@ impl<'hir> Map<'hir> {

/// Wether `hir_id` corresponds to a `mod` or a crate.
pub fn is_hir_id_module(&self, hir_id: HirId) -> bool {
match self.lookup(hir_id) {
match self.find_and_read_entry(hir_id) {
Some(Entry { node: Node::Item(Item { kind: ItemKind::Mod(_), .. }), .. })
| Some(Entry { node: Node::Crate, .. }) => true,
_ => false,
Expand Down Expand Up @@ -1150,7 +1161,7 @@ impl<'a> NodesMatchingSuffix<'a> {
}

fn matches_suffix(&self, hir: HirId) -> bool {
let name = match self.map.find_entry(hir).map(|entry| entry.node) {
let name = match self.map.find_and_read_entry(hir).map(|entry| entry.node) {
Some(Node::Item(n)) => n.name(),
Some(Node::ForeignItem(n)) => n.name(),
Some(Node::TraitItem(n)) => n.name(),
Expand Down
11 changes: 10 additions & 1 deletion src/librustc/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ pub mod map;

use crate::ty::query::Providers;
use crate::ty::TyCtxt;
use rustc_hir::def_id::LOCAL_CRATE;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::print;
use rustc_hir::Crate;
use rustc_hir::HirId;
use std::ops::Deref;

/// A wrapper type which allows you to access HIR.
Expand Down Expand Up @@ -46,9 +47,17 @@ impl<'tcx> TyCtxt<'tcx> {
pub fn hir(self) -> Hir<'tcx> {
Hir { tcx: self, map: &self.hir_map }
}

pub fn hir_id_parent_module(self, id: HirId) -> DefId {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe parent_hir_module would be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe renaming the query to parent_module_from_def_id and letting this take parent_module is a better option?

self.parent_module(DefId::local(id.owner))
}
}

pub fn provide(providers: &mut Providers<'_>) {
providers.parent_module = |tcx, id| {
let hir = tcx.hir();
hir.get_module_parent(hir.as_local_hir_id(id).unwrap())
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this function get_module_parent and get_module_parent_node less public? The query is always the better choice, right?

};
providers.hir_crate = |tcx, _| tcx.hir_map.untracked_krate();
map::provide(providers);
}
4 changes: 4 additions & 0 deletions src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ rustc_queries! {
eval_always
desc { "computing the lint levels for items in this crate" }
}

query parent_module(_: DefId) -> DefId {
eval_always
}
}

Codegen {
Expand Down
6 changes: 2 additions & 4 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,7 @@ impl Visibility {
Res::Err => Visibility::Public,
def => Visibility::Restricted(def.def_id()),
},
hir::VisibilityKind::Inherited => {
Visibility::Restricted(tcx.hir().get_module_parent(id))
}
hir::VisibilityKind::Inherited => Visibility::Restricted(tcx.hir_id_parent_module(id)),
}
}

Expand Down Expand Up @@ -2968,7 +2966,7 @@ impl<'tcx> TyCtxt<'tcx> {
Some(actual_expansion) => {
self.hir().definitions().parent_module_of_macro_def(actual_expansion)
}
None => self.hir().get_module_parent(block),
None => self.hir_id_parent_module(block),
};
(ident, scope)
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_lint/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
plural_len: usize,
) -> bool {
if ty.is_unit()
|| cx.tcx.is_ty_uninhabited_from(cx.tcx.hir().get_module_parent(expr.hir_id), ty)
|| cx.tcx.is_ty_uninhabited_from(cx.tcx.hir_id_parent_module(expr.hir_id), ty)
{
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir_build/hair/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ impl<'tcx> MatchVisitor<'_, 'tcx> {
}

fn check_in_cx(&self, hir_id: HirId, f: impl FnOnce(MatchCheckCtxt<'_, 'tcx>)) {
let module = self.tcx.hir().get_module_parent(hir_id);
let module = self.tcx.hir_id_parent_module(hir_id);
MatchCheckCtxt::create_and_enter(self.tcx, self.param_env, module, |cx| f(cx));
}

Expand Down
4 changes: 2 additions & 2 deletions src/librustc_passes/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1125,7 +1125,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
}

hir::ExprKind::Call(ref f, ref args) => {
let m = self.ir.tcx.hir().get_module_parent(expr.hir_id);
let m = self.ir.tcx.hir_id_parent_module(expr.hir_id);
let succ = if self.ir.tcx.is_ty_uninhabited_from(m, self.tables.expr_ty(expr)) {
self.s.exit_ln
} else {
Expand All @@ -1136,7 +1136,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
}

hir::ExprKind::MethodCall(.., ref args) => {
let m = self.ir.tcx.hir().get_module_parent(expr.hir_id);
let m = self.ir.tcx.hir_id_parent_module(expr.hir_id);
let succ = if self.ir.tcx.is_ty_uninhabited_from(m, self.tables.expr_ty(expr)) {
self.s.exit_ln
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_privacy/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ fn def_id_visibility<'tcx>(
}
Node::Expr(expr) => {
return (
ty::Visibility::Restricted(tcx.hir().get_module_parent(expr.hir_id)),
ty::Visibility::Restricted(tcx.hir_id_parent_module(expr.hir_id)),
expr.span,
"private",
);
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_typeck/check/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
});

if let Some((field, field_ty)) = field_receiver {
let scope = self.tcx.hir().get_module_parent(self.body_id);
let scope = self.tcx.hir_id_parent_module(self.body_id);
let is_accessible = field.vis.is_accessible_from(scope, self.tcx);

if is_accessible {
Expand Down Expand Up @@ -686,7 +686,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
mut msg: String,
candidates: Vec<DefId>,
) {
let module_did = self.tcx.hir().get_module_parent(self.body_id);
let module_did = self.tcx.hir_id_parent_module(self.body_id);
let module_id = self.tcx.hir().as_local_hir_id(module_did).unwrap();
let krate = self.tcx.hir().krate();
let (span, found_use) = UsePlacementFinder::check(self.tcx, krate, module_id);
Expand Down
19 changes: 11 additions & 8 deletions src/test/incremental/ich_nested_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,21 @@
#![crate_type = "rlib"]
#![feature(rustc_attrs)]

#[rustc_clean(label="Hir", cfg="cfail2")]
#[rustc_dirty(label="HirBody", cfg="cfail2")]
#[rustc_clean(label = "Hir", cfg = "cfail2")]
#[rustc_dirty(label = "HirBody", cfg = "cfail2")]
pub fn foo() {
#[cfg(cfail1)]
pub fn baz() { } // order is different...
pub fn baz() {} // order is different...

#[rustc_clean(label="Hir", cfg="cfail2")]
#[rustc_clean(label="HirBody", cfg="cfail2")]
pub fn bar() { } // but that doesn't matter.
// FIXME: Make "Hir" use `rustc_clean` here. Currently "Hir" includes a reference to
// the parent node, which is the statement holding this item. Changing the position of
// `bar` in `foo` will update that reference and make `Hir(bar)` dirty.
#[rustc_dirty(label = "Hir", cfg = "cfail2")]
#[rustc_clean(label = "HirBody", cfg = "cfail2")]
pub fn bar() {} // but that doesn't matter.

#[cfg(cfail2)]
pub fn baz() { } // order is different...
pub fn baz() {} // order is different...

pub fn bap() { } // neither does adding a new item
pub fn bap() {} // neither does adding a new item
}