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

crates/block2/CHANGELOG.md

@@ -8,8 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Added
 * 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()`.
+* **BREAKING**: Added `Block::copy` to convert blocks to `RcBlock`. This
+  replaces `StackBlock::copy`, but since `StackBlock` implements `Deref`, this
+  will likely work as before.
 
 ### Changed
 * **BREAKING**: Renamed `RcBlock::new(ptr)` to `RcBlock::from_raw(ptr)`.
@@ -19,15 +21,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   `_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.
-
 ### Fixed
 * **BREAKING**: `StackBlock::new` now requires the closure to be `Clone`. If
   this is not desired, use `RcBlock::new` instead.

crates/block2/src/block.rs

@@ -1,11 +1,13 @@
 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`.
@@ -16,48 +18,161 @@ use crate::BlockArguments;
 /// 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.
+/// An opaque type that holds an Objective-C block.
+///
+/// This is the Objective-C equivalent of [`dyn Fn`][Fn], ... TODO
+///
+/// Takes arguments `A` and returns `R`.
+///
+/// Safety invariant:
+///
+/// This invokes foreign code that the caller must verify doesn't violate
+/// any of Rust's safety rules.
+///
+/// For example, if this block is shared with multiple references, the
+/// caller must ensure that calling it will not cause a data race.
+//
+// We don't technically need to restrict the type to `Closure: BlockFnOnce`
+// here, it is restricted elsewhere, but it helps giving better error messages
+// TODO: Verify and test this claim.
 #[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, 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> {
+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.
-    ///
-    /// # Safety
-    ///
-    /// This invokes foreign code that the caller must verify doesn't violate
-    /// any of Rust's safety rules.
+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`.
     ///
-    /// 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 {
+    /// TODO: This bumps reference count, or creates new.
+    //
+    // TODO: Place this on `Block<A, R>`, once that carries a lifetime.
+    #[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 block will be alive for the lifetime of the new
+        // `RcBlock`, and the lifetime is carried over. TODO
+        unsafe { RcBlock::copy(ptr) }.unwrap_or_else(|| block_copy_fail())
+    }
+
+    /// Call self with the given arguments.
+    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 `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::*;
+
+    /// <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
+    }
+}