Change syntax for blocks, and allow them to carry a lifetime

README.md

@@ -3,15 +3,18 @@
 [![License](https://badgen.net/badge/license/MIT/blue)](./LICENSE.txt)
 [![CI](https://github.com/madsmtm/objc2/actions/workflows/ci.yml/badge.svg)](https://github.com/madsmtm/objc2/actions/workflows/ci.yml)
 
-The two crates you're interested in is probably [`icrate`], which provide a
-mostly autogenerated interface for Apple's Objective-C frameworks (`AppKit`,
-`Foundation`, `Metal`, `WebKit`, you name it), and [`objc2`], which contains
-the "lower level" details for interop between Rust and Objective-C (and e.g.
-support for declaring classes, which is needed to implement delegates, a
-common operation in the aforementioned frameworks).
+The three crates you're interested in is probably:
+- [`icrate`], which provides an autogenerated interfaces to Apple's
+  Objective-C frameworks (`AppKit`, `Foundation`, `Metal`, `WebKit`, you name
+  it, we [aim to have it](https://github.com/madsmtm/objc2/issues/393)).
+- [`objc2`], which provides bi-directional interop between Rust and
+  Objective-C, including support for defining Objective-C classes in Rust.
+- [`block2`], which provides bindings for Apple's C blocks, the
+  C-equivalent of a Rust closure.
 
-[`objc2`]: ./crates/objc2
 [`icrate`]: ./crates/icrate
+[`objc2`]: ./crates/objc2
+[`block2`]: ./crates/block2
 
 
 ## Goals

crates/block2/CHANGELOG.md

@@ -7,26 +7,51 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Unreleased - YYYY-MM-DD
 
 ### Added
+* **BREAKING**: Added `Block::copy` to convert blocks to `RcBlock`. This
+  replaces `StackBlock::copy`, but since `StackBlock` implements `Deref`, this
+  will likely work as before.
 * Added `RcBlock::new(closure)` as a more efficient and flexible alternative
-  to `StackBlock::new(closure).to_rc()`.
-* Added `StackBlock::to_rc` to convert stack blocks to `RcBlock`.
+  to `StackBlock::new(closure).copy()`.
+* Added `BlockFn` trait to describe valid `dyn Fn` types for blocks.
 
 ### Changed
+* **BREAKING**: Changed how blocks specify their parameter and return types.
+  We now use `dyn Fn` so that it is more clear what the parameter and return
+  types are. This also allows us to support non-`'static` blocks.
+
+  ```rust
+  // Before
+  let block: &Block<(), ()>;
+  let block: &Block<(i32,), i32>;
+  let block: &Block<(i32, u32), (i32, u32)>;
+
+  // After
+  let block: &Block<dyn Fn()>;
+  let block: &Block<dyn Fn(i32) -> i32>;
+  let block: &Block<dyn Fn(i32, u32) -> (i32, u32)>;
+  // Now possible
+  let block: &Block<dyn Fn() + '_>; // Non-'static block
+  ```
+* **BREAKING**: Make `Block::call` safe, and instead move the upholding of the
+  safety invariant to the type itself.
 * **BREAKING**: Renamed `RcBlock::new(ptr)` to `RcBlock::from_raw(ptr)`.
 * **BREAKING**: Made `RcBlock` use the null-pointer optimization;
   `RcBlock::from_raw` and `RcBlock::copy` now return an `Option`.
 * **BREAKING**: Only expose the actually public symbols `_Block_copy`,
   `_Block_release`, `_Block_object_assign`, `_Block_object_dispose`,
   `_NSConcreteGlobalBlock`, `_NSConcreteStackBlock` and `Class` in `ffi`
   module.
-* **BREAKING**: Renamed `IntoConcreteBlock` to `IntoBlock`, and moved
-  associated type `Output` to be a generic parameter.
+* **BREAKING**: Renamed `IntoConcreteBlock` to `IntoBlock`, moved
+  associated type `Output` to be a generic parameter, and added lifetime
+  parameter.`
 * No longer use the `block-sys` crate for linking to the blocks runtime.
-* Renamed `ConcreteBlock` to `StackBlock`. The old name is deprecated.
+* Renamed `ConcreteBlock` to `StackBlock`, and added a lifetime parameter. The
+  old name is deprecated.
 * Added `Copy` implementation for `StackBlock`.
 
-### Deprecated
-* Deprecated `StackBlock::copy`, it is no longer necessary.
+### Removed
+* **BREAKING**: Removed `BlockArguments` in favour of `BlockFn`, which
+  describes both the parameter types, as well as the return type.
 
 ### Fixed
 * **BREAKING**: `StackBlock::new` now requires the closure to be `Clone`. If
@@ -101,7 +126,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Changed
 * **BREAKING**: Updated `objc2-encode` to `v2.0.0-pre.0`.
 * **BREAKING**: Updated `ffi` module to `block-sys v0.0.4`. This tweaks the
-  types of a lot of fields and arguments, and makes the apple runtime always
+  types of a lot of fields and parameters, and makes the apple runtime always
   be the default.
 
 ### Removed
@@ -144,9 +169,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## 0.2.0-alpha.0 - 2021-10-28
 
 ### Added
-* **BREAKING**: Blocks now require that arguments and return type implement
+* **BREAKING**: Blocks now require that parameter and return types implement
   `objc2_encode::Encode`. This is a safety measure to prevent creating blocks
-  with invalid arguments.
+  with invalid parameters.
 * Blocks now implements `objc2_encode::RefEncode` (and as such can be used in
   Objective-C message sends).
 * Update to 2018 edition.

crates/block2/src/abi.rs

@@ -193,11 +193,11 @@ pub struct BlockHeader {
     pub(crate) reserved: MaybeUninit<c_int>,
     /// The function that implements the block.
     ///
-    /// The first argument is a pointer to this structure, the subsequent
-    /// arguments are the block's explicit parameters.
+    /// The first parameter is a pointer to this structure, the subsequent
+    /// parameters are the block's explicit parameters.
     ///
     /// If the BLOCK_USE_SRET & BLOCK_HAS_SIGNATURE flag is set, there is an
-    /// additional hidden argument, which is a pointer to the space on the
+    /// additional hidden parameter, which is a pointer to the space on the
     /// stack allocated to hold the return value.
     pub invoke: Option<unsafe extern "C" fn()>,
     /// The block's descriptor.

crates/block2/src/block.rs

@@ -1,63 +1,205 @@
 use core::fmt;
 use core::marker::PhantomData;
+use core::ptr::NonNull;
 
-use objc2::encode::{EncodeReturn, Encoding, RefEncode};
+use objc2::encode::{Encoding, RefEncode};
 
 use crate::abi::BlockHeader;
 use crate::debug::debug_block_header;
-use crate::BlockArguments;
+use crate::rc_block::block_copy_fail;
+use crate::{BlockFn, RcBlock};
 
-/// An Objective-C block that takes arguments of `A` when called and
-/// returns a value of `R`.
+/// An opaque type that holds an Objective-C block.
+///
+/// The generic type `F` must be a [`dyn`] [`Fn`] that implements
+/// the [`BlockFn`] trait (which means parameter and return types must be
+/// "encodable"), and describes the parameter and return types of the block.
+///
+/// For example, you may have the type `Block<dyn Fn(u8, u8) -> i32>`, and
+/// that would be a `'static` block that takes two `u8`s, and returns an
+/// `i32`.
+///
+/// If you want the block to carry a lifetime, use `Block<dyn Fn() + 'a>`,
+/// just like you'd usually do with `dyn Fn`.
+///
+/// [`dyn`]: https://doc.rust-lang.org/std/keyword.dyn.html
 ///
 ///
 /// # Memory layout
 ///
 /// This is intented to be an `extern type`, and as such the memory layout of
-/// this type is _not_ guaranteed. That said, pointers to this type are always
-/// thin, and match that of Objective-C blocks.
+/// this type is _not_ guaranteed. That said, **pointers** to this type are
+/// always thin, and match that of Objective-C blocks. So the layout of e.g.
+/// `&Block<dyn Fn(...) -> ... + '_>` is defined, and guaranteed to be
+/// pointer-sized and ABI-compatible with a block pointer.
+///
+///
+/// # Safety invariant
+///
+/// Calling this potentially invokes foreign code, so you must verify, when
+/// creating a reference to this, or returning it from an external API, that
+/// it doesn't violate any of Rust's safety rules.
+///
+/// In particular, blocks are sharable with multiple references (see e.g.
+/// [`Block::copy`]), so the caller must ensure that calling it can never
+/// cause a data race. This usually means you'll have to use some form of
+/// interior mutability, if you need to mutate something from inside a block.
+//
+// TODO: Potentially restrict to `F: BlockFn`, for better error messages?
 #[repr(C)]
-pub struct Block<A, R> {
+pub struct Block<F: ?Sized> {
     _inner: [u8; 0],
-    // We store `BlockHeader` + the closure captures, but `Block` has to
-    // remain an empty type otherwise the compiler thinks we only have
-    // provenance over `BlockHeader`.
+    /// We store `BlockHeader` + the closure captures, but `Block` has to
+    /// remain an empty type because we don't know the size of the closure,
+    /// and otherwise the compiler would think we only have provenance over
+    /// `BlockHeader`.
+    ///
+    /// This is possible to improve once we have extern types.
     _header: PhantomData<BlockHeader>,
-    // To get correct variance on args and return types
-    _p: PhantomData<fn(A) -> R>,
+    _p: PhantomData<F>,
 }
 
 // SAFETY: Pointers to `Block` is an Objective-C block.
-unsafe impl<A: BlockArguments, R: EncodeReturn> RefEncode for Block<A, R> {
+// This is only valid when `F: BlockFn`, as that bounds the parameters and
+// return type to be encodable too.
+unsafe impl<F: ?Sized + BlockFn> RefEncode for Block<F> {
     const ENCODING_REF: Encoding = Encoding::Block;
 }
 
-impl<A: BlockArguments, R: EncodeReturn> Block<A, R> {
-    /// Call self with the given arguments.
+impl<F: ?Sized> Block<F> {
+    fn header(&self) -> &BlockHeader {
+        let ptr: NonNull<Self> = NonNull::from(self);
+        let ptr: NonNull<BlockHeader> = ptr.cast();
+        // SAFETY: `Block` is `BlockHeader` + closure
+        unsafe { ptr.as_ref() }
+    }
+
+    /// Copy the block onto the heap as an [`RcBlock`].
     ///
-    /// # Safety
+    /// The behaviour of this function depends on whether the block is from a
+    /// [`RcBlock`] or a [`StackBlock`]. In the former case, it will bump the
+    /// reference-count (just as-if you'd `Clone`'d the `RcBlock`), in the
+    /// latter case it will construct a new `RcBlock` from the `StackBlock`.
     ///
-    /// This invokes foreign code that the caller must verify doesn't violate
-    /// any of Rust's safety rules.
+    /// This distiction should not matter, except for micro-optimizations.
     ///
-    /// For example, if this block is shared with multiple references, the
-    /// caller must ensure that calling it will not cause a data race.
-    pub unsafe fn call(&self, args: A) -> R {
+    /// [`StackBlock`]: crate::StackBlock
+    #[doc(alias = "Block_copy")]
+    #[doc(alias = "_Block_copy")]
+    #[inline]
+    pub fn copy(&self) -> RcBlock<F> {
         let ptr: *const Self = self;
-        let header = unsafe { ptr.cast::<BlockHeader>().as_ref().unwrap_unchecked() };
+        let ptr: *mut Block<F> = ptr as *mut _;
+        // SAFETY: The lifetime of the block is extended from `&self` to that
+        // of the `RcBlock`, which is fine, because the lifetime of the
+        // contained closure `F` is still carried along to the `RcBlock`.
+        unsafe { RcBlock::copy(ptr) }.unwrap_or_else(|| block_copy_fail())
+    }
+
+    /// Call the block.
+    ///
+    /// The arguments must be passed as a tuple. The return is the output of
+    /// the block.
+    #[doc(alias = "invoke")]
+    pub fn call(&self, args: F::Args) -> F::Output
+    where
+        F: BlockFn,
+    {
         // TODO: Is `invoke` actually ever null?
-        let invoke = header.invoke.unwrap_or_else(|| unreachable!());
+        let invoke = self.header().invoke.unwrap_or_else(|| unreachable!());
 
-        unsafe { A::__call_block(invoke, ptr as *mut Self, args) }
+        let ptr: NonNull<Self> = NonNull::from(self);
+        let ptr: *mut Self = ptr.as_ptr();
+
+        // SAFETY: The closure is an `Fn`, and as such is safe to call from an
+        // immutable reference.
+        unsafe { F::__call_block(invoke, ptr, args) }
     }
 }
 
-impl<A, R> fmt::Debug for Block<A, R> {
+impl<F: ?Sized> fmt::Debug for Block<F> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut f = f.debug_struct("Block");
-        let ptr: *const Self = self;
-        let header = unsafe { ptr.cast::<BlockHeader>().as_ref().unwrap() };
-        debug_block_header(header, &mut f);
+        debug_block_header(self.header(), &mut f);
         f.finish_non_exhaustive()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use core::cell::Cell;
+    use core::sync::atomic::{AtomicUsize, Ordering};
+
+    use crate::RcBlock;
+
+    use super::*;
+
+    /// Test that the way you specify lifetimes are as documented in the
+    /// reference.
+    /// <https://doc.rust-lang.org/nightly/reference/lifetime-elision.html#default-trait-object-lifetimes>
+    #[test]
+    fn test_rust_dyn_lifetime_semantics() {
+        fn takes_static(block: &Block<dyn Fn() + 'static>) {
+            block.call(());
+        }
+
+        fn takes_elided(block: &Block<dyn Fn() + '_>) {
+            block.call(());
+        }
+
+        fn takes_unspecified(block: &Block<dyn Fn()>) {
+            block.call(());
+        }
+
+        // Static lifetime
+        static MY_STATIC: AtomicUsize = AtomicUsize::new(0);
+        MY_STATIC.store(0, Ordering::Relaxed);
+        let static_lifetime: RcBlock<dyn Fn() + 'static> = RcBlock::new(|| {
+            MY_STATIC.fetch_add(1, Ordering::Relaxed);
+        });
+        takes_static(&static_lifetime);
+        takes_elided(&static_lifetime);
+        takes_unspecified(&static_lifetime);
+        assert_eq!(MY_STATIC.load(Ordering::Relaxed), 3);
+
+        // Lifetime declared with `'_`
+        let captured = Cell::new(0);
+        let elided_lifetime: RcBlock<dyn Fn() + '_> = RcBlock::new(|| {
+            captured.set(captured.get() + 1);
+        });
+        // takes_static(&elided_lifetime); // Compile error
+        takes_elided(&elided_lifetime);
+        // takes_unspecified(&elided_lifetime); // Compile error
+        assert_eq!(captured.get(), 1);
+
+        // Lifetime kept unspecified
+        let captured = Cell::new(0);
+        let unspecified_lifetime: RcBlock<dyn Fn()> = RcBlock::new(|| {
+            captured.set(captured.get() + 1);
+        });
+        // takes_static(&unspecified_lifetime); // Compile error
+        takes_elided(&unspecified_lifetime);
+        // takes_unspecified(&unspecified_lifetime); // Compile error
+        assert_eq!(captured.get(), 1);
+    }
+
+    #[allow(dead_code)]
+    fn unspecified_in_fn_is_static(block: &Block<dyn Fn()>) -> &Block<dyn Fn() + 'static> {
+        block
+    }
+
+    #[allow(dead_code)]
+    fn lending_block<'b>(block: &Block<dyn Fn() -> &'b i32 + 'b>) {
+        let _ = *block.call(());
+    }
+
+    #[allow(dead_code)]
+    fn takes_lifetime(_: &Block<dyn Fn(&i32) -> &i32>) {
+        // Not actually callable yet
+    }
+
+    #[allow(dead_code)]
+    fn covariant<'b, 'f>(b: &'b Block<dyn Fn() + 'static>) -> &'b Block<dyn Fn() + 'f> {
+        b
+    }
+}