TCTags as PassByValue

integration-tests/src/bindings_tests/task.c

@@ -315,11 +315,11 @@ static void test_task_get_tags(void) {
     TCTags tags = tc_task_get_tags(task);
 
     int found_pending = false, found_next = false;
-    for (size_t i = 0; i < tags.num_tags; i++) {
-        if (strcmp("PENDING", tc_string_content(tags.tags[i])) == 0) {
+    for (size_t i = 0; i < tags.len; i++) {
+        if (strcmp("PENDING", tc_string_content(tags.items[i])) == 0) {
             found_pending = true;
         }
-        if (strcmp("next", tc_string_content(tags.tags[i])) == 0) {
+        if (strcmp("next", tc_string_content(tags.items[i])) == 0) {
             found_next = true;
         }
     }
@@ -327,7 +327,7 @@ static void test_task_get_tags(void) {
     TEST_ASSERT_TRUE(found_next);
 
     tc_tags_free(&tags);
-    TEST_ASSERT_NULL(tags.tags);
+    TEST_ASSERT_NULL(tags.items);
 
     tc_task_free(task);
     tc_replica_free(rep);

lib/src/arrays.rs

@@ -2,8 +2,6 @@ use crate::string::TCString;
 use crate::traits::*;
 use std::ptr::NonNull;
 
-// TODO: generalize to TCStrings?
-
 /// TCTags represents a list of tags associated with a task.
 ///
 /// The content of this struct must be treated as read-only.
@@ -12,86 +10,74 @@ use std::ptr::NonNull;
 /// will remain valid even if the task is freed.
 #[repr(C)]
 pub struct TCTags {
-    // TODO: can we use NonNull here?
-    /// strings representing each tag. these remain owned by the
-    /// TCTags instance and will be freed by tc_tags_free.
-    tags: *const NonNull<TCString<'static>>,
-    /// number of tags in tags
-    num_tags: libc::size_t,
-    /// total size of tags (internal use only)
+    // WARNING: this struct must match CPointerArray exactly, in size and order
+    // of fields.  Names can differ, as can the pointer type.
+    /// number of tags in items
+    len: libc::size_t,
+
+    /// total size of items (internal use only)
     _capacity: libc::size_t,
+
+    /// strings representing each tag. these remain owned by the TCTags instance and will be freed
+    /// by tc_tags_free.
+    items: *const NonNull<TCString<'static>>,
+}
+
+impl PassByValue for Vec<NonNull<TCString<'static>>> {
+    type CType = TCTags;
+
+    unsafe fn from_ctype(arg: TCTags) -> Self {
+        // SAFETY:
+        //  - C treats TCTags as read-only, so items, len, and _capacity all came
+        //    from a Vec originally.
+        unsafe { Vec::from_raw_parts(arg.items as *mut _, arg.len, arg._capacity) }
+    }
+
+    fn as_ctype(self) -> TCTags {
+        // emulate Vec::into_raw_parts():
+        // - disable dropping the Vec with ManuallyDrop
+        // - extract ptr, len, and capacity using those methods
+        let mut vec = std::mem::ManuallyDrop::new(self);
+        TCTags {
+            len: vec.len(),
+            _capacity: vec.capacity(),
+            items: vec.as_mut_ptr(),
+        }
+    }
 }
 
 impl Default for TCTags {
     fn default() -> Self {
         Self {
-            tags: std::ptr::null_mut(),
-            num_tags: 0,
+            len: 0,
             _capacity: 0,
+            items: std::ptr::null(),
         }
     }
 }
 
-impl TCTags {
-    /// Create a Vec of TCStrings into a TCTags instance.
-    pub(crate) fn new(tags: Vec<NonNull<TCString<'static>>>) -> Self {
-        // emulate Vec::into_raw_parts():
-        // - disable dropping the Vec with ManuallyDrop
-        // - extract ptr, len, and capacity using those methods
-        let mut tags = std::mem::ManuallyDrop::new(tags);
-        Self {
-            tags: tags.as_mut_ptr(),
-            num_tags: tags.len(),
-            _capacity: tags.capacity(),
-        }
-    }
-
-    /// Convert a TCTags to a Vec<_>.
-    ///
-    /// # Safety
-    ///
-    /// Tags must be _exactly_ as created by [`new`]
-    unsafe fn into_vec(self) -> Vec<NonNull<TCString<'static>>> {
-        // SAFETY:
-        //
-        // * tags.tags needs to have been previously allocated via Vec<*mut TCString>
-        // * TCString needs to have the same size and alignment as what ptr was allocated with.
-        // * length needs to be less than or equal to capacity.
-        // * capacity needs to be the capacity that the pointer was allocated with.
-        // * vec elements are not NULL
-        //
-        // All of this is true for a value returned from `new`, which the caller promised
-        // not to change.
-        unsafe { Vec::from_raw_parts(self.tags as *mut _, self.num_tags, self._capacity) }
-    }
-}
-
-/// Free a TCTags instance.  The given pointer must not be NULL.  The instance must not be used
-/// after this call.
+/// Free a TCTags instance.  The instance, and all TCStrings it contains, must not be used after
+/// this call.
 #[no_mangle]
 pub extern "C" fn tc_tags_free<'a>(tctags: *mut TCTags) {
     debug_assert!(!tctags.is_null());
     // SAFETY:
-    //  - tctags is not NULL
-    //  - tctags is valid (caller promises it has not been changed)
-    //  - caller will not use the TCTags after this (promised by caller)
-    let tctags: &'a mut TCTags = unsafe { &mut *tctags };
+    //  - *tctags is a valid TCTags (caller promises to treat it as read-only)
+    let tags = unsafe { Vec::take_from_arg(tctags, TCTags::default()) };
 
-    debug_assert!(!tctags.tags.is_null());
-
-    // replace the caller's TCTags with one containing a NULL pointer
-    let tctags: TCTags = std::mem::take(tctags);
-
-    // convert to a regular Vec
-    // SAFETY:
-    //  - tctags is exactly as returned from TCTags::new (promised by caller)
-    let mut vec: Vec<_> = unsafe { tctags.into_vec() };
-
-    // drop each contained string
-    for tcstring in vec.drain(..) {
-        // SAFETY: see TCString docstring
-        drop(unsafe { TCString::take_from_arg(tcstring.as_ptr()) });
-    }
-
-    drop(vec);
+    // tags is a Vec<NonNull<TCString>>, so we convert it to a Vec<TCString> that
+    // will properly drop those strings when dropped.
+    let tags: Vec<TCString> = tags
+        .iter()
+        .map(|p| {
+            // SAFETY:
+            // *p is a pointer delivered to us from a Vec<NonNull<TCString>>, so
+            //  - *p is not NULL
+            //  - *p was generated by Rust
+            //  - *p was not modified (promised by caller)
+            //  - the caller will not use this pointer again (promised by caller)
+            unsafe { TCString::take_from_arg(p.as_ptr()) }
+        })
+        .collect();
+    drop(tags);
 }

lib/src/replica.rs

@@ -142,7 +142,8 @@ pub extern "C" fn tc_replica_get_task(rep: *mut TCReplica, tcuuid: TCUuid) -> *m
     wrap(
         rep,
         |rep| {
-            let uuid = Uuid::from_arg(tcuuid);
+            // SAFETY: see TCUuid docstring
+            let uuid = unsafe { Uuid::from_arg(tcuuid) };
             if let Some(task) = rep.get_task(uuid)? {
                 Ok(TCTask::from(task).return_val())
             } else {
@@ -185,7 +186,8 @@ pub extern "C" fn tc_replica_import_task_with_uuid(
     wrap(
         rep,
         |rep| {
-            let uuid = Uuid::from_arg(tcuuid);
+            // SAFETY: see TCUuid docstring
+            let uuid = unsafe { Uuid::from_arg(tcuuid) };
             let task = rep.import_task_with_uuid(uuid)?;
             Ok(TCTask::from(task).return_val())
         },

lib/src/task.rs

@@ -324,13 +324,14 @@ pub extern "C" fn tc_task_get_tags<'a>(task: *mut TCTask) -> TCTags {
         let tcstrings: Vec<NonNull<TCString<'static>>> = task
             .get_tags()
             .map(|t| {
-                // SAFETY: see TCString docstring
-                let t_ptr = unsafe { TCString::from(t.as_ref()).return_val() };
-                // SAFETY: t_ptr was just created and is not NULL
-                unsafe { NonNull::new_unchecked(t_ptr) }
+                NonNull::new(
+                    // SAFETY: see TCString docstring
+                    unsafe { TCString::from(t.as_ref()).return_val() },
+                )
+                .expect("TCString::return_val() returned NULL")
             })
             .collect();
-        TCTags::new(tcstrings)
+        tcstrings.return_val()
     })
 }
 

lib/src/traits.rs

@@ -0,0 +1,143 @@
+/// Support for values passed to Rust by value.  These are represented as full structs in C.  Such
+/// values are implicitly copyable, via C's struct assignment.
+///
+/// The Rust and C types may differ, with from_ctype and as_ctype converting between them.
+pub(crate) trait PassByValue: Sized {
+    type CType;
+
+    /// Convert a C value to a Rust value.
+    ///
+    /// # Safety
+    ///
+    /// `arg` must be a valid CType.
+    unsafe fn from_ctype(arg: Self::CType) -> Self;
+
+    /// Convert a Rust value to a C value.
+    fn as_ctype(self) -> Self::CType;
+
+    /// Take a value from C as an argument.
+    ///
+    /// # Safety
+    ///
+    /// `arg` must be a valid CType.  This is typically ensured either by requiring that C
+    /// code not modify it, or by defining the valid values in C comments.
+    unsafe fn from_arg(arg: Self::CType) -> Self {
+        // SAFETY:
+        //  - arg is a valid CType (promised by caller)
+        unsafe { Self::from_ctype(arg) }
+    }
+
+    /// Take a value from C as a pointer argument, replacing it with the given value.  This is used
+    /// to invalidate the C value as an additional assurance against subsequent use of the value.
+    ///
+    /// # Safety
+    ///
+    /// `*arg` must be a valid CType, as with [`from_arg`].
+    unsafe fn take_from_arg(arg: *mut Self::CType, mut replacement: Self::CType) -> Self {
+        // SAFETY:
+        //  - arg is valid (promised by caller)
+        //  - replacement is valid (guaranteed by Rust)
+        unsafe { std::ptr::swap(arg, &mut replacement) };
+        // SAFETY:
+        //  - replacement (formerly *arg) is a valid CType (promised by caller)
+        unsafe { PassByValue::from_arg(replacement) }
+    }
+
+    /// Return a value to C
+    fn return_val(self) -> Self::CType {
+        self.as_ctype()
+    }
+
+    /// Return a value to C, via an "output parameter"
+    ///
+    /// # Safety
+    ///
+    /// `arg_out` must not be NULL and must be properly aligned and pointing to valid memory
+    /// of the size of CType.
+    unsafe fn to_arg_out(self, arg_out: *mut Self::CType) {
+        debug_assert!(!arg_out.is_null());
+        // SAFETY:
+        //  - arg_out is not NULL (promised by caller, asserted)
+        //  - arg_out is properly aligned and points to valid memory (promised by caller)
+        unsafe { *arg_out = self.as_ctype() };
+    }
+}
+
+/// Support for values passed to Rust by pointer.  These are represented as opaque structs in C,
+/// and always handled as pointers.
+///
+/// # Safety
+///
+/// The functions provided by this trait are used directly in C interface functions, and make the
+/// following expectations of the C code:
+///
+///  - When passing a value to Rust (via the `…arg…` functions),
+///    - the pointer must not be NULL;
+///    - the pointer must be one previously returned from Rust; and
+///    - the memory addressed by the pointer must never be modified by C code.
+///  - For `from_arg_ref`, the value must not be modified during the call to the Rust function
+///  - For `from_arg_ref_mut`, the value must not be accessed (read or write) during the call
+///    (these last two points are trivially ensured by all TC… types being non-threadsafe)
+///  - For `take_from_arg`, the pointer becomes invalid and must not be used in _any way_ after it
+///    is passed to the Rust function.
+///  - For `return_val` and `to_arg_out`, it is the C caller's responsibility to later free the value.
+///  - For `to_arg_out`, `arg_out` must not be NULL and must be properly aligned and pointing to
+///    valid memory.
+///
+/// These requirements should be expressed in the C documentation for the type implementing this
+/// trait.
+pub(crate) trait PassByPointer: Sized {
+    /// Take a value from C as an argument.
+    ///
+    /// # Safety
+    ///
+    /// See trait documentation.
+    unsafe fn take_from_arg(arg: *mut Self) -> Self {
+        debug_assert!(!arg.is_null());
+        // SAFETY: see trait documentation
+        unsafe { *(Box::from_raw(arg)) }
+    }
+
+    /// Borrow a value from C as an argument.
+    ///
+    /// # Safety
+    ///
+    /// See trait documentation.
+    unsafe fn from_arg_ref<'a>(arg: *const Self) -> &'a Self {
+        debug_assert!(!arg.is_null());
+        // SAFETY: see trait documentation
+        unsafe { &*arg }
+    }
+
+    /// Mutably borrow a value from C as an argument.
+    ///
+    /// # Safety
+    ///
+    /// See trait documentation.
+    unsafe fn from_arg_ref_mut<'a>(arg: *mut Self) -> &'a mut Self {
+        debug_assert!(!arg.is_null());
+        // SAFETY: see trait documentation
+        unsafe { &mut *arg }
+    }
+
+    /// Return a value to C, transferring ownership
+    ///
+    /// # Safety
+    ///
+    /// See trait documentation.
+    unsafe fn return_val(self) -> *mut Self {
+        Box::into_raw(Box::new(self))
+    }
+
+    /// Return a value to C, transferring ownership, via an "output parameter".
+    ///
+    /// # Safety
+    ///
+    /// See trait documentation.
+    unsafe fn to_arg_out(self, arg_out: *mut *mut Self) {
+        // SAFETY: see trait documentation
+        unsafe {
+            *arg_out = self.return_val();
+        }
+    }
+}

lib/src/uuid.rs

@@ -54,7 +54,9 @@ pub extern "C" fn tc_uuid_to_buf<'a>(tcuuid: TCUuid, buf: *mut libc::c_char) {
     let buf: &'a mut [u8] = unsafe {
         std::slice::from_raw_parts_mut(buf as *mut u8, ::uuid::adapter::Hyphenated::LENGTH)
     };
-    let uuid: Uuid = Uuid::from_arg(tcuuid);
+    // SAFETY:
+    //  - tcuuid is a valid TCUuid (all byte patterns are valid)
+    let uuid: Uuid = unsafe { Uuid::from_arg(tcuuid) };
     uuid.to_hyphenated().encode_lower(buf);
 }
 
@@ -62,7 +64,9 @@ pub extern "C" fn tc_uuid_to_buf<'a>(tcuuid: TCUuid, buf: *mut libc::c_char) {
 /// at least TC_UUID_STRING_BYTES long.  No NUL terminator is added.
 #[no_mangle]
 pub extern "C" fn tc_uuid_to_str(tcuuid: TCUuid) -> *mut TCString<'static> {
-    let uuid: Uuid = Uuid::from_arg(tcuuid);
+    // SAFETY:
+    //  - tcuuid is a valid TCUuid (all byte patterns are valid)
+    let uuid: Uuid = unsafe { Uuid::from_arg(tcuuid) };
     let s = uuid.to_string();
     // SAFETY: see TCString docstring
     unsafe { TCString::from(s).return_val() }

lib/taskchampion.h

@@ -127,18 +127,18 @@ typedef struct TCTask TCTask;
  */
 typedef struct TCTags {
   /**
-   * strings representing each tag. these remain owned by the
-   * TCTags instance and will be freed by tc_tags_free.
+   * number of tags in items
    */
-  struct TCString *const *tags;
+  size_t len;
   /**
-   * number of tags in tags
-   */
-  size_t num_tags;
-  /**
-   * total size of tags (internal use only)
+   * total size of items (internal use only)
    */
   size_t _capacity;
+  /**
+   * strings representing each tag. these remain owned by the TCTags instance and will be freed
+   * by tc_tags_free.
+   */
+  struct TCString *const *items;
 } TCTags;
 
 /**
@@ -155,8 +155,8 @@ extern "C" {
 #endif // __cplusplus
 
 /**
- * Free a TCTags instance.  The given pointer must not be NULL.  The instance must not be used
- * after this call.
+ * Free a TCTags instance.  The instance, and all TCStrings it contains, must not be used after
+ * this call.
  */
 void tc_tags_free(struct TCTags *tctags);