From 364ca57736fa3e7051c61eefcb003a3236c8ab04 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 30 Jan 2022 23:42:52 +0000 Subject: [PATCH] Slightly more ergonomic task mutation --- integration-tests/src/bindings_tests/task.c | 4 +- lib/src/task.rs | 73 ++++++++++++++------- lib/src/taskmut.rs | 67 ------------------- lib/taskchampion.h | 15 +++-- 4 files changed, 61 insertions(+), 98 deletions(-) delete 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 b6f323583..b4173a1eb 100644 --- a/integration-tests/src/bindings_tests/task.c +++ b/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); diff --git a/lib/src/task.rs b/lib/src/task.rs index ac65b22eb..f929416d9 100644 --- a/lib/src/task.rs +++ b/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(); - TCTask::Mutable(task.into_mut(rep_ref)) + // SAFETY: + // - 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) => { - tcreplica.release_borrow(); + TCTask::Mutable(task, tcreplica) => { + // 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 -/// it is not allowed for more than one task associated with a replica to be mutable at any time. +/// The replica must not be NULL. After this function returns, 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`: /// @@ -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) }; - tctask.to_mut(tcreplica); + // SAFETY: + // - 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(tcreplica); + tctask.to_immut(); } /// Get a task's UUID. diff --git a/lib/src/taskmut.rs b/lib/src/taskmut.rs deleted file mode 100644 index 311647e54..000000000 --- a/lib/src/taskmut.rs +++ /dev/null @@ -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, -} - -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 f632938d5..18aa00faf 100644 --- a/lib/taskchampion.h +++ b/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 - * it is not allowed for more than one task associated with a replica to be mutable at any time. + * The replica must not be NULL. After this function returns, 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`: * @@ -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.