Replace most raw pointer usage with NonNull

This newly stabilized feature indicates that a pointer cannot be null.
This allows certain compiler optimizations, including but not limited to
the "null pointer optimization", which means that `Option<NonNull<T>>`
has the same size as `NonNull<T>`.

Using `NonNull` over raw pointers as much as possible in our code
potentially enables additional compiler optimizations, and better
expresses intent in several places.

Since this type was only stabilized in Rust 1.25, I have stuck the use
of the real thing behind the unstable flag, and provided a polyfill
by default. This keeps with our policy of "only bump the minimum Rust
version if you have a good reason". I don't think this is a good enough
reason to bump on its own. The polyfill lets us structure our code the
same regardless, and it should be trivial to switch to always using the
real thing once we no longer support Rust 1.24 and earlier.

Author: sgrif

diesel/src/mysql/connection/raw.rs

@@ -8,24 +8,24 @@ use std::sync::{Once, ONCE_INIT};
 use result::{ConnectionError, ConnectionResult, QueryResult};
 use super::url::ConnectionOptions;
 use super::stmt::Statement;
+use util::NonNull;
 
-pub struct RawConnection(*mut ffi::MYSQL);
+pub struct RawConnection(NonNull<ffi::MYSQL>);
 
 impl RawConnection {
     pub fn new() -> Self {
         perform_thread_unsafe_library_initialization();
         let raw_connection = unsafe { ffi::mysql_init(ptr::null_mut()) };
-        if raw_connection.is_null() {
-            // We're trusting https://dev.mysql.com/doc/refman/5.7/en/mysql-init.html
-            // that null return always means OOM
-            panic!("Insufficient memory to allocate connection");
-        }
+        // We're trusting https://dev.mysql.com/doc/refman/5.7/en/mysql-init.html
+        // that null return always means OOM
+        let raw_connection =
+            NonNull::new(raw_connection).expect("Insufficient memory to allocate connection");
         let result = RawConnection(raw_connection);
 
         // This is only non-zero for unrecognized options, which should never happen.
         let charset_result = unsafe {
             ffi::mysql_options(
-                result.0,
+                result.0.as_ptr(),
                 ffi::mysql_option::MYSQL_SET_CHARSET_NAME,
                 b"utf8mb4\0".as_ptr() as *const libc::c_void,
             )
@@ -50,7 +50,7 @@ impl RawConnection {
         unsafe {
             // Make sure you don't use the fake one!
             ffi::mysql_real_connect(
-                self.0,
+                self.0.as_ptr(),
                 host.map(CStr::as_ptr).unwrap_or_else(|| ptr::null_mut()),
                 user.as_ptr(),
                 password
@@ -74,7 +74,7 @@ impl RawConnection {
     }
 
     pub fn last_error_message(&self) -> String {
-        unsafe { CStr::from_ptr(ffi::mysql_error(self.0)) }
+        unsafe { CStr::from_ptr(ffi::mysql_error(self.0.as_ptr())) }
             .to_string_lossy()
             .into_owned()
     }
@@ -83,7 +83,7 @@ impl RawConnection {
         unsafe {
             // Make sure you don't use the fake one!
             ffi::mysql_real_query(
-                self.0,
+                self.0.as_ptr(),
                 query.as_ptr() as *const libc::c_char,
                 query.len() as libc::c_ulong,
             );
@@ -99,7 +99,7 @@ impl RawConnection {
     {
         unsafe {
             ffi::mysql_set_server_option(
-                self.0,
+                self.0.as_ptr(),
                 ffi::enum_mysql_set_option::MYSQL_OPTION_MULTI_STATEMENTS_ON,
             );
         }
@@ -109,7 +109,7 @@ impl RawConnection {
 
         unsafe {
             ffi::mysql_set_server_option(
-                self.0,
+                self.0.as_ptr(),
                 ffi::enum_mysql_set_option::MYSQL_OPTION_MULTI_STATEMENTS_OFF,
             );
         }
@@ -119,16 +119,16 @@ impl RawConnection {
     }
 
     pub fn affected_rows(&self) -> usize {
-        let affected_rows = unsafe { ffi::mysql_affected_rows(self.0) };
+        let affected_rows = unsafe { ffi::mysql_affected_rows(self.0.as_ptr()) };
         affected_rows as usize
     }
 
     pub fn prepare(&self, query: &str) -> QueryResult<Statement> {
-        let stmt = unsafe { ffi::mysql_stmt_init(self.0) };
+        let stmt = unsafe { ffi::mysql_stmt_init(self.0.as_ptr()) };
         // It is documented that the only reason `mysql_stmt_init` will fail
         // is because of OOM.
         // https://dev.mysql.com/doc/refman/5.7/en/mysql-stmt-init.html
-        assert!(!stmt.is_null(), "Out of memory creating prepared statement");
+        let stmt = NonNull::new(stmt).expect("Out of memory creating prepared statement");
         let stmt = Statement::new(stmt);
         try!(stmt.prepare(query));
         Ok(stmt)
@@ -163,7 +163,7 @@ impl RawConnection {
 
     fn consume_current_result(&self) -> QueryResult<()> {
         unsafe {
-            let res = ffi::mysql_store_result(self.0);
+            let res = ffi::mysql_store_result(self.0.as_ptr());
             if !res.is_null() {
                 ffi::mysql_free_result(res);
             }
@@ -174,7 +174,7 @@ impl RawConnection {
     /// Calls `mysql_next_result` and returns whether there are more results
     /// after this one.
     fn next_result(&self) -> QueryResult<bool> {
-        let more_results = unsafe { ffi::mysql_next_result(self.0) == 0 };
+        let more_results = unsafe { ffi::mysql_next_result(self.0.as_ptr()) == 0 };
         self.did_an_error_occur()?;
         Ok(more_results)
     }
@@ -183,7 +183,7 @@ impl RawConnection {
 impl Drop for RawConnection {
     fn drop(&mut self) {
         unsafe {
-            ffi::mysql_close(self.0);
+            ffi::mysql_close(self.0.as_ptr());
         }
     }
 }

diesel/src/mysql/connection/stmt/iterator.rs

@@ -131,7 +131,7 @@ fn execute_statement(stmt: &mut Statement, binds: &mut Binds) -> QueryResult<()>
 }
 
 fn populate_row_buffers(stmt: &Statement, binds: &mut Binds) -> QueryResult<Option<()>> {
-    let next_row_result = unsafe { ffi::mysql_stmt_fetch(stmt.stmt) };
+    let next_row_result = unsafe { ffi::mysql_stmt_fetch(stmt.stmt.as_ptr()) };
     match next_row_result as libc::c_uint {
         ffi::MYSQL_NO_DATA => Ok(None),
         ffi::MYSQL_DATA_TRUNCATED => binds.populate_dynamic_buffers(stmt).map(Some),

diesel/src/mysql/connection/stmt/mod.rs

@@ -11,14 +11,15 @@ use result::{DatabaseErrorKind, QueryResult};
 use self::iterator::*;
 use self::metadata::*;
 use super::bind::Binds;
+use util::NonNull;
 
 pub struct Statement {
-    stmt: *mut ffi::MYSQL_STMT,
+    stmt: NonNull<ffi::MYSQL_STMT>,
     input_binds: Option<Binds>,
 }
 
 impl Statement {
-    pub fn new(stmt: *mut ffi::MYSQL_STMT) -> Self {
+    pub(crate) fn new(stmt: NonNull<ffi::MYSQL_STMT>) -> Self {
         Statement {
             stmt: stmt,
             input_binds: None,
@@ -28,7 +29,7 @@ impl Statement {
     pub fn prepare(&self, query: &str) -> QueryResult<()> {
         unsafe {
             ffi::mysql_stmt_prepare(
-                self.stmt,
+                self.stmt.as_ptr(),
                 query.as_ptr() as *const libc::c_char,
                 query.len() as libc::c_ulong,
             );
@@ -45,7 +46,7 @@ impl Statement {
             // This relies on the invariant that the current value of `self.input_binds`
             // will not change without this function being called
             unsafe {
-                ffi::mysql_stmt_bind_param(self.stmt, bind_ptr);
+                ffi::mysql_stmt_bind_param(self.stmt.as_ptr(), bind_ptr);
             }
         });
         self.input_binds = Some(input_binds);
@@ -56,15 +57,15 @@ impl Statement {
     /// have no return value. It should never be called on a statement on
     /// which `results` has previously been called?
     pub unsafe fn execute(&self) -> QueryResult<()> {
-        ffi::mysql_stmt_execute(self.stmt);
+        ffi::mysql_stmt_execute(self.stmt.as_ptr());
         self.did_an_error_occur()?;
-        ffi::mysql_stmt_store_result(self.stmt);
+        ffi::mysql_stmt_store_result(self.stmt.as_ptr());
         self.did_an_error_occur()?;
         Ok(())
     }
 
     pub fn affected_rows(&self) -> usize {
-        let affected_rows = unsafe { ffi::mysql_stmt_affected_rows(self.stmt) };
+        let affected_rows = unsafe { ffi::mysql_stmt_affected_rows(self.stmt.as_ptr()) };
         affected_rows as usize
     }
 
@@ -83,15 +84,15 @@ impl Statement {
     }
 
     fn last_error_message(&self) -> String {
-        unsafe { CStr::from_ptr(ffi::mysql_stmt_error(self.stmt)) }
+        unsafe { CStr::from_ptr(ffi::mysql_stmt_error(self.stmt.as_ptr())) }
             .to_string_lossy()
             .into_owned()
     }
 
     /// If the pointers referenced by the `MYSQL_BIND` structures are invalidated,
     /// you must call this function again before calling `mysql_stmt_fetch`.
     pub unsafe fn bind_result(&self, binds: *mut ffi::MYSQL_BIND) -> QueryResult<()> {
-        ffi::mysql_stmt_bind_result(self.stmt, binds);
+        ffi::mysql_stmt_bind_result(self.stmt.as_ptr(), binds);
         self.did_an_error_occur()
     }
 
@@ -102,7 +103,7 @@ impl Statement {
         offset: usize,
     ) -> QueryResult<()> {
         ffi::mysql_stmt_fetch_column(
-            self.stmt,
+            self.stmt.as_ptr(),
             bind,
             idx as libc::c_uint,
             offset as libc::c_ulong,
@@ -113,7 +114,7 @@ impl Statement {
     fn metadata(&self) -> QueryResult<StatementMetadata> {
         use result::Error::DeserializationError;
 
-        let result_ptr = unsafe { ffi::mysql_stmt_result_metadata(self.stmt).as_mut() };
+        let result_ptr = unsafe { ffi::mysql_stmt_result_metadata(self.stmt.as_ptr()).as_mut() };
         self.did_an_error_occur()?;
         result_ptr
             .map(StatementMetadata::new)
@@ -135,7 +136,7 @@ impl Statement {
     }
 
     fn last_error_type(&self) -> DatabaseErrorKind {
-        let last_error_number = unsafe { ffi::mysql_stmt_errno(self.stmt) };
+        let last_error_number = unsafe { ffi::mysql_stmt_errno(self.stmt.as_ptr()) };
         // These values are not exposed by the C API, but are documented
         // at https://dev.mysql.com/doc/refman/5.7/en/error-messages-server.html
         // and are from the ANSI SQLSTATE standard
@@ -149,6 +150,6 @@ impl Statement {
 
 impl Drop for Statement {
     fn drop(&mut self) {
-        unsafe { ffi::mysql_stmt_close(self.stmt) };
+        unsafe { ffi::mysql_stmt_close(self.stmt.as_ptr()) };
     }
 }

diesel/src/pg/connection/raw.rs

@@ -8,10 +8,11 @@ use std::os::raw as libc;
 use std::{ptr, str};
 
 use result::*;
+use util::NonNull;
 
 #[allow(missing_debug_implementations, missing_copy_implementations)]
 pub struct RawConnection {
-    internal_connection: *mut PGconn,
+    internal_connection: NonNull<PGconn>,
 }
 
 impl RawConnection {
@@ -23,9 +24,12 @@ impl RawConnection {
         let connection_status = unsafe { PQstatus(connection_ptr) };
 
         match connection_status {
-            CONNECTION_OK => Ok(RawConnection {
-                internal_connection: connection_ptr,
-            }),
+            CONNECTION_OK => {
+                let connection_ptr = unsafe { NonNull::new_unchecked(connection_ptr) };
+                Ok(RawConnection {
+                    internal_connection: connection_ptr,
+                })
+            }
             _ => {
                 let message = last_error_message(connection_ptr);
                 Err(ConnectionError::BadConnection(message))
@@ -34,21 +38,21 @@ impl RawConnection {
     }
 
     pub fn last_error_message(&self) -> String {
-        last_error_message(self.internal_connection)
+        last_error_message(self.internal_connection.as_ptr())
     }
 
     pub fn set_notice_processor(&self, notice_processor: NoticeProcessor) {
         unsafe {
             PQsetNoticeProcessor(
-                self.internal_connection,
+                self.internal_connection.as_ptr(),
                 Some(notice_processor),
                 ptr::null_mut(),
             );
         }
     }
 
     pub unsafe fn exec(&self, query: *const libc::c_char) -> QueryResult<RawResult> {
-        RawResult::new(PQexec(self.internal_connection, query), self)
+        RawResult::new(PQexec(self.internal_connection.as_ptr(), query), self)
     }
 
     pub unsafe fn exec_prepared(
@@ -61,7 +65,7 @@ impl RawConnection {
         result_format: libc::c_int,
     ) -> QueryResult<RawResult> {
         let ptr = PQexecPrepared(
-            self.internal_connection,
+            self.internal_connection.as_ptr(),
             stmt_name,
             param_count,
             param_values,
@@ -80,7 +84,7 @@ impl RawConnection {
         param_types: *const Oid,
     ) -> QueryResult<RawResult> {
         let ptr = PQprepare(
-            self.internal_connection,
+            self.internal_connection.as_ptr(),
             stmt_name,
             query,
             param_count,
@@ -94,7 +98,7 @@ pub type NoticeProcessor = extern "C" fn(arg: *mut libc::c_void, message: *const
 
 impl Drop for RawConnection {
     fn drop(&mut self) {
-        unsafe { PQfinish(self.internal_connection) };
+        unsafe { PQfinish(self.internal_connection.as_ptr()) };
     }
 }
 
@@ -112,36 +116,34 @@ fn last_error_message(conn: *const PGconn) -> String {
 ///
 /// If `Unique` is ever stabilized, we should use it here.
 #[allow(missing_debug_implementations)]
-pub struct RawResult(*mut PGresult);
+pub struct RawResult(NonNull<PGresult>);
 
 unsafe impl Send for RawResult {}
 unsafe impl Sync for RawResult {}
 
 impl RawResult {
     fn new(ptr: *mut PGresult, conn: &RawConnection) -> QueryResult<Self> {
-        if ptr.is_null() {
-            Err(Error::DatabaseError(
+        NonNull::new(ptr).map(RawResult).ok_or_else(|| {
+            Error::DatabaseError(
                 DatabaseErrorKind::UnableToSendCommand,
                 Box::new(conn.last_error_message()),
-            ))
-        } else {
-            Ok(RawResult(ptr))
-        }
+            )
+        })
     }
 
     pub fn as_ptr(&self) -> *mut PGresult {
-        self.0
+        self.0.as_ptr()
     }
 
     pub fn error_message(&self) -> &str {
-        let ptr = unsafe { PQresultErrorMessage(self.0) };
+        let ptr = unsafe { PQresultErrorMessage(self.0.as_ptr()) };
         let cstr = unsafe { CStr::from_ptr(ptr) };
         cstr.to_str().unwrap_or_default()
     }
 }
 
 impl Drop for RawResult {
     fn drop(&mut self) {
-        unsafe { PQclear(self.0) }
+        unsafe { PQclear(self.0.as_ptr()) }
     }
 }