Refactor value type boxing. (#659) r=nalexander

* Pre: eliminate some occurrences of Rc, largely through the magic of Into.
* Pre: introduce FromRc to convert between refcounted types.
* Introduce ValueRc as an abstraction over Rc/Arc choice.
* Move Cloned to core.
* Move CString-creation methods to TypedValue.
* Finish transition.

core/src/lib.rs

@@ -55,12 +55,15 @@ mod value_type_set;
 mod sql_types;
 
 pub use types::{
+    Cloned,
     Entid,
+    FromRc,
     KnownEntid,
     TypedValue,
     Binding,
     ValueType,
     ValueTypeTag,
+    ValueRc,
     now,
 };
 

core/src/types.rs

@@ -8,9 +8,20 @@
 // CONDITIONS OF ANY KIND, either express or implied. See the License for the
 // specific language governing permissions and limitations under the License.
 
-use std::fmt;
+use ::std::ffi::{
-use std::rc::Rc;
+    CString,
+};
+use ::std::os::raw::c_char;
 
+use ::std::rc::{
+    Rc,
+};
+
+use ::std::sync::{
+    Arc,
+};
+
+use std::fmt;
 use ::enum_set::EnumSet;
 
 use ::ordered_float::OrderedFloat;
@@ -35,6 +46,83 @@ use ::edn::{
 
 use values;
 
+pub trait FromRc<T> {
+    fn from_rc(val: Rc<T>) -> Self;
+    fn from_arc(val: Arc<T>) -> Self;
+}
+
+impl<T> FromRc<T> for Rc<T> where T: Sized + Clone {
+    fn from_rc(val: Rc<T>) -> Self {
+        val.clone()
+    }
+
+    fn from_arc(val: Arc<T>) -> Self {
+        match ::std::sync::Arc::<T>::try_unwrap(val) {
+            Ok(v) => Self::new(v),
+            Err(r) => Self::new(r.cloned()),
+        }
+    }
+}
+
+impl<T> FromRc<T> for Arc<T> where T: Sized + Clone {
+    fn from_rc(val: Rc<T>) -> Self {
+        match ::std::rc::Rc::<T>::try_unwrap(val) {
+            Ok(v) => Self::new(v),
+            Err(r) => Self::new(r.cloned()),
+        }
+    }
+
+    fn from_arc(val: Arc<T>) -> Self {
+        val.clone()
+    }
+}
+
+impl<T> FromRc<T> for Box<T> where T: Sized + Clone {
+    fn from_rc(val: Rc<T>) -> Self {
+        match ::std::rc::Rc::<T>::try_unwrap(val) {
+            Ok(v) => Self::new(v),
+            Err(r) => Self::new(r.cloned()),
+        }
+    }
+
+    fn from_arc(val: Arc<T>) -> Self {
+        match ::std::sync::Arc::<T>::try_unwrap(val) {
+            Ok(v) => Self::new(v),
+            Err(r) => Self::new(r.cloned()),
+        }
+    }
+}
+
+// We do this a lot for errors.
+pub trait Cloned<T> {
+    fn cloned(&self) -> T;
+}
+
+impl<T: Clone> Cloned<T> for Rc<T> where T: Sized + Clone {
+    fn cloned(&self) -> T {
+        (*self.as_ref()).clone()
+    }
+}
+
+impl<T: Clone> Cloned<T> for Arc<T> where T: Sized + Clone {
+    fn cloned(&self) -> T {
+        (*self.as_ref()).clone()
+    }
+}
+
+impl<T: Clone> Cloned<T> for Box<T> where T: Sized + Clone {
+    fn cloned(&self) -> T {
+        self.as_ref().clone()
+    }
+}
+
+///
+/// This type alias exists to allow us to use different boxing mechanisms for values.
+/// This type must implement `FromRc` and `Cloned`, and a `From` implementation must exist for
+/// `TypedValue`.
+///
+pub type ValueRc<T> = Rc<T>;
+
 /// Represents one entid in the entid space.
 ///
 /// Per https://www.sqlite.org/datatype3.html (see also http://stackoverflow.com/a/8499544), SQLite
@@ -53,12 +141,6 @@ impl From<KnownEntid> for Entid {
     }
 }
 
-impl From<KnownEntid> for Binding {
-    fn from(k: KnownEntid) -> Binding {
-        Binding::Scalar(TypedValue::Ref(k.0))
-    }
-}
-
 impl From<KnownEntid> for TypedValue {
     fn from(k: KnownEntid) -> TypedValue {
         TypedValue::Ref(k.0)
@@ -177,8 +259,8 @@ pub enum TypedValue {
     Double(OrderedFloat<f64>),
     Instant(DateTime<Utc>),               // Use `into()` to ensure truncation.
     // TODO: &str throughout?
-    String(Rc<String>),
+    String(ValueRc<String>),
-    Keyword(Rc<NamespacedKeyword>),
+    Keyword(ValueRc<NamespacedKeyword>),
     Uuid(Uuid),                        // It's only 128 bits, so this should be acceptable to clone.
 }
 
@@ -197,26 +279,13 @@ pub enum TypedValue {
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum Binding {
     Scalar(TypedValue),
-    Vec(Rc<Vec<Binding>>),
+    Vec(ValueRc<Vec<Binding>>),
-    Map(Rc<StructuredMap>),
+    Map(ValueRc<StructuredMap>),
 }
 
-impl From<TypedValue> for Binding {
+impl<T> From<T> for Binding where T: Into<TypedValue> {
-    fn from(src: TypedValue) -> Self {
+    fn from(value: T) -> Binding {
-        Binding::Scalar(src)
+        Binding::Scalar(value.into())
-    }
-}
-
-
-impl<'a> From<&'a str> for Binding {
-    fn from(value: &'a str) -> Binding {
-        Binding::Scalar(TypedValue::String(Rc::new(value.to_string())))
-    }
-}
-
-impl From<String> for Binding {
-    fn from(value: String) -> Binding {
-        Binding::Scalar(TypedValue::String(Rc::new(value)))
     }
 }
 
@@ -239,8 +308,8 @@ impl Binding {
 ///
 /// We entirely support the former, and partially support the latter -- you can alias
 /// using a different keyword only.
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Clone, Debug, Eq, PartialEq)]
-pub struct StructuredMap(IndexMap<Rc<NamespacedKeyword>, Binding>);
+pub struct StructuredMap(IndexMap<ValueRc<NamespacedKeyword>, Binding>);
 
 impl Binding {
     /// Returns true if the provided type is `Some` and matches this value's type, or if the
@@ -293,17 +362,17 @@ impl TypedValue {
     }
 
     /// Construct a new `TypedValue::Keyword` instance by cloning the provided
-    /// values and wrapping them in a new `Rc`. This is expensive, so this might
+    /// values and wrapping them in a new `ValueRc`. This is expensive, so this might
     /// be best limited to tests.
     pub fn typed_ns_keyword(ns: &str, name: &str) -> TypedValue {
-        TypedValue::Keyword(Rc::new(NamespacedKeyword::new(ns, name)))
+        NamespacedKeyword::new(ns, name).into()
     }
 
     /// Construct a new `TypedValue::String` instance by cloning the provided
-    /// value and wrapping it in a new `Rc`. This is expensive, so this might
+    /// value and wrapping it in a new `ValueRc`. This is expensive, so this might
     /// be best limited to tests.
     pub fn typed_string(s: &str) -> TypedValue {
-        TypedValue::String(Rc::new(s.to_string()))
+        s.into()
     }
 
     pub fn current_instant() -> TypedValue {
@@ -363,19 +432,49 @@ impl From<Uuid> for TypedValue {
 
 impl<'a> From<&'a str> for TypedValue {
     fn from(value: &'a str) -> TypedValue {
-        TypedValue::String(Rc::new(value.to_string()))
+        TypedValue::String(ValueRc::new(value.to_string()))
+    }
+}
+
+impl From<Arc<String>> for TypedValue {
+    fn from(value: Arc<String>) -> TypedValue {
+        TypedValue::String(ValueRc::from_arc(value))
+    }
+}
+
+impl From<Rc<String>> for TypedValue {
+    fn from(value: Rc<String>) -> TypedValue {
+        TypedValue::String(ValueRc::from_rc(value))
+    }
+}
+
+impl From<Box<String>> for TypedValue {
+    fn from(value: Box<String>) -> TypedValue {
+        TypedValue::String(ValueRc::new(*value))
     }
 }
 
 impl From<String> for TypedValue {
     fn from(value: String) -> TypedValue {
-        TypedValue::String(Rc::new(value))
+        TypedValue::String(ValueRc::new(value))
+    }
+}
+
+impl From<Arc<NamespacedKeyword>> for TypedValue {
+    fn from(value: Arc<NamespacedKeyword>) -> TypedValue {
+        TypedValue::Keyword(ValueRc::from_arc(value))
+    }
+}
+
+impl From<Rc<NamespacedKeyword>> for TypedValue {
+    fn from(value: Rc<NamespacedKeyword>) -> TypedValue {
+        TypedValue::Keyword(ValueRc::from_rc(value))
     }
 }
 
 impl From<NamespacedKeyword> for TypedValue {
     fn from(value: NamespacedKeyword) -> TypedValue {
-        TypedValue::Keyword(Rc::new(value))
+        TypedValue::Keyword(ValueRc::new(value))
     }
 }
 
@@ -412,7 +511,7 @@ impl TypedValue {
         }
     }
 
-    pub fn into_kw(self) -> Option<Rc<NamespacedKeyword>> {
+    pub fn into_kw(self) -> Option<ValueRc<NamespacedKeyword>> {
         match self {
             TypedValue::Keyword(v) => Some(v),
             _ => None,
@@ -454,13 +553,61 @@ impl TypedValue {
         }
     }
 
-    pub fn into_string(self) -> Option<Rc<String>> {
+    pub fn into_string(self) -> Option<ValueRc<String>> {
         match self {
             TypedValue::String(v) => Some(v),
             _ => None,
         }
     }
 
+    pub fn into_c_string(self) -> Option<*mut c_char> {
+        match self {
+            TypedValue::String(v) => {
+                // Get an independent copy of the string.
+                let s: String = v.cloned();
+
+                // Make a CString out of the new bytes.
+                let c: CString = CString::new(s).expect("String conversion failed!");
+
+                // Return a C-owned pointer.
+                Some(c.into_raw())
+            },
+            _ => None,
+        }
+    }
+
+    pub fn into_kw_c_string(self) -> Option<*mut c_char> {
+        match self {
+            TypedValue::Keyword(v) => {
+                // Get an independent copy of the string.
+                let s: String = v.to_string();
+
+                // Make a CString out of the new bytes.
+                let c: CString = CString::new(s).expect("String conversion failed!");
+
+                // Return a C-owned pointer.
+                Some(c.into_raw())
+            },
+            _ => None,
+        }
+    }
+
+    pub fn into_uuid_c_string(self) -> Option<*mut c_char> {
+        match self {
+            TypedValue::Uuid(v) => {
+                // Get an independent copy of the string.
+                let s: String = v.hyphenated().to_string();
+
+                // Make a CString out of the new bytes.
+                let c: CString = CString::new(s).expect("String conversion failed!");
+
+                // Return a C-owned pointer.
+                Some(c.into_raw())
+            },
+            _ => None,
+        }
+    }
+
     pub fn into_uuid(self) -> Option<Uuid> {
         match self {
             TypedValue::Uuid(v) => Some(v),
@@ -491,7 +638,7 @@ impl Binding {
         }
     }
 
-    pub fn into_kw(self) -> Option<Rc<NamespacedKeyword>> {
+    pub fn into_kw(self) -> Option<ValueRc<NamespacedKeyword>> {
         match self {
             Binding::Scalar(TypedValue::Keyword(v)) => Some(v),
             _ => None,
@@ -533,7 +680,7 @@ impl Binding {
         }
     }
 
-    pub fn into_string(self) -> Option<Rc<String>> {
+    pub fn into_string(self) -> Option<ValueRc<String>> {
         match self {
             Binding::Scalar(TypedValue::String(v)) => Some(v),
             _ => None,

db/src/cache.rs

@@ -68,8 +68,6 @@ use std::iter::{
 
 use std::mem;
 
-use std::rc::Rc;
-
 use std::sync::Arc;
 
 use std::iter::Peekable;
@@ -85,6 +83,7 @@ use mentat_core::{
     Schema,
     TypedValue,
     UpdateableCache,
+    ValueRc,
 };
 
 use mentat_core::util::{
@@ -190,7 +189,7 @@ pub type Aev = (Entid, Entid, TypedValue);
 
 pub struct AevFactory {
     // Our own simple string-interning system.
-    strings: HashSet<Rc<String>>,
+    strings: HashSet<ValueRc<String>>,
 }
 
 impl AevFactory {

db/src/db.rs

@@ -19,7 +19,6 @@ use std::fmt::Display;
 use std::iter::{once, repeat};
 use std::ops::Range;
 use std::path::Path;
-use std::rc::Rc;
 
 use itertools;
 use itertools::Itertools;
@@ -51,7 +50,9 @@ use mentat_core::{
     TypedValue,
     ToMicros,
     ValueType,
+    ValueRc,
 };
+
 use errors::{ErrorKind, Result, ResultExt};
 use metadata;
 use schema::{
@@ -389,7 +390,7 @@ impl TypedSQLValue for TypedValue {
             // share a tag.
             (5, rusqlite::types::Value::Integer(x)) => Ok(TypedValue::Long(x)),
             (5, rusqlite::types::Value::Real(x)) => Ok(TypedValue::Double(x.into())),
-            (10, rusqlite::types::Value::Text(x)) => Ok(TypedValue::String(Rc::new(x))),
+            (10, rusqlite::types::Value::Text(x)) => Ok(x.into()),
             (11, rusqlite::types::Value::Blob(x)) => {
                 let u = Uuid::from_bytes(x.as_slice());
                 if u.is_err() {
@@ -400,7 +401,7 @@ impl TypedSQLValue for TypedValue {
                 Ok(TypedValue::Uuid(u.unwrap()))
             },
             (13, rusqlite::types::Value::Text(x)) => {
-                to_namespaced_keyword(&x).map(|k| TypedValue::Keyword(Rc::new(k)))
+                to_namespaced_keyword(&x).map(|k| k.into())
             },
             (_, value) => bail!(ErrorKind::BadSQLValuePair(value, value_type_tag)),
         }
@@ -420,8 +421,8 @@ impl TypedSQLValue for TypedValue {
             &Value::Integer(x) => Some(TypedValue::Long(x)),
             &Value::Uuid(x) => Some(TypedValue::Uuid(x)),
             &Value::Float(ref x) => Some(TypedValue::Double(x.clone())),
-            &Value::Text(ref x) => Some(TypedValue::String(Rc::new(x.clone()))),
+            &Value::Text(ref x) => Some(x.clone().into()),
-            &Value::NamespacedKeyword(ref x) => Some(TypedValue::Keyword(Rc::new(x.clone()))),
+            &Value::NamespacedKeyword(ref x) => Some(x.clone().into()),
             _ => None
         }
     }
@@ -872,7 +873,7 @@ impl MentatStoring for rusqlite::Connection {
         let chunks: itertools::IntoChunks<_> = entities.into_iter().chunks(max_vars / bindings_per_statement);
 
         // From string to (searchid, value_type_tag).
-        let mut seen: HashMap<Rc<String>, (i64, i32)> = HashMap::with_capacity(entities.len());
+        let mut seen: HashMap<ValueRc<String>, (i64, i32)> = HashMap::with_capacity(entities.len());
 
         // We'd like to flat_map here, but it's not obvious how to flat_map across Result.
         let results: Result<Vec<()>> = chunks.into_iter().map(|chunk| -> Result<()> {

db/src/debug.rs

@@ -14,7 +14,6 @@
 
 use std::borrow::Borrow;
 use std::io::{Write};
-use std::rc::Rc;
 
 use itertools::Itertools;
 use rusqlite;
@@ -112,7 +111,7 @@ trait ToIdent {
 impl ToIdent for TypedValue {
     fn map_ident(self, schema: &Schema) -> Self {
         if let TypedValue::Ref(e) = self {
-            schema.get_ident(e).cloned().map(|i| TypedValue::Keyword(Rc::new(i))).unwrap_or(TypedValue::Ref(e))
+            schema.get_ident(e).cloned().map(|i| i.into()).unwrap_or(TypedValue::Ref(e))
         } else {
             self
         }

ffi/src/lib.rs

@@ -52,7 +52,6 @@ pub mod utils;
 
 pub use utils::strings::{
     c_char_to_string,
-    c_char_from_rc,
     kw_from_string,
     string_to_c_char,
 };
@@ -276,7 +275,7 @@ pub unsafe extern "C" fn typed_value_as_entid(typed_value: *mut TypedValue) ->
 #[no_mangle]
 pub unsafe extern "C" fn typed_value_as_kw(typed_value: *mut TypedValue) ->  *const c_char {
     let typed_value = Box::from_raw(typed_value);
-    string_to_c_char(typed_value.into_kw().expect("Typed value cannot be coerced into a Namespaced Keyword").to_string())
+    typed_value.into_kw_c_string().expect("Typed value cannot be coerced into a Namespaced Keyword")
 }
 
 //as_boolean
@@ -305,20 +304,20 @@ pub unsafe extern "C" fn typed_value_as_timestamp(typed_value: *mut TypedValue)
 #[no_mangle]
 pub unsafe extern "C" fn typed_value_as_string(typed_value: *mut TypedValue) ->  *const c_char {
     let typed_value = Box::from_raw(typed_value);
-    c_char_from_rc(typed_value.into_string().expect("Typed value cannot be coerced into a String"))
+    typed_value.into_c_string().expect("Typed value cannot be coerced into a String")
 }
 
 //as_uuid
 #[no_mangle]
 pub unsafe extern "C" fn typed_value_as_uuid(typed_value: *mut TypedValue) ->  *const c_char {
     let typed_value = Box::from_raw(typed_value);
-    string_to_c_char(typed_value.into_uuid_string().expect("Typed value cannot be coerced into a Uuid"))
+    typed_value.into_uuid_c_string().expect("Typed value cannot be coerced into a String")
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn row_at_index(rows: *mut Vec<Vec<TypedValue>>, index: c_int) ->  *mut Vec<TypedValue> {
     let result = &*rows;
-    result.get(index as usize).map_or(std::ptr::null_mut(), |v| Box::into_raw(Box::new(v.clone())))
+    result.get(index as usize).map_or_else(std::ptr::null_mut, |v| Box::into_raw(Box::new(v.clone())))
 }
 
 #[no_mangle]
@@ -342,62 +341,62 @@ pub unsafe extern "C" fn values_iter(values: *mut Vec<TypedValue>) ->  *mut Type
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next(iter: *mut TypedValueIterator) ->  *const TypedValue {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| &v as *const TypedValue)
+    iter.next().map_or_else(std::ptr::null, |v| &v as *const TypedValue)
 }
 
 //as_long
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_long(iter: *mut TypedValueIterator) ->  *const i64 {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| &v.into_long().expect("Typed value cannot be coerced into a Long") as *const i64)
+    iter.next().map_or_else(std::ptr::null, |v| &v.into_long().expect("Typed value cannot be coerced into a Long") as *const i64)
 }
 // as ref
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_entid(iter: *mut TypedValueIterator) ->  *const Entid {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| &v.into_entid().expect("Typed value cannot be coerced into am Entid") as *const Entid)
+    iter.next().map_or_else(std::ptr::null, |v| &v.into_entid().expect("Typed value cannot be coerced into am Entid") as *const Entid)
 }
 
 // as kw
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_kw(iter: *mut TypedValueIterator) ->  *const c_char {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| string_to_c_char(v.into_kw().expect("Typed value cannot be coerced into a Namespaced Keyword").to_string()))
+    iter.next().map_or_else(std::ptr::null, |v| v.into_kw_c_string().expect("Typed value cannot be coerced into a Namespaced Keyword"))
 }
 
 //as_boolean
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_boolean(iter: *mut TypedValueIterator) ->  *const bool {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| &v.into_boolean().expect("Typed value cannot be coerced into a Boolean") as *const bool)
+    iter.next().map_or_else(std::ptr::null, |v| &v.into_boolean().expect("Typed value cannot be coerced into a Boolean") as *const bool)
 }
 
 //as_double
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_double(iter: *mut TypedValueIterator) ->  *const f64 {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| &v.into_double().expect("Typed value cannot be coerced into a Double") as *const f64)
+    iter.next().map_or_else(std::ptr::null, |v| &v.into_double().expect("Typed value cannot be coerced into a Double") as *const f64)
 }
 
 //as_timestamp
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_timestamp(iter: *mut TypedValueIterator) ->  *const i64 {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| v.into_timestamp().expect("Typed value cannot be coerced into a Timestamp") as *const i64)
+    iter.next().map_or_else(std::ptr::null, |v| v.into_timestamp().expect("Typed value cannot be coerced into a Timestamp") as *const i64)
 }
 
 //as_string
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_string(iter: *mut TypedValueIterator) ->  *const c_char {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| c_char_from_rc(v.into_string().expect("Typed value cannot be coerced into a String")))
+    iter.next().map_or_else(std::ptr::null, |v| v.into_c_string().expect("Typed value cannot be coerced into a String"))
 }
 
 //as_uuid
 #[no_mangle]
 pub unsafe extern "C" fn values_iter_next_as_uuid(iter: *mut TypedValueIterator) ->  *const c_char {
     let iter = &mut *iter;
-    iter.next().map_or(std::ptr::null_mut(), |v| string_to_c_char(v.into_uuid_string().expect("Typed value cannot be coerced into a Uuid")))
+    iter.next().map_or_else(std::ptr::null, |v| v.into_uuid_c_string().expect("Typed value cannot be coerced into a Uuid"))
 }
 
 #[no_mangle]
@@ -426,7 +425,7 @@ pub unsafe extern "C" fn value_at_index_as_entid(values: *mut Vec<TypedValue>, i
 pub unsafe extern "C" fn value_at_index_as_kw(values: *mut Vec<TypedValue>, index: c_int) ->  *const c_char {
     let result = &*values;
     let value = result.get(index as usize).expect("No value at index");
-    string_to_c_char(value.clone().into_kw().expect("Typed value cannot be coerced into a Namespaced Keyword").to_string())
+    value.clone().into_kw_c_string().expect("Typed value cannot be coerced into a Namespaced Keyword")
 }
 
 //as_boolean
@@ -458,7 +457,7 @@ pub unsafe extern "C" fn value_at_index_as_timestamp(values: *mut Vec<TypedValue
 pub unsafe extern "C" fn value_at_index_as_string(values: *mut Vec<TypedValue>, index: c_int) ->  *mut c_char {
     let result = &*values;
     let value = result.get(index as usize).expect("No value at index");
-    c_char_from_rc(value.clone().into_string().expect("Typed value cannot be coerced into a String"))
+    value.clone().into_c_string().expect("Typed value cannot be coerced into a String")
 }
 
 //as_uuid
@@ -466,7 +465,7 @@ pub unsafe extern "C" fn value_at_index_as_string(values: *mut Vec<TypedValue>,
 pub unsafe extern "C" fn value_at_index_as_uuid(values: *mut Vec<TypedValue>, index: c_int) ->  *mut c_char {
     let result = &*values;
     let value = result.get(index as usize).expect("No value at index");
-    string_to_c_char(value.clone().into_uuid_string().expect("Typed value cannot be coerced into a Uuid"))
+    value.clone().into_uuid_c_string().expect("Typed value cannot be coerced into a Uuid")
 }
 
 // TODO: q_prepare

ffi/src/utils.rs

@@ -9,13 +9,11 @@
 // specific language governing permissions and limitations under the License.
 
 pub mod strings {
-    use std;
     use std::ffi::{
         CString,
         CStr
     };
     use std::os::raw::c_char;
-    use std::rc::Rc;
 
     use mentat::{
         NamespacedKeyword,
@@ -31,19 +29,12 @@ pub mod strings {
         CString::new(r_string.into()).unwrap().into_raw()
     }
 
+    // TODO: validate. The input might not be a keyword!
     pub fn kw_from_string(mut keyword_string: String) -> NamespacedKeyword {
         let attr_name = keyword_string.split_off(1);
         let parts: Vec<&str> = attr_name.split("/").collect();
         NamespacedKeyword::new(parts[0], parts[1])
     }
-
-    pub fn c_char_from_rc(rc_string: Rc<String>) -> *mut c_char {
-        if let Some(str_ptr) = unsafe { Rc::into_raw(rc_string).as_ref() } {
-            string_to_c_char(str_ptr.clone())
-        } else {
-            std::ptr::null_mut()
-        }
-    }
 }
 
 pub mod log {

query-algebrizer/src/clauses/convert.rs

@@ -8,8 +8,6 @@
 // CONDITIONS OF ANY KIND, either express or implied. See the License for the
 // specific language governing permissions and limitations under the License.
 
-use std::rc::Rc;
-
 use mentat_core::{
     HasSchema,
     Schema,
@@ -166,7 +164,7 @@ impl ConjoiningClauses {
                     (true, true) => {
                         // Ambiguous: this could be a keyword or an ident.
                         // Default to keyword.
-                        Ok(Val(TypedValue::Keyword(Rc::new(x))))
+                        Ok(Val(x.into()))
                     },
                     (true, false) => {
                         // This can only be an ident. Look it up!
@@ -176,7 +174,7 @@ impl ConjoiningClauses {
                         }
                     },
                     (false, true) => {
-                        Ok(Val(TypedValue::Keyword(Rc::new(x))))
+                        Ok(Val(TypedValue::Keyword(x.into())))
                     },
                     (false, false) => {
                         Ok(Impossible(EmptyBecause::TypeMismatch {

query-algebrizer/src/clauses/fulltext.rs

@@ -260,8 +260,6 @@ impl ConjoiningClauses {
 mod testing {
     use super::*;
 
-    use std::rc::Rc;
-
     use mentat_core::{
         Attribute,
         Schema,
@@ -309,7 +307,7 @@ mod testing {
             args: vec![
                 FnArg::SrcVar(SrcVar::DefaultSrc),
                 FnArg::IdentOrKeyword(NamespacedKeyword::new("foo", "fts")),
-                FnArg::Constant(NonIntegerConstant::Text(Rc::new("needle".into()))),
+                FnArg::Constant("needle".into()),
             ],
             binding: Binding::BindRel(vec![VariableOrPlaceholder::Variable(Variable::from_valid_name("?entity")),
                                            VariableOrPlaceholder::Variable(Variable::from_valid_name("?value")),
@@ -331,7 +329,7 @@ mod testing {
         assert_eq!(clauses.0[1], ColumnConstraint::Equals(QualifiedAlias("datoms01".to_string(), Column::Fixed(DatomsColumn::Value)),
                                                           QueryValue::Column(QualifiedAlias("fulltext_values00".to_string(), Column::Fulltext(FulltextColumn::Rowid)))).into());
         assert_eq!(clauses.0[2], ColumnConstraint::Matches(QualifiedAlias("fulltext_values00".to_string(), Column::Fulltext(FulltextColumn::Text)),
-                                                           QueryValue::TypedValue(TypedValue::String(Rc::new("needle".into())))).into());
+                                                           QueryValue::TypedValue("needle".into())).into());
 
         let bindings = cc.column_bindings;
         assert_eq!(bindings.len(), 3);
@@ -367,7 +365,7 @@ mod testing {
             args: vec![
                 FnArg::SrcVar(SrcVar::DefaultSrc),
                 FnArg::IdentOrKeyword(NamespacedKeyword::new("foo", "bar")),
-                FnArg::Constant(NonIntegerConstant::Text(Rc::new("needle".into()))),
+                FnArg::Constant("needle".into()),
             ],
             binding: Binding::BindRel(vec![VariableOrPlaceholder::Variable(Variable::from_valid_name("?entity")),
                                            VariableOrPlaceholder::Variable(Variable::from_valid_name("?value")),

query-algebrizer/src/clauses/mod.rs

@@ -27,6 +27,7 @@ use std::fmt::{
 
 use mentat_core::{
     Attribute,
+    Cloned,
     Entid,
     HasSchema,
     KnownEntid,
@@ -96,17 +97,6 @@ pub use self::inputs::QueryInputs;
 
 use Known;
 
-// We do this a lot for errors.
-trait RcCloned<T> {
-    fn cloned(&self) -> T;
-}
-
-impl<T: Clone> RcCloned<T> for ::std::rc::Rc<T> {
-    fn cloned(&self) -> T {
-        self.as_ref().clone()
-    }
-}
-
 trait Contains<K, T> {
     fn when_contains<F: FnOnce() -> T>(&self, k: &K, f: F) -> Option<T>;
 }
@@ -1159,7 +1149,7 @@ fn add_attribute(schema: &mut Schema, e: Entid, a: Attribute) {
 
 #[cfg(test)]
 pub(crate) fn ident(ns: &str, name: &str) -> PatternNonValuePlace {
-    PatternNonValuePlace::Ident(::std::rc::Rc::new(NamespacedKeyword::new(ns, name)))
+    NamespacedKeyword::new(ns, name).into()
 }
 
 #[cfg(test)]

query-algebrizer/src/clauses/not.rs

@@ -88,7 +88,6 @@ impl ConjoiningClauses {
 mod testing {
     extern crate mentat_query_parser;
 
-    use std::rc::Rc;
     use std::collections::BTreeSet;
 
     use super::*;
@@ -469,7 +468,9 @@ mod testing {
              :where [?x :foo/knows "Bill"]
                     (not [?x :foo/knows ?y])]"#;
 
-        let inputs = QueryInputs::with_value_sequence(vec![(Variable::from_valid_name("?y"),TypedValue::String(Rc::new("John".to_string())))]);
+        let inputs = QueryInputs::with_value_sequence(vec![
+            (Variable::from_valid_name("?y"), "John".into())
+        ]);
         let cc = alg_with_inputs(&schema, query, inputs);
 
         let vx = Variable::from_valid_name("?x");

query-algebrizer/src/clauses/pattern.rs

@@ -9,6 +9,7 @@
 // specific language governing permissions and limitations under the License.
 
 use mentat_core::{
+    Cloned,
     Entid,
     HasSchema,
     TypedValue,
@@ -24,8 +25,6 @@ use mentat_query::{
     Variable,
 };
 
-use super::RcCloned;
-
 use clauses::{
     ConjoiningClauses,
 };
@@ -646,7 +645,6 @@ mod testing {
 
     use std::collections::BTreeMap;
     use std::collections::BTreeSet;
-    use std::rc::Rc;
 
     use mentat_core::attribute::Unique;
     use mentat_core::{
@@ -827,7 +825,7 @@ mod testing {
         let v = Variable::from_valid_name("?v");
 
         cc.input_variables.insert(a.clone());
-        cc.value_bindings.insert(a.clone(), TypedValue::Keyword(Rc::new(NamespacedKeyword::new("foo", "bar"))));
+        cc.value_bindings.insert(a.clone(), TypedValue::typed_ns_keyword("foo", "bar"));
         let known = Known::for_schema(&schema);
         cc.apply_parsed_pattern(known, Pattern {
             source: None,
@@ -931,7 +929,7 @@ mod testing {
             source: None,
             entity: PatternNonValuePlace::Variable(x.clone()),
             attribute: PatternNonValuePlace::Placeholder,
-            value: PatternValuePlace::Constant(NonIntegerConstant::Text(Rc::new("hello".to_string()))),
+            value: PatternValuePlace::Constant("hello".into()),
             tx: PatternNonValuePlace::Placeholder,
         });
 
@@ -954,7 +952,7 @@ mod testing {
         // - datoms0.value_type_tag = string
         // TODO: implement expand_type_tags.
         assert_eq!(cc.wheres, vec![
-                   ColumnConstraint::Equals(d0_v, QueryValue::TypedValue(TypedValue::String(Rc::new("hello".to_string())))),
+                   ColumnConstraint::Equals(d0_v, QueryValue::TypedValue(TypedValue::typed_string("hello"))),
                    ColumnConstraint::has_unit_type("all_datoms00".to_string(), ValueType::String),
         ].into());
     }
@@ -982,7 +980,7 @@ mod testing {
             source: None,
             entity: PatternNonValuePlace::Variable(x.clone()),
             attribute: ident("foo", "roz"),
-            value: PatternValuePlace::Constant(NonIntegerConstant::Text(Rc::new("idgoeshere".to_string()))),
+            value: PatternValuePlace::Constant("idgoeshere".into()),
             tx: PatternNonValuePlace::Placeholder,
         });
         cc.apply_parsed_pattern(known, Pattern {

query-algebrizer/src/types.rs

@@ -15,11 +15,10 @@ use std::fmt::{
     Result,
 };
 
-use std::rc::Rc;
-
 use mentat_core::{
     Entid,
     TypedValue,
+    ValueRc,
     ValueType,
     ValueTypeSet,
 };
@@ -709,7 +708,7 @@ pub enum EvolvedValuePlace {
     Entid(Entid),
     Value(TypedValue),
     EntidOrInteger(i64),
-    IdentOrKeyword(Rc<NamespacedKeyword>),
+    IdentOrKeyword(ValueRc<NamespacedKeyword>),
 }
 
 pub enum PlaceOrEmpty<T> {

query-algebrizer/src/validate.rs

@@ -120,7 +120,7 @@ mod tests {
     };
 
     fn value_ident(ns: &str, name: &str) -> PatternValuePlace {
-        PatternValuePlace::IdentOrKeyword(::std::rc::Rc::new(NamespacedKeyword::new(ns, name)))
+        NamespacedKeyword::new(ns, name).into()
     }
 
     /// Tests that the top-level form is a valid `or`, returning the clauses.

query-parser/src/parse.rs

@@ -626,7 +626,7 @@ mod test {
     }
 
     fn ident_kw(kw: edn::NamespacedKeyword) -> PatternNonValuePlace {
-        PatternNonValuePlace::Ident(Rc::new(kw))
+        PatternNonValuePlace::Ident(kw.into())
     }
 
     fn ident(ns: &str, name: &str) -> PatternNonValuePlace {

query-parser/tests/find_tests.rs

@@ -16,8 +16,6 @@ extern crate mentat_core;
 extern crate mentat_query;
 extern crate mentat_query_parser;
 
-use std::rc::Rc;
-
 use edn::{
     NamespacedKeyword,
     PlainSymbol,
@@ -164,7 +162,7 @@ fn can_parse_simple_or_join() {
 
 #[cfg(test)]
 fn ident(ns: &str, name: &str) -> PatternNonValuePlace {
-    PatternNonValuePlace::Ident(::std::rc::Rc::new(NamespacedKeyword::new(ns, name)))
+    NamespacedKeyword::new(ns, name).into()
 }
 
 #[test]
@@ -283,7 +281,7 @@ fn can_parse_uuid() {
                WhereClause::Pattern(
                    Pattern::new(None,
                                 PatternNonValuePlace::Variable(Variable::from_valid_name("?x")),
-                                PatternNonValuePlace::Ident(Rc::new(NamespacedKeyword::new("foo", "baz"))),
+                                NamespacedKeyword::new("foo", "baz").into(),
                                 PatternValuePlace::Constant(NonIntegerConstant::Uuid(expected)),
                                 PatternNonValuePlace::Placeholder)
                        .expect("valid pattern")));

query-sql/src/lib.rs

@@ -761,7 +761,7 @@ mod tests {
         let c = Constraint::Infix {
             op: Op("MATCHES"),
             left: ColumnOrExpression::Column(QualifiedAlias("fulltext01".to_string(), Column::Fulltext(FulltextColumn::Text))),
-            right: ColumnOrExpression::Value(TypedValue::String(Rc::new("needle".to_string()))),
+            right: ColumnOrExpression::Value("needle".into()),
         };
         let q = build_query(&c);
         assert_eq!("`fulltext01`.text MATCHES $v0", q.sql);

query-translator/tests/translate.rs

@@ -739,7 +739,7 @@ fn test_ground_scalar() {
 
     // Verify that we accept bound input constants.
     let query = r#"[:find ?x . :in ?v :where [(ground ?v) ?x]]"#;
-    let inputs = QueryInputs::with_value_sequence(vec![(Variable::from_valid_name("?v"), TypedValue::String(Rc::new("aaa".into())))]);
+    let inputs = QueryInputs::with_value_sequence(vec![(Variable::from_valid_name("?v"), "aaa".into())]);
     let constant = translate_with_inputs_to_constant(&schema, query, inputs);
     assert_eq!(constant.project_without_rows().unwrap()
                        .into_scalar().unwrap(),
@@ -760,7 +760,7 @@ fn test_ground_tuple() {
     // Verify that we accept bound input constants.
     let query = r#"[:find [?x ?y] :in ?u ?v :where [(ground [?u ?v]) [?x ?y]]]"#;
     let inputs = QueryInputs::with_value_sequence(vec![(Variable::from_valid_name("?u"), TypedValue::Long(2)),
-                                                       (Variable::from_valid_name("?v"), TypedValue::String(Rc::new("aaa".into()))),]);
+                                                       (Variable::from_valid_name("?v"), "aaa".into()),]);
 
     let constant = translate_with_inputs_to_constant(&schema, query, inputs);
     assert_eq!(constant.project_without_rows().unwrap()

query/src/lib.rs

@@ -55,7 +55,9 @@ pub use edn::{
 };
 
 use mentat_core::{
+    FromRc,
     TypedValue,
+    ValueRc,
     ValueType,
 };
 
@@ -205,7 +207,7 @@ pub enum NonIntegerConstant {
     Boolean(bool),
     BigInteger(BigInt),
     Float(OrderedFloat<f64>),
-    Text(Rc<String>),
+    Text(ValueRc<String>),
     Instant(DateTime<Utc>),
     Uuid(Uuid),
 }
@@ -216,13 +218,25 @@ impl NonIntegerConstant {
             NonIntegerConstant::BigInteger(_) => unimplemented!(),     // TODO: #280.
             NonIntegerConstant::Boolean(v) => TypedValue::Boolean(v),
             NonIntegerConstant::Float(v) => TypedValue::Double(v),
-            NonIntegerConstant::Text(v) => TypedValue::String(v),
+            NonIntegerConstant::Text(v) => v.into(),
             NonIntegerConstant::Instant(v) => TypedValue::Instant(v),
             NonIntegerConstant::Uuid(v) => TypedValue::Uuid(v),
         }
     }
 }
 
+impl<'a> From<&'a str> for NonIntegerConstant {
+    fn from(val: &'a str) -> NonIntegerConstant {
+        NonIntegerConstant::Text(ValueRc::new(val.to_string()))
+    }
+}
+
+impl From<String> for NonIntegerConstant {
+    fn from(val: String) -> NonIntegerConstant {
+        NonIntegerConstant::Text(ValueRc::new(val))
+    }
+}
+
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum FnArg {
     Variable(Variable),
@@ -260,7 +274,7 @@ impl FromValue<FnArg> for FnArg {
                 Some(FnArg::Constant(NonIntegerConstant::BigInteger(x.clone()))),
             Text(ref x) =>
                 // TODO: intern strings. #398.
-                Some(FnArg::Constant(NonIntegerConstant::Text(Rc::new(x.clone())))),
+                Some(FnArg::Constant(x.clone().into())),
             Nil |
             NamespacedSymbol(_) |
             Keyword(_) |
@@ -312,7 +326,19 @@ pub enum PatternNonValuePlace {
     Placeholder,
     Variable(Variable),
     Entid(i64),                       // Will always be +ve. See #190.
-    Ident(Rc<NamespacedKeyword>),
+    Ident(ValueRc<NamespacedKeyword>),
+}
+
+impl From<Rc<NamespacedKeyword>> for PatternNonValuePlace {
+    fn from(value: Rc<NamespacedKeyword>) -> Self {
+        PatternNonValuePlace::Ident(ValueRc::from_rc(value))
+    }
+}
+
+impl From<NamespacedKeyword> for PatternNonValuePlace {
+    fn from(value: NamespacedKeyword) -> Self {
+        PatternNonValuePlace::Ident(ValueRc::new(value))
+    }
 }
 
 impl PatternNonValuePlace {
@@ -355,7 +381,7 @@ impl FromValue<PatternNonValuePlace> for PatternNonValuePlace {
                 }
             },
             edn::SpannedValue::NamespacedKeyword(ref x) =>
-                Some(PatternNonValuePlace::Ident(Rc::new(x.clone()))),
+                Some(x.clone().into()),
             _ => None,
         }
     }
@@ -363,7 +389,7 @@ impl FromValue<PatternNonValuePlace> for PatternNonValuePlace {
 
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum IdentOrEntid {
-    Ident(Rc<NamespacedKeyword>),
+    Ident(NamespacedKeyword),
     Entid(i64),
 }
 
@@ -375,10 +401,22 @@ pub enum PatternValuePlace {
     Placeholder,
     Variable(Variable),
     EntidOrInteger(i64),
-    IdentOrKeyword(Rc<NamespacedKeyword>),
+    IdentOrKeyword(ValueRc<NamespacedKeyword>),
     Constant(NonIntegerConstant),
 }
 
+impl From<Rc<NamespacedKeyword>> for PatternValuePlace {
+    fn from(value: Rc<NamespacedKeyword>) -> Self {
+        PatternValuePlace::IdentOrKeyword(ValueRc::from_rc(value))
+    }
+}
+
+impl From<NamespacedKeyword> for PatternValuePlace {
+    fn from(value: NamespacedKeyword) -> Self {
+        PatternValuePlace::IdentOrKeyword(ValueRc::new(value))
+    }
+}
+
 impl FromValue<PatternValuePlace> for PatternValuePlace {
     fn from_value(v: &edn::ValueAndSpan) -> Option<PatternValuePlace> {
         match v.inner {
@@ -389,7 +427,7 @@ impl FromValue<PatternValuePlace> for PatternValuePlace {
             edn::SpannedValue::PlainSymbol(ref x) =>
                 Variable::from_symbol(x).map(PatternValuePlace::Variable),
             edn::SpannedValue::NamespacedKeyword(ref x) =>
-                Some(PatternValuePlace::IdentOrKeyword(Rc::new(x.clone()))),
+                Some(x.clone().into()),
             edn::SpannedValue::Boolean(x) =>
                 Some(PatternValuePlace::Constant(NonIntegerConstant::Boolean(x))),
             edn::SpannedValue::Float(x) =>
@@ -400,7 +438,7 @@ impl FromValue<PatternValuePlace> for PatternValuePlace {
                 Some(PatternValuePlace::Constant(NonIntegerConstant::Instant(x))),
             edn::SpannedValue::Text(ref x) =>
                 // TODO: intern strings. #398.
-                Some(PatternValuePlace::Constant(NonIntegerConstant::Text(Rc::new(x.clone())))),
+                Some(PatternValuePlace::Constant(x.clone().into())),
             edn::SpannedValue::Uuid(ref u) =>
                 Some(PatternValuePlace::Constant(NonIntegerConstant::Uuid(u.clone()))),
 
@@ -754,7 +792,7 @@ impl Pattern {
                     return Some(Pattern {
                         source: src,
                         entity: v_e,
-                        attribute: PatternNonValuePlace::Ident(Rc::new(k.to_reversed())),
+                        attribute: k.to_reversed().into(),
                         value: e_v,
                         tx: tx,
                     });

sql/src/lib.rs

@@ -24,6 +24,7 @@ use ordered_float::OrderedFloat;
 use mentat_core::{
     ToMicros,
     TypedValue,
+    ValueRc,
 };
 
 pub use rusqlite::types::Value;
@@ -103,7 +104,7 @@ pub struct SQLiteQueryBuilder {
     // Instead we track byte and String arguments separately, mapping them to their argument name,
     // in order to dedupe. We'll add these to the regular argument vector later.
     byte_args: HashMap<Vec<u8>, String>,             // From value to argument name.
-    string_args: HashMap<Rc<String>, String>,        // From value to argument name.
+    string_args: HashMap<ValueRc<String>, String>,   // From value to argument name.
     args: Vec<(String, Rc<rusqlite::types::Value>)>, // (arg, value).
 }
 

tests/query.rs

@@ -1321,22 +1321,22 @@ fn test_aggregation_implicit_grouping() {
             assert_eq!(vals,
                 vec![
                     vec![TypedValue::Ref(ids.get("a").cloned().unwrap()),
-                         TypedValue::String("Alice".to_string().into()),
+                         "Alice".into(),
                          TypedValue::Long(99),
                          TypedValue::Long(3),
                          TypedValue::Double((127f64 / 3f64).into())],
                     vec![TypedValue::Ref(ids.get("b").cloned().unwrap()),
-                         TypedValue::String("Beli".to_string().into()),
+                         "Beli".into(),
                          TypedValue::Long(22),
                          TypedValue::Long(2),
                          TypedValue::Double((33f64 / 2f64).into())],
                     vec![TypedValue::Ref(ids.get("c").cloned().unwrap()),
-                         TypedValue::String("Carlos".to_string().into()),
+                         "Carlos".into(),
                          TypedValue::Long(42),
                          TypedValue::Long(1),
                          TypedValue::Double(42f64.into())],
                     vec![TypedValue::Ref(ids.get("d").cloned().unwrap()),
-                         TypedValue::String("Diana".to_string().into()),
+                         "Diana".into(),
                          TypedValue::Long(28),
                          TypedValue::Long(2),
                          TypedValue::Double((33f64 / 2f64).into())]].into());
@@ -1454,6 +1454,6 @@ fn test_tx_data() {
         }
     };
 
-    assert_tx_data(&store, &tx1, TypedValue::String("1".to_string().into()));
+    assert_tx_data(&store, &tx1, "1".into());
-    assert_tx_data(&store, &tx2, TypedValue::String("2".to_string().into()));
+    assert_tx_data(&store, &tx2, "2".into());
 }