Repository: rust-analyzer/rust-analyzer
Pull request #517 (2019-01-13 09:28:28)

gracefully handle cycles in crate graph

bors[bot]: # Build failed
  * [continuous-integration/travis-ci/push](https://travis-ci.org/rust-analyzer/rust-analyzer/builds/478968683?utm_source=github_status&utm_medium=notification)
matklad: bors r+
bors[bot]: # Build failed
  * [continuous-integration/travis-ci/push](https://travis-ci.org/rust-analyzer/rust-analyzer/builds/478969782?utm_source=github_status&utm_medium=notification)

Patch:
diff --git a/crates/ra_db/src/input.rs b/crates/ra_db/src/input.rs
index 023183e290..2b761ea0c4 100644
--- a/crates/ra_db/src/input.rs
+++ b/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());
+    }
+}
diff --git a/crates/ra_ide_api/src/runnables.rs b/crates/ra_ide_api/src/runnables.rs
index 53e49da5ba..98b1d2d552 100644
--- a/crates/ra_ide_api/src/runnables.rs
+++ b/crates/ra_ide_api/src/runnables.rs
@@ -17,7 +17,6 @@ pub struct Runnable {
 pub enum RunnableKind {
     Test { name: String },
     TestMod { path: String },
-    Bench { name: String },
     Bin,
 }
 
@@ -49,10 +48,6 @@ fn runnable_fn(fn_def: &ast::FnDef) -> Option<Runnable> {
         RunnableKind::Test {
             name: name.to_string(),
         }
-    } else if fn_def.has_atom_attr("bench") {
-        RunnableKind::Bench {
-            name: name.to_string(),
-        }
     } else {
         return None;
     };
diff --git a/crates/ra_lsp_server/src/cargo_target_spec.rs b/crates/ra_lsp_server/src/cargo_target_spec.rs
index db9496bbe0..a66f14b821 100644
--- a/crates/ra_lsp_server/src/cargo_target_spec.rs
+++ b/crates/ra_lsp_server/src/cargo_target_spec.rs
@@ -32,15 +32,6 @@ pub(crate) fn runnable_args(
             res.push(path.to_string());
             res.push("--nocapture".to_string());
         }
-        RunnableKind::Bench { name } => {
-            res.push("bench".to_string());
-            if let Some(spec) = spec {
-                spec.push_to(&mut res);
-            }
-            res.push("--".to_string());
-            res.push(name.to_string());
-            res.push("--nocapture".to_string());
-        }
         RunnableKind::Bin => {
             res.push("run".to_string());
             if let Some(spec) = spec {
diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs
index 7326a727dc..a781df181b 100644
--- a/crates/ra_lsp_server/src/main_loop/handlers.rs
+++ b/crates/ra_lsp_server/src/main_loop/handlers.rs
@@ -2,23 +2,22 @@ use std::collections::HashMap;
 
 use gen_lsp_server::ErrorCode;
 use languageserver_types::{
-    CodeActionResponse, CodeLens, Command, Diagnostic, DiagnosticSeverity,
-    DocumentFormattingParams, DocumentHighlight, DocumentSymbol, Documentation, FoldingRange,
-    FoldingRangeKind, FoldingRangeParams, Hover, HoverContents, Location, MarkupContent,
-    MarkupKind, ParameterInformation, ParameterLabel, Position, PrepareRenameResponse, Range,
-    RenameParams, SignatureInformation, SymbolInformation, TextDocumentIdentifier, TextEdit,
-    WorkspaceEdit,
+    CodeActionResponse, Command, CodeLens, Diagnostic, DiagnosticSeverity, DocumentFormattingParams,
+    DocumentHighlight, DocumentSymbol, Documentation, FoldingRange, FoldingRangeKind,
+    FoldingRangeParams, Hover, HoverContents, Location, MarkupContent, MarkupKind,
+    ParameterInformation, ParameterLabel, Position, PrepareRenameResponse, Range, RenameParams,
+    SignatureInformation, SymbolInformation, TextDocumentIdentifier, TextEdit, WorkspaceEdit,
 };
 use ra_ide_api::{
-    FileId, FilePosition, FileRange, FoldKind, Query, RangeInfo, RunnableKind, Severity,
+    FileId, FilePosition, FileRange, FoldKind, Query, RunnableKind, Severity, RangeInfo,
 };
-use ra_syntax::{AstNode, TextUnit};
+use ra_syntax::{TextUnit, AstNode};
 use rustc_hash::FxHashMap;
 use serde_json::to_value;
 use std::io::Write;
 
 use crate::{
-    cargo_target_spec::{runnable_args, CargoTargetSpec},
+    cargo_target_spec::{CargoTargetSpec, runnable_args},
     conv::{to_location, to_location_link, Conv, ConvWith, MapConvWith, TryConvWith},
     req::{self, Decoration},
     server_world::ServerWorld,
@@ -259,7 +258,6 @@ pub fn handle_runnables(
             label: match &runnable.kind {
                 RunnableKind::Test { name } => format!("test {}", name),
                 RunnableKind::TestMod { path } => format!("test-mod {}", path),
-                RunnableKind::Bench { name } => format!("bench {}", name),
                 RunnableKind::Bin => "run binary".to_string(),
             },
             bin: "cargo".to_string(),
@@ -588,37 +586,35 @@ pub fn handle_code_lens(
     let mut lenses: Vec<CodeLens> = Default::default();
 
     for runnable in world.analysis().runnables(file_id)? {
-        let title = match &runnable.kind {
-            RunnableKind::Test { name: _ } | RunnableKind::TestMod { path: _ } => Some("Run Test"),
-            RunnableKind::Bench { name: _ } => Some("Run Bench"),
-            _ => None,
-        };
-
-        if let Some(title) = title {
-            let args = runnable_args(&world, file_id, &runnable.kind)?;
-            let range = runnable.range.conv_with(&line_index);
-
-            // This represents the actual command that will be run.
-            let r: req::Runnable = req::Runnable {
-                range,
-                label: Default::default(),
-                bin: "cargo".into(),
-                args,
-                env: Default::default(),
-            };
+        match &runnable.kind {
+            RunnableKind::Test { name: _ } | RunnableKind::TestMod { path: _ } => {
+                let args = runnable_args(&world, file_id, &runnable.kind)?;
+
+                let range = runnable.range.conv_with(&line_index);
+
+                // This represents the actual command that will be run.
+                let r: req::Runnable = req::Runnable {
+                    range,
+                    label: Default::default(),
+                    bin: "cargo".into(),
+                    args,
+                    env: Default::default(),
+                };
 
-            let lens = CodeLens {
-                range,
-                command: Some(Command {
-                    title: title.into(),
-                    command: "ra-lsp.run-single".into(),
-                    arguments: Some(vec![to_value(r).unwrap()]),
-                }),
-                data: None,
-            };
+                let lens = CodeLens {
+                    range,
+                    command: Some(Command {
+                        title: "Run Test".into(),
+                        command: "ra-lsp.run-single".into(),
+                        arguments: Some(vec![to_value(r).unwrap()]),
+                    }),
+                    data: None,
+                };
 
-            lenses.push(lens);
-        }
+                lenses.push(lens);
+            }
+            _ => continue,
+        };
     }
 
     return Ok(Some(lenses));
diff --git a/crates/ra_lsp_server/src/server_world.rs b/crates/ra_lsp_server/src/server_world.rs
index 4f3c231d31..d5dbf999fd 100644
--- a/crates/ra_lsp_server/src/server_world.rs
+++ b/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)
+                                )
+                            }
                         }
                     }
                 }
diff --git a/editors/code/src/commands/apply_source_change.ts b/editors/code/src/commands/apply_source_change.ts
index 675a534c8f..d96ace9795 100644
--- a/editors/code/src/commands/apply_source_change.ts
+++ b/editors/code/src/commands/apply_source_change.ts
@@ -10,9 +10,7 @@ export interface SourceChange {
 }
 
 export async function handle(change: SourceChange) {
-    const wsEdit = Server.client.protocol2CodeConverter.asWorkspaceEdit(
-        change.workspaceEdit
-    );
+    const wsEdit = Server.client.protocol2CodeConverter.asWorkspaceEdit(change.workspaceEdit);
     let created;
     let moved;
     if (change.workspaceEdit.documentChanges) {
diff --git a/editors/code/src/commands/on_enter.ts b/editors/code/src/commands/on_enter.ts
index bb376e3cb4..64401b6840 100644
--- a/editors/code/src/commands/on_enter.ts
+++ b/editors/code/src/commands/on_enter.ts
@@ -6,6 +6,7 @@ import {
     SourceChange
 } from './apply_source_change';
 
+
 export async function handle(event: { text: string }): Promise<boolean> {
     const editor = vscode.window.activeTextEditor;
     if (
diff --git a/editors/code/src/commands/runnables.ts b/editors/code/src/commands/runnables.ts
index aa5817c21f..f9a4e2fc93 100644
--- a/editors/code/src/commands/runnables.ts
+++ b/editors/code/src/commands/runnables.ts
@@ -48,11 +48,7 @@ function createTask(spec: Runnable): vscode.Task {
         cwd: '.',
         env: definition.env
     };
-    const exec = new vscode.ShellExecution(
-        definition.command,
-        definition.args,
-        execOption
-    );
+    const exec = new vscode.ShellExecution(definition.command, definition.args, execOption);
 
     const f = vscode.workspace.workspaceFolders![0];
     const t = new vscode.Task(
@@ -63,7 +59,7 @@ function createTask(spec: Runnable): vscode.Task {
         exec,
         ['$rustc']
     );
-    t.presentationOptions.clear = true;
+    t.presentationOptions.clear = true
     return t;
 }
 
@@ -118,8 +114,8 @@ export async function handleSingle(runnable: Runnable) {
     task.group = vscode.TaskGroup.Build;
     task.presentationOptions = {
         reveal: vscode.TaskRevealKind.Always,
-        panel: vscode.TaskPanelKind.Dedicated
+        panel: vscode.TaskPanelKind.Dedicated,
     };
-
+    
     return vscode.tasks.executeTask(task);
-}
+}
\ No newline at end of file
diff --git a/editors/code/src/extension.ts b/editors/code/src/extension.ts
index 0098c94547..9edfb13b5f 100644
--- a/editors/code/src/extension.ts
+++ b/editors/code/src/extension.ts
@@ -37,10 +37,8 @@ export function activate(context: vscode.ExtensionContext) {
                     return await original(...args);
                 }
             });
-        } catch (_) {
-            vscode.window.showWarningMessage(
-                'Enhanced typing feature is disabled because of incompatibility with VIM extension'
-            );
+        } catch(_) {
+            vscode.window.showWarningMessage('Enhanced typing feature is disabled because of incompatibility with VIM extension');
         }
     }
 


Test patch:
diff --git a/crates/ra_hir/src/nameres/tests.rs b/crates/ra_hir/src/nameres/tests.rs
index ba9fcb3d1d..647fd92aad 100644
--- a/crates/ra_hir/src/nameres/tests.rs
+++ b/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);