From 5d7172f17e5b39737dc8d692112cf969926b7f05 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 26 Jun 2020 17:30:27 +0200 Subject: [PATCH 1/3] Simplify inner item lowering --- crates/ra_hir_def/src/body/lower.rs | 91 ++++++++++------------------- 1 file changed, 30 insertions(+), 61 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 3ced648e56..2e433528d1 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,23 @@ impl ExprCollector<'_> { } } - fn find_inner_item(&self, id: AstId) -> FileItemTreeId { + fn find_inner_item(&self, ast: &N::Source) -> ItemTreeId { + 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::()) - .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::()) + .find(|tree_id| tree[*tree_id].ast_id().upcast() == id.value.upcast()) + .unwrap_or_else(|| { + panic!("couldn't find inner {} item for {:?}", type_name::(), id) + }); + + ItemTreeId::new(id.file_id, item_tree_id) } fn collect_expr_opt(&mut self, expr: Option) -> ExprId { @@ -611,82 +617,45 @@ impl ExprCollector<'_> { .filter_map(|item| { let (def, name): (ModuleDefId, Option) = 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(_) From 7f6694b12eaae4aa0359319e57abfced97344227 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 26 Jun 2020 18:02:41 +0200 Subject: [PATCH 2/3] find_inner_item: more detailed panic message --- crates/ra_hir_def/src/body/lower.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 2e433528d1..961064d29f 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -574,7 +574,13 @@ impl ExprCollector<'_> { .filter_map(|mod_item| mod_item.downcast::()) .find(|tree_id| tree[*tree_id].ast_id().upcast() == id.value.upcast()) .unwrap_or_else(|| { - panic!("couldn't find inner {} item for {:?}", type_name::(), id) + panic!( + "couldn't find inner {} item for {:?} (AST: `{}` - {:?})", + type_name::(), + id, + ast.syntax(), + ast.syntax(), + ) }); ItemTreeId::new(id.file_id, item_tree_id) From efe378d2b43b90f8cf549781e870bfa2ebe90fd0 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 26 Jun 2020 18:06:01 +0200 Subject: [PATCH 3/3] Make find_inner_item fallible The ItemTree does not collect incomplete items, such as traits with no name, so the (malformed) AST node might have no corresponding item. --- crates/ra_hir_def/src/body/lower.rs | 29 +++++++++++++++-------------- crates/ra_hir_def/src/body/scope.rs | 13 +++++++++++++ 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 961064d29f..a7e2e09822 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -561,7 +561,7 @@ impl ExprCollector<'_> { } } - fn find_inner_item(&self, ast: &N::Source) -> ItemTreeId { + fn find_inner_item(&self, ast: &N::Source) -> Option> { let id = self.expander.ast_id(ast); let tree = &self.item_trees[&id.file_id]; @@ -573,17 +573,18 @@ impl ExprCollector<'_> { .chain(tree.top_level_items().iter().copied()) .filter_map(|mod_item| mod_item.downcast::()) .find(|tree_id| tree[*tree_id].ast_id().upcast() == id.value.upcast()) - .unwrap_or_else(|| { - panic!( + .or_else(|| { + log::debug!( "couldn't find inner {} item for {:?} (AST: `{}` - {:?})", type_name::(), id, ast.syntax(), ast.syntax(), - ) - }); + ); + None + })?; - ItemTreeId::new(id.file_id, item_tree_id) + Some(ItemTreeId::new(id.file_id, item_tree_id)) } fn collect_expr_opt(&mut self, expr: Option) -> ExprId { @@ -623,44 +624,44 @@ impl ExprCollector<'_> { .filter_map(|item| { let (def, name): (ModuleDefId, Option) = match item { ast::ModuleItem::FnDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; ( FunctionLoc { container: container.into(), id }.intern(self.db).into(), def.name(), ) } ast::ModuleItem::TypeAliasDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; ( TypeAliasLoc { container: container.into(), id }.intern(self.db).into(), def.name(), ) } ast::ModuleItem::ConstDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; ( ConstLoc { container: container.into(), id }.intern(self.db).into(), def.name(), ) } ast::ModuleItem::StaticDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; (StaticLoc { container, id }.intern(self.db).into(), def.name()) } ast::ModuleItem::StructDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; (StructLoc { container, id }.intern(self.db).into(), def.name()) } ast::ModuleItem::EnumDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; (EnumLoc { container, id }.intern(self.db).into(), def.name()) } ast::ModuleItem::UnionDef(def) => { - let id = self.find_inner_item(&def); + let id = self.find_inner_item(&def)?; (UnionLoc { container, id }.intern(self.db).into(), def.name()) } ast::ModuleItem::TraitDef(def) => { - let id = self.find_inner_item(&def); + 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 diff --git a/crates/ra_hir_def/src/body/scope.rs b/crates/ra_hir_def/src/body/scope.rs index 81397b0631..99e8766835 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;