Merge #5081

5081: Fix a panic with malformed inner items r=jonas-schievink a=jonas-schievink

bors r+

Co-authored-by: Jonas Schievink <[email protected]>

2 files changed, 50 insertions, 61 deletions

diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs
index 3ced648e5..a7e2e0982 100644
--- a/crates/ra_hir_def/src/body/lower.rs
+++ b/crates/ra_hir_def/src/body/lower.rs
@@ -5,7 +5,7 @@ use either::Either;
 use hir_expand::{
     hygiene::Hygiene,
     name::{name, AsName, Name},
-    AstId, HirFileId, MacroDefId, MacroDefKind,
+    HirFileId, MacroDefId, MacroDefKind,
 };
 use ra_arena::Arena;
 use ra_syntax::{
@@ -27,7 +27,7 @@ use crate::{
         LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement,
     },
     item_scope::BuiltinShadowMode,
-    item_tree::{FileItemTreeId, ItemTree, ItemTreeNode},
+    item_tree::{ItemTree, ItemTreeId, ItemTreeNode},
     path::{GenericArgs, Path},
     type_ref::{Mutability, Rawness, TypeRef},
     AdtId, ConstLoc, ContainerId, DefWithBodyId, EnumLoc, FunctionLoc, Intern, ModuleDefId,
@@ -37,7 +37,7 @@ use crate::{
 use super::{ExprSource, PatSource};
 use ast::AstChildren;
 use rustc_hash::FxHashMap;
-use std::sync::Arc;
+use std::{any::type_name, sync::Arc};
 
 pub(crate) struct LowerCtx {
     hygiene: Hygiene,
@@ -561,17 +561,30 @@ impl ExprCollector<'_> {
         }
     }
 
-    fn find_inner_item<S: ItemTreeNode>(&self, id: AstId<ast::ModuleItem>) -> FileItemTreeId<S> {
+    fn find_inner_item<N: ItemTreeNode>(&self, ast: &N::Source) -> Option<ItemTreeId<N>> {
+        let id = self.expander.ast_id(ast);
         let tree = &self.item_trees[&id.file_id];
 
         // FIXME: This probably breaks with `use` items, since they produce multiple item tree nodes
 
         // Root file (non-macro).
-        tree.all_inner_items()
+        let item_tree_id = tree
+            .all_inner_items()
             .chain(tree.top_level_items().iter().copied())
-            .filter_map(|mod_item| mod_item.downcast::<S>())
-            .find(|tree_id| tree[*tree_id].ast_id().upcast() == id.value)
-            .unwrap_or_else(|| panic!("couldn't find inner item for {:?}", id))
+            .filter_map(|mod_item| mod_item.downcast::<N>())
+            .find(|tree_id| tree[*tree_id].ast_id().upcast() == id.value.upcast())
+            .or_else(|| {
+                log::debug!(
+                    "couldn't find inner {} item for {:?} (AST: `{}` - {:?})",
+                    type_name::<N>(),
+                    id,
+                    ast.syntax(),
+                    ast.syntax(),
+                );
+                None
+            })?;
+
+        Some(ItemTreeId::new(id.file_id, item_tree_id))
     }
 
     fn collect_expr_opt(&mut self, expr: Option<ast::Expr>) -> ExprId {
@@ -611,82 +624,45 @@ impl ExprCollector<'_> {
             .filter_map(|item| {
                 let (def, name): (ModuleDefId, Option<ast::Name>) = match item {
                     ast::ModuleItem::FnDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
+                        let id = self.find_inner_item(&def)?;
                         (
-                            FunctionLoc { container: container.into(), id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
+                            FunctionLoc { container: container.into(), id }.intern(self.db).into(),
                             def.name(),
                         )
                     }
                     ast::ModuleItem::TypeAliasDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
+                        let id = self.find_inner_item(&def)?;
                         (
-                            TypeAliasLoc { container: container.into(), id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
+                            TypeAliasLoc { container: container.into(), id }.intern(self.db).into(),
                             def.name(),
                         )
                     }
                     ast::ModuleItem::ConstDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
+                        let id = self.find_inner_item(&def)?;
                         (
-                            ConstLoc { container: container.into(), id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
+                            ConstLoc { container: container.into(), id }.intern(self.db).into(),
                             def.name(),
                         )
                     }
                     ast::ModuleItem::StaticDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
-                        (
-                            StaticLoc { container, id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
-                            def.name(),
-                        )
+                        let id = self.find_inner_item(&def)?;
+                        (StaticLoc { container, id }.intern(self.db).into(), def.name())
                     }
                     ast::ModuleItem::StructDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
-                        (
-                            StructLoc { container, id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
-                            def.name(),
-                        )
+                        let id = self.find_inner_item(&def)?;
+                        (StructLoc { container, id }.intern(self.db).into(), def.name())
                     }
                     ast::ModuleItem::EnumDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
-                        (
-                            EnumLoc { container, id: ast_id.with_value(id) }.intern(self.db).into(),
-                            def.name(),
-                        )
+                        let id = self.find_inner_item(&def)?;
+                        (EnumLoc { container, id }.intern(self.db).into(), def.name())
                     }
                     ast::ModuleItem::UnionDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
-                        (
-                            UnionLoc { container, id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
-                            def.name(),
-                        )
+                        let id = self.find_inner_item(&def)?;
+                        (UnionLoc { container, id }.intern(self.db).into(), def.name())
                     }
                     ast::ModuleItem::TraitDef(def) => {
-                        let ast_id = self.expander.ast_id(&def);
-                        let id = self.find_inner_item(ast_id.map(|id| id.upcast()));
-                        (
-                            TraitLoc { container, id: ast_id.with_value(id) }
-                                .intern(self.db)
-                                .into(),
-                            def.name(),
-                        )
+                        let id = self.find_inner_item(&def)?;
+                        (TraitLoc { container, id }.intern(self.db).into(), def.name())
                     }
                     ast::ModuleItem::ExternBlock(_) => return None, // FIXME: collect from extern blocks
                     ast::ModuleItem::ImplDef(_)
diff --git a/crates/ra_hir_def/src/body/scope.rs b/crates/ra_hir_def/src/body/scope.rs
index 81397b063..99e876683 100644
--- a/crates/ra_hir_def/src/body/scope.rs
+++ b/crates/ra_hir_def/src/body/scope.rs
@@ -337,6 +337,19 @@ fn foo() {
         );
     }
 
+    #[test]
+    fn broken_inner_item() {
+        do_check(
+            r"
+            fn foo() {
+                trait {}
+                <|>
+            }
+        ",
+            &[],
+        );
+    }
+
     fn do_check_local_name(ra_fixture: &str, expected_offset: u32) {
         let (db, position) = TestDB::with_position(ra_fixture);
         let file_id = position.file_id;