Merge #842

842: Turn ImplBlock into a copy type just containing IDs r=matklad a=flodiebold

This makes it more like the other code model types.

Also make Module::definition_source/declaration_source return HirFileIds, to
make them more like the other source functions.

Co-authored-by: Florian Diebold <[email protected]>

9 files changed, 65 insertions, 77 deletions

diff --git a/crates/ra_cli/src/analysis_stats.rs b/crates/ra_cli/src/analysis_stats.rs
index a46ac974d..ee410c548 100644
--- a/crates/ra_cli/src/analysis_stats.rs
+++ b/crates/ra_cli/src/analysis_stats.rs
@@ -36,10 +36,10 @@ pub fn run(verbose: bool) -> Result<()> {
             }
 
             for impl_block in module.impl_blocks(&db) {
-                for item in impl_block.items() {
+                for item in impl_block.items(&db) {
                     num_decls += 1;
                     match item {
-                        ImplItem::Method(f) => funcs.push(*f),
+                        ImplItem::Method(f) => funcs.push(f),
                         _ => {}
                     }
                 }
diff --git a/crates/ra_hir/src/code_model_api.rs b/crates/ra_hir/src/code_model_api.rs
index fb7ad0867..99f004905 100644
--- a/crates/ra_hir/src/code_model_api.rs
+++ b/crates/ra_hir/src/code_model_api.rs
@@ -1,7 +1,7 @@
 use std::sync::Arc;
 
 use relative_path::RelativePathBuf;
-use ra_db::{CrateId, FileId, SourceRootId, Edition};
+use ra_db::{CrateId, SourceRootId, Edition};
 use ra_syntax::{ast::self, TreeArc, SyntaxNode};
 
 use crate::{
@@ -16,7 +16,7 @@ use crate::{
     docs::{Documentation, Docs, docs_from_ast},
     module_tree::ModuleId,
     ids::{FunctionId, StructId, EnumId, AstItemDef, ConstId, StaticId, TraitId, TypeId},
-    impl_block::{ImplId, ImplBlock},
+    impl_block::ImplBlock,
     resolve::Resolver,
 };
 
@@ -107,7 +107,7 @@ impl Module {
     }
 
     /// Returns a node which defines this module. That is, a file or a `mod foo {}` with items.
-    pub fn definition_source(&self, db: &impl PersistentHirDatabase) -> (FileId, ModuleSource) {
+    pub fn definition_source(&self, db: &impl PersistentHirDatabase) -> (HirFileId, ModuleSource) {
         self.definition_source_impl(db)
     }
 
@@ -116,7 +116,7 @@ impl Module {
     pub fn declaration_source(
         &self,
         db: &impl HirDatabase,
-    ) -> Option<(FileId, TreeArc<ast::Module>)> {
+    ) -> Option<(HirFileId, TreeArc<ast::Module>)> {
         self.declaration_source_impl(db)
     }
 
@@ -129,11 +129,6 @@ impl Module {
         self.import_source_impl(db, import)
     }
 
-    /// Returns the syntax of the impl block in this module
-    pub fn impl_source(&self, db: &impl HirDatabase, impl_id: ImplId) -> TreeArc<ast::ImplBlock> {
-        self.impl_source_impl(db, impl_id)
-    }
-
     /// Returns the crate this module is part of.
     pub fn krate(&self, _db: &impl PersistentHirDatabase) -> Option<Crate> {
         Some(self.krate)
@@ -202,7 +197,7 @@ impl Module {
         module_impl_blocks
             .impls
             .iter()
-            .map(|(impl_id, _)| ImplBlock::from_id(module_impl_blocks.clone(), impl_id))
+            .map(|(impl_id, _)| ImplBlock::from_id(self, impl_id))
             .collect()
     }
 }
diff --git a/crates/ra_hir/src/code_model_impl/module.rs b/crates/ra_hir/src/code_model_impl/module.rs
index 1425fa693..2d3058afd 100644
--- a/crates/ra_hir/src/code_model_impl/module.rs
+++ b/crates/ra_hir/src/code_model_impl/module.rs
@@ -1,13 +1,12 @@
-use ra_db::FileId;
 use ra_syntax::{ast, SyntaxNode, TreeArc};
 
 use crate::{
     Module, ModuleSource, Problem,
     Name,
     module_tree::ModuleId,
-    impl_block::ImplId,
-    nameres::{lower::ImportId},
+    nameres::lower::ImportId,
     HirDatabase, PersistentHirDatabase,
+    HirFileId
 };
 
 impl Module {
@@ -24,22 +23,21 @@ impl Module {
     pub(crate) fn definition_source_impl(
         &self,
         db: &impl PersistentHirDatabase,
-    ) -> (FileId, ModuleSource) {
+    ) -> (HirFileId, ModuleSource) {
         let module_tree = db.module_tree(self.krate);
         let file_id = self.module_id.file_id(&module_tree);
         let decl_id = self.module_id.decl_id(&module_tree);
         let module_source = ModuleSource::new(db, file_id, decl_id);
-        let file_id = file_id.as_original_file();
         (file_id, module_source)
     }
 
     pub(crate) fn declaration_source_impl(
         &self,
         db: &impl HirDatabase,
-    ) -> Option<(FileId, TreeArc<ast::Module>)> {
+    ) -> Option<(HirFileId, TreeArc<ast::Module>)> {
         let module_tree = db.module_tree(self.krate);
         let link = self.module_id.parent_link(&module_tree)?;
-        let file_id = link.owner(&module_tree).file_id(&module_tree).as_original_file();
+        let file_id = link.owner(&module_tree).file_id(&module_tree);
         let src = link.source(&module_tree, db);
         Some((file_id, src))
     }
@@ -54,16 +52,6 @@ impl Module {
         source_map.get(&source, import)
     }
 
-    pub(crate) fn impl_source_impl(
-        &self,
-        db: &impl HirDatabase,
-        impl_id: ImplId,
-    ) -> TreeArc<ast::ImplBlock> {
-        let source_map = db.impls_in_module_source_map(*self);
-        let (_, source) = self.definition_source(db);
-        source_map.get(&source, impl_id)
-    }
-
     pub(crate) fn crate_root_impl(&self, db: &impl PersistentHirDatabase) -> Module {
         let module_tree = db.module_tree(self.krate);
         let module_id = self.module_id.crate_root(&module_tree);
diff --git a/crates/ra_hir/src/ids.rs b/crates/ra_hir/src/ids.rs
index 8d81d5ebf..3e11dd6ad 100644
--- a/crates/ra_hir/src/ids.rs
+++ b/crates/ra_hir/src/ids.rs
@@ -74,7 +74,10 @@ impl HirFileId {
         }
     }
 
-    pub(crate) fn as_original_file(self) -> FileId {
+    /// XXX: this is a temporary function, which should go away when we implement the
+    /// nameresolution+macro expansion combo. Prefer using `original_file` if
+    /// possible.
+    pub fn as_original_file(self) -> FileId {
         match self.0 {
             HirFileIdRepr::File(file_id) => file_id,
             HirFileIdRepr::Macro(_r) => panic!("macro generated file: {:?}", self),
diff --git a/crates/ra_hir/src/impl_block.rs b/crates/ra_hir/src/impl_block.rs
index b2fbee8d7..4d8bdf33a 100644
--- a/crates/ra_hir/src/impl_block.rs
+++ b/crates/ra_hir/src/impl_block.rs
@@ -38,9 +38,9 @@ impl ImplSourceMap {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub struct ImplBlock {
-    module_impl_blocks: Arc<ModuleImplBlocks>,
+    module: Module,
     impl_id: ImplId,
 }
 
@@ -50,42 +50,45 @@ impl ImplBlock {
         item: ImplItem,
     ) -> Option<ImplBlock> {
         let impl_id = *module_impl_blocks.impls_by_def.get(&item)?;
-        Some(ImplBlock { module_impl_blocks, impl_id })
+        Some(ImplBlock { module: module_impl_blocks.module, impl_id })
     }
 
-    pub(crate) fn from_id(module_impl_blocks: Arc<ModuleImplBlocks>, impl_id: ImplId) -> ImplBlock {
-        ImplBlock { module_impl_blocks, impl_id }
+    pub(crate) fn from_id(module: Module, impl_id: ImplId) -> ImplBlock {
+        ImplBlock { module, impl_id }
     }
 
-    pub fn id(&self) -> ImplId {
-        self.impl_id
+    /// Returns the syntax of the impl block
+    pub fn source(&self, db: &impl HirDatabase) -> (HirFileId, TreeArc<ast::ImplBlock>) {
+        let source_map = db.impls_in_module_source_map(self.module);
+        let (file_id, source) = self.module.definition_source(db);
+        (file_id, source_map.get(&source, self.impl_id))
     }
 
-    fn impl_data(&self) -> &ImplData {
-        &self.module_impl_blocks.impls[self.impl_id]
+    pub fn id(&self) -> ImplId {
+        self.impl_id
     }
 
     pub fn module(&self) -> Module {
-        self.module_impl_blocks.module
+        self.module
     }
 
-    pub fn target_trait_ref(&self) -> Option<&TypeRef> {
-        self.impl_data().target_trait()
+    pub fn target_trait_ref(&self, db: &impl HirDatabase) -> Option<TypeRef> {
+        db.impls_in_module(self.module).impls[self.impl_id].target_trait().cloned()
     }
 
-    pub fn target_type(&self) -> &TypeRef {
-        self.impl_data().target_type()
+    pub fn target_type(&self, db: &impl HirDatabase) -> TypeRef {
+        db.impls_in_module(self.module).impls[self.impl_id].target_type().clone()
     }
 
     pub fn target_ty(&self, db: &impl HirDatabase) -> Ty {
-        Ty::from_hir(db, &self.resolver(db), self.target_type())
+        Ty::from_hir(db, &self.resolver(db), &self.target_type(db))
     }
 
     pub fn target_trait(&self, db: &impl HirDatabase) -> Option<Trait> {
-        if let Some(TypeRef::Path(path)) = self.target_trait_ref() {
+        if let Some(TypeRef::Path(path)) = self.target_trait_ref(db) {
             let resolver = self.resolver(db);
             if let Some(Resolution::Def(ModuleDef::Trait(tr))) =
-                resolver.resolve_path(db, path).take_types()
+                resolver.resolve_path(db, &path).take_types()
             {
                 return Some(tr);
             }
@@ -93,8 +96,8 @@ impl ImplBlock {
         None
     }
 
-    pub fn items(&self) -> &[ImplItem] {
-        self.impl_data().items()
+    pub fn items(&self, db: &impl HirDatabase) -> Vec<ImplItem> {
+        db.impls_in_module(self.module).impls[self.impl_id].items().to_vec()
     }
 
     pub fn resolver(&self, db: &impl HirDatabase) -> Resolver {
@@ -181,7 +184,7 @@ impl_arena_id!(ImplId);
 /// we don't need to do the second step again.
 #[derive(Debug, PartialEq, Eq)]
 pub struct ModuleImplBlocks {
-    module: Module,
+    pub(crate) module: Module,
     pub(crate) impls: Arena<ImplId, ImplData>,
     impls_by_def: FxHashMap<ImplItem, ImplId>,
 }
diff --git a/crates/ra_hir/src/ty/method_resolution.rs b/crates/ra_hir/src/ty/method_resolution.rs
index e81bd3e06..8d1076774 100644
--- a/crates/ra_hir/src/ty/method_resolution.rs
+++ b/crates/ra_hir/src/ty/method_resolution.rs
@@ -40,32 +40,25 @@ pub struct CrateImplBlocks {
 }
 
 impl CrateImplBlocks {
-    pub fn lookup_impl_blocks<'a>(
-        &'a self,
-        db: &'a impl HirDatabase,
-        ty: &Ty,
-    ) -> impl Iterator<Item = (Module, ImplBlock)> + 'a {
+    pub fn lookup_impl_blocks<'a>(&'a self, ty: &Ty) -> impl Iterator<Item = ImplBlock> + 'a {
         let fingerprint = TyFingerprint::for_impl(ty);
         fingerprint.and_then(|f| self.impls.get(&f)).into_iter().flat_map(|i| i.iter()).map(
             move |(module_id, impl_id)| {
                 let module = Module { krate: self.krate, module_id: *module_id };
-                let module_impl_blocks = db.impls_in_module(module);
-                (module, ImplBlock::from_id(module_impl_blocks, *impl_id))
+                ImplBlock::from_id(module, *impl_id)
             },
         )
     }
 
     pub fn lookup_impl_blocks_for_trait<'a>(
         &'a self,
-        db: &'a impl HirDatabase,
         tr: &Trait,
-    ) -> impl Iterator<Item = (Module, ImplBlock)> + 'a {
+    ) -> impl Iterator<Item = ImplBlock> + 'a {
         let id = tr.id;
         self.impls_by_trait.get(&id).into_iter().flat_map(|i| i.iter()).map(
             move |(module_id, impl_id)| {
                 let module = Module { krate: self.krate, module_id: *module_id };
-                let module_impl_blocks = db.impls_in_module(module);
-                (module, ImplBlock::from_id(module_impl_blocks, *impl_id))
+                ImplBlock::from_id(module, *impl_id)
             },
         )
     }
@@ -74,7 +67,7 @@ impl CrateImplBlocks {
         let module_impl_blocks = db.impls_in_module(module.clone());
 
         for (impl_id, _) in module_impl_blocks.impls.iter() {
-            let impl_block = ImplBlock::from_id(Arc::clone(&module_impl_blocks), impl_id);
+            let impl_block = ImplBlock::from_id(module_impl_blocks.module, impl_id);
 
             let target_ty = impl_block.target_ty(db);
 
@@ -159,11 +152,11 @@ impl Ty {
             };
             let impls = db.impls_in_crate(krate);
 
-            for (_, impl_block) in impls.lookup_impl_blocks(db, &derefed_ty) {
-                for item in impl_block.items() {
+            for impl_block in impls.lookup_impl_blocks(&derefed_ty) {
+                for item in impl_block.items(db) {
                     match item {
                         ImplItem::Method(f) => {
-                            if let Some(result) = callback(*f) {
+                            if let Some(result) = callback(f) {
                                 return Some(result);
                             }
                         }
@@ -185,9 +178,9 @@ impl Ty {
         let krate = def_crate(db, &self)?;
         let impls = db.impls_in_crate(krate);
 
-        for (_, impl_block) in impls.lookup_impl_blocks(db, &self) {
-            for item in impl_block.items() {
-                if let Some(result) = callback(*item) {
+        for impl_block in impls.lookup_impl_blocks(&self) {
+            for item in impl_block.items(db) {
+                if let Some(result) = callback(item) {
                     return Some(result);
                 }
             }
diff --git a/crates/ra_ide_api/src/impls.rs b/crates/ra_ide_api/src/impls.rs
index 681bd808d..896b47f10 100644
--- a/crates/ra_ide_api/src/impls.rs
+++ b/crates/ra_ide_api/src/impls.rs
@@ -50,8 +50,8 @@ fn impls_for_def(
 
     Some(
         impls
-            .lookup_impl_blocks(db, &ty)
-            .map(|(module, imp)| NavigationTarget::from_impl_block(db, module, &imp))
+            .lookup_impl_blocks(&ty)
+            .map(|imp| NavigationTarget::from_impl_block(db, imp))
             .collect(),
     )
 }
@@ -68,8 +68,8 @@ fn impls_for_trait(
 
     Some(
         impls
-            .lookup_impl_blocks_for_trait(db, &tr)
-            .map(|(module, imp)| NavigationTarget::from_impl_block(db, module, &imp))
+            .lookup_impl_blocks_for_trait(&tr)
+            .map(|imp| NavigationTarget::from_impl_block(db, imp))
             .collect(),
     )
 }
diff --git a/crates/ra_ide_api/src/navigation_target.rs b/crates/ra_ide_api/src/navigation_target.rs
index c559dca11..004921863 100644
--- a/crates/ra_ide_api/src/navigation_target.rs
+++ b/crates/ra_ide_api/src/navigation_target.rs
@@ -79,6 +79,7 @@ impl NavigationTarget {
 
     pub(crate) fn from_module(db: &RootDatabase, module: hir::Module) -> NavigationTarget {
         let (file_id, source) = module.definition_source(db);
+        let file_id = file_id.as_original_file();
         let name = module.name(db).map(|it| it.to_string().into()).unwrap_or_default();
         match source {
             ModuleSource::SourceFile(node) => {
@@ -93,6 +94,7 @@ impl NavigationTarget {
     pub(crate) fn from_module_to_decl(db: &RootDatabase, module: hir::Module) -> NavigationTarget {
         let name = module.name(db).map(|it| it.to_string().into()).unwrap_or_default();
         if let Some((file_id, source)) = module.declaration_source(db) {
+            let file_id = file_id.as_original_file();
             return NavigationTarget::from_syntax(file_id, name, None, source.syntax());
         }
         NavigationTarget::from_module(db, module)
@@ -151,12 +153,15 @@ impl NavigationTarget {
 
     pub(crate) fn from_impl_block(
         db: &RootDatabase,
-        module: hir::Module,
-        impl_block: &hir::ImplBlock,
+        impl_block: hir::ImplBlock,
     ) -> NavigationTarget {
-        let (file_id, _) = module.definition_source(db);
-        let node = module.impl_source(db, impl_block.id());
-        NavigationTarget::from_syntax(file_id, "impl".into(), None, node.syntax())
+        let (file_id, node) = impl_block.source(db);
+        NavigationTarget::from_syntax(
+            file_id.as_original_file(),
+            "impl".into(),
+            None,
+            node.syntax(),
+        )
     }
 
     #[cfg(test)]
diff --git a/crates/ra_ide_api/src/references.rs b/crates/ra_ide_api/src/references.rs
index ca145f3e4..e7ebf9f6e 100644
--- a/crates/ra_ide_api/src/references.rs
+++ b/crates/ra_ide_api/src/references.rs
@@ -100,6 +100,7 @@ fn rename_mod(
     if let Some(module) = source_binder::module_from_declaration(db, position.file_id, &ast_module)
     {
         let (file_id, module_source) = module.definition_source(db);
+        let file_id = file_id.as_original_file();
         match module_source {
             ModuleSource::SourceFile(..) => {
                 let mod_path: RelativePathBuf = db.file_relative_path(file_id);