Better error handling in msg_send! and msg_send_id!

objc2/CHANGELOG.md

@@ -23,7 +23,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
       msg_send_id![NSObject::alloc(), init]
   };
   ```
-* Add `Class::class_method`.
+* Added `Class::class_method`.
+* Added the ability to specify `error: _`, `somethingReturningError: _` and
+  so on at the end of `msg_send!`/`msg_send_id!`, and have it automatically
+  return a `Result<..., Id<NSError, Shared>>`.
+* Added the ability to specify an extra parameter at the end of the selector
+  in methods declared with `extern_methods!`, and let that be the `NSError**`
+  parameter.
 
 ### Changed
 * Allow other types than `&Class` as the receiver in `msg_send_id!` methods

objc2/CHANGELOG_FOUNDATION.md

@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * Added `NSString::concat` and `NSString::join_path`.
 * Added `CGSize`, `CGPoint` and `CGRect` (just aliases to equivalent
   `NS`-types, but helps readability).
+* Added `NSString::write_to_file`.
 
 ### Changed
 * **BREAKING**: `NSSize::new` no longer requires it's arguments to be

objc2/src/__macro_helpers.rs

@@ -1,17 +1,20 @@
 #[cfg(feature = "verify_message")]
 use alloc::vec::Vec;
+use core::ptr;
 #[cfg(feature = "verify_message")]
 use std::collections::HashSet;
 
-use crate::__sel_inner;
 use crate::declare::ClassBuilder;
 #[cfg(feature = "verify_message")]
 use crate::declare::MethodImplementation;
-use crate::rc::{Allocated, Id, Ownership};
+use crate::encode::Encode;
+use crate::message::__TupleExtender;
+use crate::rc::{Allocated, Id, Ownership, Shared};
 #[cfg(feature = "verify_message")]
 use crate::runtime::MethodDescription;
 use crate::runtime::{Class, Object, Protocol, Sel};
 use crate::{Message, MessageArguments, MessageReceiver};
+use crate::{__sel_data, __sel_inner};
 
 pub use crate::cache::CachedClass;
 pub use crate::cache::CachedSel;
@@ -38,19 +41,16 @@ pub use std::sync::Once;
 // actual assembly is as one would expect.
 
 #[inline]
-pub fn alloc() -> Sel {
-    // SAFETY: Must have NUL byte
-    __sel_inner!("alloc\0", "alloc")
+pub fn alloc_sel() -> Sel {
+    __sel_inner!(__sel_data!(alloc), "alloc")
 }
 #[inline]
-pub fn init() -> Sel {
-    // SAFETY: Must have NUL byte
-    __sel_inner!("init\0", "init")
+pub fn init_sel() -> Sel {
+    __sel_inner!(__sel_data!(init), "init")
 }
 #[inline]
-pub fn new() -> Sel {
-    // SAFETY: Must have NUL byte
-    __sel_inner!("new\0", "new")
+pub fn new_sel() -> Sel {
+    __sel_inner!(__sel_data!(new), "new")
 }
 
 /// Helper for specifying the retain semantics for a given selector family.
@@ -72,29 +72,90 @@ pub fn new() -> Sel {
 ///     means it can't be a class method!
 ///
 /// <https://clang.llvm.org/docs/AutomaticReferenceCounting.html#retainable-object-pointers-as-operands-and-arguments>
-pub struct RetainSemantics<
-    // `new` family
-    const NEW: bool,
-    // `alloc` family
-    const ALLOC: bool,
-    // `init` family
-    const INIT: bool,
-    // `copy` or `mutableCopy` family
-    const COPY_OR_MUT_COPY: bool,
-> {}
-
-type New = RetainSemantics<true, false, false, false>;
-type Alloc = RetainSemantics<false, true, false, false>;
-type Init = RetainSemantics<false, false, true, false>;
-type CopyOrMutCopy = RetainSemantics<false, false, false, true>;
-type Other = RetainSemantics<false, false, false, false>;
+// TODO: Use an enum instead of u8 here when stable
+pub struct RetainSemantics<const INNER: u8> {}
+
+pub type New = RetainSemantics<1>;
+pub type Alloc = RetainSemantics<2>;
+pub type Init = RetainSemantics<3>;
+pub type CopyOrMutCopy = RetainSemantics<4>;
+pub type Other = RetainSemantics<5>;
+
+pub const fn retain_semantics(selector: &str) -> u8 {
+    let selector = selector.as_bytes();
+    match (
+        in_selector_family(selector, b"new"),
+        in_selector_family(selector, b"alloc"),
+        in_selector_family(selector, b"init"),
+        in_selector_family(selector, b"copy"),
+        in_selector_family(selector, b"mutableCopy"),
+    ) {
+        (true, false, false, false, false) => 1,
+        (false, true, false, false, false) => 2,
+        (false, false, true, false, false) => 3,
+        (false, false, false, true, false) => 4,
+        (false, false, false, false, true) => 4,
+        (false, false, false, false, false) => 5,
+        _ => unreachable!(),
+    }
+}
 
 pub trait MsgSendId<T, U> {
     unsafe fn send_message_id<A: MessageArguments, R: MaybeUnwrap<Input = U>>(
         obj: T,
         sel: Sel,
         args: A,
     ) -> R;
+
+    /// Add an extra error argument to the argument list, call
+    /// `send_message_id` with that, and return an error if one occurred.
+    #[inline]
+    #[track_caller]
+    unsafe fn send_message_id_error<A, E>(obj: T, sel: Sel, args: A) -> Result<U, Id<E, Shared>>
+    where
+        *mut *mut E: Encode,
+        A: __TupleExtender<*mut *mut E>,
+        <A as __TupleExtender<*mut *mut E>>::PlusOneArgument: MessageArguments,
+        E: Message,
+        Option<U>: MaybeUnwrap<Input = U>,
+    {
+        let mut err: *mut E = ptr::null_mut();
+        let args = args.add_argument(&mut err);
+        let res: Option<U> = unsafe { Self::send_message_id(obj, sel, args) };
+        // As per the Cocoa documentation:
+        // > Success or failure is indicated by the return value of the
+        // > method. Although Cocoa methods that indirectly return error
+        // > objects in the Cocoa error domain are guaranteed to return such
+        // > objects if the method indicates failure by directly returning
+        // > `nil` or `NO`, you should always check that the return value is
+        // > `nil` or `NO` before attempting to do anything with the `NSError`
+        // > object.
+        if let Some(res) = res {
+            // In this case, the error is likely not created. If it is, it is
+            // autoreleased anyhow, so it would be a waste to retain and
+            // release it.
+            Ok(res)
+        } else {
+            // In this case, the error has very likely been created, but has
+            // been autoreleased (as is common for "out parameters"). Hence we
+            // need to retain it if we want it to live across autorelease
+            // pools.
+            //
+            // SAFETY: The closure `f` is guaranteed to populate the error
+            // object, or leave it as NULL. The error is shared, and all
+            // holders of the error know this, so is safe to retain.
+            Err(unsafe { encountered_error(err) })
+        }
+    }
+}
+
+// Marked `cold` to tell the optimizer that errors are comparatively rare.
+// And intentionally not inlined, for much the same reason.
+#[cold]
+#[track_caller]
+unsafe fn encountered_error<E: Message>(err: *mut E) -> Id<E, Shared> {
+    // SAFETY: Ensured by caller
+    unsafe { Id::retain(err) }.expect("error parameter should be set if the method returns NULL")
 }
 
 impl<T: MessageReceiver, U: ?Sized + Message, O: Ownership> MsgSendId<T, Id<U, O>> for New {
@@ -266,7 +327,7 @@ impl<'a> MsgSendIdFailed<'a> for New {
         if let Some(obj) = obj {
             let cls = obj.class();
             if cls.is_metaclass() {
-                if sel == new() {
+                if sel == new_sel() {
                     panic!("failed creating new instance of {:?}", cls)
                 } else {
                     panic!("failed creating new instance using +[{:?} {:?}]", cls, sel)
@@ -286,7 +347,7 @@ impl<'a> MsgSendIdFailed<'a> for Alloc {
     #[cold]
     #[track_caller]
     fn failed((cls, sel): Self::Args) -> ! {
-        if sel == alloc() {
+        if sel == alloc_sel() {
             panic!("failed allocating {:?}", cls)
         } else {
             panic!("failed allocating with +[{:?} {:?}]", cls, sel)
@@ -305,7 +366,7 @@ impl MsgSendIdFailed<'_> for Init {
         } else {
             // We can't really display a more descriptive message here since the
             // object is consumed by `init` and may not be valid any more.
-            if sel == init() {
+            if sel == init_sel() {
                 panic!("failed initializing object")
             } else {
                 panic!("failed initializing object with -{:?}", sel)
@@ -347,7 +408,7 @@ impl<'a> MsgSendIdFailed<'a> for Other {
 /// Checks whether a given selector is said to be in a given selector family.
 ///
 /// <https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-method-families>
-pub const fn in_selector_family(mut selector: &[u8], mut family: &[u8]) -> bool {
+const fn in_selector_family(mut selector: &[u8], mut family: &[u8]) -> bool {
     // Skip leading underscores from selector
     loop {
         selector = match selector {
@@ -547,6 +608,7 @@ impl Drop for ClassProtocolMethodsBuilder<'_, '_> {
 mod tests {
     use super::*;
 
+    use alloc::string::ToString;
     use alloc::vec;
     use core::ptr;
 
@@ -769,80 +831,92 @@ mod tests {
 
     #[test]
     fn test_in_selector_family() {
+        fn assert_in_family(selector: &str, family: &str) {
+            assert!(in_selector_family(selector.as_bytes(), family.as_bytes()));
+            let selector = selector.to_string() + "\0";
+            assert!(in_selector_family(selector.as_bytes(), family.as_bytes()));
+        }
+
+        fn assert_not_in_family(selector: &str, family: &str) {
+            assert!(!in_selector_family(selector.as_bytes(), family.as_bytes()));
+            let selector = selector.to_string() + "\0";
+            assert!(!in_selector_family(selector.as_bytes(), family.as_bytes()));
+        }
+
         // Common cases
 
-        assert!(in_selector_family(b"alloc", b"alloc"));
-        assert!(in_selector_family(b"allocWithZone:", b"alloc"));
-        assert!(!in_selector_family(b"dealloc", b"alloc"));
-        assert!(!in_selector_family(b"initialize", b"init"));
-        assert!(!in_selector_family(b"decimalNumberWithDecimal:", b"init"));
-        assert!(in_selector_family(b"initWithCapacity:", b"init"));
-        assert!(in_selector_family(b"_initButPrivate:withParam:", b"init"));
-        assert!(!in_selector_family(b"description", b"init"));
-        assert!(!in_selector_family(b"inIT", b"init"));
-
-        assert!(!in_selector_family(b"init", b"copy"));
-        assert!(!in_selector_family(b"copyingStuff:", b"copy"));
-        assert!(in_selector_family(b"copyWithZone:", b"copy"));
-        assert!(!in_selector_family(b"initWithArray:copyItems:", b"copy"));
-        assert!(in_selector_family(b"copyItemAtURL:toURL:error:", b"copy"));
-
-        assert!(!in_selector_family(b"mutableCopying", b"mutableCopy"));
-        assert!(in_selector_family(b"mutableCopyWithZone:", b"mutableCopy"));
-        assert!(in_selector_family(b"mutableCopyWithZone:", b"mutableCopy"));
-
-        assert!(in_selector_family(
-            b"newScriptingObjectOfClass:forValueForKey:withContentsValue:properties:",
-            b"new"
-        ));
-        assert!(in_selector_family(
-            b"newScriptingObjectOfClass:forValueForKey:withContentsValue:properties:",
-            b"new"
-        ));
-        assert!(!in_selector_family(b"newsstandAssetDownload", b"new"));
+        assert_in_family("alloc", "alloc");
+        assert_in_family("allocWithZone:", "alloc");
+        assert_not_in_family("dealloc", "alloc");
+        assert_not_in_family("initialize", "init");
+        assert_not_in_family("decimalNumberWithDecimal:", "init");
+        assert_in_family("initWithCapacity:", "init");
+        assert_in_family("_initButPrivate:withParam:", "init");
+        assert_not_in_family("description", "init");
+        assert_not_in_family("inIT", "init");
+
+        assert_not_in_family("init", "copy");
+        assert_not_in_family("copyingStuff:", "copy");
+        assert_in_family("copyWithZone:", "copy");
+        assert_not_in_family("initWithArray:copyItems:", "copy");
+        assert_in_family("copyItemAtURL:toURL:error:", "copy");
+
+        assert_not_in_family("mutableCopying", "mutableCopy");
+        assert_in_family("mutableCopyWithZone:", "mutableCopy");
+        assert_in_family("mutableCopyWithZone:", "mutableCopy");
+
+        assert_in_family(
+            "newScriptingObjectOfClass:forValueForKey:withContentsValue:properties:",
+            "new",
+        );
+        assert_in_family(
+            "newScriptingObjectOfClass:forValueForKey:withContentsValue:properties:",
+            "new",
+        );
+        assert_not_in_family("newsstandAssetDownload", "new");
 
         // Trying to weed out edge-cases:
 
-        assert!(in_selector_family(b"__abcDef", b"abc"));
-        assert!(in_selector_family(b"_abcDef", b"abc"));
-        assert!(in_selector_family(b"abcDef", b"abc"));
-        assert!(in_selector_family(b"___a", b"a"));
-        assert!(in_selector_family(b"__a", b"a"));
-        assert!(in_selector_family(b"_a", b"a"));
-        assert!(in_selector_family(b"a", b"a"));
-
-        assert!(!in_selector_family(b"_abcdef", b"abc"));
-        assert!(!in_selector_family(b"_abcdef", b"def"));
-        assert!(!in_selector_family(b"_bcdef", b"abc"));
-        assert!(!in_selector_family(b"a_bc", b"abc"));
-        assert!(!in_selector_family(b"abcdef", b"abc"));
-        assert!(!in_selector_family(b"abcdef", b"def"));
-        assert!(!in_selector_family(b"abcdef", b"abb"));
-        assert!(!in_selector_family(b"___", b"a"));
-        assert!(!in_selector_family(b"_", b"a"));
-        assert!(!in_selector_family(b"", b"a"));
-
-        assert!(in_selector_family(b"copy", b"copy"));
-        assert!(in_selector_family(b"copy:", b"copy"));
-        assert!(in_selector_family(b"copyMe", b"copy"));
-        assert!(in_selector_family(b"_copy", b"copy"));
-        assert!(in_selector_family(b"_copy:", b"copy"));
-        assert!(in_selector_family(b"_copyMe", b"copy"));
-        assert!(!in_selector_family(b"copying", b"copy"));
-        assert!(!in_selector_family(b"copying:", b"copy"));
-        assert!(!in_selector_family(b"_copying", b"copy"));
-        assert!(!in_selector_family(b"Copy", b"copy"));
-        assert!(!in_selector_family(b"COPY", b"copy"));
+        assert_in_family("__abcDef", "abc");
+        assert_in_family("_abcDef", "abc");
+        assert_in_family("abcDef", "abc");
+        assert_in_family("___a", "a");
+        assert_in_family("__a", "a");
+        assert_in_family("_a", "a");
+        assert_in_family("a", "a");
+
+        assert_not_in_family("_abcdef", "abc");
+        assert_not_in_family("_abcdef", "def");
+        assert_not_in_family("_bcdef", "abc");
+        assert_not_in_family("a_bc", "abc");
+        assert_not_in_family("abcdef", "abc");
+        assert_not_in_family("abcdef", "def");
+        assert_not_in_family("abcdef", "abb");
+        assert_not_in_family("___", "a");
+        assert_not_in_family("_", "a");
+        assert_not_in_family("", "a");
+
+        assert_in_family("copy", "copy");
+        assert_in_family("copy:", "copy");
+        assert_in_family("copyMe", "copy");
+        assert_in_family("_copy", "copy");
+        assert_in_family("_copy:", "copy");
+        assert_in_family("_copyMe", "copy");
+        assert_not_in_family("copying", "copy");
+        assert_not_in_family("copying:", "copy");
+        assert_not_in_family("_copying", "copy");
+        assert_not_in_family("Copy", "copy");
+        assert_not_in_family("COPY", "copy");
 
         // Empty family (not supported)
-        assert!(in_selector_family(b"___", b""));
-        assert!(in_selector_family(b"__", b""));
-        assert!(in_selector_family(b"_", b""));
-        assert!(in_selector_family(b"", b""));
-        assert!(!in_selector_family(b"_a", b""));
-        assert!(!in_selector_family(b"a", b""));
-        assert!(in_selector_family(b"_A", b""));
-        assert!(in_selector_family(b"A", b""));
+        assert_in_family("___", "");
+        assert_in_family("__", "");
+        assert_in_family("_", "");
+        assert_in_family("", "");
+        assert_not_in_family("_a", "");
+        assert_not_in_family("a", "");
+        assert_in_family("_A", "");
+        assert_in_family("A", "");
     }
 
     mod test_trait_disambugated {