Skip to content

internal: Make block_def_map infallible #14574

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

Merged
merged 1 commit into from
Apr 14, 2023
Merged
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
4 changes: 1 addition & 3 deletions crates/hir-def/src/body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,9 +461,7 @@ impl Body {
&'a self,
db: &'a dyn DefDatabase,
) -> impl Iterator<Item = (BlockId, Arc<DefMap>)> + '_ {
self.block_scopes
.iter()
.map(move |&block| (block, db.block_def_map(block).expect("block ID without DefMap")))
self.block_scopes.iter().map(move |&block| (block, db.block_def_map(block)))
}

pub fn pretty_print(&self, db: &dyn DefDatabase, owner: DefWithBodyId) -> String {
Expand Down
17 changes: 8 additions & 9 deletions crates/hir-def/src/body/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -948,15 +948,14 @@ impl ExprCollector<'_> {
None
};

let (module, def_map) = match block_id
.and_then(|block_id| self.db.block_def_map(block_id).zip(Some(block_id)))
{
Some((def_map, block_id)) => {
self.body.block_scopes.push(block_id);
(def_map.root(), def_map)
}
None => (self.expander.module, self.expander.def_map.clone()),
};
let (module, def_map) =
match block_id.map(|block_id| (self.db.block_def_map(block_id), block_id)) {
Some((def_map, block_id)) => {
self.body.block_scopes.push(block_id);
(def_map.root(), def_map)
}
None => (self.expander.module, self.expander.def_map.clone()),
};
let prev_def_map = mem::replace(&mut self.expander.def_map, def_map);
let prev_local_module = mem::replace(&mut self.expander.module, module);

Expand Down
2 changes: 1 addition & 1 deletion crates/hir-def/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ pub trait DefDatabase: InternDatabase + ExpandDatabase + Upcast<dyn ExpandDataba
// FIXME: This actually can't return None anymore as we no longer allocate block scopes for
// non item declaring blocks
#[salsa::invoke(DefMap::block_def_map_query)]
fn block_def_map(&self, block: BlockId) -> Option<Arc<DefMap>>;
fn block_def_map(&self, block: BlockId) -> Arc<DefMap>;

#[salsa::invoke(StructData::struct_data_query)]
fn struct_data(&self, id: StructId) -> Arc<StructData>;
Expand Down
8 changes: 1 addition & 7 deletions crates/hir-def/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,7 @@ pub struct ModuleId {
impl ModuleId {
pub fn def_map(&self, db: &dyn db::DefDatabase) -> Arc<DefMap> {
match self.block {
Some(block) => {
db.block_def_map(block).unwrap_or_else(|| {
// NOTE: This should be unreachable - all `ModuleId`s come from their `DefMap`s,
// so the `DefMap` here must exist.
unreachable!("no `block_def_map` for `ModuleId` {:?}", self);
})
}
Some(block) => db.block_def_map(block),
None => db.crate_def_map(self.krate),
}
}
Expand Down
11 changes: 2 additions & 9 deletions crates/hir-def/src/nameres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,17 +243,10 @@ impl DefMap {
Arc::new(def_map)
}

pub(crate) fn block_def_map_query(
db: &dyn DefDatabase,
block_id: BlockId,
) -> Option<Arc<DefMap>> {
pub(crate) fn block_def_map_query(db: &dyn DefDatabase, block_id: BlockId) -> Arc<DefMap> {
let block: BlockLoc = db.lookup_intern_block(block_id);

let tree_id = TreeId::new(block.ast_id.file_id, Some(block_id));
let item_tree = tree_id.item_tree(db);
if item_tree.top_level_items().is_empty() {
return None;
}

let parent_map = block.module.def_map(db);
let krate = block.module.krate;
Expand All @@ -269,7 +262,7 @@ impl DefMap {
def_map.block = Some(BlockInfo { block: block_id, parent: block.module });

let def_map = collector::collect_defs(db, def_map, tree_id);
Some(Arc::new(def_map))
Arc::new(def_map)
}

fn empty(krate: CrateId, edition: Edition, module_data: ModuleData) -> DefMap {
Expand Down
28 changes: 12 additions & 16 deletions crates/hir-def/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,15 +572,12 @@ impl Resolver {
scope_id,
}));
if let Some(block) = expr_scopes.block(scope_id) {
if let Some(def_map) = db.block_def_map(block) {
let root = def_map.root();
resolver
.scopes
.push(Scope::BlockScope(ModuleItemMap { def_map, module_id: root }));
// FIXME: This adds as many module scopes as there are blocks, but resolving in each
// already traverses all parents, so this is O(n²). I think we could only store the
// innermost module scope instead?
}
let def_map = db.block_def_map(block);
let root = def_map.root();
resolver.scopes.push(Scope::BlockScope(ModuleItemMap { def_map, module_id: root }));
// FIXME: This adds as many module scopes as there are blocks, but resolving in each
// already traverses all parents, so this is O(n²). I think we could only store the
// innermost module scope instead?
}
}

Expand Down Expand Up @@ -741,13 +738,12 @@ fn resolver_for_scope_(

for scope in scope_chain.into_iter().rev() {
if let Some(block) = scopes.block(scope) {
if let Some(def_map) = db.block_def_map(block) {
let root = def_map.root();
r = r.push_block_scope(def_map, root);
// FIXME: This adds as many module scopes as there are blocks, but resolving in each
// already traverses all parents, so this is O(n²). I think we could only store the
// innermost module scope instead?
}
let def_map = db.block_def_map(block);
let root = def_map.root();
r = r.push_block_scope(def_map, root);
// FIXME: This adds as many module scopes as there are blocks, but resolving in each
// already traverses all parents, so this is O(n²). I think we could only store the
// innermost module scope instead?
}

r = r.push_expr_scope(owner, Arc::clone(&scopes), scope);
Expand Down
8 changes: 3 additions & 5 deletions crates/hir-def/src/test_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,11 @@ impl TestDB {
});

for scope in scope_iter {
let containing_blocks =
let mut containing_blocks =
scopes.scope_chain(Some(scope)).filter_map(|scope| scopes.block(scope));

for block in containing_blocks {
if let Some(def_map) = self.block_def_map(block) {
return Some(def_map);
}
if let Some(block) = containing_blocks.next().map(|block| self.block_def_map(block)) {
return Some(block);
}
}

Expand Down
5 changes: 1 addition & 4 deletions crates/hir-ty/src/chalk_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,7 @@ impl<'a> chalk_solve::RustIrDatabase<Interner> for ChalkContext<'a> {
let impl_maps = [in_deps, in_self];
let block_impls = iter::successors(self.block, |&block_id| {
cov_mark::hit!(block_local_impls);
self.db
.block_def_map(block_id)
.and_then(|map| map.parent())
.and_then(|module| module.containing_block())
self.db.block_def_map(block_id).parent().and_then(|module| module.containing_block())
})
.inspect(|&block_id| {
// make sure we don't search the same block twice
Expand Down
9 changes: 3 additions & 6 deletions crates/hir-ty/src/method_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl TraitImpls {
let _p = profile::span("trait_impls_in_block_query");
let mut impls = Self { map: FxHashMap::default() };

let block_def_map = db.block_def_map(block)?;
let block_def_map = db.block_def_map(block);
impls.collect_def_map(db, &block_def_map);
impls.shrink_to_fit();

Expand Down Expand Up @@ -290,7 +290,7 @@ impl InherentImpls {
let _p = profile::span("inherent_impls_in_block_query");
let mut impls = Self { map: FxHashMap::default(), invalid_impls: Vec::default() };

let block_def_map = db.block_def_map(block)?;
let block_def_map = db.block_def_map(block);
Copy link
Member

Choose a reason for hiding this comment

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

Should this and trait_impls_in_block_query be infallible now?

Copy link
Member Author

Choose a reason for hiding this comment

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

ye they should, didn't notice that

impls.collect_def_map(db, &block_def_map);
impls.shrink_to_fit();

Expand Down Expand Up @@ -1191,10 +1191,7 @@ fn iterate_inherent_methods(
)?;
}

block = db
.block_def_map(block_id)
.and_then(|map| map.parent())
.and_then(|module| module.containing_block());
block = db.block_def_map(block_id).parent().and_then(|module| module.containing_block());
}

for krate in def_crates {
Expand Down