Redo FromSql to make implementing it not unsafe.

Pass implementers a BorrowedValue instead of relying on them to use
the FFI interface. We take the responsibility of converting the raw
statement and column index into a BorrowedValue.

src/lib.rs

@@ -75,7 +75,7 @@ use std::result;
 use std::str;
 use libc::{c_int, c_char};
 
-use types::{ToSql, FromSql};
+use types::{ToSql, FromSql, BorrowedValue};
 use error::{error_from_sqlite_code, error_from_handle};
 use raw_statement::RawStatement;
 use cache::StatementCache;
@@ -1064,15 +1064,9 @@ impl<'a, 'stmt> Row<'a, 'stmt> {
     /// Returns an `Error::InvalidColumnName` if `idx` is not a valid column name
     /// for this row.
     pub fn get_checked<I: RowIndex, T: FromSql>(&self, idx: I) -> Result<T> {
-        unsafe {
         let idx = try!(idx.idx(self.stmt));
-
-            if T::column_has_valid_sqlite_type(self.stmt.stmt.ptr(), idx) {
-                FromSql::column_result(self.stmt.stmt.ptr(), idx)
-            } else {
-                Err(Error::InvalidColumnType)
-            }
-        }
+        let value = unsafe { BorrowedValue::new(&self.stmt.stmt, idx) };
+        FromSql::column_result(value)
     }
 
     /// Return the number of columns in the current row.
@@ -1106,6 +1100,39 @@ impl<'a> RowIndex for &'a str {
     }
 }
 
+impl<'a> BorrowedValue<'a> {
+    unsafe fn new(stmt: &RawStatement, col: c_int) -> BorrowedValue {
+        use std::slice::from_raw_parts;
+
+        let raw = stmt.ptr();
+
+        match stmt.column_type(col) {
+            ffi::SQLITE_NULL => BorrowedValue::Null,
+            ffi::SQLITE_INTEGER => BorrowedValue::Integer(ffi::sqlite3_column_int64(raw, col)),
+            ffi::SQLITE_FLOAT => BorrowedValue::Real(ffi::sqlite3_column_double(raw, col)),
+            ffi::SQLITE_TEXT => {
+                let text = ffi::sqlite3_column_text(raw, col);
+                assert!(!text.is_null(), "unexpected SQLITE_TEXT column type with NULL data");
+                let s = CStr::from_ptr(text as *const c_char);
+
+                // sqlite3_column_text returns UTF8 data, so our unwrap here should be fine.
+                let s = s.to_str().expect("sqlite3_column_text returned invalid UTF-8");
+                BorrowedValue::Text(s)
+            }
+            ffi::SQLITE_BLOB => {
+                let blob = ffi::sqlite3_column_blob(raw, col);
+                assert!(!blob.is_null(), "unexpected SQLITE_BLOB column type with NULL data");
+
+                let len = ffi::sqlite3_column_bytes(raw, col);
+                assert!(len >= 0, "unexpected negative return from sqlite3_column_bytes");
+
+                BorrowedValue::Blob(from_raw_parts(blob as *const u8, len as usize))
+            }
+            _ => unreachable!("sqlite3_column_type returned invalid value")
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     extern crate tempdir;

src/types/chrono.rs

@@ -1,11 +1,13 @@
 //! Convert most of the [Time Strings](http://sqlite.org/lang_datefunc.html) to chrono types.
 extern crate chrono;
 
+use std::borrow::Cow;
+
 use self::chrono::{NaiveDate, NaiveTime, NaiveDateTime, DateTime, TimeZone, UTC, Local};
 use libc::c_int;
 
 use {Error, Result};
-use types::{FromSql, ToSql};
+use types::{FromSql, ToSql, BorrowedValue};
 
 use ffi::sqlite3_stmt;
 
@@ -19,16 +21,11 @@ impl ToSql for NaiveDate {
 
 /// "YYYY-MM-DD" => ISO 8601 calendar date without timezone.
 impl FromSql for NaiveDate {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<NaiveDate> {
-        let s = try!(String::column_result(stmt, col));
-        match NaiveDate::parse_from_str(&s, "%Y-%m-%d") {
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_str().and_then(|s| match NaiveDate::parse_from_str(s, "%Y-%m-%d") {
             Ok(dt) => Ok(dt),
             Err(err) => Err(Error::FromSqlConversionFailure(Box::new(err))),
-        }
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        String::column_has_valid_sqlite_type(stmt, col)
+        })
     }
 }
 
@@ -42,21 +39,18 @@ impl ToSql for NaiveTime {
 
 /// "HH:MM"/"HH:MM:SS"/"HH:MM:SS.SSS" => ISO 8601 time without timezone.
 impl FromSql for NaiveTime {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<NaiveTime> {
-        let s = try!(String::column_result(stmt, col));
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_str().and_then(|s| {
             let fmt = match s.len() {
                 5 => "%H:%M",
                 8 => "%H:%M:%S",
                 _ => "%H:%M:%S%.f",
             };
-        match NaiveTime::parse_from_str(&s, fmt) {
+            match NaiveTime::parse_from_str(s, fmt) {
                 Ok(dt) => Ok(dt),
                 Err(err) => Err(Error::FromSqlConversionFailure(Box::new(err))),
             }
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        String::column_has_valid_sqlite_type(stmt, col)
+        })
     }
 }
 
@@ -71,23 +65,19 @@ impl ToSql for NaiveDateTime {
 /// "YYYY-MM-DD HH:MM:SS"/"YYYY-MM-DD HH:MM:SS.SSS" => ISO 8601 combined date and time
 /// without timezone. ("YYYY-MM-DDTHH:MM:SS"/"YYYY-MM-DDTHH:MM:SS.SSS" also supported)
 impl FromSql for NaiveDateTime {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<NaiveDateTime> {
-        let s = try!(String::column_result(stmt, col));
-
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_str().and_then(|s| {
             let fmt = if s.len() >= 11 && s.as_bytes()[10] == b'T' {
                 "%Y-%m-%dT%H:%M:%S%.f"
             } else {
                 "%Y-%m-%d %H:%M:%S%.f"
             };
 
-        match NaiveDateTime::parse_from_str(&s, fmt) {
+            match NaiveDateTime::parse_from_str(s, fmt) {
                 Ok(dt) => Ok(dt),
                 Err(err) => Err(Error::FromSqlConversionFailure(Box::new(err))),
             }
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        String::column_has_valid_sqlite_type(stmt, col)
+        })
     }
 }
 
@@ -101,38 +91,40 @@ impl<Tz: TimeZone> ToSql for DateTime<Tz> {
 
 /// RFC3339 ("YYYY-MM-DDTHH:MM:SS.SSS[+-]HH:MM") into DateTime<UTC>.
 impl FromSql for DateTime<UTC> {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<DateTime<UTC>> {
-        let s = {
-            let mut s = try!(String::column_result(stmt, col));
-            if s.len() >= 11 {
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        {
+            // Try to parse value as rfc3339 first.
+            let s = try!(value.as_str());
+
+            // If timestamp looks space-separated, make a copy and replace it with 'T'.
+            let s = if s.len() >= 11 && s.as_bytes()[10] == b' ' {
+                let mut s = s.to_string();
+                unsafe {
                     let sbytes = s.as_mut_vec();
-                if sbytes[10] == b' ' {
                     sbytes[10] = b'T';
                 }
-            }
-            s
+                Cow::Owned(s)
+            } else {
+                Cow::Borrowed(s)
             };
+
             match DateTime::parse_from_rfc3339(&s) {
-            Ok(dt) => Ok(dt.with_timezone(&UTC)),
-            Err(_) => NaiveDateTime::column_result(stmt, col).map(|dt| UTC.from_utc_datetime(&dt)),
+                Ok(dt) => return Ok(dt.with_timezone(&UTC)),
+                Err(_) => (),
             }
         }
 
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        String::column_has_valid_sqlite_type(stmt, col)
+        // Couldn't parse as rfc3339 - fall back to NaiveDateTime.
+        NaiveDateTime::column_result(value).map(|dt| UTC.from_utc_datetime(&dt))
     }
 }
 
 /// RFC3339 ("YYYY-MM-DDTHH:MM:SS.SSS[+-]HH:MM") into DateTime<Local>.
 impl FromSql for DateTime<Local> {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<DateTime<Local>> {
-        let utc_dt = try!(DateTime::<UTC>::column_result(stmt, col));
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        let utc_dt = try!(DateTime::<UTC>::column_result(value));
         Ok(utc_dt.with_timezone(&Local))
     }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        DateTime::<UTC>::column_has_valid_sqlite_type(stmt, col)
-    }
 }
 
 #[cfg(test)]

src/types/from_sql.rs

@@ -1,119 +1,60 @@
-use std::ffi::CStr;
-use std::mem;
-use std::str;
+use super::{BorrowedValue, Value};
+use ::Result;
 
-use libc::{c_char, c_double, c_int};
-
-use super::Value;
-use ffi::{sqlite3_stmt, sqlite3_column_type};
-use ::{ffi, Result};
-use ::error::Error;
-
-/// A trait for types that can be created from a SQLite value.
 pub trait FromSql: Sized {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<Self>;
+    fn column_result(value: BorrowedValue) -> Result<Self>;
+}
 
-    /// FromSql types can implement this method and use sqlite3_column_type to check that
-    /// the type reported by SQLite matches a type suitable for Self. This method is used
-    /// by `Row::get_checked` to confirm that the column contains a valid type before
-    /// attempting to retrieve the value.
-    unsafe fn column_has_valid_sqlite_type(_: *mut sqlite3_stmt, _: c_int) -> bool {
-        true
+impl FromSql for i32 {
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        i64::column_result(value).map(|i| i as i32)
     }
 }
 
-macro_rules! raw_from_impl(
-    ($t:ty, $f:ident, $c:expr) => (
-        impl FromSql for $t {
-            unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<$t> {
-                Ok(ffi::$f(stmt, col))
+impl FromSql for i64 {
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_i64()
     }
+}
 
-            unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-                sqlite3_column_type(stmt, col) == $c
+impl FromSql for f64 {
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_f64()
     }
-        }
-    )
-);
-
-raw_from_impl!(c_int, sqlite3_column_int, ffi::SQLITE_INTEGER); // i32
-raw_from_impl!(i64, sqlite3_column_int64, ffi::SQLITE_INTEGER);
-raw_from_impl!(c_double, sqlite3_column_double, ffi::SQLITE_FLOAT); // f64
+}
 
 impl FromSql for bool {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<bool> {
-        match ffi::sqlite3_column_int(stmt, col) {
-            0 => Ok(false),
-            _ => Ok(true),
-        }
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        sqlite3_column_type(stmt, col) == ffi::SQLITE_INTEGER
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        i64::column_result(value).map(|i| match i {
+            0 => false,
+            _ => true,
+        })
     }
 }
 
 impl FromSql for String {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<String> {
-        let c_text = ffi::sqlite3_column_text(stmt, col);
-        if c_text.is_null() {
-            Ok("".to_owned())
-        } else {
-            let c_slice = CStr::from_ptr(c_text as *const c_char).to_bytes();
-            let utf8_str = try!(str::from_utf8(c_slice));
-            Ok(utf8_str.into())
-        }
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        sqlite3_column_type(stmt, col) == ffi::SQLITE_TEXT
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_str().map(|s| s.to_string())
     }
 }
 
 impl FromSql for Vec<u8> {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<Vec<u8>> {
-        use std::slice::from_raw_parts;
-        let c_blob = ffi::sqlite3_column_blob(stmt, col);
-        let len = ffi::sqlite3_column_bytes(stmt, col);
-
-        // The documentation for sqlite3_column_bytes indicates it is always non-negative,
-        // but we should assert here just to be sure.
-        assert!(len >= 0,
-                "unexpected negative return from sqlite3_column_bytes");
-        let len = len as usize;
-
-        Ok(from_raw_parts(mem::transmute(c_blob), len).to_vec())
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        sqlite3_column_type(stmt, col) == ffi::SQLITE_BLOB
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_blob().map(|b| b.to_vec())
     }
 }
 
 impl<T: FromSql> FromSql for Option<T> {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<Option<T>> {
-        if sqlite3_column_type(stmt, col) == ffi::SQLITE_NULL {
-            Ok(None)
-        } else {
-            FromSql::column_result(stmt, col).map(Some)
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        match value {
+            BorrowedValue::Null => Ok(None),
+            _ => FromSql::column_result(value).map(Some),
         }
     }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        sqlite3_column_type(stmt, col) == ffi::SQLITE_NULL ||
-        T::column_has_valid_sqlite_type(stmt, col)
-    }
 }
 
 impl FromSql for Value {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<Value> {
-        match sqlite3_column_type(stmt, col) {
-            ffi::SQLITE_TEXT => FromSql::column_result(stmt, col).map(Value::Text),
-            ffi::SQLITE_INTEGER => Ok(Value::Integer(ffi::sqlite3_column_int64(stmt, col))),
-            ffi::SQLITE_FLOAT => Ok(Value::Real(ffi::sqlite3_column_double(stmt, col))),
-            ffi::SQLITE_NULL => Ok(Value::Null),
-            ffi::SQLITE_BLOB => FromSql::column_result(stmt, col).map(Value::Blob),
-            _ => Err(Error::InvalidColumnType),
-        }
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        Ok(value.to_value())
     }
 }

src/types/serde_json.rs

@@ -5,11 +5,9 @@ use libc::c_int;
 use self::serde_json::Value;
 
 use {Error, Result};
-use types::{FromSql, ToSql};
+use types::{FromSql, ToSql, BorrowedValue};
 
-use ffi;
 use ffi::sqlite3_stmt;
-use ffi::sqlite3_column_type;
 
 /// Serialize JSON `Value` to text.
 impl ToSql for Value {
@@ -21,19 +19,12 @@ impl ToSql for Value {
 
 /// Deserialize text/blob to JSON `Value`.
 impl FromSql for Value {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<Value> {
-        let value_result = match sqlite3_column_type(stmt, col) {
-            ffi::SQLITE_TEXT => {
-                let s = try!(String::column_result(stmt, col));
-                serde_json::from_str(&s)
-            }
-            ffi::SQLITE_BLOB => {
-                let blob = try!(Vec::<u8>::column_result(stmt, col));
-                serde_json::from_slice(&blob)
-            }
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        match value {
+            BorrowedValue::Text(ref s) => serde_json::from_str(s),
+            BorrowedValue::Blob(ref b) => serde_json::from_slice(b),
             _ => return Err(Error::InvalidColumnType),
-        };
-        value_result.map_err(|err| Error::FromSqlConversionFailure(Box::new(err)))
+        }.map_err(|err| Error::FromSqlConversionFailure(Box::new(err)))
     }
 }
 

src/types/time.rs

@@ -2,7 +2,7 @@ extern crate time;
 
 use libc::c_int;
 use {Error, Result};
-use types::{FromSql, ToSql};
+use types::{FromSql, ToSql, BorrowedValue};
 
 use ffi::sqlite3_stmt;
 
@@ -16,16 +16,11 @@ impl ToSql for time::Timespec {
 }
 
 impl FromSql for time::Timespec {
-    unsafe fn column_result(stmt: *mut sqlite3_stmt, col: c_int) -> Result<time::Timespec> {
-        let s = try!(String::column_result(stmt, col));
-        match time::strptime(&s, SQLITE_DATETIME_FMT) {
+    fn column_result(value: BorrowedValue) -> Result<Self> {
+        value.as_str().and_then(|s| match time::strptime(s, SQLITE_DATETIME_FMT) {
             Ok(tm) => Ok(tm.to_timespec()),
             Err(err) => Err(Error::FromSqlConversionFailure(Box::new(err))),
-        }
-    }
-
-    unsafe fn column_has_valid_sqlite_type(stmt: *mut sqlite3_stmt, col: c_int) -> bool {
-        String::column_has_valid_sqlite_type(stmt, col)
+        })
     }
 }