Merge #255

255: Binders r=matklad a=matklad

Binding sources to hir is a fuzzy operation, so let's move it to a special enclave in the source code. 

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/ra_analysis/src/completion/mod.rs

@@ -9,6 +9,7 @@ use ra_syntax::{
 };
 use ra_db::SyntaxDatabase;
 use rustc_hash::{FxHashMap};
+use hir::source_binder;
 
 use crate::{
     db,
@@ -36,7 +37,7 @@ pub(crate) fn completions(
         original_file.reparse(&edit)
     };
 
-    let module = ctry!(hir::Module::guess_from_position(db, position)?);
+    let module = ctry!(source_binder::module_from_position(db, position)?);
 
     let mut res = Vec::new();
     let mut has_completions = false;

crates/ra_analysis/src/imp.rs

@@ -16,6 +16,7 @@ use rustc_hash::FxHashSet;
 use salsa::{Database, ParallelDatabase};
 use hir::{
     self,
+    source_binder,
     FnSignatureInfo,
     Problem,
 };
@@ -166,7 +167,7 @@ impl AnalysisImpl {
     /// This return `Vec`: a module may be included from several places. We
     /// don't handle this case yet though, so the Vec has length at most one.
     pub fn parent_module(&self, position: FilePosition) -> Cancelable<Vec<(FileId, FileSymbol)>> {
-        let descr = match hir::Module::guess_from_position(&*self.db, position)? {
+        let descr = match source_binder::module_from_position(&*self.db, position)? {
             None => return Ok(Vec::new()),
             Some(it) => it,
         };
@@ -185,7 +186,7 @@ impl AnalysisImpl {
     }
     /// Returns `Vec` for the same reason as `parent_module`
     pub fn crate_for(&self, file_id: FileId) -> Cancelable<Vec<CrateId>> {
-        let descr = match hir::Module::guess_from_file_id(&*self.db, file_id)? {
+        let descr = match source_binder::module_from_file_id(&*self.db, file_id)? {
             None => return Ok(Vec::new()),
             Some(it) => it,
         };
@@ -209,9 +210,11 @@ impl AnalysisImpl {
         let file = self.db.source_file(position.file_id);
         let syntax = file.syntax();
         if let Some(name_ref) = find_node_at_offset::<ast::NameRef>(syntax, position.offset) {
-            if let Some(fn_descr) =
+            if let Some(fn_descr) = source_binder::function_from_child_node(
-                hir::Function::guess_for_name_ref(&*self.db, position.file_id, name_ref)?
+                &*self.db,
-            {
+                position.file_id,
+                name_ref.syntax(),
+            )? {
                 let scope = fn_descr.scope(&*self.db);
                 // First try to resolve the symbol locally
                 if let Some(entry) = scope.resolve_local_name(name_ref) {
@@ -234,7 +237,7 @@ impl AnalysisImpl {
             if let Some(module) = name.syntax().parent().and_then(ast::Module::cast) {
                 if module.has_semi() {
                     let parent_module =
-                        hir::Module::guess_from_file_id(&*self.db, position.file_id)?;
+                        source_binder::module_from_file_id(&*self.db, position.file_id)?;
                     let child_name = module.name();
                     match (parent_module, child_name) {
                         (Some(parent_module), Some(child_name)) => {
@@ -282,18 +285,18 @@ impl AnalysisImpl {
         ) -> Cancelable<Option<(ast::BindPat<'a>, hir::Function)>> {
             let syntax = source_file.syntax();
             if let Some(binding) = find_node_at_offset::<ast::BindPat>(syntax, position.offset) {
-                let descr = ctry!(hir::Function::guess_for_bind_pat(
+                let descr = ctry!(source_binder::function_from_child_node(
                     db,
                     position.file_id,
-                    binding
+                    binding.syntax(),
                 )?);
                 return Ok(Some((binding, descr)));
             };
             let name_ref = ctry!(find_node_at_offset::<ast::NameRef>(syntax, position.offset));
-            let descr = ctry!(hir::Function::guess_for_name_ref(
+            let descr = ctry!(source_binder::function_from_child_node(
                 db,
                 position.file_id,
-                name_ref
+                name_ref.syntax(),
             )?);
             let scope = descr.scope(db);
             let resolved = ctry!(scope.resolve_local_name(name_ref));
@@ -327,7 +330,7 @@ impl AnalysisImpl {
                 fix: None,
             })
             .collect::<Vec<_>>();
-        if let Some(m) = hir::Module::guess_from_file_id(&*self.db, file_id)? {
+        if let Some(m) = source_binder::module_from_file_id(&*self.db, file_id)? {
             for (name_node, problem) in m.problems(&*self.db) {
                 let diag = match problem {
                     Problem::UnresolvedModule { candidate } => {
@@ -418,7 +421,7 @@ impl AnalysisImpl {
             if fs.kind == FN_DEF {
                 let fn_file = self.db.source_file(fn_file_id);
                 if let Some(fn_def) = find_node_at_offset(fn_file.syntax(), fs.node_range.start()) {
-                    let descr = ctry!(hir::Function::guess_from_source(
+                    let descr = ctry!(source_binder::function_from_source(
                         &*self.db, fn_file_id, fn_def
                     )?);
                     if let Some(descriptor) = descr.signature_info(&*self.db) {

crates/ra_hir/src/function/mod.rs

@@ -6,16 +6,11 @@ use std::{
 };
 
 use ra_syntax::{
-    TextRange, TextUnit, SyntaxNodeRef,
+    TextRange, TextUnit,
     ast::{self, AstNode, DocCommentsOwner, NameOwner},
 };
-use ra_db::FileId;
 
-use crate::{
+use crate::{ DefId, HirDatabase };
-    Cancelable,
-    DefLoc, DefKind, DefId, HirDatabase, SourceItemId,
-    Module,
-};
 
 pub use self::scope::FnScopes;
 
@@ -32,49 +27,6 @@ impl Function {
         Function { fn_id }
     }
 
-    pub fn guess_from_source(
-        db: &impl HirDatabase,
-        file_id: FileId,
-        fn_def: ast::FnDef,
-    ) -> Cancelable<Option<Function>> {
-        let module = ctry!(Module::guess_from_child_node(db, file_id, fn_def.syntax())?);
-        let file_items = db.file_items(file_id);
-        let item_id = file_items.id_of(fn_def.syntax());
-        let source_item_id = SourceItemId { file_id, item_id };
-        let def_loc = DefLoc {
-            kind: DefKind::Function,
-            source_root_id: module.source_root_id,
-            module_id: module.module_id,
-            source_item_id,
-        };
-        Ok(Some(Function::new(def_loc.id(db))))
-    }
-
-    pub fn guess_for_name_ref(
-        db: &impl HirDatabase,
-        file_id: FileId,
-        name_ref: ast::NameRef,
-    ) -> Cancelable<Option<Function>> {
-        Function::guess_for_node(db, file_id, name_ref.syntax())
-    }
-
-    pub fn guess_for_bind_pat(
-        db: &impl HirDatabase,
-        file_id: FileId,
-        bind_pat: ast::BindPat,
-    ) -> Cancelable<Option<Function>> {
-        Function::guess_for_node(db, file_id, bind_pat.syntax())
-    }
-
-    fn guess_for_node(
-        db: &impl HirDatabase,
-        file_id: FileId,
-        node: SyntaxNodeRef,
-    ) -> Cancelable<Option<Function>> {
-        let fn_def = ctry!(node.ancestors().find_map(ast::FnDef::cast));
-        Function::guess_from_source(db, file_id, fn_def)
-    }
-
     pub fn scope(&self, db: &impl HirDatabase) -> Arc<FnScopes> {
         db.fn_scopes(self.fn_id)
     }

crates/ra_hir/src/lib.rs

@@ -22,6 +22,7 @@ mod function;
 mod module;
 mod path;
 mod arena;
+pub mod source_binder;
 
 use std::ops::Index;
 

crates/ra_hir/src/module/mod.rs

@@ -3,14 +3,12 @@ pub(super) mod nameres;
 
 use std::sync::Arc;
 
-use ra_editor::find_node_at_offset;
-
 use ra_syntax::{
     algo::generate,
     ast::{self, AstNode, NameOwner},
-    SmolStr, SyntaxNode, SyntaxNodeRef,
+    SmolStr, SyntaxNode,
 };
-use ra_db::{SourceRootId, FileId, FilePosition, Cancelable};
+use ra_db::{SourceRootId, FileId, Cancelable};
 use relative_path::RelativePathBuf;
 
 use crate::{
@@ -30,68 +28,6 @@ pub struct Module {
 }
 
 impl Module {
-    /// Lookup `Module` by `FileId`. Note that this is inherently
-    /// lossy transformation: in general, a single source might correspond to
-    /// several modules.
-    pub fn guess_from_file_id(
-        db: &impl HirDatabase,
-        file_id: FileId,
-    ) -> Cancelable<Option<Module>> {
-        let module_source = ModuleSource::new_file(db, file_id);
-        Module::guess_from_source(db, module_source)
-    }
-
-    /// Lookup `Module` by position in the source code. Note that this
-    /// is inherently lossy transformation: in general, a single source might
-    /// correspond to several modules.
-    pub fn guess_from_position(
-        db: &impl HirDatabase,
-        position: FilePosition,
-    ) -> Cancelable<Option<Module>> {
-        let file = db.source_file(position.file_id);
-        let module_source = match find_node_at_offset::<ast::Module>(file.syntax(), position.offset)
-        {
-            Some(m) if !m.has_semi() => ModuleSource::new_inline(db, position.file_id, m),
-            _ => ModuleSource::new_file(db, position.file_id),
-        };
-        Module::guess_from_source(db, module_source)
-    }
-
-    pub fn guess_from_child_node(
-        db: &impl HirDatabase,
-        file_id: FileId,
-        node: SyntaxNodeRef,
-    ) -> Cancelable<Option<Module>> {
-        let module_source = if let Some(m) = node
-            .ancestors()
-            .filter_map(ast::Module::cast)
-            .find(|it| !it.has_semi())
-        {
-            ModuleSource::new_inline(db, file_id, m)
-        } else {
-            ModuleSource::new_file(db, file_id)
-        };
-        Module::guess_from_source(db, module_source)
-    }
-
-    fn guess_from_source(
-        db: &impl HirDatabase,
-        module_source: ModuleSource,
-    ) -> Cancelable<Option<Module>> {
-        let source_root_id = db.file_source_root(module_source.file_id());
-        let module_tree = db.module_tree(source_root_id)?;
-
-        let res = match module_tree.any_module_for_source(module_source) {
-            None => None,
-            Some(module_id) => Some(Module {
-                tree: module_tree,
-                source_root_id,
-                module_id,
-            }),
-        };
-        Ok(res)
-    }
-
     pub(super) fn new(
         db: &impl HirDatabase,
         source_root_id: SourceRootId,
@@ -217,16 +153,10 @@ impl ModuleTree {
         self.mods.iter().map(|(id, _)| id)
     }
 
-    fn modules_for_source(&self, source: ModuleSource) -> Vec<ModuleId> {
+    pub(crate) fn modules_with_sources<'a>(
-        self.mods
+        &'a self,
-            .iter()
+    ) -> impl Iterator<Item = (ModuleId, ModuleSource)> + 'a {
-            .filter(|(_idx, it)| it.source == source)
+        self.mods.iter().map(|(id, m)| (id, m.source))
-            .map(|(idx, _)| idx)
-            .collect()
-    }
-
-    fn any_module_for_source(&self, source: ModuleSource) -> Option<ModuleId> {
-        self.modules_for_source(source).pop()
     }
 }
 

crates/ra_hir/src/module/nameres.rs

@@ -363,7 +363,9 @@ mod tests {
     fn item_map(fixture: &str) -> (Arc<hir::ItemMap>, hir::ModuleId) {
         let (db, pos) = MockDatabase::with_position(fixture);
         let source_root = db.file_source_root(pos.file_id);
-        let module = hir::Module::guess_from_position(&db, pos).unwrap().unwrap();
+        let module = hir::source_binder::module_from_position(&db, pos)
+            .unwrap()
+            .unwrap();
         let module_id = module.module_id;
         (db.item_map(source_root).unwrap(), module_id)
     }

crates/ra_hir/src/source_binder.rs

@@ -0,0 +1,96 @@
+/// Lookup hir elements using position in the source code. This is a lossy
+/// transformation: in general, a single source might correspond to several
+/// modules, functions, etc, due to macros, cfgs and `#[path=]` attributes on
+/// modules.
+///
+/// So, this modules should not be used during hir construction, it exists
+/// purely for "IDE needs".
+use ra_db::{FileId, FilePosition, Cancelable};
+use ra_editor::find_node_at_offset;
+use ra_syntax::{
+    ast::{self, AstNode},
+    SyntaxNodeRef,
+};
+
+use crate::{
+    HirDatabase, Module, Function, SourceItemId,
+    module::ModuleSource,
+    DefKind, DefLoc
+};
+
+/// Locates the module by `FileId`. Picks topmost module in the file.
+pub fn module_from_file_id(db: &impl HirDatabase, file_id: FileId) -> Cancelable<Option<Module>> {
+    let module_source = ModuleSource::new_file(db, file_id);
+    module_from_source(db, module_source)
+}
+
+/// Locates the module by position in the source code.
+pub fn module_from_position(
+    db: &impl HirDatabase,
+    position: FilePosition,
+) -> Cancelable<Option<Module>> {
+    let file = db.source_file(position.file_id);
+    let module_source = match find_node_at_offset::<ast::Module>(file.syntax(), position.offset) {
+        Some(m) if !m.has_semi() => ModuleSource::new_inline(db, position.file_id, m),
+        _ => ModuleSource::new_file(db, position.file_id),
+    };
+    module_from_source(db, module_source)
+}
+
+/// Locates the module by child syntax element within the module
+pub fn module_from_child_node(
+    db: &impl HirDatabase,
+    file_id: FileId,
+    child: SyntaxNodeRef,
+) -> Cancelable<Option<Module>> {
+    let module_source = if let Some(m) = child
+        .ancestors()
+        .filter_map(ast::Module::cast)
+        .find(|it| !it.has_semi())
+    {
+        ModuleSource::new_inline(db, file_id, m)
+    } else {
+        ModuleSource::new_file(db, file_id)
+    };
+    module_from_source(db, module_source)
+}
+
+fn module_from_source(
+    db: &impl HirDatabase,
+    module_source: ModuleSource,
+) -> Cancelable<Option<Module>> {
+    let source_root_id = db.file_source_root(module_source.file_id());
+    let module_tree = db.module_tree(source_root_id)?;
+    let m = module_tree
+        .modules_with_sources()
+        .find(|(_id, src)| src == &module_source);
+    let module_id = ctry!(m).0;
+    Ok(Some(Module::new(db, source_root_id, module_id)?))
+}
+
+pub fn function_from_source(
+    db: &impl HirDatabase,
+    file_id: FileId,
+    fn_def: ast::FnDef,
+) -> Cancelable<Option<Function>> {
+    let module = ctry!(module_from_child_node(db, file_id, fn_def.syntax())?);
+    let file_items = db.file_items(file_id);
+    let item_id = file_items.id_of(fn_def.syntax());
+    let source_item_id = SourceItemId { file_id, item_id };
+    let def_loc = DefLoc {
+        kind: DefKind::Function,
+        source_root_id: module.source_root_id,
+        module_id: module.module_id,
+        source_item_id,
+    };
+    Ok(Some(Function::new(def_loc.id(db))))
+}
+
+pub fn function_from_child_node(
+    db: &impl HirDatabase,
+    file_id: FileId,
+    node: SyntaxNodeRef,
+) -> Cancelable<Option<Function>> {
+    let fn_def = ctry!(node.ancestors().find_map(ast::FnDef::cast));
+    function_from_source(db, file_id, fn_def)
+}