Merge 'Clean up extension types API, introduce json text subtype' from Preston Thorpe

This PR cleans up some comments in the extension API and prevents
extensions themselves from calling 'free' on Value types that are
exposed to the user facing traits, as well as changes the `from_ffi`
method for OwnedValues to take ownership and automatically free the
values to prevent memory leaks.
This PR also finds the name of the `args: &[Value]` argument for scalar
functions in extensions, and uses that in the proc macro, instead of
relying on documentation to communicate that the parameter must be named
`args`.

Closes #1054

extensions/core/Cargo.toml

@@ -8,7 +8,7 @@ repository.workspace = true
 description = "Limbo extensions core"
 
 [features]
-default = []
+core_only = []
 static = []
 
 [dependencies]

extensions/core/src/lib.rs

@@ -73,6 +73,7 @@ pub struct VTabModuleImpl {
     pub rowid: VtabRowIDFn,
 }
 
+#[cfg(feature = "core_only")]
 impl VTabModuleImpl {
     pub fn init_schema(&self, args: Vec<Value>) -> ExtResult<String> {
         let schema = unsafe { (self.create_schema)(args.as_ptr(), args.len() as i32) };
@@ -80,7 +81,7 @@ impl VTabModuleImpl {
             return Err(ResultCode::InvalidArgs);
         }
         for arg in args {
-            unsafe { arg.free() };
+            unsafe { arg.__free_internal_type() };
         }
         let schema = unsafe { std::ffi::CString::from_raw(schema) };
         Ok(schema.to_string_lossy().to_string())

extensions/core/src/types.rs

@@ -27,11 +27,15 @@ pub enum ResultCode {
 
 impl ResultCode {
     pub fn is_ok(&self) -> bool {
-        matches!(self, ResultCode::OK)
+        matches!(self, Self::OK)
+    }
+
+    pub fn is_error(&self) -> bool {
+        !matches!(self, Self::OK | Self::RowID | Self::EOF)
     }
 
     pub fn has_error_set(&self) -> bool {
-        matches!(self, ResultCode::CustomError)
+        matches!(self, Self::CustomError)
     }
 }
 
@@ -86,6 +90,13 @@ union ValueData {
     error: *const ErrValue,
 }
 
+#[repr(C)]
+#[derive(PartialEq, Clone, Copy, Debug)]
+enum TextSubtype {
+    Text,
+    Json,
+}
+
 impl std::fmt::Debug for Value {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self.value_type {
@@ -109,6 +120,7 @@ impl std::fmt::Debug for Value {
 
 #[repr(C)]
 pub struct TextValue {
+    _type: TextSubtype,
     text: *const u8,
     len: u32,
 }
@@ -126,6 +138,7 @@ impl std::fmt::Debug for TextValue {
 impl Default for TextValue {
     fn default() -> Self {
         Self {
+            _type: TextSubtype::Text,
             text: std::ptr::null(),
             len: 0,
         }
@@ -133,24 +146,31 @@ impl Default for TextValue {
 }
 
 impl TextValue {
-    pub(crate) fn new(text: *const u8, len: usize) -> Self {
+    fn new(text: *const u8, len: usize) -> Self {
         Self {
+            _type: TextSubtype::Text,
             text,
             len: len as u32,
         }
     }
 
-    pub(crate) fn new_boxed(s: String) -> Box<Self> {
+    fn new_boxed(s: String, sub: TextSubtype) -> Box<Self> {
+        let len = s.len();
         let buffer = s.into_boxed_str();
-        let ptr = buffer.as_ptr();
-        let len = buffer.len();
-        std::mem::forget(buffer);
+        let strbox = Box::into_raw(buffer);
         Box::new(Self {
-            text: ptr,
+            _type: sub,
+            text: strbox as *const u8,
             len: len as u32,
         })
     }
 
+    fn free(self) {
+        if !self.text.is_null() {
+            let _ = unsafe { Box::from_raw(self.text as *mut u8) };
+        }
+    }
+
     fn as_str(&self) -> &str {
         if self.text.is_null() {
             return "";
@@ -162,7 +182,7 @@ impl TextValue {
 }
 
 #[repr(C)]
-pub struct ErrValue {
+struct ErrValue {
     code: ResultCode,
     message: *mut TextValue,
 }
@@ -186,16 +206,10 @@ impl ErrValue {
             message: Box::into_raw(Box::new(text_value)),
         }
     }
-
-    unsafe fn free(self) {
-        if !self.message.is_null() {
-            let _ = Box::from_raw(self.message); // Freed by the same library
-        }
-    }
 }
 
 #[repr(C)]
-pub struct Blob {
+struct Blob {
     data: *const u8,
     size: u64,
 }
@@ -217,6 +231,12 @@ impl Blob {
         }
         unsafe { std::slice::from_raw_parts(self.data, self.size as usize) }
     }
+
+    fn free(self) {
+        if !self.data.is_null() {
+            let _ = unsafe { Box::from_raw(self.data as *mut u8) };
+        }
+    }
 }
 
 impl Value {
@@ -229,10 +249,6 @@ impl Value {
     }
 
     /// Returns the value type of the Value
-    /// # Safety
-    /// This function accesses the value_type field of the union.
-    /// it is safe to call this function as long as the value was properly
-    /// constructed with one of the provided methods
     pub fn value_type(&self) -> ValueType {
         self.value_type
     }
@@ -262,20 +278,36 @@ impl Value {
         }
     }
 
-    /// Returns the blob value if the Value is the proper type
-    pub fn to_blob(&self) -> Option<Vec<u8>> {
-        if self.value_type != ValueType::Blob {
-            return None;
+    pub fn is_json(&self) -> bool {
+        unsafe {
+            if self.value_type == ValueType::Text && !self.value.text.is_null() {
+                let txt = &*self.value.text;
+                return txt._type == TextSubtype::Json;
+            }
         }
-        if unsafe { self.value.blob.is_null() } {
-            return None;
-        }
-        let blob = unsafe { &*(self.value.blob) };
-        let slice = unsafe { std::slice::from_raw_parts(blob.data, blob.size as usize) };
-        Some(slice.to_vec())
+        false
     }
 
-    /// Returns the integer value if the Value is the proper type
+    /// Returns the blob value if the ValueType is Blob
+    pub fn to_blob(&self) -> Option<Vec<u8>> {
+        match self.value_type {
+            ValueType::Blob => {
+                if unsafe { self.value.blob.is_null() } {
+                    return None;
+                }
+                let blob = unsafe { &*(self.value.blob) };
+                let slice = unsafe { std::slice::from_raw_parts(blob.data, blob.size as usize) };
+                Some(slice.to_vec())
+            }
+            ValueType::Text => {
+                let txt = self.to_text().unwrap_or_default();
+                Some(txt.as_bytes().to_vec())
+            }
+            _ => None,
+        }
+    }
+
+    /// Returns the value as an integer (casts or converts if possible)
     pub fn to_integer(&self) -> Option<i64> {
         match self.value_type() {
             ValueType::Integer => Some(unsafe { self.value.int }),
@@ -319,7 +351,6 @@ impl Value {
     // Return ValueData as raw bytes
     pub fn as_bytes(&self) -> Vec<u8> {
         let mut bytes = vec![];
-
         unsafe {
             match self.value_type {
                 ValueType::Integer => bytes.extend_from_slice(&self.value.int.to_le_bytes()),
@@ -356,10 +387,17 @@ impl Value {
     }
 
     /// Creates a new text Value from a String
-    /// This function allocates/leaks the string
-    /// and must be free'd manually
     pub fn from_text(s: String) -> Self {
-        let txt_value = TextValue::new_boxed(s);
+        let txt_value = TextValue::new_boxed(s, TextSubtype::Text);
+        let ptr = Box::into_raw(txt_value);
+        Self {
+            value_type: ValueType::Text,
+            value: ValueData { text: ptr },
+        }
+    }
+
+    pub fn from_json(s: String) -> Self {
+        let txt_value = TextValue::new_boxed(s, TextSubtype::Json);
         let ptr = Box::into_raw(txt_value);
         Self {
             value_type: ValueType::Text,
@@ -368,8 +406,6 @@ impl Value {
     }
 
     /// Creates a new error Value from a ResultCode
-    /// This function allocates/leaks the error
-    /// and must be free'd manually
     pub fn error(code: ResultCode) -> Self {
         let err_val = ErrValue::new(code);
         Self {
@@ -381,7 +417,6 @@ impl Value {
     }
 
     /// Creates a new error Value from a ResultCode and a message
-    /// This function allocates/leaks the error, must be free'd manually
     pub fn error_with_message(message: String) -> Self {
         let err_value = ErrValue::new_with_message(ResultCode::CustomError, message);
         let err_box = Box::new(err_value);
@@ -394,35 +429,41 @@ impl Value {
     }
 
     /// Creates a new blob Value from a Vec<u8>
-    /// This function allocates/leaks the blob
-    /// and must be free'd manually
     pub fn from_blob(value: Vec<u8>) -> Self {
-        let boxed = Box::new(Blob::new(value.as_ptr(), value.len() as u64));
-        std::mem::forget(value);
+        let len = value.len();
+        let boxed_data = value.into_boxed_slice();
+        let ptr = Box::into_raw(boxed_data) as *const u8;
+        let boxed_blob = Box::new(Blob::new(ptr, len as u64));
         Self {
             value_type: ValueType::Blob,
             value: ValueData {
-                blob: Box::into_raw(boxed) as *const Blob,
+                blob: Box::into_raw(boxed_blob) as *const Blob,
             },
         }
     }
 
-    /// Extension authors should __not__ use this function.
+    /// Extension authors should __not__ use this function, or enable the 'core_only' feature
+    ///
     /// # Safety
     /// consumes the value while freeing the underlying memory with null check.
     /// however this does assume that the type was properly constructed with
     /// the appropriate value_type and value.
-    pub unsafe fn free(self) {
+    #[cfg(feature = "core_only")]
+    pub unsafe fn __free_internal_type(self) {
         match self.value_type {
             ValueType::Text => {
-                let _ = Box::from_raw(self.value.text as *mut TextValue);
+                let txt = Box::from_raw(self.value.text as *mut TextValue);
+                txt.free();
             }
             ValueType::Blob => {
-                let _ = Box::from_raw(self.value.blob as *mut Blob);
+                let blob = Box::from_raw(self.value.blob as *mut Blob);
+                blob.free();
             }
             ValueType::Error => {
                 let err_val = Box::from_raw(self.value.error as *mut ErrValue);
-                err_val.free();
+                if !err_val.message.is_null() {
+                    let _ = Box::from_raw(err_val.message);
+                }
             }
             _ => {}
         }

extensions/uuid/src/lib.rs

@@ -89,7 +89,7 @@ fn uuid7_ts(args: &[Value]) -> Value {
             let Some(text) = args[0].to_text() else {
                 return Value::null();
             };
-            let Ok(uuid) = uuid::Uuid::parse_str(&text) else {
+            let Ok(uuid) = uuid::Uuid::parse_str(text) else {
                 return Value::null();
             };
             let unix = uuid_to_unix(uuid.as_bytes());
@@ -118,7 +118,7 @@ fn uuid_blob(&self, args: &[Value]) -> Value {
     let Some(text) = args[0].to_text() else {
         return Value::null();
     };
-    match uuid::Uuid::parse_str(&text) {
+    match uuid::Uuid::parse_str(text) {
         Ok(uuid) => Value::from_blob(uuid.as_bytes().to_vec()),
         Err(_) => Value::null(),
     }