From 76cbc2880b6cef3781d6a2e67027d644306280cd Mon Sep 17 00:00:00 2001
From: "Dustin J. Mitchell" <dustin@v.igoro.us>
Date: Sat, 12 Feb 2022 00:26:57 +0000
Subject: [PATCH] refactor annotations to handle invalid strings

---
 lib/src/annotation.rs | 22 ++++++++++------------
 lib/src/task.rs       | 13 +++++++++----
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/lib/src/annotation.rs b/lib/src/annotation.rs
index 8dfeb1b73..167afc498 100644
--- a/lib/src/annotation.rs
+++ b/lib/src/annotation.rs
@@ -1,6 +1,6 @@
 use crate::traits::*;
 use crate::types::*;
-use taskchampion::Annotation;
+use chrono::prelude::*;
 
 /// TCAnnotation contains the details of an annotation.
 #[repr(C)]
@@ -12,9 +12,11 @@ pub struct TCAnnotation {
 }
 
 impl PassByValue for TCAnnotation {
-    type RustType = Annotation;
+    // NOTE: we cannot use `RustType = Annotation` here because conversion of the
+    // TCString to a String can fail.
+    type RustType = (DateTime<Utc>, TCString<'static>);
 
-    unsafe fn from_ctype(self) -> Annotation {
+    unsafe fn from_ctype(self) -> Self::RustType {
         // SAFETY:
         //  - any time_t value is valid
         //  - time_t is not zero, so unwrap is safe (see type docstring)
@@ -22,18 +24,14 @@ impl PassByValue for TCAnnotation {
         // SAFETY:
         //  - self is owned, so we can take ownership of this TCString
         //  - self.description is a valid, non-null TCString (see type docstring)
-        let description = unsafe { TCString::take_from_arg(self.description) }
-            .into_string()
-            // TODO: might not be valid utf-8
-            .unwrap();
-        Annotation { entry, description }
+        let description = unsafe { TCString::take_from_arg(self.description) };
+        (entry, description)
     }
 
-    fn as_ctype(arg: Annotation) -> Self {
-        let description: TCString = arg.description.into();
+    fn as_ctype((entry, description): Self::RustType) -> Self {
         TCAnnotation {
-            entry: libc::time_t::as_ctype(Some(arg.entry)),
-            // SAFETY: caller will later free this value via tc_annotation_free
+            entry: libc::time_t::as_ctype(Some(entry)),
+            // SAFETY: caller assumes ownership of this value
             description: unsafe { description.return_val() },
         }
     }
diff --git a/lib/src/task.rs b/lib/src/task.rs
index 717f5de78..03b86d806 100644
--- a/lib/src/task.rs
+++ b/lib/src/task.rs
@@ -6,7 +6,7 @@ use std::convert::TryFrom;
 use std::ops::Deref;
 use std::ptr::NonNull;
 use std::str::FromStr;
-use taskchampion::{Tag, Task, TaskMut};
+use taskchampion::{Annotation, Tag, Task, TaskMut};
 
 /// A task, as publicly exposed by this library.
 ///
@@ -331,7 +331,10 @@ pub unsafe extern "C" fn tc_task_get_annotations<'a>(task: *mut TCTask) -> TCAnn
     wrap(task, |task| {
         let vec: Vec<TCAnnotation> = task
             .get_annotations()
-            .map(|a| TCAnnotation::as_ctype(a))
+            .map(|a| {
+                let description = TCString::from(a.description);
+                TCAnnotation::as_ctype((a.entry, description))
+            })
             .collect();
         TCAnnotationList::return_val(vec)
     })
@@ -513,11 +516,13 @@ pub unsafe extern "C" fn tc_task_add_annotation(
     annotation: *mut TCAnnotation,
 ) -> TCResult {
     // SAFETY: see TCAnnotation docstring
-    let ann = unsafe { TCAnnotation::take_from_arg(annotation, TCAnnotation::default()) };
+    let (entry, description) =
+        unsafe { TCAnnotation::take_from_arg(annotation, TCAnnotation::default()) };
     wrap_mut(
         task,
         |task| {
-            task.add_annotation(ann)?;
+            let description = description.into_string()?;
+            task.add_annotation(Annotation { entry, description })?;
             Ok(TCResult::Ok)
         },
         TCResult::Error,