From 792dabc0a6a198d0fdeb6449b457a4e61009178e Mon Sep 17 00:00:00 2001
From: gfreezy <gfreezy@gmail.com>
Date: Fri, 21 Dec 2018 22:27:04 +0800
Subject: [PATCH] When constructing a crate graph, detect and forbid cycles.

fixed #300
---
 crates/ra_db/src/input.rs | 58 +++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/crates/ra_db/src/input.rs b/crates/ra_db/src/input.rs
index f12dd93454..f220eda9ef 100644
--- a/crates/ra_db/src/input.rs
+++ b/crates/ra_db/src/input.rs
@@ -7,11 +7,12 @@
 /// actual IO is done and lowered to input.
 use std::sync::Arc;
 
-use rustc_hash::{FxHashMap};
 use relative_path::RelativePathBuf;
-use ra_syntax::SmolStr;
+use rustc_hash::FxHashMap;
 use salsa;
 
+use ra_syntax::SmolStr;
+
 /// `FileId` is an integer which uniquely identifies a file. File paths are
 /// messy and system-dependent, so most of the code should work directly with
 /// `FileId`, without inspecting the path. The mapping between `FileId` and path
@@ -92,10 +93,10 @@ impl CrateGraph {
         assert!(prev.is_none());
         crate_id
     }
-    // FIXME: check that we don't have cycles here.
-    // Just a simple depth first search from `to` should work,
-    // the graph is small.
     pub fn add_dep(&mut self, from: CrateId, name: SmolStr, to: CrateId) {
+        if self.dfs(from, to) {
+           panic!("Cycle dependencies found.")
+        }
         self.arena.get_mut(&from).unwrap().add_dep(name, to)
     }
     pub fn crate_root(&self, crate_id: CrateId) -> FileId {
@@ -111,11 +112,56 @@ impl CrateGraph {
     pub fn dependencies<'a>(
         &'a self,
         crate_id: CrateId,
-    ) -> impl Iterator<Item = &'a Dependency> + 'a {
+    ) -> impl Iterator<Item=&'a Dependency> + 'a {
         self.arena[&crate_id].dependencies.iter()
     }
+    fn dfs(&self, target: CrateId, from: CrateId) -> bool {
+        for dep in self.dependencies(from) {
+            let crate_id = dep.crate_id();
+            if crate_id == target {
+                return true;
+            }
+            if self.arena.contains_key(&crate_id) {
+                if self.dfs(target, crate_id) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
 }
 
+
+mod test {
+    use super::{CrateGraph, FxHashMap, FileId, SmolStr};
+    #[test]
+    #[should_panic]
+    fn it_should_painc_because_of_cycle_dependencies() {
+        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);
+        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.