Merge #517 #518

517: gracefully handle cycles in crate graph r=matklad a=matklad



518: Add an explanatory message when we use the Query fallback r=matklad a=DJMcNab

Fixes https://github.com/rust-analyzer/rust-analyzer/issues/269.

There is no good way to explain it for go_to_def, so I've just fallen back on to_vec.

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>

crates/ra_db/src/input.rs

@@ -53,6 +53,9 @@ pub struct CrateGraph {
     arena: FxHashMap<CrateId, CrateData>,
 }
 
+#[derive(Debug)]
+pub struct CyclicDependencies;
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct CrateId(pub u32);
 
@@ -94,12 +97,16 @@ impl CrateGraph {
         assert!(prev.is_none());
         crate_id
     }
-    pub fn add_dep(&mut self, from: CrateId, name: SmolStr, to: CrateId) {
-        let mut visited = FxHashSet::default();
-        if self.dfs_find(from, to, &mut visited) {
-            panic!("Cycle dependencies found.")
+    pub fn add_dep(
+        &mut self,
+        from: CrateId,
+        name: SmolStr,
+        to: CrateId,
+    ) -> Result<(), CyclicDependencies> {
+        if self.dfs_find(from, to, &mut FxHashSet::default()) {
+            return Err(CyclicDependencies);
         }
-        self.arena.get_mut(&from).unwrap().add_dep(name, to)
+        Ok(self.arena.get_mut(&from).unwrap().add_dep(name, to))
     }
     pub fn is_empty(&self) -> bool {
         self.arena.is_empty()
@@ -139,35 +146,6 @@ impl CrateGraph {
     }
 }
 
-#[cfg(test)]
-mod tests {
-    use super::{CrateGraph, FxHashMap, FileId, SmolStr};
-
-    #[test]
-    #[should_panic]
-    fn it_should_painc_because_of_cycle_dependencies() {
-        let mut graph = CrateGraph::default();
-        let crate1 = graph.add_crate_root(FileId(1u32));
-        let crate2 = graph.add_crate_root(FileId(2u32));
-        let crate3 = graph.add_crate_root(FileId(3u32));
-        graph.add_dep(crate1, SmolStr::new("crate2"), crate2);
-        graph.add_dep(crate2, SmolStr::new("crate3"), crate3);
-        graph.add_dep(crate3, SmolStr::new("crate1"), crate1);
-    }
-
-    #[test]
-    fn it_works() {
-        let mut graph = CrateGraph {
-            arena: FxHashMap::default(),
-        };
-        let crate1 = graph.add_crate_root(FileId(1u32));
-        let crate2 = graph.add_crate_root(FileId(2u32));
-        let crate3 = graph.add_crate_root(FileId(3u32));
-        graph.add_dep(crate1, SmolStr::new("crate2"), crate2);
-        graph.add_dep(crate2, SmolStr::new("crate3"), crate3);
-    }
-}
-
 salsa::query_group! {
     pub trait FilesDatabase: salsa::Database {
         /// Text of the file.
@@ -209,3 +187,39 @@ salsa::query_group! {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{CrateGraph, FileId, SmolStr};
+
+    #[test]
+    fn it_should_painc_because_of_cycle_dependencies() {
+        let mut graph = CrateGraph::default();
+        let crate1 = graph.add_crate_root(FileId(1u32));
+        let crate2 = graph.add_crate_root(FileId(2u32));
+        let crate3 = graph.add_crate_root(FileId(3u32));
+        assert!(graph
+            .add_dep(crate1, SmolStr::new("crate2"), crate2)
+            .is_ok());
+        assert!(graph
+            .add_dep(crate2, SmolStr::new("crate3"), crate3)
+            .is_ok());
+        assert!(graph
+            .add_dep(crate3, SmolStr::new("crate1"), crate1)
+            .is_err());
+    }
+
+    #[test]
+    fn it_works() {
+        let mut graph = CrateGraph::default();
+        let crate1 = graph.add_crate_root(FileId(1u32));
+        let crate2 = graph.add_crate_root(FileId(2u32));
+        let crate3 = graph.add_crate_root(FileId(3u32));
+        assert!(graph
+            .add_dep(crate1, SmolStr::new("crate2"), crate2)
+            .is_ok());
+        assert!(graph
+            .add_dep(crate2, SmolStr::new("crate3"), crate3)
+            .is_ok());
+    }
+}

crates/ra_hir/src/nameres/tests.rs

@@ -235,7 +235,9 @@ fn item_map_across_crates() {
     let mut crate_graph = CrateGraph::default();
     let main_crate = crate_graph.add_crate_root(main_id);
     let lib_crate = crate_graph.add_crate_root(lib_id);
-    crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
+    crate_graph
+        .add_dep(main_crate, "test_crate".into(), lib_crate)
+        .unwrap();
 
     db.set_crate_graph(crate_graph);
 
@@ -288,7 +290,9 @@ fn import_across_source_roots() {
     let mut crate_graph = CrateGraph::default();
     let main_crate = crate_graph.add_crate_root(main_id);
     let lib_crate = crate_graph.add_crate_root(lib_id);
-    crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
+    crate_graph
+        .add_dep(main_crate, "test_crate".into(), lib_crate)
+        .unwrap();
 
     db.set_crate_graph(crate_graph);
 
@@ -330,7 +334,9 @@ fn reexport_across_crates() {
     let mut crate_graph = CrateGraph::default();
     let main_crate = crate_graph.add_crate_root(main_id);
     let lib_crate = crate_graph.add_crate_root(lib_id);
-    crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
+    crate_graph
+        .add_dep(main_crate, "test_crate".into(), lib_crate)
+        .unwrap();
 
     db.set_crate_graph(crate_graph);
 

crates/ra_ide_api/src/goto_definition.rs

@@ -13,8 +13,11 @@ pub(crate) fn goto_definition(
     let file = db.source_file(position.file_id);
     let syntax = file.syntax();
     if let Some(name_ref) = find_node_at_offset::<ast::NameRef>(syntax, position.offset) {
-        let navs = reference_definition(db, position.file_id, name_ref)?;
-        return Ok(Some(RangeInfo::new(name_ref.syntax().range(), navs)));
+        let navs = reference_definition(db, position.file_id, name_ref)?.to_vec();
+        return Ok(Some(RangeInfo::new(
+            name_ref.syntax().range(),
+            navs.to_vec(),
+        )));
     }
     if let Some(name) = find_node_at_offset::<ast::Name>(syntax, position.offset) {
         let navs = ctry!(name_definition(db, position.file_id, name)?);
@@ -23,11 +26,27 @@ pub(crate) fn goto_definition(
     Ok(None)
 }
 
+pub(crate) enum ReferenceResult {
+    Exact(NavigationTarget),
+    Approximate(Vec<NavigationTarget>),
+}
+
+impl ReferenceResult {
+    fn to_vec(self) -> Vec<NavigationTarget> {
+        use self::ReferenceResult::*;
+        match self {
+            Exact(target) => vec![target],
+            Approximate(vec) => vec,
+        }
+    }
+}
+
 pub(crate) fn reference_definition(
     db: &RootDatabase,
     file_id: FileId,
     name_ref: &ast::NameRef,
-) -> Cancelable<Vec<NavigationTarget>> {
+) -> Cancelable<ReferenceResult> {
+    use self::ReferenceResult::*;
     if let Some(fn_descr) =
         hir::source_binder::function_from_child_node(db, file_id, name_ref.syntax())?
     {
@@ -35,7 +54,7 @@ pub(crate) fn reference_definition(
         // First try to resolve the symbol locally
         if let Some(entry) = scope.resolve_local_name(name_ref) {
             let nav = NavigationTarget::from_scope_entry(file_id, &entry);
-            return Ok(vec![nav]);
+            return Ok(Exact(nav));
         };
     }
     // Then try module name resolution
@@ -51,7 +70,7 @@ pub(crate) fn reference_definition(
             let resolved = module.resolve_path(db, &path)?;
             if let Some(def_id) = resolved.take_types().or(resolved.take_values()) {
                 if let Some(target) = NavigationTarget::from_def(db, def_id.resolve(db)?)? {
-                    return Ok(vec![target]);
+                    return Ok(Exact(target));
                 }
             }
         }
@@ -62,7 +81,7 @@ pub(crate) fn reference_definition(
         .into_iter()
         .map(NavigationTarget::from_symbol)
         .collect();
-    Ok(navs)
+    Ok(Approximate(navs))
 }
 
 fn name_definition(

crates/ra_ide_api/src/hover.rs

@@ -16,9 +16,16 @@ pub(crate) fn hover(
 
     let mut range = None;
     if let Some(name_ref) = find_node_at_offset::<ast::NameRef>(file.syntax(), position.offset) {
-        let navs = crate::goto_definition::reference_definition(db, position.file_id, name_ref)?;
-        for nav in navs {
-            res.extend(doc_text_for(db, nav)?)
+        use crate::goto_definition::{ReferenceResult::*, reference_definition};
+        let ref_result = reference_definition(db, position.file_id, name_ref)?;
+        match ref_result {
+            Exact(nav) => res.extend(doc_text_for(db, nav)?),
+            Approximate(navs) => {
+                res.push("Failed to exactly resolve the symbol. This is probably because rust_analyzer does not yet support glob imports or traits.  \nThese methods were found instead:".to_string());
+                for nav in navs {
+                    res.extend(doc_text_for(db, nav)?)
+                }
+            }
         }
         if !res.is_empty() {
             range = Some(name_ref.syntax().range())
@@ -34,7 +41,7 @@ pub(crate) fn hover(
             file_id: position.file_id,
             range: node.range(),
         };
-        res.extend(type_of(db, frange)?);
+        res.extend(type_of(db, frange)?.map(Into::into));
         range = Some(node.range());
     };
 

crates/ra_lsp_server/src/server_world.rs

@@ -73,7 +73,9 @@ impl ServerWorldState {
                     if let (Some(&from), Some(&to)) =
                         (sysroot_crates.get(&from), sysroot_crates.get(&to))
                     {
-                        crate_graph.add_dep(from, name.clone(), to);
+                        if let Err(_) = crate_graph.add_dep(from, name.clone(), to) {
+                            log::error!("cyclic dependency between sysroot crates")
+                        }
                     }
                 }
             }
@@ -108,11 +110,20 @@ impl ServerWorldState {
                 for &from in pkg_crates.get(&pkg).into_iter().flatten() {
                     if let Some(to) = lib_tgt {
                         if to != from {
-                            crate_graph.add_dep(from, pkg.name(&ws.cargo).into(), to);
+                            if let Err(_) =
+                                crate_graph.add_dep(from, pkg.name(&ws.cargo).into(), to)
+                            {
+                                log::error!(
+                                    "cyclic dependency between targets of {}",
+                                    pkg.name(&ws.cargo)
+                                )
+                            }
                         }
                     }
                     if let Some(std) = libstd {
-                        crate_graph.add_dep(from, "std".into(), std);
+                        if let Err(_) = crate_graph.add_dep(from, "std".into(), std) {
+                            log::error!("cyclic dependency on std for {}", pkg.name(&ws.cargo))
+                        }
                     }
                 }
             }
@@ -123,7 +134,13 @@ impl ServerWorldState {
                 for dep in pkg.dependencies(&ws.cargo) {
                     if let Some(&to) = pkg_to_lib_crate.get(&dep.pkg) {
                         for &from in pkg_crates.get(&pkg).into_iter().flatten() {
-                            crate_graph.add_dep(from, dep.name.clone(), to);
+                            if let Err(_) = crate_graph.add_dep(from, dep.name.clone(), to) {
+                                log::error!(
+                                    "cyclic dependency {} -> {}",
+                                    pkg.name(&ws.cargo),
+                                    dep.pkg.name(&ws.cargo)
+                                )
+                            }
                         }
                     }
                 }