Redo RcBlock and StackBlock relationship

crates/block2/CHANGELOG.md

@@ -6,16 +6,30 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased - YYYY-MM-DD
 
+### 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`.
+
 ### Changed
+* **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`.
 * No longer use the `block-sys` crate for linking to the blocks runtime.
 * Renamed `ConcreteBlock` to `StackBlock`. The old name is deprecated.
-* **BREAKING**: Renamed `IntoConcreteBlock` to `IntoBlock`.
+* 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.
 * Relaxed the `F: Debug` bound on `StackBlock`'s `Debug` implementation.
 
 

crates/block2/src/abi.rs

@@ -254,8 +254,14 @@ pub(crate) struct BlockDescriptorCopyDispose {
     pub(crate) size: c_ulong,
 
     /// Helper to copy the block if it contains nontrivial copy operations.
+    ///
+    /// This may be NULL since macOS 11.0.1 in Apple's runtime, but this
+    /// should not be relied on.
     pub(crate) copy: Option<unsafe extern "C" fn(dst: *mut c_void, src: *const c_void)>,
     /// Helper to destroy the block after being copied.
+    ///
+    /// This may be NULL since macOS 11.0.1 in Apple's runtime, but this
+    /// should not be relied on.
     pub(crate) dispose: Option<unsafe extern "C" fn(src: *mut c_void)>,
 }
 
@@ -293,8 +299,14 @@ pub(crate) struct BlockDescriptorCopyDisposeSignature {
     pub(crate) size: c_ulong,
 
     /// Helper to copy the block if it contains nontrivial copy operations.
+    ///
+    /// This may be NULL since macOS 11.0.1 in Apple's runtime, but this
+    /// should not be relied on.
     pub(crate) copy: Option<unsafe extern "C" fn(dst: *mut c_void, src: *const c_void)>,
     /// Helper to destroy the block after being copied.
+    ///
+    /// This may be NULL since macOS 11.0.1 in Apple's runtime, but this
+    /// should not be relied on.
     pub(crate) dispose: Option<unsafe extern "C" fn(src: *mut c_void)>,
 
     /// Objective-C type encoding of the block.

crates/block2/src/global.rs

@@ -23,10 +23,10 @@ const GLOBAL_DESCRIPTOR: BlockDescriptor = BlockDescriptor {
 /// This is effectively a glorified function pointer, and can created and
 /// stored in static memory using the [`global_block!`] macro.
 ///
-/// If [`StackBlock`] is the [`Fn`]-block equivalent, this is likewise the
+/// If [`RcBlock`] is the [`Fn`]-block equivalent, this is likewise the
 /// [`fn`]-block equivalent.
 ///
-/// [`StackBlock`]: crate::StackBlock
+/// [`RcBlock`]: crate::RcBlock
 /// [`global_block!`]: crate::global_block
 #[repr(C)]
 pub struct GlobalBlock<A, R = ()> {

crates/block2/src/lib.rs

@@ -45,35 +45,112 @@
 //!
 //! ## Creating blocks
 //!
-//! Creating a block to pass to Objective-C can be done with the
-//! [`StackBlock`] struct. For example, to create a block that adds two
-//! integers, we could write:
+//! Creating a block to pass to Objective-C can be done with [`RcBlock`] or
+//! [`StackBlock`], depending on if you want to move the block to the heap,
+//! or let the callee decide if it needs to do that.
+//!
+//! To declare external functions or methods that takes blocks, use
+//! `&Block<A, R>` or `Option<&Block<A, R>>`, where `A` is a tuple with the
+//! argument types, and `R` is the return type.
+//!
+//! As an example, we're going to work with a block that adds two integers.
 //!
 //! ```
-//! use block2::StackBlock;
-//! let block = StackBlock::new(|a: i32, b: i32| a + b);
-//! let block = block.copy();
-//! assert_eq!(unsafe { block.call((5, 8)) }, 13);
+//! use block2::Block;
+//!
+//! // External function that takes a block
+//! extern "C" {
+//!     fn add_numbers_using_block(block: &Block<(i32, i32), i32>);
+//! }
+//! #
+//! # use objc2::ClassType;
+//! # objc2::extern_class!(
+//! #     struct MyClass;
+//! #
+//! #     unsafe impl ClassType for MyClass {
+//! #         type Super = objc2::runtime::NSObject;
+//! #         type Mutability = objc2::mutability::InteriorMutable;
+//! #         const NAME: &'static str = "NSObject";
+//! #     }
+//! # );
+//!
+//! // External method that takes a block
+//! objc2::extern_methods!(
+//!     unsafe impl MyClass {
+//!         #[method(addNumbersUsingBlock:)]
+//!         pub fn addNumbersUsingBlock(&self, block: &Block<(i32, i32), i32>);
+//!     }
+//! );
 //! ```
 //!
-//! It is important to copy your block to the heap (with the [`copy`] method)
-//! before passing it to Objective-C; this is because our [`StackBlock`] is
-//! only meant to be copied once, and we can enforce this in Rust, but if
-//! Objective-C code were to copy it twice we could have a double free.
+//! To call such a function / method, we could create a new block from a
+//! closure using [`RcBlock::new`].
+//!
+//! ```
+//! use block2::RcBlock;
+//! #
+//! # extern "C" {
+//! #     fn add_numbers_using_block(block: &block2::Block<(i32, i32), i32>);
+//! # }
+//! # mod imp {
+//! #     #[no_mangle]
+//! #     extern "C" fn add_numbers_using_block(block: &block2::Block<(i32, i32), i32>) {
+//! #         assert_eq!(unsafe { block.call((5, 8)) }, 13);
+//! #     }
+//! # }
+//!
+//! let block = RcBlock::new(|a: i32, b: i32| a + b);
+//! unsafe { add_numbers_using_block(&block) };
+//! ```
 //!
-//! [`copy`]: StackBlock::copy
+//! This creates the block on the heap. If the external function you're
+//! calling is not going to copy the block, it may be more performant if you
+//! construct a [`StackBlock`] directly, using [`StackBlock::new`].
 //!
-//! As an optimization if your block doesn't capture any variables, you can
-//! use the [`global_block!`] macro to create a static block:
+//! Note though that this requires that the closure is [`Clone`], as the
+//! external code may want to copy the block to the heap in the future.
+//!
+//! ```
+//! use block2::StackBlock;
+//! #
+//! # extern "C" {
+//! #     fn add_numbers_using_block(block: &block2::Block<(i32, i32), i32>);
+//! # }
+//! # mod imp {
+//! #     #[no_mangle]
+//! #     extern "C" fn add_numbers_using_block(block: &block2::Block<(i32, i32), i32>) {
+//! #         assert_eq!(unsafe { block.call((5, 8)) }, 13);
+//! #     }
+//! # }
+//!
+//! let block = StackBlock::new(|a: i32, b: i32| a + b);
+//! unsafe { add_numbers_using_block(&block) };
+//! ```
+//!
+//! As an optimization if your block doesn't capture any variables (as in the
+//! above examples), you can use the [`global_block!`] macro to create a
+//! static block.
 //!
 //! ```
 //! use block2::global_block;
+//! #
+//! # extern "C" {
+//! #     fn add_numbers_using_block(block: &block2::Block<(i32, i32), i32>);
+//! # }
+//! # mod imp {
+//! #     #[no_mangle]
+//! #     extern "C" fn add_numbers_using_block(block: &block2::Block<(i32, i32), i32>) {
+//! #         assert_eq!(unsafe { block.call((5, 8)) }, 13);
+//! #     }
+//! # }
+//!
 //! global_block! {
-//!     static MY_BLOCK = || -> f32 {
-//!         10.0
+//!     static MY_BLOCK = |a: i32, b: i32| -> i32 {
+//!         a + b
 //!     };
 //! }
-//! assert_eq!(unsafe { MY_BLOCK.call(()) }, 10.0);
+//!
+//! unsafe { add_numbers_using_block(&MY_BLOCK) };
 //! ```
 //!
 //!

crates/block2/src/rc_block.rs

@@ -1,70 +1,155 @@
 use core::fmt;
+use core::mem::ManuallyDrop;
 use core::ops::Deref;
+use core::ptr::NonNull;
+
+use objc2::encode::EncodeReturn;
 
 use crate::abi::BlockHeader;
 use crate::debug::debug_block_header;
-use crate::{ffi, Block};
+use crate::{ffi, Block, BlockArguments, IntoBlock, StackBlock};
 
-/// A reference-counted Objective-C block.
+/// A reference-counted Objective-C block that is stored on the heap.
+///
+/// This is a smart pointer that [`Deref`]s to [`Block`].
+///
+///
+/// # Memory-layout
+///
+/// This is guaranteed to have the same size and alignment as a pointer to a
+/// block, `*const Block<A, R>`.
+///
+/// Additionally, it participates in the null-pointer optimization, that is,
+/// `Option<RcBlock<A, R>>` is guaranteed to have the same size as
+/// `RcBlock<A, R>`.
+#[doc(alias = "MallocBlock")]
 pub struct RcBlock<A, R> {
-    pub(crate) ptr: *mut Block<A, R>,
+    ptr: NonNull<Block<A, R>>,
 }
 
 impl<A, R> RcBlock<A, R> {
-    /// Construct an `RcBlock` for the given block without copying it.
-    /// The caller must ensure the block has a +1 reference count.
+    /// Construct an `RcBlock` from the given block pointer by taking
+    /// ownership.
+    ///
+    /// This will return `None` if the pointer is NULL.
     ///
     /// # Safety
     ///
-    /// The given pointer must point to a valid `Block` and must have a +1
-    /// reference count or it will be overreleased when the `RcBlock` is
-    /// dropped.
-    pub unsafe fn new(ptr: *mut Block<A, R>) -> Self {
-        RcBlock { ptr }
+    /// The given pointer must point to a valid block, the arguments and
+    /// return type must be correct, and the block must have a +1 reference /
+    /// retain count from somewhere else.
+    #[inline]
+    pub unsafe fn from_raw(ptr: *mut Block<A, R>) -> Option<Self> {
+        NonNull::new(ptr).map(|ptr| Self { ptr })
     }
 
-    /// Constructs an `RcBlock` by copying the given block.
+    /// Construct an `RcBlock` from the given block pointer.
+    ///
+    /// The block will be copied, and have its reference-count increased by
+    /// one.
+    ///
+    /// This will return `None` if the pointer is NULL, or if an allocation
+    /// failure occurred.
     ///
     /// # Safety
     ///
-    /// The given pointer must point to a valid `Block`.
-    pub unsafe fn copy(ptr: *mut Block<A, R>) -> Self {
-        // SAFETY: The caller ensures the pointer is valid.
+    /// The given pointer must point to a valid block, and the arguments and
+    /// return type must be correct.
+    #[doc(alias = "Block_copy")]
+    #[doc(alias = "_Block_copy")]
+    #[inline]
+    pub unsafe fn copy(ptr: *mut Block<A, R>) -> Option<Self> {
         let ptr: *mut Block<A, R> = unsafe { ffi::_Block_copy(ptr.cast()) }.cast();
         // SAFETY: We just copied the block, so the reference count is +1
+        unsafe { Self::from_raw(ptr) }
+    }
+}
+
+impl<A: BlockArguments, R: EncodeReturn> RcBlock<A, R> {
+    /// Construct a `RcBlock` with the given closure.
+    ///
+    /// The closure will be coped to the heap on construction.
+    ///
+    /// When the block is called, it will return the value that results from
+    /// calling the closure.
+    //
+    // Note: Unsure if this should be #[inline], but I think it may be able to
+    // benefit from not being so.
+    pub fn new<F>(closure: F) -> Self
+    where
+        // The `F: 'static` bound is required because the `RcBlock` has no way
+        // of tracking a lifetime.
+        F: IntoBlock<A, Output = R> + 'static,
+    {
+        // SAFETY: The stack block is copied once below.
+        //
+        // Note: We could theoretically use `_NSConcreteMallocBlock`, and use
+        // `malloc` ourselves to put the block on the heap, but that symbol is
+        // not part of the public ABI, and may break in the future.
         //
-        // TODO: Does _Block_copy always returns a valid pointer?
-        unsafe { Self::new(ptr) }
+        // Clang doesn't do this optimization either.
+        let block = unsafe { StackBlock::new_no_clone(closure) };
+
+        // Transfer ownership from the stack to the heap.
+        let mut block = ManuallyDrop::new(block);
+        let ptr: *mut StackBlock<A, R, F> = &mut *block;
+        let ptr: *mut Block<A, R> = ptr.cast();
+        // SAFETY: The block will be moved to the heap, and we forget the
+        // original block because the heap block will drop in our dispose
+        // helper.
+        unsafe { Self::copy(ptr) }.unwrap_or_else(|| rc_new_fail())
     }
 }
 
 impl<A, R> Clone for RcBlock<A, R> {
-    fn clone(&self) -> RcBlock<A, R> {
+    #[inline]
+    fn clone(&self) -> Self {
         // SAFETY: The pointer is valid, since the only way to get an RcBlock
         // in the first place is through unsafe functions.
-        unsafe { RcBlock::copy(self.ptr) }
+        unsafe { Self::copy(self.ptr.as_ptr()) }.unwrap_or_else(|| rc_clone_fail())
     }
 }
 
+// Intentionally not `#[track_caller]`, to keep the code-size smaller (as this
+// error is very unlikely).
+pub(crate) fn rc_new_fail() -> ! {
+    // This likely means the system is out of memory.
+    panic!("failed creating RcBlock")
+}
+
+// Intentionally not `#[track_caller]`, see above.
+fn rc_clone_fail() -> ! {
+    unreachable!("cloning a RcBlock bumps the reference count, which should be infallible")
+}
+
 impl<A, R> Deref for RcBlock<A, R> {
     type Target = Block<A, R>;
 
+    #[inline]
     fn deref(&self) -> &Block<A, R> {
-        // SAFETY: The pointer is ensured valid by creator functions.
-        unsafe { self.ptr.as_ref().unwrap_unchecked() }
+        // SAFETY: The pointer is valid, as ensured by creation methods, and
+        // will be so for as long as the `RcBlock` is, since that holds +1
+        // reference count.
+        unsafe { self.ptr.as_ref() }
     }
 }
 
 impl<A, R> Drop for RcBlock<A, R> {
+    /// Release the block, decreasing the reference-count by 1.
+    #[doc(alias = "Block_release")]
+    #[doc(alias = "_Block_release")]
+    #[inline]
     fn drop(&mut self) {
-        unsafe { ffi::_Block_release(self.ptr.cast()) };
+        // SAFETY: The pointer has +1 reference count, as ensured by creation
+        // methods.
+        unsafe { ffi::_Block_release(self.ptr.as_ptr().cast()) };
     }
 }
 
 impl<A, R> fmt::Debug for RcBlock<A, R> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut f = f.debug_struct("RcBlock");
-        let header = unsafe { self.ptr.cast::<BlockHeader>().as_ref().unwrap() };
+        let header = unsafe { self.ptr.cast::<BlockHeader>().as_ref() };
         debug_block_header(header, &mut f);
         f.finish_non_exhaustive()
     }