Merge #410

410: Detangle symbols r=matklad a=matklad

Previously, we used `FileSymbol` both to represent bytes which are stored in the index and as an API of `ra_analysis`. Mixing internal storage format and an API is not a really bright idea, so we introduce `NavigationTarget` to handle API part.

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

crates/ra_analysis/src/imp.rs

@@ -8,11 +8,11 @@
 use ra_db::{FilesDatabase, SourceRoot, SourceRootId, SyntaxDatabase};
 use ra_editor::{self, find_node_at_offset, LocalEdit, Severity};
 use ra_syntax::{
-    algo::find_covering_node,
+    algo::{find_covering_node, visit::{visitor, Visitor}},
     ast::{self, ArgListOwner, Expr, FnDef, NameOwner},
     AstNode, SourceFileNode,
     SyntaxKind::*,
-    SyntaxNodeRef, TextRange, TextUnit,
+    SyntaxNode, SyntaxNodeRef, TextRange, TextUnit,
 };
 
 use crate::{
@@ -116,12 +116,13 @@ pub(crate) fn parent_module(
         };
         let decl = decl.borrowed();
         let decl_name = decl.name().unwrap();
-        let symbol = FileSymbol {
+        Ok(vec![NavigationTarget {
+            file_id,
             name: decl_name.text(),
-            node_range: decl_name.syntax().range(),
+            range: decl_name.syntax().range(),
             kind: MODULE,
-        };
+            ptr: None,
-        Ok(vec![NavigationTarget { file_id, symbol }])
+        }])
     }
     /// Returns `Vec` for the same reason as `parent_module`
     pub(crate) fn crate_for(&self, file_id: FileId) -> Cancelable<Vec<CrateId>> {
@@ -153,14 +154,13 @@ pub(crate) fn approximately_resolve_symbol(
                 let scope = fn_descr.scopes(self);
                 // First try to resolve the symbol locally
                 if let Some(entry) = scope.resolve_local_name(name_ref) {
-                    rr.add_resolution(
+                    rr.resolves_to.push(NavigationTarget {
-                        position.file_id,
+                        file_id: position.file_id,
-                        FileSymbol {
+                        name: entry.name().to_string().into(),
-                            name: entry.name().to_string().into(),
+                        range: entry.ptr().range(),
-                            node_range: entry.ptr().range(),
+                        kind: NAME,
-                            kind: NAME,
+                        ptr: None,
-                        },
+                    });
-                    );
                     return Ok(Some(rr));
                 };
             }
@@ -182,12 +182,14 @@ pub(crate) fn approximately_resolve_symbol(
                             Some(name) => name.to_string().into(),
                             None => "".into(),
                         };
-                        let symbol = FileSymbol {
+                        let symbol = NavigationTarget {
+                            file_id,
                             name,
-                            node_range: TextRange::offset_len(0.into(), 0.into()),
+                            range: TextRange::offset_len(0.into(), 0.into()),
                             kind: MODULE,
+                            ptr: None,
                         };
-                        rr.add_resolution(file_id, symbol);
+                        rr.resolves_to.push(symbol);
                         return Ok(Some(rr));
                     }
                 }
@@ -253,8 +255,7 @@ fn find_binding<'a>(
         }
     }
     pub(crate) fn doc_text_for(&self, nav: NavigationTarget) -> Cancelable<Option<String>> {
-        let file = self.source_file(nav.file_id);
+        let result = match (nav.description(self), nav.docs(self)) {
-        let result = match (nav.symbol.description(&file), nav.symbol.docs(&file)) {
             (Some(desc), Some(docs)) => {
                 Some("```rust\n".to_string() + &*desc + "\n```\n\n" + &*docs)
             }
@@ -362,52 +363,52 @@ pub(crate) fn resolve_callable(
         // Resolve the function's NameRef (NOTE: this isn't entirely accurate).
         let file_symbols = self.index_resolve(name_ref)?;
         for (fn_file_id, fs) in file_symbols {
-            if fs.kind == FN_DEF {
+            if fs.ptr.kind() == FN_DEF {
                 let fn_file = self.source_file(fn_file_id);
-                if let Some(fn_def) = find_node_at_offset(fn_file.syntax(), fs.node_range.start()) {
+                let fn_def = fs.ptr.resolve(&fn_file);
-                    let descr = ctry!(source_binder::function_from_source(
+                let fn_def = ast::FnDef::cast(fn_def.borrowed()).unwrap();
-                        self, fn_file_id, fn_def
+                let descr = ctry!(source_binder::function_from_source(
-                    )?);
+                    self, fn_file_id, fn_def
-                    if let Some(descriptor) = descr.signature_info(self) {
+                )?);
-                        // If we have a calling expression let's find which argument we are on
+                if let Some(descriptor) = descr.signature_info(self) {
-                        let mut current_parameter = None;
+                    // If we have a calling expression let's find which argument we are on
+                    let mut current_parameter = None;
 
-                        let num_params = descriptor.params.len();
+                    let num_params = descriptor.params.len();
-                        let has_self = fn_def.param_list().and_then(|l| l.self_param()).is_some();
+                    let has_self = fn_def.param_list().and_then(|l| l.self_param()).is_some();
 
-                        if num_params == 1 {
+                    if num_params == 1 {
-                            if !has_self {
+                        if !has_self {
-                                current_parameter = Some(0);
+                            current_parameter = Some(0);
-                            }
-                        } else if num_params > 1 {
-                            // Count how many parameters into the call we are.
-                            // TODO: This is best effort for now and should be fixed at some point.
-                            // It may be better to see where we are in the arg_list and then check
-                            // where offset is in that list (or beyond).
-                            // Revisit this after we get documentation comments in.
-                            if let Some(ref arg_list) = calling_node.arg_list() {
-                                let start = arg_list.syntax().range().start();
-
-                                let range_search = TextRange::from_to(start, position.offset);
-                                let mut commas: usize = arg_list
-                                    .syntax()
-                                    .text()
-                                    .slice(range_search)
-                                    .to_string()
-                                    .matches(',')
-                                    .count();
-
-                                // If we have a method call eat the first param since it's just self.
-                                if has_self {
-                                    commas += 1;
-                                }
-
-                                current_parameter = Some(commas);
-                            }
                         }
+                    } else if num_params > 1 {
+                        // Count how many parameters into the call we are.
+                        // TODO: This is best effort for now and should be fixed at some point.
+                        // It may be better to see where we are in the arg_list and then check
+                        // where offset is in that list (or beyond).
+                        // Revisit this after we get documentation comments in.
+                        if let Some(ref arg_list) = calling_node.arg_list() {
+                            let start = arg_list.syntax().range().start();
 
-                        return Ok(Some((descriptor, current_parameter)));
+                            let range_search = TextRange::from_to(start, position.offset);
+                            let mut commas: usize = arg_list
+                                .syntax()
+                                .text()
+                                .slice(range_search)
+                                .to_string()
+                                .matches(',')
+                                .count();
+
+                            // If we have a method call eat the first param since it's just self.
+                            if has_self {
+                                commas += 1;
+                            }
+
+                            current_parameter = Some(commas);
+                        }
                     }
+
+                    return Ok(Some((descriptor, current_parameter)));
                 }
             }
         }
@@ -511,3 +512,91 @@ pub fn arg_list(&self) -> Option<ast::ArgList> {
         }
     }
 }
+
+impl NavigationTarget {
+    fn node(&self, db: &db::RootDatabase) -> Option<SyntaxNode> {
+        let source_file = db.source_file(self.file_id);
+        let source_file = source_file.syntax();
+        let node = source_file
+            .descendants()
+            .find(|node| node.kind() == self.kind && node.range() == self.range)?
+            .owned();
+        Some(node)
+    }
+
+    fn docs(&self, db: &db::RootDatabase) -> Option<String> {
+        let node = self.node(db)?;
+        let node = node.borrowed();
+        fn doc_comments<'a, N: ast::DocCommentsOwner<'a>>(node: N) -> Option<String> {
+            let comments = node.doc_comment_text();
+            if comments.is_empty() {
+                None
+            } else {
+                Some(comments)
+            }
+        }
+
+        visitor()
+            .visit(doc_comments::<ast::FnDef>)
+            .visit(doc_comments::<ast::StructDef>)
+            .visit(doc_comments::<ast::EnumDef>)
+            .visit(doc_comments::<ast::TraitDef>)
+            .visit(doc_comments::<ast::Module>)
+            .visit(doc_comments::<ast::TypeDef>)
+            .visit(doc_comments::<ast::ConstDef>)
+            .visit(doc_comments::<ast::StaticDef>)
+            .accept(node)?
+    }
+
+    /// Get a description of this node.
+    ///
+    /// e.g. `struct Name`, `enum Name`, `fn Name`
+    fn description(&self, db: &db::RootDatabase) -> Option<String> {
+        // TODO: After type inference is done, add type information to improve the output
+        let node = self.node(db)?;
+        let node = node.borrowed();
+        // TODO: Refactor to be have less repetition
+        visitor()
+            .visit(|node: ast::FnDef| {
+                let mut string = "fn ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::StructDef| {
+                let mut string = "struct ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::EnumDef| {
+                let mut string = "enum ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::TraitDef| {
+                let mut string = "trait ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::Module| {
+                let mut string = "mod ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::TypeDef| {
+                let mut string = "type ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::ConstDef| {
+                let mut string = "const ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::StaticDef| {
+                let mut string = "static ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .accept(node)?
+    }
+}

crates/ra_analysis/src/lib.rs

@@ -1,6 +1,8 @@
-//! ra_analyzer crate is the brain of Rust analyzer. It relies on the `salsa`
+//! ra_analyzer crate provides "ide-centric" APIs for the rust-analyzer. What
-//! crate, which provides and incremental on-demand database of facts.
+//! powers this API are the `RootDatabase` struct, which defines a `salsa`
-
+//! database, and the `ra_hir` crate, where majority of the analysis happens.
+//! However, IDE specific bits of the analysis (most notably completion) happen
+//! in this crate.
 macro_rules! ctry {
     ($expr:expr) => {
         match $expr {
@@ -41,7 +43,7 @@ macro_rules! ctry {
 pub use hir::FnSignatureInfo;
 
 pub use ra_db::{
-    Canceled, Cancelable, FilePosition, FileRange,
+    Canceled, Cancelable, FilePosition, FileRange, LocalSyntaxPtr,
     CrateGraph, CrateId, SourceRootId, FileId, SyntaxDatabase, FilesDatabase
 };
 
@@ -219,24 +221,42 @@ pub fn limit(&mut self, limit: usize) {
     }
 }
 
+/// `NavigationTarget` represents and element in the editor's UI whihc you can
+/// click on to navigate to a particular piece of code.
+///
+/// Typically, a `NavigationTarget` corresponds to some element in the source
+/// code, like a function or a struct, but this is not strictly required.
 #[derive(Debug)]
 pub struct NavigationTarget {
     file_id: FileId,
-    symbol: FileSymbol,
+    name: SmolStr,
+    kind: SyntaxKind,
+    range: TextRange,
+    // Should be DefId ideally
+    ptr: Option<LocalSyntaxPtr>,
 }
 
 impl NavigationTarget {
-    pub fn name(&self) -> SmolStr {
+    fn from_symbol(file_id: FileId, symbol: FileSymbol) -> NavigationTarget {
-        self.symbol.name.clone()
+        NavigationTarget {
+            name: symbol.name.clone(),
+            kind: symbol.ptr.kind(),
+            file_id,
+            range: symbol.ptr.range(),
+            ptr: Some(symbol.ptr.clone()),
+        }
+    }
+    pub fn name(&self) -> &SmolStr {
+        &self.name
     }
     pub fn kind(&self) -> SyntaxKind {
-        self.symbol.kind
+        self.kind
     }
     pub fn file_id(&self) -> FileId {
         self.file_id
     }
     pub fn range(&self) -> TextRange {
-        self.symbol.node_range
+        self.range
     }
 }
 
@@ -260,7 +280,8 @@ fn new(reference_range: TextRange) -> ReferenceResolution {
     }
 
     fn add_resolution(&mut self, file_id: FileId, symbol: FileSymbol) {
-        self.resolves_to.push(NavigationTarget { file_id, symbol })
+        self.resolves_to
+            .push(NavigationTarget::from_symbol(file_id, symbol))
     }
 }
 
@@ -359,7 +380,7 @@ pub fn folding_ranges(&self, file_id: FileId) -> Vec<Fold> {
     pub fn symbol_search(&self, query: Query) -> Cancelable<Vec<NavigationTarget>> {
         let res = symbol_index::world_symbols(&*self.db, query)?
             .into_iter()
-            .map(|(file_id, symbol)| NavigationTarget { file_id, symbol })
+            .map(|(file_id, symbol)| NavigationTarget::from_symbol(file_id, symbol))
             .collect();
         Ok(res)
     }

crates/ra_analysis/src/symbol_index.rs

@@ -1,3 +1,24 @@
+//! This module handles fuzzy-searching of functions, structs and other symbols
+//! by name across the whole workspace and dependencies.
+//!
+//! It works by building an incrementally-updated text-search index of all
+//! symbols. The backbone of the index is the **awesome** `fst` crate by
+//! @BurntSushi.
+//!
+//! In a nutshell, you give a set of strings to the `fst`, and it builds a
+//! finite state machine describing this set of strtings. The strings which
+//! could fuzzy-match a pattern can also be described by a finite state machine.
+//! What is freakingly cool is that you can now traverse both state machines in
+//! lock-step to enumerate the strings which are both in the input set and
+//! fuzz-match the query. Or, more formally, given two langauges described by
+//! fsts, one can build an product fst which describes the intersection of the
+//! languages.
+//!
+//! `fst` does not support cheap updating of the index, but it supports unioning
+//! of state machines. So, to account for changing source code, we build an fst
+//! for each library (which is assumed to never change) and an fst for each rust
+//! file in the current workspace, and run a query aginst the union of all
+//! thouse fsts.
 use std::{
     hash::{Hash, Hasher},
     sync::Arc,
@@ -5,12 +26,12 @@
 
 use fst::{self, Streamer};
 use ra_syntax::{
-    AstNode, SyntaxNodeRef, SourceFileNode, SmolStr, TextRange,
+    SyntaxNodeRef, SourceFileNode, SmolStr,
     algo::visit::{visitor, Visitor},
     SyntaxKind::{self, *},
-    ast::{self, NameOwner, DocCommentsOwner},
+    ast::{self, NameOwner},
 };
-use ra_db::{SyntaxDatabase, SourceRootId, FilesDatabase};
+use ra_db::{SyntaxDatabase, SourceRootId, FilesDatabase, LocalSyntaxPtr};
 use salsa::ParallelDatabase;
 use rayon::prelude::*;
 
@@ -140,7 +161,7 @@ pub(crate) fn search(self, indices: &[Arc<SymbolIndex>]) -> Vec<(FileId, FileSym
                 let idx = indexed_value.value as usize;
 
                 let (file_id, symbol) = &file_symbols.symbols[idx];
-                if self.only_types && !is_type(symbol.kind) {
+                if self.only_types && !is_type(symbol.ptr.kind()) {
                     continue;
                 }
                 if self.exact && symbol.name != self.query {
@@ -160,96 +181,12 @@ fn is_type(kind: SyntaxKind) -> bool {
     }
 }
 
+/// The actual data that is stored in the index. It should be as compact as
+/// possible.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub(crate) struct FileSymbol {
     pub(crate) name: SmolStr,
-    pub(crate) node_range: TextRange,
+    pub(crate) ptr: LocalSyntaxPtr,
-    pub(crate) kind: SyntaxKind,
-}
-
-impl FileSymbol {
-    pub(crate) fn docs(&self, file: &SourceFileNode) -> Option<String> {
-        file.syntax()
-            .descendants()
-            .filter(|node| node.kind() == self.kind && node.range() == self.node_range)
-            .filter_map(|node: SyntaxNodeRef| {
-                fn doc_comments<'a, N: DocCommentsOwner<'a>>(node: N) -> Option<String> {
-                    let comments = node.doc_comment_text();
-                    if comments.is_empty() {
-                        None
-                    } else {
-                        Some(comments)
-                    }
-                }
-
-                visitor()
-                    .visit(doc_comments::<ast::FnDef>)
-                    .visit(doc_comments::<ast::StructDef>)
-                    .visit(doc_comments::<ast::EnumDef>)
-                    .visit(doc_comments::<ast::TraitDef>)
-                    .visit(doc_comments::<ast::Module>)
-                    .visit(doc_comments::<ast::TypeDef>)
-                    .visit(doc_comments::<ast::ConstDef>)
-                    .visit(doc_comments::<ast::StaticDef>)
-                    .accept(node)?
-            })
-            .nth(0)
-    }
-    /// Get a description of this node.
-    ///
-    /// e.g. `struct Name`, `enum Name`, `fn Name`
-    pub(crate) fn description(&self, file: &SourceFileNode) -> Option<String> {
-        // TODO: After type inference is done, add type information to improve the output
-        file.syntax()
-            .descendants()
-            .filter(|node| node.kind() == self.kind && node.range() == self.node_range)
-            .filter_map(|node: SyntaxNodeRef| {
-                // TODO: Refactor to be have less repetition
-                visitor()
-                    .visit(|node: ast::FnDef| {
-                        let mut string = "fn ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::StructDef| {
-                        let mut string = "struct ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::EnumDef| {
-                        let mut string = "enum ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::TraitDef| {
-                        let mut string = "trait ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::Module| {
-                        let mut string = "mod ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::TypeDef| {
-                        let mut string = "type ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::ConstDef| {
-                        let mut string = "const ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .visit(|node: ast::StaticDef| {
-                        let mut string = "static ".to_string();
-                        node.name()?.syntax().text().push_to(&mut string);
-                        Some(string)
-                    })
-                    .accept(node)?
-            })
-            .nth(0)
-    }
 }
 
 fn to_symbol(node: SyntaxNodeRef) -> Option<FileSymbol> {
@@ -257,8 +194,7 @@ fn decl<'a, N: NameOwner<'a>>(node: N) -> Option<FileSymbol> {
         let name = node.name()?;
         Some(FileSymbol {
             name: name.text(),
-            node_range: node.syntax().range(),
+            ptr: LocalSyntaxPtr::new(node.syntax()),
-            kind: node.syntax().kind(),
         })
     }
     visitor()

crates/ra_analysis/tests/tests.rs

@@ -25,7 +25,7 @@ enum E { X(Foo<|>) }
     assert_eq_dbg(
         r#"ReferenceResolution {
             reference_range: [23; 26),
-            resolves_to: [NavigationTarget { file_id: FileId(1), symbol: FileSymbol { name: "Foo", node_range: [0; 11), kind: STRUCT_DEF } }]
+            resolves_to: [NavigationTarget { file_id: FileId(1), name: "Foo", kind: STRUCT_DEF, range: [0; 11), ptr: Some(LocalSyntaxPtr { range: [0; 11), kind: STRUCT_DEF }) }]
         }"#,
         &symbols,
     );
@@ -46,7 +46,7 @@ fn test_resolve_module() {
     assert_eq_dbg(
         r#"ReferenceResolution {
             reference_range: [4; 7),
-            resolves_to: [NavigationTarget { file_id: FileId(2), symbol: FileSymbol { name: "foo", node_range: [0; 0), kind: MODULE } }]
+            resolves_to: [NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]
         }"#,
         &symbols,
     );
@@ -64,7 +64,7 @@ fn test_resolve_module() {
     assert_eq_dbg(
         r#"ReferenceResolution {
             reference_range: [4; 7),
-            resolves_to: [NavigationTarget { file_id: FileId(2), symbol: FileSymbol { name: "foo", node_range: [0; 0), kind: MODULE } }]
+            resolves_to: [NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]
         }"#,
         &symbols,
     );
@@ -107,7 +107,7 @@ fn test_resolve_parent_module() {
     );
     let symbols = analysis.parent_module(pos).unwrap();
     assert_eq_dbg(
-        r#"[NavigationTarget { file_id: FileId(1), symbol: FileSymbol { name: "foo", node_range: [4; 7), kind: MODULE } }]"#,
+        r#"[NavigationTarget { file_id: FileId(1), name: "foo", kind: MODULE, range: [4; 7), ptr: None }]"#,
         &symbols,
     );
 }
@@ -126,7 +126,7 @@ mod baz { <|> }
     );
     let symbols = analysis.parent_module(pos).unwrap();
     assert_eq_dbg(
-        r#"[NavigationTarget { file_id: FileId(1), symbol: FileSymbol { name: "bar", node_range: [18; 21), kind: MODULE } }]"#,
+        r#"[NavigationTarget { file_id: FileId(1), name: "bar", kind: MODULE, range: [18; 21), ptr: None }]"#,
         &symbols,
     );
 }

crates/ra_db/src/syntax_ptr.rs

@@ -31,6 +31,10 @@ pub fn resolve(self, file: &SourceFileNode) -> SyntaxNode {
     pub fn range(self) -> TextRange {
         self.range
     }
+
+    pub fn kind(self) -> SyntaxKind {
+        self.kind
+    }
 }
 
 #[test]

crates/ra_lsp_server/src/main_loop/handlers.rs

@@ -190,7 +190,7 @@ fn exec_query(world: &ServerWorld, query: Query) -> Result<Vec<SymbolInformation
         let mut res = Vec::new();
         for nav in world.analysis().symbol_search(query)? {
             let info = SymbolInformation {
-                name: nav.name().into(),
+                name: nav.name().to_string(),
                 kind: nav.kind().conv(),
                 location: nav.try_conv_with(world)?,
                 container_name: None,