From 1b73171fa75940a0a67607bb9653be596d532e94 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 1 Jan 2025 13:57:57 -0500 Subject: [PATCH] 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 https://github.com/GothenburgBitFactory/taskchampion/pull/514 is released. --- src/dependency_map.rs | 36 +++++++++++++++++++++++++---- src/lib.rs | 3 ++- src/replica.rs | 24 +++++++++++--------- tests/test_dependency_map.py | 44 ++++++++++++++++++++++++++++++++++++ tests/test_replica.py | 5 ---- 5 files changed, 91 insertions(+), 21 deletions(-) create mode 100644 tests/test_dependency_map.py diff --git a/src/dependency_map.rs b/src/dependency_map.rs index 07d8ca0..e1ff65c 100644 --- a/src/dependency_map.rs +++ b/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 { 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 { 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 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 } } } diff --git a/src/lib.rs b/src/lib.rs index 395f5f8..509819c 100644 --- a/src/lib.rs +++ b/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::()?; m.add_class::()?; m.add_class::()?; + m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; diff --git a/src/replica.rs b/src/replica.rs index 8a2c6b6..ce1e64d 100644 --- a/src/replica.rs +++ b/src/replica.rs @@ -83,17 +83,19 @@ impl Replica { } pub fn dependency_map(&mut self, force: bool) -> anyhow::Result { - // 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` 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> { diff --git a/tests/test_dependency_map.py b/tests/test_dependency_map.py new file mode 100644 index 0000000..d28731b --- /dev/null +++ b/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: [] }" diff --git a/tests/test_replica.py b/tests/test_replica.py index 098efcf..74bf579 100644 --- a/tests/test_replica.py +++ b/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