Update DependencyMap so that it actually works (#21)

The existing `Rc::into_inner` trick _never_ works, because the Replica
always holds one count of the `Rc` for itself, and returns a second
count -- so the reference count is always two. And, Rc::unwrap_or_clone`
doesn't work because `DependencyMap` can't be cloned.

This is a giant hack, as explained by comments inline. It's also
temporary, until
GothenburgBitFactory/taskchampion#514 is
released.

src/dependency_map.rs

@@ -1,20 +1,48 @@
 use pyo3::prelude::*;
 use taskchampion::{DependencyMap as TCDependencyMap, Uuid};
 
+// See `Replica::dependency_map` for the rationale for using a raw pointer here.
+
 #[pyclass]
-pub struct DependencyMap(pub(crate) TCDependencyMap);
+pub struct DependencyMap(*const TCDependencyMap);
+
+// SAFETY: `Replica::dependency_map` ensures that the TCDependencyMap is never freed (as the Rc is
+// leaked) and TaskChampion does not modify it, so no races can occur.
+unsafe impl Send for DependencyMap {}
 
 #[pymethods]
 impl DependencyMap {
-    // TODO: possibly optimize this later, if possible
+    pub fn __repr__(&self) -> String {
+        format!("{:?}", self.as_ref())
+    }
+
     pub fn dependencies(&self, dep_of: String) -> Vec<String> {
         let uuid = Uuid::parse_str(&dep_of).unwrap();
-        self.0.dependencies(uuid).map(|uuid| uuid.into()).collect()
+        self.as_ref()
+            .dependencies(uuid)
+            .map(|uuid| uuid.into())
+            .collect()
     }
 
     pub fn dependents(&self, dep_on: String) -> Vec<String> {
         let uuid = Uuid::parse_str(&dep_on).unwrap();
+        self.as_ref()
+            .dependents(uuid)
+            .map(|uuid| uuid.into())
+            .collect()
+    }
+}
+
+impl From<*const TCDependencyMap> for DependencyMap {
+    fn from(value: *const TCDependencyMap) -> Self {
+        DependencyMap(value)
+    }
+}
 
-        self.0.dependents(uuid).map(|uuid| uuid.into()).collect()
+impl AsRef<TCDependencyMap> for DependencyMap {
+    fn as_ref(&self) -> &TCDependencyMap {
+        // SAFETY: `Replica::dependency_map` ensures that the TCDependencyMap is never freed (as
+        // the Rc is leaked) and TaskChampion does not modify it, so no races can occur.
+        unsafe { &*self.0 as &TCDependencyMap }
     }
 }

src/lib.rs

@@ -10,13 +10,14 @@ use dependency_map::*;
 pub mod operation;
 use operation::*;
 mod task;
-use task::{Annotation, Status, Tag, Task};
+use task::{Annotation, Status, Tag, Task, TaskData};
 
 #[pymodule]
 fn taskchampion(m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_class::<Status>()?;
     m.add_class::<Replica>()?;
     m.add_class::<Task>()?;
+    m.add_class::<TaskData>()?;
     m.add_class::<Annotation>()?;
     m.add_class::<WorkingSet>()?;
     m.add_class::<Tag>()?;

src/replica.rs

@@ -83,17 +83,19 @@ impl Replica {
     }
 
     pub fn dependency_map(&mut self, force: bool) -> anyhow::Result<DependencyMap> {
-        // TODO: kinda spaghetti here, it will do for now
-        let s = self
-            .0
-            .dependency_map(force)
-            .map(|rc| {
-                // TODO: better error handling here
-                Rc::into_inner(rc).unwrap()
-            })
-            .map(DependencyMap)?;
-
-        Ok(s)
+        // `Rc<T>` is not thread-safe, so we must get an owned copy of the data it contains.
+        // Unfortunately, it cannot be cloned, so this is impossible (but both issues are fixed in
+        // https://github.com/GothenburgBitFactory/taskchampion/pull/514).
+        //
+        // Until that point, we leak the Rc (preventing it from ever being freed) and use a static
+        // reference to its contents. This is safe based on the weak but currently valid assumption
+        // that TaskChampion does not modify a DependencyMap after creating it.
+        //
+        // This is a temporary hack, and should not be used in "real" code!
+        let dm = self.0.dependency_map(force)?;
+        // NOTE: this does not decrement the reference count and thus "leaks" the Rc.
+        let dm_ptr = Rc::into_raw(dm);
+        Ok(dm_ptr.into())
     }
 
     pub fn get_task(&mut self, uuid: String) -> anyhow::Result<Option<Task>> {

tests/test_dependency_map.py

@@ -0,0 +1,44 @@
+import uuid
+import pytest
+from taskchampion import Replica, TaskData
+
+
+def test_dependency_map():
+    r = Replica.new_in_memory()
+    u1 = str(uuid.uuid4())
+    u2 = str(uuid.uuid4())
+    u3 = str(uuid.uuid4())
+    ops = []
+
+    # Set up t3 depending on t2 depending on t1.
+    t1, op = TaskData.create(u1)
+    ops.append(op)
+    op = t1.update("status", "pending")
+    ops.append(op)
+    t2, op = TaskData.create(u2)
+    ops.append(op)
+    op = t2.update("status", "pending")
+    ops.append(op)
+    op = t2.update(f"dep_{u1}", "x")
+    ops.append(op)
+    t3, op = TaskData.create(u3)
+    ops.append(op)
+    op = t3.update("status", "pending")
+    ops.append(op)
+    op = t3.update(f"dep_{u2}", "x")
+    ops.append(op)
+
+    r.commit_operations(ops)
+
+    dm = r.dependency_map(True)
+    assert dm.dependencies(u1) == []
+    assert dm.dependents(u1) == [u2]
+    assert dm.dependencies(u2) == [u1]
+    assert dm.dependents(u2) == [u3]
+    assert dm.dependencies(u3) == [u2]
+
+
+def test_dependency_map_repr():
+    r = Replica.new_in_memory()
+    dm = r.dependency_map(True)
+    assert repr(dm) == "DependencyMap { edges: [] }"

tests/test_replica.py

@@ -146,8 +146,3 @@ def test_num_undo_points(replica_with_tasks: Replica):
     replica_with_tasks.commit_operations(op)
 
     assert replica_with_tasks.num_undo_points() == 4
-
-
-@pytest.mark.skip("Skipping as gotta actually polish it")
-def test_dependency_map(replica_with_tasks: Replica):
-    assert replica_with_tasks.dependency_map(False) is not None