Add cycle detection. See #22.

src/error.rs

@@ -76,6 +76,9 @@ impl fmt::Display for Error {
                 write!(f, "Can't insert item {} into nonexistent parent {}",
                        child_guid, parent_guid)
             },
+            ErrorKind::Cycle(guid) => {
+                write!(f, "Item {} can't contain itself", guid)
+            },
             ErrorKind::MergeConflict => {
                 write!(f, "Local tree changed during merge")
             },
@@ -103,6 +106,7 @@ pub enum ErrorKind {
     InvalidParent(Guid, Guid),
     MissingParent(Guid, Guid),
     MissingItem(Guid),
+    Cycle(Guid),
     MergeConflict,
     UnmergedLocalItems,
     UnmergedRemoteItems,

src/tests.rs

@@ -2134,3 +2134,31 @@ fn moved_user_content_roots() {
 
     assert_eq!(merger.telemetry(), &expected_telem);
 }
+
+#[test]
+fn cycle() {
+    before_each();
+
+    // Try to create a cycle: move A into B, and B into the menu, but keep
+    // B's parent by children as A.
+    let mut b = nodes!({
+        ("menu________", Folder)
+    }).into_builder().unwrap();
+
+    b.item(Item::new("folderAAAAAA".into(), Kind::Folder))
+     .and_then(|p| p.by_parent_guid("folderBBBBBB".into()))
+     .expect("Should insert A");
+
+    b.item(Item::new("folderBBBBBB".into(), Kind::Folder))
+     .and_then(|p| p.by_parent_guid("menu________".into()))
+     .and_then(|b| b.parent_for(&"folderBBBBBB".into()).by_children(&"folderAAAAAA".into()))
+     .expect("Should insert B");
+
+    match b.into_tree() {
+        Ok(_) => assert!(false, "Should not build tree with cycles"),
+        Err(err) => match err.kind() {
+            ErrorKind::Cycle(guid) => assert_eq!(guid, &Guid::from("folderAAAAAA")),
+            _ => assert!(false, "Wrong error kind for cycle: {}", err),
+        },
+    }
+}

src/tree.rs

@@ -448,6 +448,9 @@ impl IntoTree for Builder {
         // a vector of resolved parents, and a lookup table for reparented
         // orphaned children.
         let (parents, mut reparented_orphans_by_parent) = self.resolve();
+        if let Some(index) = detect_cycles(&parents) {
+            return Err(ErrorKind::Cycle(self.entries[index].item.guid.clone()).into());
+        }
         for reparented_orphans in reparented_orphans_by_parent.values_mut() {
             // Use a deterministic order for reparented orphans.
             reparented_orphans.sort_unstable_by(|&index, &other_index| {
@@ -742,6 +745,46 @@ enum ResolvedParent {
     ByParentGuid(Index),
 }
 
+impl ResolvedParent {
+    fn index(&self) -> Option<Index> {
+        match self {
+            ResolvedParent::Root => None,
+            ResolvedParent::Unchanged(index)
+            | ResolvedParent::ByChildren(index)
+            | ResolvedParent::ByParentGuid(index) => Some(*index),
+        }
+    }
+}
+
+/// Detects cycles in resolved parents, using Floyd's tortoise and the hare
+/// algorithm. Returns the index of the entry where the cycle was detected,
+/// or `None` if there aren't any cycles.
+fn detect_cycles(parents: &[ResolvedParent]) -> Option<Index> {
+    let mut seen = vec![false; parents.len()];
+    for (entry_index, parent) in parents.iter().enumerate() {
+        if seen[entry_index] {
+            continue;
+        }
+        let mut parent_index = parent.index();
+        let mut grandparent_index = parent.index()
+            .and_then(|index| parents[index].index());
+        while let (Some(i), Some(j)) = (parent_index, grandparent_index) {
+            if i == j {
+                return Some(i);
+            }
+            if seen[i] || seen[j] {
+                break;
+            }
+            parent_index = parent_index.and_then(|index| parents[index].index());
+            grandparent_index = grandparent_index
+                .and_then(|index| parents[index].index())
+                .and_then(|index| parents[index].index());
+        }
+        seen[entry_index] = true;
+    }
+    None
+}
+
 #[derive(Debug)]
 enum Divergence {
     /// The node's structure is already correct, and doesn't need to be