Tigthen unsafe in FFI. (#458)

arrow-pyarrow-integration-testing/src/lib.rs

@@ -65,8 +65,8 @@ fn to_rust_array(ob: PyObject, py: Python) -> PyResult<Arc<dyn Array>> {
         (array_ptr as Py_uintptr_t, schema_ptr as Py_uintptr_t),
     )?;
 
-    let field = ffi::import_field_from_c(schema.as_ref()).map_err(PyO3ArrowError::from)?;
-    let array = ffi::import_array_from_c(array, &field).map_err(PyO3ArrowError::from)?;
+    let field = unsafe { ffi::import_field_from_c(schema.as_ref()).map_err(PyO3ArrowError::from)? };
+    let array = unsafe { ffi::import_array_from_c(array, &field).map_err(PyO3ArrowError::from)? };
 
     Ok(array.into())
 }
@@ -108,7 +108,7 @@ fn to_rust_field(ob: PyObject, py: Python) -> PyResult<Field> {
     // this changes the pointer's memory and is thus unsafe. In particular, `_export_to_c` can go out of bounds
     ob.call_method1(py, "_export_to_c", (schema_ptr as Py_uintptr_t,))?;
 
-    let field = ffi::import_field_from_c(schema.as_ref()).map_err(PyO3ArrowError::from)?;
+    let field = unsafe { ffi::import_field_from_c(schema.as_ref()).map_err(PyO3ArrowError::from)? };
 
     Ok(field)
 }

examples/ffi.rs

@@ -14,7 +14,7 @@ unsafe fn export(
     ffi::export_field_to_c(&field, schema_ptr);
 }
 
-fn import(
+unsafe fn import(
     array: Box<ffi::Ffi_ArrowArray>,
     schema: &ffi::Ffi_ArrowSchema,
 ) -> Result<Box<dyn Array>> {
@@ -47,7 +47,8 @@ fn main() -> Result<()> {
     let schema_ptr = unsafe { Box::from_raw(schema_ptr) };
 
     // and finally interpret the written memory into a new array.
-    let new_array = import(array_ptr, schema_ptr.as_ref())?;
+    // Safety: we used `export`, which is a valid exporter to the C data interface
+    let new_array = unsafe { import(array_ptr, schema_ptr.as_ref())? };
 
     // which is equal to the exported array
     assert_eq!(array.as_ref(), new_array.as_ref());

src/array/binary/ffi.rs

@@ -23,8 +23,8 @@ unsafe impl<O: Offset> ToFfi for BinaryArray<O> {
     }
 }
 
-unsafe impl<O: Offset, A: ffi::ArrowArrayRef> FromFfi<A> for BinaryArray<O> {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<O: Offset, A: ffi::ArrowArrayRef> FromFfi<A> for BinaryArray<O> {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let data_type = array.field().data_type().clone();
         let expected = if O::is_large() {
             DataType::LargeBinary

src/array/boolean/ffi.rs

@@ -21,8 +21,8 @@ unsafe impl ToFfi for BooleanArray {
     }
 }
 
-unsafe impl<A: ffi::ArrowArrayRef> FromFfi<A> for BooleanArray {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<A: ffi::ArrowArrayRef> FromFfi<A> for BooleanArray {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let data_type = array.field().data_type().clone();
         assert_eq!(data_type, DataType::Boolean);
         let length = array.array().len();

src/array/dictionary/ffi.rs

@@ -17,8 +17,8 @@ unsafe impl<K: DictionaryKey> ToFfi for DictionaryArray<K> {
     }
 }
 
-unsafe impl<K: DictionaryKey, A: ffi::ArrowArrayRef> FromFfi<A> for DictionaryArray<K> {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<K: DictionaryKey, A: ffi::ArrowArrayRef> FromFfi<A> for DictionaryArray<K> {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         // keys: similar to PrimitiveArray, but the datatype is the inner one
         let length = array.array().len();
         let offset = array.array().offset();

src/array/ffi.rs

@@ -22,9 +22,12 @@ pub unsafe trait ToFfi {
 
 /// Trait describing how a struct imports into itself from the
 /// [C data interface](https://arrow.apache.org/docs/format/CDataInterface.html) (FFI).
-pub unsafe trait FromFfi<T: ffi::ArrowArrayRef>: Sized {
+pub trait FromFfi<T: ffi::ArrowArrayRef>: Sized {
     /// Convert itself from FFI.
-    fn try_from_ffi(array: T) -> Result<Self>;
+    /// # Safety
+    /// This function is intrinsically `unsafe` as it requires the FFI to be made according
+    /// to the [C data interface](https://arrow.apache.org/docs/format/CDataInterface.html)
+    unsafe fn try_from_ffi(array: T) -> Result<Self>;
 }
 
 macro_rules! ffi_dyn {

src/array/list/ffi.rs

@@ -22,8 +22,8 @@ unsafe impl<O: Offset> ToFfi for ListArray<O> {
     }
 }
 
-unsafe impl<O: Offset, A: ffi::ArrowArrayRef> FromFfi<A> for ListArray<O> {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<O: Offset, A: ffi::ArrowArrayRef> FromFfi<A> for ListArray<O> {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let data_type = array.field().data_type().clone();
         let length = array.array().len();
         let offset = array.array().offset();

src/array/primitive/ffi.rs

@@ -22,8 +22,8 @@ unsafe impl<T: NativeType> ToFfi for PrimitiveArray<T> {
     }
 }
 
-unsafe impl<T: NativeType, A: ffi::ArrowArrayRef> FromFfi<A> for PrimitiveArray<T> {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<T: NativeType, A: ffi::ArrowArrayRef> FromFfi<A> for PrimitiveArray<T> {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let data_type = array.field().data_type().clone();
         let length = array.array().len();
         let offset = array.array().offset();

src/array/struct_.rs

@@ -233,8 +233,8 @@ unsafe impl ToFfi for StructArray {
     }
 }
 
-unsafe impl<A: ffi::ArrowArrayRef> FromFfi<A> for StructArray {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<A: ffi::ArrowArrayRef> FromFfi<A> for StructArray {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let field = array.field();
         let fields = Self::get_fields(field.data_type()).to_vec();
 

src/array/union/ffi.rs

@@ -27,8 +27,8 @@ unsafe impl ToFfi for UnionArray {
     }
 }
 
-unsafe impl<A: ffi::ArrowArrayRef> FromFfi<A> for UnionArray {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<A: ffi::ArrowArrayRef> FromFfi<A> for UnionArray {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let field = array.field();
         let data_type = field.data_type().clone();
         let fields = Self::get_fields(field.data_type());

src/array/utf8/ffi.rs

@@ -20,8 +20,8 @@ unsafe impl<O: Offset> ToFfi for Utf8Array<O> {
     }
 }
 
-unsafe impl<O: Offset, A: ffi::ArrowArrayRef> FromFfi<A> for Utf8Array<O> {
-    fn try_from_ffi(array: A) -> Result<Self> {
+impl<O: Offset, A: ffi::ArrowArrayRef> FromFfi<A> for Utf8Array<O> {
+    unsafe fn try_from_ffi(array: A) -> Result<Self> {
         let length = array.array().len();
         let offset = array.array().offset();
         let mut validity = unsafe { array.validity() }?;

src/ffi/array.rs

@@ -1,20 +1,3 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements.  See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership.  The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License.  You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied.  See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
 //! Contains functionality to load an ArrayData from the C Data Interface
 
 use super::ffi::ArrowArrayRef;
@@ -27,7 +10,7 @@ use crate::{array::*, datatypes::PhysicalType};
 /// If and only if:
 /// * the data type is not supported
 /// * the interface is not valid (e.g. a null pointer)
-pub fn try_from<A: ArrowArrayRef>(array: A) -> Result<Box<dyn Array>> {
+pub unsafe fn try_from<A: ArrowArrayRef>(array: A) -> Result<Box<dyn Array>> {
     use PhysicalType::*;
     Ok(match array.field().data_type().to_physical_type() {
         Boolean => Box::new(BooleanArray::try_from_ffi(array)?),

src/ffi/mod.rs

@@ -34,11 +34,20 @@ pub unsafe fn export_field_to_c(field: &Field, ptr: *mut Ffi_ArrowSchema) {
 }
 
 /// Imports a [`Field`] from the C data interface.
-pub fn import_field_from_c(field: &Ffi_ArrowSchema) -> Result<Field> {
+/// # Safety
+/// This function is intrinsically `unsafe` and relies on a [`Ffi_ArrowSchema`]
+/// valid according to the [C data interface](https://arrow.apache.org/docs/format/CDataInterface.html) (FFI).
+pub unsafe fn import_field_from_c(field: &Ffi_ArrowSchema) -> Result<Field> {
     to_field(field)
 }
 
-/// Imports a [`Field`] from the C data interface.
-pub fn import_array_from_c(array: Box<Ffi_ArrowArray>, field: &Field) -> Result<Box<dyn Array>> {
+/// Imports an [`Array`] from the C data interface.
+/// # Safety
+/// This function is intrinsically `unsafe` and relies on a [`Ffi_ArrowArray`]
+/// valid according to the [C data interface](https://arrow.apache.org/docs/format/CDataInterface.html) (FFI).
+pub unsafe fn import_array_from_c(
+    array: Box<Ffi_ArrowArray>,
+    field: &Field,
+) -> Result<Box<dyn Array>> {
     try_from(Arc::new(ArrowArray::new(array, field.clone())))
 }

src/ffi/schema.rs

@@ -199,7 +199,7 @@ impl Drop for Ffi_ArrowSchema {
     }
 }
 
-pub fn to_field(schema: &Ffi_ArrowSchema) -> Result<Field> {
+pub(crate) unsafe fn to_field(schema: &Ffi_ArrowSchema) -> Result<Field> {
     let dictionary = schema.dictionary();
     let data_type = if let Some(dictionary) = dictionary {
         let indices_data_type = to_data_type(schema)?;
@@ -224,7 +224,7 @@ pub fn to_field(schema: &Ffi_ArrowSchema) -> Result<Field> {
     Ok(field)
 }
 
-fn to_data_type(schema: &Ffi_ArrowSchema) -> Result<DataType> {
+unsafe fn to_data_type(schema: &Ffi_ArrowSchema) -> Result<DataType> {
     Ok(match schema.format() {
         "n" => DataType::Null,
         "b" => DataType::Boolean,

src/lib.rs

@@ -1,5 +1,7 @@
 //! Doc provided by README
 
+// So that we have more control over what is `unsafe` inside an `unsafe` block
+#![allow(unused_unsafe)]
 #![cfg_attr(docsrs, feature(doc_cfg))]
 
 #[macro_use]

tests/it/ffi.rs

@@ -24,8 +24,8 @@ fn test_round_trip(expected: impl Array + Clone + 'static) -> Result<()> {
     let schema_ptr = unsafe { Box::from_raw(schema_ptr) };
 
     // import references
-    let result_field = ffi::import_field_from_c(schema_ptr.as_ref())?;
-    let result_array = ffi::import_array_from_c(array_ptr, &result_field)?;
+    let result_field = unsafe { ffi::import_field_from_c(schema_ptr.as_ref())? };
+    let result_array = unsafe { ffi::import_array_from_c(array_ptr, &result_field)? };
 
     assert_eq!(&result_array, &expected);
     assert_eq!(result_field, field);
@@ -42,7 +42,7 @@ fn test_round_trip_schema(field: Field) -> Result<()> {
 
     let schema_ptr = unsafe { Box::from_raw(schema_ptr) };
 
-    let result = ffi::import_field_from_c(schema_ptr.as_ref())?;
+    let result = unsafe { ffi::import_field_from_c(schema_ptr.as_ref())? };
 
     assert_eq!(result, field);
     Ok(())