Slightly more ergonomic task mutation

integration-tests/src/bindings_tests/task.c

@@ -42,7 +42,7 @@ static void test_task_get_set_status(void) {
     tc_task_to_mut(task, rep);
     TEST_ASSERT_TRUE(tc_task_set_status(task, TC_STATUS_DELETED));
     TEST_ASSERT_EQUAL(TC_STATUS_DELETED, tc_task_get_status(task)); // while mut
-    tc_task_to_immut(task, rep);
+    tc_task_to_immut(task);
     TEST_ASSERT_EQUAL(TC_STATUS_DELETED, tc_task_get_status(task)); // while immut
 
     tc_task_free(task);
@@ -71,7 +71,7 @@ static void test_task_get_set_description(void) {
     TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(desc));
     tc_string_free(desc);
 
-    tc_task_to_immut(task, rep);
+    tc_task_to_immut(task);
 
     desc = tc_task_get_description(task);
     TEST_ASSERT_NOT_NULL(desc);

lib/src/task.rs

@@ -11,10 +11,16 @@ use taskchampion::{Task, TaskMut};
 /// A task carries no reference to the replica that created it, and can
 /// be used until it is freed or converted to a TaskMut.
 pub enum TCTask {
+    /// A regular, immutable task
     Immutable(Task),
-    Mutable(TaskMut<'static>),
 
-    /// A transitional state for a TCTask as it goes from mutable to immutable.
+    /// A mutable task, together with the replica to which it holds an exclusive
+    /// reference.
+    Mutable(TaskMut<'static>, *mut TCReplica),
+
+    /// A transitional state for a TCTask as it goes from mutable to immutable and back.  A task
+    /// can only be in this state outside of [`to_mut`] and [`to_immut`] if a panic occurs during
+    /// one of those methods.
     Invalid,
 }
 
@@ -52,24 +58,38 @@ impl TCTask {
 
     /// Make an immutable TCTask into a mutable TCTask.  Does nothing if the task
     /// is already mutable.
-    fn to_mut(&mut self, tcreplica: &'static mut TCReplica) {
+    ///
+    /// # Safety
+    ///
+    /// The tcreplica pointer must not be NULL, and the replica it points to must not
+    /// be freed before TCTask.to_immut completes.
+    unsafe fn to_mut(&mut self, tcreplica: *mut TCReplica) {
         *self = match std::mem::replace(self, TCTask::Invalid) {
             TCTask::Immutable(task) => {
-                let rep_ref = tcreplica.borrow_mut();
+                // SAFETY:
-                TCTask::Mutable(task.into_mut(rep_ref))
+                //  - tcreplica is not null (promised by caller)
+                //  - tcreplica outlives the pointer in this variant (promised by caller)
+                let tcreplica_ref: &mut TCReplica = TCReplica::from_arg_ref(tcreplica);
+                let rep_ref = tcreplica_ref.borrow_mut();
+                TCTask::Mutable(task.into_mut(rep_ref), tcreplica)
             }
-            TCTask::Mutable(task) => TCTask::Mutable(task),
+            TCTask::Mutable(task, tcreplica) => TCTask::Mutable(task, tcreplica),
             TCTask::Invalid => unreachable!(),
         }
     }
 
     /// Make an mutable TCTask into a immutable TCTask.  Does nothing if the task
     /// is already immutable.
-    fn to_immut(&mut self, tcreplica: &mut TCReplica) {
+    fn to_immut(&mut self) {
         *self = match std::mem::replace(self, TCTask::Invalid) {
             TCTask::Immutable(task) => TCTask::Immutable(task),
-            TCTask::Mutable(task) => {
+            TCTask::Mutable(task, tcreplica) => {
-                tcreplica.release_borrow();
+                // SAFETY:
+                //  - tcreplica is not null (promised by caller of to_mut, which created this
+                //    variant)
+                //  - tcreplica is still alive (promised by caller of to_mut)
+                let tcreplica_ref: &mut TCReplica = unsafe { TCReplica::from_arg_ref(tcreplica) };
+                tcreplica_ref.release_borrow();
                 TCTask::Immutable(task.into_immut())
             }
             TCTask::Invalid => unreachable!(),
@@ -94,7 +114,7 @@ where
     let tctask: &'a TCTask = unsafe { TCTask::from_arg_ref(task) };
     let task: &'a Task = match tctask {
         TCTask::Immutable(t) => t,
-        TCTask::Mutable(t) => t.deref(),
+        TCTask::Mutable(t, _) => t.deref(),
         TCTask::Invalid => unreachable!(),
     };
     f(task)
@@ -112,7 +132,7 @@ where
     let tctask: &'a mut TCTask = unsafe { TCTask::from_arg_ref_mut(task) };
     let task: &'a mut TaskMut = match tctask {
         TCTask::Immutable(_) => panic!("Task is immutable"),
-        TCTask::Mutable(ref mut t) => t,
+        TCTask::Mutable(ref mut t, _) => t,
         TCTask::Invalid => unreachable!(),
     };
     // TODO: add TCTask error handling, like replica
@@ -121,10 +141,11 @@ where
 
 /// Convert an immutable task into a mutable task.
 ///
-/// The task is modified in-place, and becomes mutable.
+/// The task must not be NULL. It is modified in-place, and becomes mutable.
 ///
-/// The replica _cannot be used at all_ until this task is made immutable again.  This implies that
+/// The replica must not be NULL. After this function returns, the replica _cannot be used at all_
-/// it is not allowed for more than one task associated with a replica to be mutable at any time.
+/// until this task is made immutable again.  This implies that it is not allowed for more than one
+/// task associated with a replica to be mutable at any time.
 ///
 /// Typical mutation of tasks is bracketed with `tc_task_to_mut` and `tc_task_to_immut`:
 ///
@@ -135,22 +156,30 @@ where
 /// if (!success) { ... }
 /// ```
 #[no_mangle]
-pub extern "C" fn tc_task_to_mut<'a>(task: *mut TCTask, rep: *mut TCReplica) {
+pub extern "C" fn tc_task_to_mut<'a>(task: *mut TCTask, tcreplica: *mut TCReplica) {
+    // SAFETY:
+    //  - task is not null (promised by caller)
+    //  - task outlives 'a (promised by caller)
     let tctask: &'a mut TCTask = unsafe { TCTask::from_arg_ref_mut(task) };
-    let tcreplica: &'static mut TCReplica = unsafe { TCReplica::from_arg_ref(rep) };
+    // SAFETY:
-    tctask.to_mut(tcreplica);
+    //  - tcreplica is not NULL (promised by caller)
+    //  - tcreplica lives until later call to to_immut via tc_task_to_immut (promised by caller,
+    //    who cannot call tc_replica_free during this time)
+    unsafe { tctask.to_mut(tcreplica) };
 }
 
 /// Convert a mutable task into an immutable task.
 ///
-/// The task is modified in-place, and becomes immutable.
+/// The task must not be NULL.  It is modified in-place, and becomes immutable.
 ///
-/// The replica may be used freely after this call.
+/// The replica passed to `tc_task_to_mut` may be used freely after this call.
 #[no_mangle]
-pub extern "C" fn tc_task_to_immut<'a>(task: *mut TCTask, rep: *mut TCReplica) {
+pub extern "C" fn tc_task_to_immut<'a>(task: *mut TCTask) {
+    // SAFETY:
+    //  - task is not null (promised by caller)
+    //  - task outlives 'a (promised by caller)
     let tctask: &'a mut TCTask = unsafe { TCTask::from_arg_ref_mut(task) };
-    let tcreplica: &'static mut TCReplica = unsafe { TCReplica::from_arg_ref(rep) };
+    tctask.to_immut();
-    tctask.to_immut(tcreplica);
 }
 
 /// Get a task's UUID.

lib/src/taskmut.rs

@@ -1,67 +0,0 @@
-use crate::{status::TCStatus, string::TCString, uuid::TCUuid};
-use taskchampion::{TaskMut, Replica};
-use std::cell::RefMut;
-
-/// A mutable task.
-///
-/// A mutable task carries an exclusive reference to the replica,
-/// meaning that no other replica operations can be carried out while
-/// the mutable task still exists.
-pub struct TCTaskMut {
-    inner: TaskMut<'static>,
-    replica: RefMut<Replica>,
-}
-
-impl TCTaskMut {
-    /// Borrow a TCTaskMut from C as an argument.
-    ///
-    /// # Safety
-    ///
-    /// The pointer must not be NULL.  It is the caller's responsibility to ensure that the
-    /// lifetime assigned to the reference and the lifetime of the TCTaskMut itself do not outlive
-    /// the lifetime promised by C.
-    pub(crate) unsafe fn from_arg_ref<'a>(tctask: *mut TCTaskMut) -> &'a mut Self {
-        debug_assert!(!tctask.is_null());
-        &mut *tctask
-    }
-
-    /// Convert this to a return value for handing off to C.
-    pub(crate) fn return_val(self) -> *mut TCTaskMut {
-        Box::into_raw(Box::new(self))
-    }
-
-    /// Create a new TCTaskMut, given a RefMut to the replica.
-    pub(crate) fn from_immut(task: Task, rep: RefMut<Replica>) -> Self {
-        // SAFETY:
-        // This ref will be embedded in the TaskMut, and we will hang onto
-        // the RefMut for that duration, guaranteeing no other mutable borrows.
-        let rep_ref: &'static mut = unsafe { rep.deref_mut() }
-        let task_mut = task.into_mut(rep_ref);
-        TCTaskMut {
-            inner: task_mut,
-            replica: rep,
-        }
-    }
-}
-
-impl From<TaskMut> for TCTaskMut {
-    fn from(rep: Replica) -> TCReplica {
-        TCReplica {
-            inner: RefCell::new(rep),
-            error: None,
-        }
-    }
-}
-
-
-/// Free a task.  The given task must not be NULL.  The task must not be used after this function
-/// returns, and must not be freed more than once.
-#[no_mangle]
-pub extern "C" fn tc_task_mut_free(task: *mut TCTaskMut) {
-    debug_assert!(!task.is_null());
-    // SAFETY:
-    //  - task is not NULL (promised by caller)
-    //  - task's lifetime exceeds the drop (promised by caller)
-    //  - task does not outlive this function (promised by caller)
-    drop(unsafe { Box::from_raw(task) });
-}

lib/taskchampion.h

@@ -205,10 +205,11 @@ void tc_string_free(struct TCString *tcstring);
 /**
  * Convert an immutable task into a mutable task.
  *
- * The task is modified in-place, and becomes mutable.
+ * The task must not be NULL. It is modified in-place, and becomes mutable.
  *
- * The replica _cannot be used at all_ until this task is made immutable again.  This implies that
+ * The replica must not be NULL. After this function returns, the replica _cannot be used at all_
- * it is not allowed for more than one task associated with a replica to be mutable at any time.
+ * until this task is made immutable again.  This implies that it is not allowed for more than one
+ * task associated with a replica to be mutable at any time.
  *
  * Typical mutation of tasks is bracketed with `tc_task_to_mut` and `tc_task_to_immut`:
  *
@@ -219,16 +220,16 @@ void tc_string_free(struct TCString *tcstring);
  * if (!success) { ... }
  * ```
  */
-void tc_task_to_mut(struct TCTask *task, struct TCReplica *rep);
+void tc_task_to_mut(struct TCTask *task, struct TCReplica *tcreplica);
 
 /**
  * Convert a mutable task into an immutable task.
  *
- * The task is modified in-place, and becomes immutable.
+ * The task must not be NULL.  It is modified in-place, and becomes immutable.
  *
- * The replica may be used freely after this call.
+ * The replica passed to `tc_task_to_mut` may be used freely after this call.
  */
-void tc_task_to_immut(struct TCTask *task, struct TCReplica *rep);
+void tc_task_to_immut(struct TCTask *task);
 
 /**
  * Get a task's UUID.