some fixes, add docs

5 files changed, 73 insertions, 72 deletions

diff --git a/crates/ra_ide_api/src/goto_definition.rs b/crates/ra_ide_api/src/goto_definition.rs
index d14908b25..1f3fa6c57 100644
--- a/crates/ra_ide_api/src/goto_definition.rs
+++ b/crates/ra_ide_api/src/goto_definition.rs
@@ -206,7 +206,7 @@ fn named_target(file_id: FileId, node: &SyntaxNode) -> Option<NavigationTarget>
 
 #[cfg(test)]
 mod tests {
-    // use test_utils::covers;
+    use test_utils::covers;
 
     use crate::mock_analysis::analysis_and_position;
 
@@ -274,7 +274,7 @@ mod tests {
 
     #[test]
     fn goto_definition_works_for_macros() {
-        // covers!(goto_definition_works_for_macros);
+        covers!(goto_definition_works_for_macros);
         check_goto(
             "
             //- /lib.rs
@@ -294,7 +294,7 @@ mod tests {
 
     #[test]
     fn goto_definition_works_for_macros_from_other_crates() {
-        // covers!(goto_definition_works_for_macros);
+        covers!(goto_definition_works_for_macros);
         check_goto(
             "
             //- /lib.rs
@@ -317,7 +317,7 @@ mod tests {
 
     #[test]
     fn goto_definition_works_for_methods() {
-        // covers!(goto_definition_works_for_methods);
+        covers!(goto_definition_works_for_methods);
         check_goto(
             "
             //- /lib.rs
@@ -336,7 +336,7 @@ mod tests {
 
     #[test]
     fn goto_definition_works_for_fields() {
-        // covers!(goto_definition_works_for_fields);
+        covers!(goto_definition_works_for_fields);
         check_goto(
             "
             //- /lib.rs
@@ -354,7 +354,7 @@ mod tests {
 
     #[test]
     fn goto_definition_works_for_record_fields() {
-        // covers!(goto_definition_works_for_record_fields);
+        covers!(goto_definition_works_for_record_fields);
         check_goto(
             "
             //- /lib.rs
diff --git a/crates/ra_ide_api/src/references.rs b/crates/ra_ide_api/src/references.rs
index aadd52616..f35d835ac 100644
--- a/crates/ra_ide_api/src/references.rs
+++ b/crates/ra_ide_api/src/references.rs
@@ -1,4 +1,13 @@
-//! FIXME: write short doc here
+//! This module implements a reference search.
+//! First, the element at the cursor position must be either an `ast::Name`
+//! or `ast::NameRef`. If it's a `ast::NameRef`, at the classification step we
+//! try to resolve the direct tree parent of this element, otherwise we
+//! already have a definition and just need to get its HIR together with
+//! some information that is needed for futher steps of searching.
+//! After that, we collect files that might contain references and look
+//! for text occurrences of the identifier. If there's an `ast::NameRef`
+//! at the index that the match starts at and its tree parent is
+//! resolved to the search element definition, we get a reference.
 
 mod classify;
 mod name_definition;
@@ -9,7 +18,7 @@ use once_cell::unsync::Lazy;
 use ra_db::{SourceDatabase, SourceDatabaseExt};
 use ra_syntax::{algo::find_node_at_offset, ast, AstNode, SourceFile, SyntaxNode, TextUnit};
 
-use crate::{db::RootDatabase, FileId, FilePosition, FileRange, NavigationTarget, RangeInfo};
+use crate::{db::RootDatabase, FilePosition, FileRange, NavigationTarget, RangeInfo};
 
 pub(crate) use self::{
     classify::{classify_name, classify_name_ref},
@@ -102,16 +111,7 @@ fn process_definition(db: &RootDatabase, def: NameDefinition, name: String) -> V
     let scope = def.search_scope(db);
     let mut refs = vec![];
 
-    let is_match = |file_id: FileId, name_ref: &ast::NameRef| -> bool {
-        let classified = classify_name_ref(db, file_id, &name_ref);
-        if let Some(d) = classified {
-            d == def
-        } else {
-            false
-        }
-    };
-
-    for (file_id, text_range) in scope {
+    for (file_id, search_range) in scope {
         let text = db.file_text(file_id);
         let parse = Lazy::new(|| SourceFile::parse(&text));
 
@@ -122,19 +122,20 @@ fn process_definition(db: &RootDatabase, def: NameDefinition, name: String) -> V
                 find_node_at_offset::<ast::NameRef>(parse.tree().syntax(), offset)
             {
                 let range = name_ref.syntax().text_range();
-
-                if let Some(text_range) = text_range {
-                    if range.is_subrange(&text_range) && is_match(file_id, &name_ref) {
+                if let Some(search_range) = search_range {
+                    if !range.is_subrange(&search_range) {
+                        continue;
+                    }
+                }
+                if let Some(d) = classify_name_ref(db, file_id, &name_ref) {
+                    if d == def {
                         refs.push(FileRange { file_id, range });
                     }
-                } else if is_match(file_id, &name_ref) {
-                    refs.push(FileRange { file_id, range });
                 }
             }
         }
     }
-
-    return refs;
+    refs
 }
 
 #[cfg(test)]
diff --git a/crates/ra_ide_api/src/references/classify.rs b/crates/ra_ide_api/src/references/classify.rs
index 3beab9861..c8daff9b1 100644
--- a/crates/ra_ide_api/src/references/classify.rs
+++ b/crates/ra_ide_api/src/references/classify.rs
@@ -1,8 +1,9 @@
-//! FIXME: write short doc here
+//! Functions that are used to classify an element from its definition or reference.
 
 use hir::{Either, FromSource, Module, ModuleSource, Path, PathResolution, Source, SourceAnalyzer};
 use ra_db::FileId;
 use ra_syntax::{ast, match_ast, AstNode, AstPtr};
+use test_utils::tested_by;
 
 use super::{
     name_definition::{from_assoc_item, from_module_def, from_pat, from_struct_field},
@@ -111,18 +112,21 @@ pub(crate) fn classify_name_ref(
     let analyzer = SourceAnalyzer::new(db, file_id, name_ref.syntax(), None);
 
     if let Some(method_call) = ast::MethodCallExpr::cast(parent.clone()) {
+        tested_by!(goto_definition_works_for_methods);
         if let Some(func) = analyzer.resolve_method_call(&method_call) {
             return Some(from_assoc_item(db, func.into()));
         }
     }
 
     if let Some(field_expr) = ast::FieldExpr::cast(parent.clone()) {
+        tested_by!(goto_definition_works_for_fields);
         if let Some(field) = analyzer.resolve_field(&field_expr) {
             return Some(from_struct_field(db, field));
         }
     }
 
     if let Some(record_field) = ast::RecordField::cast(parent.clone()) {
+        tested_by!(goto_definition_works_for_record_fields);
         if let Some(record_lit) = record_field.syntax().ancestors().find_map(ast::RecordLit::cast) {
             let variant_def = analyzer.resolve_record_literal(&record_lit)?;
             let hir_path = Path::from_name_ref(name_ref);
@@ -139,6 +143,7 @@ pub(crate) fn classify_name_ref(
     let visibility = None;
 
     if let Some(macro_call) = parent.ancestors().find_map(ast::MacroCall::cast) {
+        tested_by!(goto_definition_works_for_macros);
         if let Some(macro_def) = analyzer.resolve_macro_call(db, &macro_call) {
             let kind = NameKind::Macro(macro_def);
             return Some(NameDefinition { kind, container, visibility });
@@ -152,7 +157,6 @@ pub(crate) fn classify_name_ref(
         AssocItem(item) => Some(from_assoc_item(db, item)),
         LocalBinding(Either::A(pat)) => from_pat(db, file_id, pat),
         LocalBinding(Either::B(par)) => {
-            // Not really supported
             let kind = NameKind::SelfParam(par);
             Some(NameDefinition { kind, container, visibility })
         }
diff --git a/crates/ra_ide_api/src/references/name_definition.rs b/crates/ra_ide_api/src/references/name_definition.rs
index 723d97237..4580bc789 100644
--- a/crates/ra_ide_api/src/references/name_definition.rs
+++ b/crates/ra_ide_api/src/references/name_definition.rs
@@ -1,4 +1,7 @@
-//! FIXME: write short doc here
+//! `NameDefinition` keeps information about the element we want to search references for.
+//! The element is represented by `NameKind`. It's located inside some `container` and
+//! has a `visibility`, which defines a search scope.
+//! Note that the reference search is possible for not all of the classified items.
 
 use hir::{
     db::AstDatabase, Adt, AssocItem, DefWithBody, FromSource, HasSource, HirFileId, MacroDef,
diff --git a/crates/ra_ide_api/src/references/search_scope.rs b/crates/ra_ide_api/src/references/search_scope.rs
index 8495a92a5..5cb69b8fc 100644
--- a/crates/ra_ide_api/src/references/search_scope.rs
+++ b/crates/ra_ide_api/src/references/search_scope.rs
@@ -1,72 +1,66 @@
-//! FIXME: write short doc here
-
-use std::collections::HashSet;
+//! Generally, `search_scope` returns files that might contain references for the element.
+//! For `pub(crate)` things it's a crate, for `pub` things it's a crate and dependant crates.
+//! In some cases, the location of the references is known to within a `TextRange`,
+//! e.g. for things like local variables.
 
 use hir::{DefWithBody, HasSource, ModuleSource};
 use ra_db::{FileId, SourceDatabase, SourceDatabaseExt};
 use ra_syntax::{AstNode, TextRange};
+use rustc_hash::FxHashSet;
 
 use crate::db::RootDatabase;
 
 use super::{NameDefinition, NameKind};
 
 impl NameDefinition {
-    pub(crate) fn search_scope(&self, db: &RootDatabase) -> HashSet<(FileId, Option<TextRange>)> {
+    pub(crate) fn search_scope(&self, db: &RootDatabase) -> FxHashSet<(FileId, Option<TextRange>)> {
         let module_src = self.container.definition_source(db);
         let file_id = module_src.file_id.original_file(db);
 
         if let NameKind::Pat((def, _)) = self.kind {
+            let mut res = FxHashSet::default();
             let range = match def {
                 DefWithBody::Function(f) => f.source(db).ast.syntax().text_range(),
                 DefWithBody::Const(c) => c.source(db).ast.syntax().text_range(),
                 DefWithBody::Static(s) => s.source(db).ast.syntax().text_range(),
             };
-            return [(file_id, Some(range))].iter().cloned().collect();
+            res.insert((file_id, Some(range)));
+            return res;
         }
 
-        if let Some(ref vis) = self.visibility {
-            let vis = vis.syntax().to_string();
-
-            // FIXME: add "pub(in path)"
-
-            if vis.as_str() == "pub(super)" {
-                if let Some(parent_module) = self.container.parent(db) {
-                    let mut files = HashSet::new();
+        let vis =
+            self.visibility.as_ref().map(|v| v.syntax().to_string()).unwrap_or("".to_string());
 
-                    let parent_src = parent_module.definition_source(db);
-                    let file_id = parent_src.file_id.original_file(db);
+        if vis.as_str() == "pub(super)" {
+            if let Some(parent_module) = self.container.parent(db) {
+                let mut files = FxHashSet::default();
+                let parent_src = parent_module.definition_source(db);
+                let file_id = parent_src.file_id.original_file(db);
 
-                    match parent_src.ast {
-                        ModuleSource::Module(m) => {
-                            let range = Some(m.syntax().text_range());
-                            files.insert((file_id, range));
-                        }
-                        ModuleSource::SourceFile(_) => {
-                            files.insert((file_id, None));
-                            files.extend(
-                                parent_module
-                                    .children(db)
-                                    .map(|m| {
-                                        let src = m.definition_source(db);
-                                        (src.file_id.original_file(db), None)
-                                    })
-                                    .collect::<HashSet<_>>(),
-                            );
-                        }
+                match parent_src.ast {
+                    ModuleSource::Module(m) => {
+                        let range = Some(m.syntax().text_range());
+                        files.insert((file_id, range));
+                    }
+                    ModuleSource::SourceFile(_) => {
+                        files.insert((file_id, None));
+                        files.extend(parent_module.children(db).map(|m| {
+                            let src = m.definition_source(db);
+                            (src.file_id.original_file(db), None)
+                        }));
                     }
-                    return files;
-                } else {
-                    let range = match module_src.ast {
-                        ModuleSource::Module(m) => Some(m.syntax().text_range()),
-                        ModuleSource::SourceFile(_) => None,
-                    };
-                    return [(file_id, range)].iter().cloned().collect();
                 }
+                return files;
             }
+        }
 
+        if vis.as_str() != "" {
             let source_root_id = db.file_source_root(file_id);
             let source_root = db.source_root(source_root_id);
-            let mut files = source_root.walk().map(|id| (id.into(), None)).collect::<HashSet<_>>();
+            let mut files =
+                source_root.walk().map(|id| (id.into(), None)).collect::<FxHashSet<_>>();
+
+            // FIXME: add "pub(in path)"
 
             if vis.as_str() == "pub(crate)" {
                 return files;
@@ -74,10 +68,8 @@ impl NameDefinition {
             if vis.as_str() == "pub" {
                 let krate = self.container.krate(db).unwrap();
                 let crate_graph = db.crate_graph();
-
                 for crate_id in crate_graph.iter() {
                     let mut crate_deps = crate_graph.dependencies(crate_id);
-
                     if crate_deps.any(|dep| dep.crate_id() == krate.crate_id()) {
                         let root_file = crate_graph.crate_root(crate_id);
                         let source_root_id = db.file_source_root(root_file);
@@ -85,15 +77,16 @@ impl NameDefinition {
                         files.extend(source_root.walk().map(|id| (id.into(), None)));
                     }
                 }
-
                 return files;
             }
         }
 
+        let mut res = FxHashSet::default();
         let range = match module_src.ast {
             ModuleSource::Module(m) => Some(m.syntax().text_range()),
             ModuleSource::SourceFile(_) => None,
         };
-        [(file_id, range)].iter().cloned().collect()
+        res.insert((file_id, range));
+        res
     }
 }