From 452ae2074f3634fe19727923f6ac6884e50c8061 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sat, 29 Jan 2022 03:08:45 +0000 Subject: [PATCH] implement task mutability --- integration-tests/src/bindings_tests/task.c | 59 ++++++ lib/src/replica.rs | 52 ++++- lib/src/task.rs | 220 +++++++++++++++++--- lib/src/taskmut.rs | 67 ++++++ lib/taskchampion.h | 52 ++++- 5 files changed, 411 insertions(+), 39 deletions(-) create mode 100644 lib/src/taskmut.rs diff --git a/integration-tests/src/bindings_tests/task.c b/integration-tests/src/bindings_tests/task.c index b0bbc4117..b6f323583 100644 --- a/integration-tests/src/bindings_tests/task.c +++ b/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(); } diff --git a/lib/src/replica.rs b/lib/src/replica.rs index 53e5d84c7..d9c28eb0b 100644 --- a/lib/src/replica.rs +++ b/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, + /// 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>, } @@ -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 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, + F: FnOnce(&mut Replica) -> anyhow::Result, { // 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) }); } diff --git a/lib/src/task.rs b/lib/src/task.rs index ed27acc81..ac65b22eb 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -1,12 +1,21 @@ -use crate::{status::TCStatus, string::TCString, uuid::TCUuid}; -use taskchampion::Task; +use crate::{replica::TCReplica, status::TCStatus, string::TCString, uuid::TCUuid}; +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 { - inner: Task, +pub enum TCTask { + 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 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, +{ + // 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 { - // SAFETY: - // - 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() +pub extern "C" fn tc_task_get_uuid(task: *const TCTask) -> TCUuid { + wrap(task, |task| task.get_uuid().into()) } /// Get a task's status. #[no_mangle] pub extern "C" fn tc_task_get_status<'a>(task: *const TCTask) -> TCStatus { - // SAFETY: - // - 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() + wrap(task, |task| 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: - // - 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() + wrap(task, |task| { + 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 -/// returns, and must not be freed more than once. +/// Set a mutable task's status. +/// +/// 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) diff --git a/lib/src/taskmut.rs b/lib/src/taskmut.rs new file mode 100644 index 000000000..311647e54 --- /dev/null +++ b/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, +} + +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) -> 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 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) }); +} diff --git a/lib/taskchampion.h b/lib/taskchampion.h index 67250584c..f632938d5 100644 --- a/lib/taskchampion.h +++ b/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 - * returns, and must not be freed more than once. + * Set a mutable task's status. + * + * 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);