Merge #7541

7541: Use block_def_map in body lowering (third time's the charm) r=jonas-schievink a=jonas-schievink

After https://github.com/rust-analyzer/rust-analyzer/pull/7380 and https://github.com/rust-analyzer/rust-analyzer/pull/7506 both had to be reverted, this should have finally resolved all remaining bugs.

Most importantly, the optimization to skip `block_def_map` computation when the block contains no inner items was fixed (which fortunately was simpler than expected).

I've ran `analysis-stats` on libstd locally, which works fine, and also ran this PR locally for a short while without issues.

Note that this *still* has no (or almost no) user-facing impact, because the rest of r-a still relies on some local item support hacks.

bors r+

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

crates/hir_def/src/body.rs

@@ -46,7 +46,7 @@ pub(crate) struct CfgExpander {
 
 pub(crate) struct Expander {
     cfg_expander: CfgExpander,
-    crate_def_map: Arc<DefMap>,
+    def_map: Arc<DefMap>,
     current_file_id: HirFileId,
     ast_id_map: Arc<AstIdMap>,
     module: ModuleId,
@@ -91,7 +91,7 @@ impl Expander {
         let ast_id_map = db.ast_id_map(current_file_id);
         Expander {
             cfg_expander,
-            crate_def_map,
+            def_map: crate_def_map,
             current_file_id,
             ast_id_map,
             module,
@@ -102,7 +102,6 @@ impl Expander {
     pub(crate) fn enter_expand<T: ast::AstNode>(
         &mut self,
         db: &dyn DefDatabase,
-        local_scope: Option<&ItemScope>,
         macro_call: ast::MacroCall,
     ) -> ExpandResult<Option<(Mark, T)>> {
         if self.recursion_limit + 1 > EXPANSION_RECURSION_LIMIT {
@@ -112,18 +111,12 @@ impl Expander {
 
         let macro_call = InFile::new(self.current_file_id, &macro_call);
 
-        let resolver = |path: ModPath| -> Option<MacroDefId> {
-            if let Some(local_scope) = local_scope {
-                if let Some(def) = path.as_ident().and_then(|n| local_scope.get_legacy_macro(n)) {
-                    return Some(def);
-                }
-            }
-            self.resolve_path_as_macro(db, &path)
-        };
+        let resolver =
+            |path: ModPath| -> Option<MacroDefId> { self.resolve_path_as_macro(db, &path) };
 
         let mut err = None;
         let call_id =
-            macro_call.as_call_id_with_errors(db, self.crate_def_map.krate(), resolver, &mut |e| {
+            macro_call.as_call_id_with_errors(db, self.def_map.krate(), resolver, &mut |e| {
                 err.get_or_insert(e);
             });
         let call_id = match call_id {
@@ -204,7 +197,7 @@ impl Expander {
     }
 
     fn resolve_path_as_macro(&self, db: &dyn DefDatabase, path: &ModPath) -> Option<MacroDefId> {
-        self.crate_def_map
+        self.def_map
             .resolve_path(db, self.module.local_id, path, BuiltinShadowMode::Other)
             .0
             .take_macros()

crates/hir_def/src/body/lower.rs

@@ -1,7 +1,7 @@
 //! Transforms `ast::Expr` into an equivalent `hir_def::expr::Expr`
 //! representation.
 
-use std::{any::type_name, sync::Arc};
+use std::{any::type_name, mem, sync::Arc};
 
 use either::Either;
 use hir_expand::{
@@ -36,8 +36,8 @@ use crate::{
     item_tree::{ItemTree, ItemTreeId, ItemTreeNode},
     path::{GenericArgs, Path},
     type_ref::{Mutability, Rawness, TypeRef},
-    AdtId, ConstLoc, ContainerId, DefWithBodyId, EnumLoc, FunctionLoc, Intern, ModuleDefId,
-    StaticLoc, StructLoc, TraitLoc, TypeAliasLoc, UnionLoc,
+    AdtId, BlockLoc, ConstLoc, ContainerId, DefWithBodyId, EnumLoc, FunctionLoc, Intern,
+    ModuleDefId, StaticLoc, StructLoc, TraitLoc, TypeAliasLoc, UnionLoc,
 };
 
 use super::{diagnostics::BodyDiagnostic, ExprSource, PatSource};
@@ -152,8 +152,8 @@ impl ExprCollector<'_> {
     fn alloc_expr_desugared(&mut self, expr: Expr) -> ExprId {
         self.make_expr(expr, Err(SyntheticSyntax))
     }
-    fn empty_block(&mut self) -> ExprId {
-        self.alloc_expr_desugared(Expr::Block { statements: Vec::new(), tail: None, label: None })
+    fn unit(&mut self) -> ExprId {
+        self.alloc_expr_desugared(Expr::Tuple { exprs: Vec::new() })
     }
     fn missing_expr(&mut self) -> ExprId {
         self.alloc_expr_desugared(Expr::Missing)
@@ -222,7 +222,7 @@ impl ExprCollector<'_> {
                                 MatchArm { pat, expr: then_branch, guard: None },
                                 MatchArm {
                                     pat: placeholder_pat,
-                                    expr: else_branch.unwrap_or_else(|| self.empty_block()),
+                                    expr: else_branch.unwrap_or_else(|| self.unit()),
                                     guard: None,
                                 },
                             ];
@@ -561,7 +561,7 @@ impl ExprCollector<'_> {
         let outer_file = self.expander.current_file_id;
 
         let macro_call = self.expander.to_source(AstPtr::new(&e));
-        let res = self.expander.enter_expand(self.db, Some(&self.body.item_scope), e);
+        let res = self.expander.enter_expand(self.db, e);
 
         match &res.err {
             Some(ExpandError::UnresolvedProcMacro) => {
@@ -697,12 +697,30 @@ impl ExprCollector<'_> {
     }
 
     fn collect_block(&mut self, block: ast::BlockExpr) -> ExprId {
-        let syntax_node_ptr = AstPtr::new(&block.clone().into());
+        let ast_id = self.expander.ast_id(&block);
+        let block_loc = BlockLoc { ast_id, module: self.expander.module };
+        let block_id = self.db.intern_block(block_loc);
+        let opt_def_map = self.db.block_def_map(block_id);
+        let has_def_map = opt_def_map.is_some();
+        let def_map = opt_def_map.unwrap_or_else(|| self.expander.def_map.clone());
+        let module =
+            if has_def_map { def_map.module_id(def_map.root()) } else { self.expander.module };
+        let prev_def_map = mem::replace(&mut self.expander.def_map, def_map);
+        let prev_module = mem::replace(&mut self.expander.module, module);
+
         self.collect_stmts_items(block.statements());
         let statements =
             block.statements().filter_map(|s| self.collect_stmt(s)).flatten().collect();
         let tail = block.tail_expr().map(|e| self.collect_expr(e));
-        self.alloc_expr(Expr::Block { statements, tail, label: None }, syntax_node_ptr)
+        let syntax_node_ptr = AstPtr::new(&block.clone().into());
+        let expr_id = self.alloc_expr(
+            Expr::Block { id: block_id, statements, tail, label: None },
+            syntax_node_ptr,
+        );
+
+        self.expander.def_map = prev_def_map;
+        self.expander.module = prev_module;
+        expr_id
     }
 
     fn collect_stmts_items(&mut self, stmts: ast::AstChildren<ast::Stmt>) {
@@ -832,7 +850,7 @@ impl ExprCollector<'_> {
                 if annotation == BindingAnnotation::Unannotated && subpat.is_none() {
                     // This could also be a single-segment path pattern. To
                     // decide that, we need to try resolving the name.
-                    let (resolved, _) = self.expander.crate_def_map.resolve_path(
+                    let (resolved, _) = self.expander.def_map.resolve_path(
                         self.db,
                         self.expander.module.local_id,
                         &name.clone().into(),

crates/hir_def/src/body/tests.rs

@@ -1,7 +1,10 @@
-use base_db::{fixture::WithFixture, SourceDatabase};
+mod block;
+
+use base_db::{fixture::WithFixture, FilePosition, SourceDatabase};
+use expect_test::Expect;
 use test_utils::mark;
 
-use crate::{test_db::TestDB, ModuleDefId};
+use crate::{test_db::TestDB, BlockId, ModuleDefId};
 
 use super::*;
 
@@ -31,6 +34,115 @@ fn check_diagnostics(ra_fixture: &str) {
     db.check_diagnostics();
 }
 
+fn block_def_map_at(ra_fixture: &str) -> Arc<DefMap> {
+    let (db, position) = crate::test_db::TestDB::with_position(ra_fixture);
+
+    let krate = db.crate_graph().iter().next().unwrap();
+    let def_map = db.crate_def_map(krate);
+
+    let mut block =
+        block_at_pos(&db, &def_map, position).expect("couldn't find enclosing function or block");
+    loop {
+        let def_map = db.block_def_map(block).unwrap_or_else(|| def_map.clone());
+        let new_block = block_at_pos(&db, &def_map, position);
+        match new_block {
+            Some(new_block) => {
+                assert_ne!(block, new_block);
+                block = new_block;
+            }
+            None => {
+                return def_map;
+            }
+        }
+    }
+}
+
+fn block_at_pos(db: &dyn DefDatabase, def_map: &DefMap, position: FilePosition) -> Option<BlockId> {
+    // Find the smallest (innermost) function containing the cursor.
+    let mut size = None;
+    let mut fn_def = None;
+    for (_, module) in def_map.modules() {
+        let file_id = module.definition_source(db).file_id;
+        if file_id != position.file_id.into() {
+            continue;
+        }
+        let root = db.parse_or_expand(file_id).unwrap();
+        let ast_map = db.ast_id_map(file_id);
+        let item_tree = db.item_tree(file_id);
+        for decl in module.scope.declarations() {
+            if let ModuleDefId::FunctionId(it) = decl {
+                let ast = ast_map.get(item_tree[it.lookup(db).id.value].ast_id).to_node(&root);
+                let range = ast.syntax().text_range();
+
+                if !range.contains(position.offset) {
+                    continue;
+                }
+
+                let new_size = match size {
+                    None => range.len(),
+                    Some(size) => {
+                        if range.len() < size {
+                            range.len()
+                        } else {
+                            size
+                        }
+                    }
+                };
+                if size != Some(new_size) {
+                    size = Some(new_size);
+                    fn_def = Some(it);
+                }
+            }
+        }
+    }
+
+    let (body, source_map) = db.body_with_source_map(fn_def?.into());
+
+    // Now find the smallest encompassing block expression in the function body.
+    let mut size = None;
+    let mut block_id = None;
+    for (expr_id, expr) in body.exprs.iter() {
+        if let Expr::Block { id, .. } = expr {
+            if let Ok(ast) = source_map.expr_syntax(expr_id) {
+                if ast.file_id != position.file_id.into() {
+                    continue;
+                }
+
+                let root = db.parse_or_expand(ast.file_id).unwrap();
+                let ast = ast.value.to_node(&root);
+                let range = ast.syntax().text_range();
+
+                if !range.contains(position.offset) {
+                    continue;
+                }
+
+                let new_size = match size {
+                    None => range.len(),
+                    Some(size) => {
+                        if range.len() < size {
+                            range.len()
+                        } else {
+                            size
+                        }
+                    }
+                };
+                if size != Some(new_size) {
+                    size = Some(new_size);
+                    block_id = Some(*id);
+                }
+            }
+        }
+    }
+
+    Some(block_id.expect("can't find block containing cursor"))
+}
+
+fn check_at(ra_fixture: &str, expect: Expect) {
+    let def_map = block_def_map_at(ra_fixture);
+    let actual = def_map.dump();
+    expect.assert_eq(&actual);
+}
+
 #[test]
 fn your_stack_belongs_to_me() {
     mark::check!(your_stack_belongs_to_me);

crates/hir_def/src/body/tests/block.rs

@@ -1,4 +1,5 @@
 use super::*;
+use expect_test::expect;
 
 #[test]
 fn inner_item_smoke() {
@@ -184,3 +185,36 @@ pub mod mark {
         "#]],
     );
 }
+
+#[test]
+fn macro_resolve_legacy() {
+    check_at(
+        r#"
+//- /lib.rs
+mod module;
+
+//- /module.rs
+macro_rules! m {
+    () => {
+        struct Def {}
+    };
+}
+
+fn f() {
+    {
+        m!();
+        $0
+    }
+}
+        "#,
+        expect![[r#"
+            block scope
+            Def: t
+            crate
+            module: t
+
+            crate::module
+            f: v
+        "#]],
+    )
+}

crates/hir_def/src/data.rs

@@ -262,7 +262,7 @@ fn collect_items(
                 let root = db.parse_or_expand(file_id).unwrap();
                 let call = ast_id_map.get(call.ast_id).to_node(&root);
 
-                if let Some((mark, mac)) = expander.enter_expand(db, None, call).value {
+                if let Some((mark, mac)) = expander.enter_expand(db, call).value {
                     let src: InFile<ast::MacroItems> = expander.to_source(mac);
                     let item_tree = db.item_tree(src.file_id);
                     let iter =

crates/hir_def/src/db.rs

@@ -59,7 +59,7 @@ pub trait DefDatabase: InternDatabase + AstDatabase + Upcast<dyn AstDatabase> {
     fn crate_def_map_query(&self, krate: CrateId) -> Arc<DefMap>;
 
     #[salsa::invoke(DefMap::block_def_map_query)]
-    fn block_def_map(&self, block: BlockId) -> Arc<DefMap>;
+    fn block_def_map(&self, block: BlockId) -> Option<Arc<DefMap>>;
 
     #[salsa::invoke(StructData::struct_data_query)]
     fn struct_data(&self, id: StructId) -> Arc<StructData>;

crates/hir_def/src/expr.rs

@@ -20,6 +20,7 @@ use crate::{
     builtin_type::{BuiltinFloat, BuiltinInt},
     path::{GenericArgs, Path},
     type_ref::{Mutability, Rawness, TypeRef},
+    BlockId,
 };
 
 pub type ExprId = Idx<Expr>;
@@ -56,6 +57,7 @@ pub enum Expr {
         else_branch: Option<ExprId>,
     },
     Block {
+        id: BlockId,
         statements: Vec<Statement>,
         tail: Option<ExprId>,
         label: Option<LabelId>,

crates/hir_def/src/item_tree.rs

@@ -24,7 +24,7 @@ use la_arena::{Arena, Idx, RawIdx};
 use profile::Count;
 use rustc_hash::FxHashMap;
 use smallvec::SmallVec;
-use syntax::{ast, match_ast};
+use syntax::{ast, match_ast, SyntaxKind};
 use test_utils::mark;
 
 use crate::{
@@ -80,6 +80,10 @@ impl ItemTree {
     pub(crate) fn item_tree_query(db: &dyn DefDatabase, file_id: HirFileId) -> Arc<ItemTree> {
         let _p = profile::span("item_tree_query").detail(|| format!("{:?}", file_id));
         let syntax = if let Some(node) = db.parse_or_expand(file_id) {
+            if node.kind() == SyntaxKind::ERROR {
+                // FIXME: not 100% sure why these crop up, but return an empty tree to avoid a panic
+                return Default::default();
+            }
             node
         } else {
             return Default::default();

crates/hir_def/src/lib.rs

@@ -81,7 +81,13 @@ 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),
+            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.
+                    panic!("no `block_def_map` for `ModuleId` {:?}", self);
+                })
+            }
             None => db.crate_def_map(self.krate),
         }
     }
@@ -239,6 +245,7 @@ pub struct BlockId(salsa::InternId);
 #[derive(Debug, Hash, PartialEq, Eq, Clone)]
 pub struct BlockLoc {
     ast_id: AstId<ast::BlockExpr>,
+    /// The containing module.
     module: ModuleId,
 }
 impl_intern!(BlockId, BlockLoc, intern_block, lookup_intern_block);

crates/hir_def/src/nameres.rs

@@ -197,12 +197,17 @@ impl DefMap {
         Arc::new(def_map)
     }
 
-    pub(crate) fn block_def_map_query(db: &dyn DefDatabase, block_id: BlockId) -> Arc<DefMap> {
+    pub(crate) fn block_def_map_query(
+        db: &dyn DefDatabase,
+        block_id: BlockId,
+    ) -> Option<Arc<DefMap>> {
         let block: BlockLoc = db.lookup_intern_block(block_id);
         let parent = block.module.def_map(db);
 
-        // FIXME: It would be good to just return the parent map when the block has no items, but
-        // we rely on `def_map.block` in a few places, which is `Some` for the inner `DefMap`.
+        let item_tree = db.item_tree(block.ast_id.file_id);
+        if item_tree.inner_items_of_block(block.ast_id.value).is_empty() {
+            return None;
+        }
 
         let block_info =
             BlockInfo { block: block_id, parent, parent_module: block.module.local_id };
@@ -211,7 +216,7 @@ impl DefMap {
         def_map.block = Some(block_info);
 
         let def_map = collector::collect_defs(db, def_map, Some(block.ast_id));
-        Arc::new(def_map)
+        Some(Arc::new(def_map))
     }
 
     fn empty(krate: CrateId, edition: Edition) -> DefMap {

crates/hir_def/src/nameres/tests.rs

@@ -4,16 +4,14 @@ mod macros;
 mod mod_resolution;
 mod diagnostics;
 mod primitives;
-mod block;
 
 use std::sync::Arc;
 
-use base_db::{fixture::WithFixture, FilePosition, SourceDatabase};
+use base_db::{fixture::WithFixture, SourceDatabase};
 use expect_test::{expect, Expect};
-use syntax::AstNode;
 use test_utils::mark;
 
-use crate::{db::DefDatabase, nameres::*, test_db::TestDB, Lookup};
+use crate::{db::DefDatabase, nameres::*, test_db::TestDB};
 
 fn compute_crate_def_map(ra_fixture: &str) -> Arc<DefMap> {
     let db = TestDB::with_files(ra_fixture);
@@ -21,74 +19,12 @@ fn compute_crate_def_map(ra_fixture: &str) -> Arc<DefMap> {
     db.crate_def_map(krate)
 }
 
-fn compute_block_def_map(ra_fixture: &str) -> Arc<DefMap> {
-    let (db, position) = TestDB::with_position(ra_fixture);
-
-    // FIXME: perhaps we should make this use body lowering tests instead?
-
-    let module = db.module_for_file(position.file_id);
-    let mut def_map = db.crate_def_map(module.krate);
-    while let Some(new_def_map) = descend_def_map_at_position(&db, position, def_map.clone()) {
-        def_map = new_def_map;
-    }
-
-    // FIXME: select the right module, not the root
-
-    def_map
-}
-
-fn descend_def_map_at_position(
-    db: &dyn DefDatabase,
-    position: FilePosition,
-    def_map: Arc<DefMap>,
-) -> Option<Arc<DefMap>> {
-    for (local_id, module_data) in def_map.modules() {
-        let mod_def = module_data.origin.definition_source(db);
-        let ast_map = db.ast_id_map(mod_def.file_id);
-        let item_tree = db.item_tree(mod_def.file_id);
-        let root = db.parse_or_expand(mod_def.file_id).unwrap();
-        for item in module_data.scope.declarations() {
-            match item {
-                ModuleDefId::FunctionId(it) => {
-                    // Technically blocks can be inside any type (due to arrays and const generics),
-                    // and also in const/static initializers. For tests we only really care about
-                    // functions though.
-
-                    let ast = ast_map.get(item_tree[it.lookup(db).id.value].ast_id).to_node(&root);
-
-                    if ast.syntax().text_range().contains(position.offset) {
-                        // Cursor inside function, descend into its body's DefMap.
-                        // Note that we don't handle block *expressions* inside function bodies.
-                        let ast_map = db.ast_id_map(position.file_id.into());
-                        let ast_id = ast_map.ast_id(&ast.body().unwrap());
-                        let block = BlockLoc {
-                            ast_id: InFile::new(position.file_id.into(), ast_id),
-                            module: def_map.module_id(local_id),
-                        };
-                        let block_id = db.intern_block(block);
-                        return Some(db.block_def_map(block_id));
-                    }
-                }
-                _ => continue,
-            }
-        }
-    }
-
-    None
-}
-
 fn check(ra_fixture: &str, expect: Expect) {
     let def_map = compute_crate_def_map(ra_fixture);
     let actual = def_map.dump();
     expect.assert_eq(&actual);
 }
 
-fn check_at(ra_fixture: &str, expect: Expect) {
-    let def_map = compute_block_def_map(ra_fixture);
-    let actual = def_map.dump();
-    expect.assert_eq(&actual);
-}
-
 #[test]
 fn crate_def_map_smoke_test() {
     check(

crates/hir_ty/src/infer/expr.rs

@@ -137,7 +137,7 @@ impl<'a> InferenceContext<'a> {
 
                 self.coerce_merge_branch(&then_ty, &else_ty)
             }
-            Expr::Block { statements, tail, label } => match label {
+            Expr::Block { statements, tail, label, id: _ } => match label {
                 Some(_) => {
                     let break_ty = self.table.new_type_var();
                     self.breakables.push(BreakableContext {