Add compilation failure tests for Handle (#220)

The FFI `Handle` class relies on several invariants that should cause
compilation failures when somebody attempts to misuse a handle. This PR
adds such negative test coverage with help from the `trybuild` crate. To
run those tests, do:
```
$ cargo test --features developer-visibility --package delta_kernel_ffi -- invalid_handle_code
```
The newly added `developer-visibility` feature is similar to the one
already present in the kernel crate, and makes additional classes and
modules public. This is important for testing, because doc tests and
compilation failure tests are not "inside" the crate and thus cannot
otherwise access `pub(crate) mod handle`. The same mechanism also allows
generating internal docs that include classes relevant to those building
or extending the kernel FFI.

ffi-proc-macros/src/lib.rs

@@ -80,10 +80,22 @@ fn bool_to_boolean(b: bool) -> TokenStream2 {
     } else {
         quote! { False }
     };
-    quote! { crate::handle::#name }
+    quote! { delta_kernel_ffi::handle::#name }
 }
 
 /// Macro for conveniently deriving a `delta_kernel_ffi::handle::HandleDescriptor`.
+///
+/// When targeting a struct, it is invoked with three arguments:
+/// ```ignore
+/// #[handle_descriptor(target = Foo, mutable = false. sized = true)]
+/// pub struct SharedFoo;
+/// ```
+///
+/// When targeting a trait, two arguments suffice (`sized = false` is implied):
+/// ```ignore
+/// #[handle_descriptor(target = dyn Bar, mutable = true)]
+/// pub struct MutableBar;
+/// ```
 #[proc_macro_attribute]
 pub fn handle_descriptor(attr: TokenStream, item: TokenStream) -> TokenStream {
     let descriptor_params = parse_macro_input!(attr as HandleDescriptorParams);
@@ -105,7 +117,7 @@ pub fn handle_descriptor(attr: TokenStream, item: TokenStream) -> TokenStream {
 
     // Inject a single unconstructible member to produce a NZST
     let ident = &st.ident;
-    let new_struct = quote! { struct #ident(crate::handle::Unconstructable); };
+    let new_struct = quote! { struct #ident(delta_kernel_ffi::handle::Unconstructable); };
     let new_struct = new_struct.into();
     let new_struct = parse_macro_input!(new_struct as ItemStruct);
     st.fields = new_struct.fields;
@@ -116,7 +128,7 @@ pub fn handle_descriptor(attr: TokenStream, item: TokenStream) -> TokenStream {
     let target = &descriptor_params.target;
     let descriptor_impl = quote! {
         #[automatically_derived]
-        impl crate::handle::HandleDescriptor for #ident {
+        impl delta_kernel_ffi::handle::HandleDescriptor for #ident {
             type Target = #target;
             type Mutable = #mutable;
             type Sized = #sized;

ffi/Cargo.toml

@@ -10,7 +10,7 @@ version.workspace = true
 build = "build.rs"
 
 [lib]
-crate-type = ["cdylib", "staticlib"]
+crate-type = ["lib", "cdylib", "staticlib"]
 
 [dependencies]
 tracing = "0.1"
@@ -35,6 +35,7 @@ libc = "0.2.147"
 
 [dev-dependencies]
 rand = "0.8.5"
+trybuild = "1.0"
 
 [features]
 default = ["default-engine"]
@@ -45,3 +46,4 @@ default-engine = [
   "arrow-schema",
 ]
 sync-engine = ["delta_kernel/sync-engine"]
+developer-visibility = []

ffi/src/handle.rs

@@ -3,7 +3,7 @@
 //!
 //! Creating a [`Handle<T>`] always implies some kind of ownership transfer. A mutable handle takes
 //! ownership of the object itself (analagous to [`Box<T>`]), while a non-mutable (shared) handle
-//! takes ownership of a shared reference to the object (analagous to [`Arc<T>`]). Thus, a created
+//! takes ownership of a shared reference to the object (analagous to [`std::sync::Arc<T>`]). Thus, a created
 //! handle remains [valid][Handle#Validity], and its underlying object remains accessible, until the
 //! handle is explicitly dropped or consumed. Dropping a mutable handle always drops the underlying
 //! object as well; dropping a shared handle only drops the underlying object if the handle was the
@@ -18,20 +18,33 @@
 //! advised to maintain "unique pointer" semantics for mutable handles.
 //!
 //! NOTE: While shared handles could conceptually impl [`Clone`], cloning would require unsafe code
-//! and so we can't actually implement the trait. Use [`CloneHandle::clone_handle`] instead.
+//! and so we can't actually implement the trait. Use [`Handle::clone_handle`] instead.
 
 /// Describes the kind of handle a given opaque pointer type represents.
 ///
 /// It is not normally necessary to implement this trait directly; instead, use the provided
-/// attribute macro `#[handle_descriptor]` to generate the implementation automatically, e.g.
+/// attribute macro [`#[handle_descriptor]`][delta_kernel_ffi_macros::handle_descriptor] to generate
+/// the implementation automatically, e.g.
 /// ```
+/// # use delta_kernel_ffi_macros::handle_descriptor;
+/// # use delta_kernel_ffi::handle::Handle;
 /// pub struct Foo {
 ///     x: usize,
 /// }
 ///
-/// #[handle_descriptor(target = "Foo", mutable = true, sized = true)]
+/// #[handle_descriptor(target = Foo, mutable = true, sized = true)]
 /// pub struct MutableFoo;
+///
+/// pub trait Bar: Send {}
+///
+/// #[handle_descriptor(target = dyn Bar, mutable = false)]
+/// pub struct SharedBar;
+///
 /// ```
+///
+/// NOTE: For obscure rust generic type constraint reasons, a `HandleDescriptor` must specifically
+/// state whether the target type is [`Sized`]. However, two arguments suffice for trait targets,
+/// because `sized=true` is implied.
 pub trait HandleDescriptor {
     /// The true type of the handle's underlying object
     type Target: ?Sized + Send;
@@ -53,7 +66,17 @@ mod private {
     use super::*;
 
     /// Represents an object that crosses the FFI boundary and which outlives the scope that created
-    /// it. It can be passed freely between rust code and external code.
+    /// it. It can be passed freely between rust code and external code. The
+    ///
+    /// An accompanying [`HandleDescriptor`] trait defines the behavior of each handle type:
+    ///
+    /// * The true underlying ("target") type the handle represents. For safety reasons, target type
+    /// must always be [`Send`].
+    ///
+    /// * Mutable (`Box`-like) vs. shared (`Arc`-like). For safety reasons, the target type of a
+    /// shared handle must always be [`Send`]+[`Sync`].
+    ///
+    /// * Sized vs. unsized. Sized types allow handle operations to be implemented more efficiently.
     ///
     /// # Validity
     ///
@@ -512,12 +535,18 @@ mod tests {
     }
     unsafe impl Send for NotSync {}
 
-    #[handle_descriptor(target=NotSync, mutable=false, sized=true)]
-    pub struct SharedNotSync;
-
     #[handle_descriptor(target=NotSync, mutable=true, sized=true)]
     pub struct MutNotSync;
 
+    // Because tests compile as binaries against packages, this test can only run correctly if we
+    // use developer-visibility to make mod handle public. Otherwise it's inaccessible for testing.
+    #[test]
+    #[cfg(feature = "developer-visibility")]
+    fn invalid_handle_code() {
+        let t = trybuild::TestCases::new();
+        t.compile_fail("tests/invalid-handle-code/*.rs");
+    }
+
     #[test]
     fn test_handle_use_cases_compile() {
         let s = NotSync {
@@ -526,11 +555,6 @@ mod tests {
         let h: Handle<MutNotSync> = Box::new(s).into();
         unsafe { h.drop_handle() };
 
-        // error[E0277]: `*mut u32` cannot be shared between threads safely
-        // let s = NotSync { ptr: std::ptr::null_mut() };
-        // let h: Handle<SharedNotSync> = Arc::new(s).into();
-        // unsafe { h.drop_handle() };
-
         let f = Foo {
             x: rand::random::<usize>(),
             y: rand::random::<usize>().to_string(),
@@ -540,18 +564,8 @@ mod tests {
         let r = unsafe { h.as_mut() };
         assert_eq!(format!("{r:?}"), s);
 
-        // error[E0599]: the method `clone_as_arc` exists for struct `Handle<FooHandle>`,
-        //               but its trait bounds were not satisfied
-        //let _ = unsafe { h.clone_as_arc() };
-
         unsafe { h.drop_handle() };
 
-        // error[E0382]: borrow of moved value: `h`
-        // let _ = unsafe { h.as_mut() };
-
-        // error[E0451]: field `ptr` of struct `Handle` is private
-        // let h = Handle::<FooHandle>{ ptr: std::ptr::null() };
-
         let b = Bar {
             x: rand::random::<usize>(),
             y: rand::random::<usize>().to_string(),
@@ -561,17 +575,10 @@ mod tests {
         let r = unsafe { h.as_ref() };
         assert_eq!(format!("{r:?}"), s);
 
-        // error[E0599]: the method `as_mut` exists for struct `Handle<BarHandle>`,
-        //               but its trait bounds were not satisfied
-        // let r = unsafe { h.as_mut() };
-
         let r = unsafe { h.clone_as_arc() };
         assert_eq!(format!("{r:?}"), s);
         unsafe { h.drop_handle() };
 
-        // error[E0382]: borrow of moved value: `h`
-        // let _ = unsafe { h.as_ref() };
-
         let b = Bar {
             x: rand::random::<usize>(),
             y: rand::random::<usize>().to_string(),
@@ -605,10 +612,6 @@ mod tests {
 
         unsafe { h.drop_handle() };
 
-        // error[E0599]: the method `clone_as_arc` exists for struct `Handle<FooHandle>`,
-        //               but its trait bounds were not satisfied
-        //let _ = unsafe { h.clone_as_arc() };
-
         let r = unsafe { h2.as_ref() };
         assert_eq!(r.squawk(), s2);
         unsafe { h2.drop_handle() };

ffi/src/lib.rs

@@ -16,9 +16,21 @@ use delta_kernel::snapshot::Snapshot;
 use delta_kernel::{DeltaResult, Engine, Error, Table};
 use delta_kernel_ffi_macros::handle_descriptor;
 
+// cbindgen doesn't understand our use of feature flags here, and by default it parses `mod handle`
+// twice. So we tell it to ignore one of the declarations to avoid double-definition errors.
+/// cbindgen:ignore
+#[cfg(feature = "developer-visibility")]
+pub mod handle;
+#[cfg(not(feature = "developer-visibility"))]
 pub(crate) mod handle;
+
 use handle::Handle;
 
+// The handle_descriptor macro needs this, because it needs to emit fully qualified type names. THe
+// actual prod code could use `crate::`, but doc tests can't because they're not "inside" the crate.
+// relies on `crate::`
+extern crate self as delta_kernel_ffi;
+
 pub mod scan;
 
 pub(crate) type NullableCvoid = Option<NonNull<c_void>>;
@@ -66,8 +78,9 @@ impl Iterator for EngineIterator {
 /// references to the slice or its data that could outlive the function call.
 ///
 /// ```
-/// fn wants_slice(slice: KernelStringSlice) { ... }
-/// let msg = String::from(...);
+/// # use delta_kernel_ffi::KernelStringSlice;
+/// fn wants_slice(slice: KernelStringSlice) { }
+/// let msg = String::from("hello");
 /// wants_slice(msg.into());
 /// ```
 #[repr(C)]

ffi/tests/invalid-handle-code/double-mut-reference.rs

@@ -0,0 +1,16 @@
+use delta_kernel_ffi_macros::handle_descriptor;
+use delta_kernel_ffi::handle::Handle;
+
+pub struct Foo(u32);
+
+#[handle_descriptor(target=Foo, mutable=true, sized=true)]
+pub struct MutFoo;
+
+fn main() {
+    let s = Foo(0);
+    let mut h: Handle<MutFoo> = Box::new(s).into();
+    let r = unsafe { h.as_mut() };
+    let _ = unsafe { h.as_mut() };
+    let _ = unsafe { h.as_ref() };
+    r.0 = 1;
+}

ffi/tests/invalid-handle-code/double-mut-reference.stderr

@@ -0,0 +1,21 @@
+error[E0499]: cannot borrow `h` as mutable more than once at a time
+  --> tests/invalid-handle-code/double-mut-reference.rs:13:22
+   |
+12 |     let r = unsafe { h.as_mut() };
+   |                      - first mutable borrow occurs here
+13 |     let _ = unsafe { h.as_mut() };
+   |                      ^ second mutable borrow occurs here
+14 |     let _ = unsafe { h.as_ref() };
+15 |     r.0 = 1;
+   |     ------- first borrow later used here
+
+error[E0502]: cannot borrow `h` as immutable because it is also borrowed as mutable
+  --> tests/invalid-handle-code/double-mut-reference.rs:14:22
+   |
+12 |     let r = unsafe { h.as_mut() };
+   |                      - mutable borrow occurs here
+13 |     let _ = unsafe { h.as_mut() };
+14 |     let _ = unsafe { h.as_ref() };
+   |                      ^ immutable borrow occurs here
+15 |     r.0 = 1;
+   |     ------- mutable borrow later used here

ffi/tests/invalid-handle-code/moved-from-handle.rs

@@ -0,0 +1,15 @@
+use delta_kernel_ffi_macros::handle_descriptor;
+use delta_kernel_ffi::handle::Handle;
+
+pub struct Foo(u32);
+
+#[handle_descriptor(target=Foo, mutable=true, sized=true)]
+pub struct MutFoo;
+
+fn main() {
+    let s = Foo(0);
+    let mut h: Handle<MutFoo> = Box::new(s).into();
+    unsafe { h.drop_handle() };
+    let _ = unsafe { h.into_inner() };
+    let _ = unsafe { h.as_mut() };
+}

ffi/tests/invalid-handle-code/moved-from-handle.stderr

@@ -0,0 +1,32 @@
+error[E0382]: use of moved value: `h`
+  --> tests/invalid-handle-code/moved-from-handle.rs:13:22
+   |
+11 |     let mut h: Handle<MutFoo> = Box::new(s).into();
+   |         ----- move occurs because `h` has type `Handle<MutFoo>`, which does not implement the `Copy` trait
+12 |     unsafe { h.drop_handle() };
+   |                ------------- `h` moved due to this method call
+13 |     let _ = unsafe { h.into_inner() };
+   |                      ^ value used here after move
+   |
+note: `Handle::<H>::drop_handle` takes ownership of the receiver `self`, which moves `h`
+  --> src/handle.rs
+   |
+   |         pub unsafe fn drop_handle(self) {
+   |                                   ^^^^
+
+error[E0382]: borrow of moved value: `h`
+  --> tests/invalid-handle-code/moved-from-handle.rs:14:22
+   |
+11 |     let mut h: Handle<MutFoo> = Box::new(s).into();
+   |         ----- move occurs because `h` has type `Handle<MutFoo>`, which does not implement the `Copy` trait
+12 |     unsafe { h.drop_handle() };
+13 |     let _ = unsafe { h.into_inner() };
+   |                        ------------ `h` moved due to this method call
+14 |     let _ = unsafe { h.as_mut() };
+   |                      ^ value borrowed here after move
+   |
+note: `Handle::<H>::into_inner` takes ownership of the receiver `self`, which moves `h`
+  --> src/handle.rs
+   |
+   |         pub unsafe fn into_inner(self) -> H::From {
+   |                                  ^^^^

ffi/tests/invalid-handle-code/mut-clone-handle.rs

@@ -0,0 +1,15 @@
+use delta_kernel_ffi_macros::handle_descriptor;
+use delta_kernel_ffi::handle::Handle;
+use std::sync::Arc;
+
+pub struct Foo(u32);
+
+#[handle_descriptor(target=Foo, mutable=true, sized=true)]
+pub struct MutFoo;
+
+fn main() {
+    let s = Foo(0);
+    let h: Handle<MutFoo> = Arc::new(s).into();
+    let r = h.clone_as_arc();
+    let h = h.clone_handle();
+}