implement task mutability

integration-tests/src/bindings_tests/task.c

@@ -26,9 +26,68 @@ static void test_task_creation(void) {
     tc_replica_free(rep);
 }
 
+// updating status on a task works
+static void test_task_get_set_status(void) {
+    TCReplica *rep = tc_replica_new_in_memory();
+    TEST_ASSERT_NULL(tc_replica_error(rep));
+
+    TCTask *task = tc_replica_new_task(
+            rep,
+            TC_STATUS_PENDING,
+            tc_string_borrow("my task"));
+    TEST_ASSERT_NOT_NULL(task);
+
+    TEST_ASSERT_EQUAL(TC_STATUS_PENDING, tc_task_get_status(task));
+
+    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);
+    TEST_ASSERT_EQUAL(TC_STATUS_DELETED, tc_task_get_status(task)); // while immut
+
+    tc_task_free(task);
+
+    tc_replica_free(rep);
+}
+
+// updating description on a task works
+static void test_task_get_set_description(void) {
+    TCReplica *rep = tc_replica_new_in_memory();
+    TEST_ASSERT_NULL(tc_replica_error(rep));
+
+    TCTask *task = tc_replica_new_task(
+            rep,
+            TC_STATUS_PENDING,
+            tc_string_borrow("my task"));
+    TEST_ASSERT_NOT_NULL(task);
+
+    TCString *desc;
+
+    tc_task_to_mut(task, rep);
+    tc_task_set_description(task, tc_string_borrow("updated"));
+
+    TEST_ASSERT_TRUE(desc = tc_task_get_description(task));
+    TEST_ASSERT_NOT_NULL(desc);
+    TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(desc));
+    tc_string_free(desc);
+
+    tc_task_to_immut(task, rep);
+
+    desc = tc_task_get_description(task);
+    TEST_ASSERT_NOT_NULL(desc);
+    TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(desc));
+    tc_string_free(desc);
+
+    tc_task_free(task);
+
+    tc_replica_free(rep);
+}
+
 int task_tests(void) {
     UNITY_BEGIN();
     // each test case above should be named here, in order.
     RUN_TEST(test_task_creation);
+    RUN_TEST(test_task_get_set_status);
+    RUN_TEST(test_task_get_set_description);
     return UNITY_END();
 }

lib/src/replica.rs

@@ -1,5 +1,4 @@
 use crate::{result::TCResult, status::TCStatus, string::TCString, task::TCTask, uuid::TCUuid};
-use std::cell::{RefCell, RefMut};
 use taskchampion::{Replica, StorageConfig, Uuid};
 
 /// A replica represents an instance of a user's task data, providing an easy interface
@@ -7,7 +6,13 @@ use taskchampion::{Replica, StorageConfig, Uuid};
 ///
 /// TCReplicas are not threadsafe.
 pub struct TCReplica {
-    inner: RefCell<Replica>,
+    /// The wrapped Replica
+    inner: Replica,
+
+    /// If true, this replica has an outstanding &mut (for a TaskMut)
+    mut_borrowed: bool,
+
+    /// The error from the most recent operation, if any
     error: Option<TCString<'static>>,
 }
 
@@ -24,16 +29,36 @@ impl TCReplica {
         &mut *tcreplica
     }
 
+    // TODO: from_arg_owned, use in drop
+
     /// Convert this to a return value for handing off to C.
     pub(crate) fn return_val(self) -> *mut TCReplica {
         Box::into_raw(Box::new(self))
     }
+
+    /// Mutably borrow the inner Replica
+    pub(crate) fn borrow_mut(&mut self) -> &mut Replica {
+        if self.mut_borrowed {
+            panic!("replica is already borrowed");
+        }
+        self.mut_borrowed = true;
+        &mut self.inner
+    }
+
+    /// Release the borrow made by [`borrow_mut`]
+    pub(crate) fn release_borrow(&mut self) {
+        if !self.mut_borrowed {
+            panic!("replica is not borrowed");
+        }
+        self.mut_borrowed = false;
+    }
 }
 
 impl From<Replica> for TCReplica {
     fn from(rep: Replica) -> TCReplica {
         TCReplica {
-            inner: RefCell::new(rep),
+            inner: rep,
+            mut_borrowed: false,
             error: None,
         }
     }
@@ -47,14 +72,17 @@ fn err_to_tcstring(e: impl std::string::ToString) -> TCString<'static> {
 /// a mutable borrow, because most Replica methods require a `&mut`.
 fn wrap<'a, T, F>(rep: *mut TCReplica, f: F, err_value: T) -> T
 where
-    F: FnOnce(RefMut<'_, Replica>) -> anyhow::Result<T>,
+    F: FnOnce(&mut Replica) -> anyhow::Result<T>,
 {
     // SAFETY:
     //  - rep is not null (promised by caller)
     //  - rep outlives 'a (promised by caller)
     let rep: &'a mut TCReplica = unsafe { TCReplica::from_arg_ref(rep) };
+    if rep.mut_borrowed {
+        panic!("replica is borrowed and cannot be used");
+    }
     rep.error = None;
-    match f(rep.inner.borrow_mut()) {
+    match f(&mut rep.inner) {
         Ok(v) => v,
         Err(e) => {
             rep.error = Some(err_to_tcstring(e));
@@ -117,7 +145,7 @@ pub extern "C" fn tc_replica_new_on_disk<'a>(
 pub extern "C" fn tc_replica_get_task(rep: *mut TCReplica, uuid: TCUuid) -> *mut TCTask {
     wrap(
         rep,
-        |mut rep| {
+        |rep| {
             let uuid: Uuid = uuid.into();
             if let Some(task) = rep.get_task(uuid)? {
                 Ok(TCTask::from(task).return_val())
@@ -146,7 +174,7 @@ pub extern "C" fn tc_replica_new_task(
     let description = unsafe { TCString::from_arg(description) };
     wrap(
         rep,
-        |mut rep| {
+        |rep| {
             let task = rep.new_task(status.into(), description.as_str()?.to_string())?;
             Ok(TCTask::from(task).return_val())
         },
@@ -164,7 +192,7 @@ pub extern "C" fn tc_replica_import_task_with_uuid(
 ) -> *mut TCTask {
     wrap(
         rep,
-        |mut rep| {
+        |rep| {
             let uuid: Uuid = uuid.into();
             let task = rep.import_task_with_uuid(uuid)?;
             Ok(TCTask::from(task).return_val())
@@ -183,7 +211,7 @@ pub extern "C" fn tc_replica_import_task_with_uuid(
 pub extern "C" fn tc_replica_undo<'a>(rep: *mut TCReplica) -> TCResult {
     wrap(
         rep,
-        |mut rep| {
+        |rep| {
             Ok(if rep.undo()? {
                 TCResult::True
             } else {
@@ -213,7 +241,11 @@ pub extern "C" fn tc_replica_error<'a>(rep: *mut TCReplica) -> *mut TCString<'st
 /// Free a TCReplica.
 #[no_mangle]
 pub extern "C" fn tc_replica_free(rep: *mut TCReplica) {
-    debug_assert!(!rep.is_null());
+    let replica: &mut TCReplica = unsafe { TCReplica::from_arg_ref(rep) };
+    if replica.mut_borrowed {
+        panic!("replica is borrowed and cannot be freed");
+    }
+    drop(replica);
     drop(unsafe { Box::from_raw(rep) });
 }
 

lib/src/task.rs

@@ -1,12 +1,21 @@
-use crate::{status::TCStatus, string::TCString, uuid::TCUuid};
+use crate::{replica::TCReplica, status::TCStatus, string::TCString, uuid::TCUuid};
-use taskchampion::Task;
+use std::ops::Deref;
+use taskchampion::{Task, TaskMut};
 
 /// A task, as publicly exposed by this library.
 ///
+/// A task begins in "immutable" mode.  It must be converted to "mutable" mode
+/// to make any changes, and doing so requires exclusive access to the replica
+/// until the task is freed or converted back to immutable mode.
+///
 /// 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 struct TCTask {
+pub enum TCTask {
-    inner: Task,
+    Immutable(Task),
+    Mutable(TaskMut<'static>),
+
+    /// A transitional state for a TCTask as it goes from mutable to immutable.
+    Invalid,
 }
 
 impl TCTask {
@@ -22,55 +31,155 @@ impl TCTask {
         &*tctask
     }
 
+    /// Borrow a TCTask from C as an argument, allowing mutation.
+    ///
+    /// # 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 TCTask itself do not outlive
+    /// the lifetime promised by C.
+    pub(crate) unsafe fn from_arg_ref_mut<'a>(tctask: *mut TCTask) -> &'a mut Self {
+        debug_assert!(!tctask.is_null());
+        &mut *tctask
+    }
+
+    // TODO: from_arg_owned, use in drop
+
     /// Convert a TCTask to a return value for handing off to C.
     pub(crate) fn return_val(self) -> *mut TCTask {
         Box::into_raw(Box::new(self))
     }
+
+    /// 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) {
+        *self = match std::mem::replace(self, TCTask::Invalid) {
+            TCTask::Immutable(task) => {
+                let rep_ref = tcreplica.borrow_mut();
+                TCTask::Mutable(task.into_mut(rep_ref))
+            }
+            TCTask::Mutable(task) => TCTask::Mutable(task),
+            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) {
+        *self = match std::mem::replace(self, TCTask::Invalid) {
+            TCTask::Immutable(task) => TCTask::Immutable(task),
+            TCTask::Mutable(task) => {
+                tcreplica.release_borrow();
+                TCTask::Immutable(task.into_immut())
+            }
+            TCTask::Invalid => unreachable!(),
+        }
+    }
 }
 
 impl From<Task> for TCTask {
     fn from(task: Task) -> TCTask {
-        TCTask { inner: task }
+        TCTask::Immutable(task)
     }
 }
 
+/// Utility function to get a shared reference to the underlying Task.
+fn wrap<'a, T, F>(task: *const TCTask, f: F) -> T
+where
+    F: FnOnce(&Task) -> T,
+{
+    // SAFETY:
+    //  - task is not null (promised by caller)
+    //  - task outlives 'a (promised by caller)
+    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::Invalid => unreachable!(),
+    };
+    f(task)
+}
+
+/// Utility function to get a mutable reference to the underlying Task.  The
+/// TCTask must be mutable.
+fn wrap_mut<'a, T, F>(task: *mut TCTask, f: F) -> T
+where
+    F: FnOnce(&mut TaskMut) -> anyhow::Result<T>,
+{
+    // 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 task: &'a mut TaskMut = match tctask {
+        TCTask::Immutable(_) => panic!("Task is immutable"),
+        TCTask::Mutable(ref mut t) => t,
+        TCTask::Invalid => unreachable!(),
+    };
+    // TODO: add TCTask error handling, like replica
+    f(task).unwrap()
+}
+
+/// Convert an immutable task into a mutable task.
+///
+/// The task is modified in-place, and becomes mutable.
+///
+/// The replica _cannot be used at all_ 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`:
+///
+/// ```c
+/// tc_task_to_mut(task, rep);
+/// success = tc_task_done(task);
+/// tc_task_to_immut(task, rep);
+/// if (!success) { ... }
+/// ```
+#[no_mangle]
+pub extern "C" fn tc_task_to_mut<'a>(task: *mut TCTask, rep: *mut TCReplica) {
+    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_mut(tcreplica);
+}
+
+/// Convert a mutable task into an immutable task.
+///
+/// The task is modified in-place, and becomes immutable.
+///
+/// The replica may be used freely after this call.
+#[no_mangle]
+pub extern "C" fn tc_task_to_immut<'a>(task: *mut TCTask, rep: *mut TCReplica) {
+    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(tcreplica);
+}
+
 /// Get a task's UUID.
 #[no_mangle]
-pub extern "C" fn tc_task_get_uuid<'a>(task: *const TCTask) -> TCUuid {
+pub extern "C" fn tc_task_get_uuid(task: *const TCTask) -> TCUuid {
-    // SAFETY:
+    wrap(task, |task| task.get_uuid().into())
-    //  - task is not NULL (promised by caller)
-    //  - task's lifetime exceeds this function (promised by caller)
-    let task: &'a Task = &unsafe { TCTask::from_arg_ref(task) }.inner;
-    let uuid = task.get_uuid();
-    uuid.into()
 }
 
 /// Get a task's status.
 #[no_mangle]
 pub extern "C" fn tc_task_get_status<'a>(task: *const TCTask) -> TCStatus {
-    // SAFETY:
+    wrap(task, |task| task.get_status().into())
-    //  - task is not NULL (promised by caller)
-    //  - task's lifetime exceeds this function (promised by caller)
-    let task: &'a Task = &unsafe { TCTask::from_arg_ref(task) }.inner;
-    task.get_status().into()
 }
 
-// TODO: into_mut
 // TODO: get_taskmap
 
 /// Get a task's description, or NULL if the task cannot be represented as a C string (e.g., if it
 /// contains embedded NUL characters).
 #[no_mangle]
 pub extern "C" fn tc_task_get_description<'a>(task: *const TCTask) -> *mut TCString<'static> {
-    // SAFETY:
+    wrap(task, |task| {
-    //  - task is not NULL (promised by caller)
-    //  - task's lifetime exceeds this function (promised by caller)
-    let task: &'a Task = &unsafe { TCTask::from_arg_ref(task) }.inner;
         let descr: TCString = task.get_description().into();
         descr.return_val()
+    })
 }
 
+// TODO: :get_entry
 // TODO: :get_wait
+// TODO: :get_modified
 // TODO: :is_waiting
 // TODO: :is_active
 // TODO: :has_tag
@@ -82,11 +191,68 @@ pub extern "C" fn tc_task_get_description<'a>(task: *const TCTask) -> *mut TCStr
 // TODO: :get_legacy_udas
 // TODO: :get_modified
 
-/// Free a task.  The given task must not be NULL.  The task must not be used after this function
+/// Set a mutable task's status.
-/// returns, and must not be freed more than once.
+///
+/// Returns false on error.
+#[no_mangle]
+pub extern "C" fn tc_task_set_status<'a>(task: *mut TCTask, status: TCStatus) -> bool {
+    wrap_mut(task, |task| {
+        task.set_status(status.into())?;
+        Ok(true)
+    })
+}
+
+/// Set a mutable task's description.
+///
+/// Returns false on error.
+#[no_mangle]
+pub extern "C" fn tc_task_set_description<'a>(
+    task: *mut TCTask,
+    description: *mut TCString,
+) -> bool {
+    // SAFETY:
+    //  - tcstring is not NULL (promised by caller)
+    //  - caller is exclusive owner of tcstring (implicitly promised by caller)
+    let description = unsafe { TCString::from_arg(description) };
+    wrap_mut(task, |task| {
+        task.set_description(description.as_str()?.to_string())?;
+        Ok(true)
+    })
+}
+
+// TODO: tc_task_set_description
+// TODO: tc_task_set_entry
+// TODO: tc_task_set_wait
+// TODO: tc_task_set_modified
+// TODO: tc_task_start
+// TODO: tc_task_stop
+// TODO: tc_task_done
+// TODO: tc_task_delete
+// TODO: tc_task_add_tag
+// TODO: tc_task_remove_tag
+// TODO: tc_task_add_annotation
+// TODO: tc_task_remove_annotation
+// TODO: tc_task_set_uda
+// TODO: tc_task_remove_uda
+// TODO: tc_task_set_legacy_uda
+// TODO: tc_task_remove_legacy_uda
+
+/// Free a task.  The given task must not be NULL and must be immutable.  The task must not be used
+/// after this function returns, and must not be freed more than once.
+///
+/// The restriction that the task must be immutable may be lifted (TODO)
 #[no_mangle]
 pub extern "C" fn tc_task_free<'a>(task: *mut TCTask) {
-    debug_assert!(!task.is_null());
+    // convert the task to immutable before freeing
+    let tctask: &'a mut TCTask = unsafe { TCTask::from_arg_ref_mut(task) };
+    if !matches!(tctask, TCTask::Immutable(_)) {
+        // this limit is in place because we require the caller to supply a pointer
+        // to the replica to make a task immutable, and no such pointer is available
+        // here.
+        panic!("Task must be immutable when freed");
+    }
+    drop(tctask);
+
     // SAFETY:
     //  - task is not NULL (promised by caller)
     //  - task's lifetime exceeds the drop (promised by caller)

lib/src/taskmut.rs

@@ -0,0 +1,67 @@
+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

@@ -61,6 +61,10 @@ typedef struct TCString TCString;
 /**
  * A task, as publicly exposed by this library.
  *
+ * A task begins in "immutable" mode.  It must be converted to "mutable" mode
+ * to make any changes, and doing so requires exclusive access to the replica
+ * until the task is freed or converted back to immutable mode.
+ *
  * A task carries no reference to the replica that created it, and can
  * be used until it is freed or converted to a TaskMut.
  */
@@ -198,6 +202,34 @@ const char *tc_string_content_with_len(struct TCString *tcstring, size_t *len_ou
  */
 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 replica _cannot be used at all_ 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`:
+ *
+ * ```c
+ * tc_task_to_mut(task, rep);
+ * success = tc_task_done(task);
+ * tc_task_to_immut(task, rep);
+ * if (!success) { ... }
+ * ```
+ */
+void tc_task_to_mut(struct TCTask *task, struct TCReplica *rep);
+
+/**
+ * Convert a mutable task into an immutable task.
+ *
+ * The task is modified in-place, and becomes immutable.
+ *
+ * The replica may be used freely after this call.
+ */
+void tc_task_to_immut(struct TCTask *task, struct TCReplica *rep);
+
 /**
  * Get a task's UUID.
  */
@@ -215,8 +247,24 @@ enum TCStatus tc_task_get_status(const struct TCTask *task);
 struct TCString *tc_task_get_description(const struct TCTask *task);
 
 /**
- * Free a task.  The given task must not be NULL.  The task must not be used after this function
+ * Set a mutable task's status.
- * returns, and must not be freed more than once.
+ *
+ * Returns false on error.
+ */
+bool tc_task_set_status(struct TCTask *task, enum TCStatus status);
+
+/**
+ * Set a mutable task's description.
+ *
+ * Returns false on error.
+ */
+bool tc_task_set_description(struct TCTask *task, struct TCString *description);
+
+/**
+ * Free a task.  The given task must not be NULL and must be immutable.  The task must not be used
+ * after this function returns, and must not be freed more than once.
+ *
+ * The restriction that the task must be immutable may be lifted (TODO)
  */
 void tc_task_free(struct TCTask *task);