Merge pull request #184 from madsmtm/error-handling

Improve message verification and error handling

Author: madsmtm

objc2/CHANGELOG.md

@@ -33,18 +33,25 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
   objects.
 * Added `Object::ivar_ptr` to allow direct access to instance variables
   through `&Object`.
+* Added `VerificationError` as more specific return type from
+  `Class::verify_sel`.
 
 ### Changed
-* **BREAKING:** `Sel` is now required to be non-null, which means that you
+* **BREAKING**: `Sel` is now required to be non-null, which means that you
   have to ensure that any selectors you receive from method calls are
   non-null before using them.
 * **BREAKING**: `ClassBuilder::root` is now generic over the function pointer,
   meaning you will have to coerce initializer functions to pointers like in
   `ClassBuilder::add_method` before you can use it.
-* **BREAKING**: Moved `MessageReceiver::verify_message` to `Class::verify_sel`.
+* **BREAKING**: Moved `MessageReceiver::verify_message` to `Class::verify_sel`
+  and changed return type.
+* Improved debug output with `verify_message` feature enabled.
+* **BREAKING**: Changed `MessageReceiver::send_message` to panic instead of
+  returning an error.
 
 ### Removed
-* **BREAKING:** Removed the `Sel::from_ptr` and `Sel::as_ptr` methods.
+* **BREAKING**: Removed the `Sel::from_ptr` and `Sel::as_ptr` methods.
+* **BREAKING**: Removed `MessageError`.
 
 
 ## 0.3.0-beta.0 - 2022-06-13

objc2/src/__macro_helpers.rs

@@ -1,14 +1,13 @@
 use crate::rc::{Id, Ownership};
 use crate::runtime::{Class, Sel};
-use crate::{Message, MessageArguments, MessageError, MessageReceiver};
+use crate::{Message, MessageArguments, MessageReceiver};
 
 pub use crate::cache::CachedClass;
 pub use crate::cache::CachedSel;
 
 pub use core::cell::UnsafeCell;
 pub use core::option::Option::{self, None, Some};
 pub use core::primitive::{bool, str, u8};
-pub use core::result::Result::{Err, Ok};
 pub use core::{compile_error, concat, panic, stringify};
 #[cfg(feature = "objc2-proc-macros")]
 pub use objc2_proc_macros::__hash_idents;
@@ -48,24 +47,24 @@ pub struct RetainSemantics<
 > {}
 
 pub trait MsgSendId<T, U> {
-    unsafe fn send_message_id<A: MessageArguments>(
-        obj: T,
-        sel: Sel,
-        args: A,
-    ) -> Result<Option<U>, MessageError>;
+    unsafe fn send_message_id<A: MessageArguments>(obj: T, sel: Sel, args: A) -> Option<U>;
 }
 
 // `new`
 impl<T: ?Sized + Message, O: Ownership> MsgSendId<&'_ Class, Id<T, O>>
     for RetainSemantics<true, false, false, false>
 {
     #[inline]
+    #[track_caller]
     unsafe fn send_message_id<A: MessageArguments>(
         obj: &Class,
         sel: Sel,
         args: A,
-    ) -> Result<Option<Id<T, O>>, MessageError> {
-        unsafe { MessageReceiver::send_message(obj, sel, args).map(|r| Id::new(r)) }
+    ) -> Option<Id<T, O>> {
+        // SAFETY: Checked by caller
+        let obj = unsafe { MessageReceiver::send_message(obj, sel, args) };
+        // SAFETY: The selector is `new`, so this has +1 retain count
+        unsafe { Id::new(obj) }
     }
 }
 
@@ -74,12 +73,16 @@ impl<T: ?Sized + Message, O: Ownership> MsgSendId<&'_ Class, Id<T, O>>
     for RetainSemantics<false, true, false, false>
 {
     #[inline]
+    #[track_caller]
     unsafe fn send_message_id<A: MessageArguments>(
         cls: &Class,
         sel: Sel,
         args: A,
-    ) -> Result<Option<Id<T, O>>, MessageError> {
-        unsafe { MessageReceiver::send_message(cls, sel, args).map(|r| Id::new(r)) }
+    ) -> Option<Id<T, O>> {
+        // SAFETY: Checked by caller
+        let obj = unsafe { MessageReceiver::send_message(cls, sel, args) };
+        // SAFETY: The selector is `alloc`, so this has +1 retain count
+        unsafe { Id::new(obj) }
     }
 }
 
@@ -88,19 +91,22 @@ impl<T: ?Sized + Message, O: Ownership> MsgSendId<Option<Id<T, O>>, Id<T, O>>
     for RetainSemantics<false, false, true, false>
 {
     #[inline]
+    #[track_caller]
     unsafe fn send_message_id<A: MessageArguments>(
         obj: Option<Id<T, O>>,
         sel: Sel,
         args: A,
-    ) -> Result<Option<Id<T, O>>, MessageError> {
+    ) -> Option<Id<T, O>> {
         let ptr = Id::option_into_ptr(obj);
         // SAFETY: `ptr` may be null here, but that's fine since the return
         // is `*mut T`, which is one of the few types where messages to nil is
         // allowed.
         //
         // We do this for efficiency, to avoid having a branch after every
         // `alloc`, that the user did not intend.
-        unsafe { MessageReceiver::send_message(ptr, sel, args).map(|r| Id::new(r)) }
+        let obj = unsafe { MessageReceiver::send_message(ptr, sel, args) };
+        // SAFETY: The selector is `init`, so this has +1 retain count
+        unsafe { Id::new(obj) }
     }
 }
 
@@ -109,12 +115,13 @@ impl<T: MessageReceiver, U: ?Sized + Message, O: Ownership> MsgSendId<T, Id<U, O
     for RetainSemantics<false, false, false, true>
 {
     #[inline]
-    unsafe fn send_message_id<A: MessageArguments>(
-        obj: T,
-        sel: Sel,
-        args: A,
-    ) -> Result<Option<Id<U, O>>, MessageError> {
-        unsafe { MessageReceiver::send_message(obj, sel, args).map(|r| Id::new(r)) }
+    #[track_caller]
+    unsafe fn send_message_id<A: MessageArguments>(obj: T, sel: Sel, args: A) -> Option<Id<U, O>> {
+        // SAFETY: Checked by caller
+        let obj = unsafe { MessageReceiver::send_message(obj, sel, args) };
+        // SAFETY: The selector is `copy` or `mutableCopy`, so this has +1
+        // retain count
+        unsafe { Id::new(obj) }
     }
 }
 
@@ -123,14 +130,16 @@ impl<T: MessageReceiver, U: Message, O: Ownership> MsgSendId<T, Id<U, O>>
     for RetainSemantics<false, false, false, false>
 {
     #[inline]
-    unsafe fn send_message_id<A: MessageArguments>(
-        obj: T,
-        sel: Sel,
-        args: A,
-    ) -> Result<Option<Id<U, O>>, MessageError> {
+    #[track_caller]
+    unsafe fn send_message_id<A: MessageArguments>(obj: T, sel: Sel, args: A) -> Option<Id<U, O>> {
+        // SAFETY: Checked by caller
+        let obj = unsafe { MessageReceiver::send_message(obj, sel, args) };
         // All code between the message send and the `retain_autoreleased`
         // must be able to be optimized away for this to work.
-        unsafe { MessageReceiver::send_message(obj, sel, args).map(|r| Id::retain_autoreleased(r)) }
+
+        // SAFETY: The selector is not `new`, `alloc`, `init`, `copy` nor
+        // `mutableCopy`, so the object must be manually retained.
+        unsafe { Id::retain_autoreleased(obj) }
     }
 }
 

objc2/src/lib.rs

@@ -199,7 +199,9 @@ pub use objc_sys as ffi;
 #[doc(no_inline)]
 pub use objc2_encode::{Encode, EncodeArguments, Encoding, RefEncode};
 
-pub use crate::message::{Message, MessageArguments, MessageError, MessageReceiver};
+pub use crate::message::{Message, MessageArguments, MessageReceiver};
+#[cfg(feature = "malloc")]
+pub use crate::verify::VerificationError;
 
 #[macro_use]
 mod macros;

objc2/src/macros.rs

@@ -455,45 +455,30 @@ macro_rules! __sel_inner {
 #[macro_export]
 macro_rules! msg_send {
     [super($obj:expr, $superclass:expr), $selector:ident $(,)?] => ({
-        // Note: this import means that using these types inside `expr` will
-        // generate an error. We won't bother with that (yet) though.
-        use $crate::__macro_helpers::{Err, Ok, panic};
         let sel = $crate::sel!($selector);
         let result;
-        match $crate::MessageReceiver::send_super_message($obj, $superclass, sel, ()) {
-            Err(s) => panic!("{}", s),
-            Ok(r) => result = r,
-        }
+        // Note: `sel` and `result` can be accessed from the `obj` and
+        // `superclass` expressions - we won't (yet) bother with preventing
+        // that though.
+        result = $crate::MessageReceiver::send_super_message($obj, $superclass, sel, ());
         result
     });
     [super($obj:expr, $superclass:expr), $($selector:ident : $argument:expr $(,)?)+] => ({
-        use $crate::__macro_helpers::{Err, Ok, panic};
         let sel = $crate::sel!($($selector :)+);
         let result;
-        match $crate::MessageReceiver::send_super_message($obj, $superclass, sel, ($($argument,)+)) {
-            Err(s) => panic!("{}", s),
-            Ok(r) => result = r,
-        }
+        result = $crate::MessageReceiver::send_super_message($obj, $superclass, sel, ($($argument,)+));
         result
     });
     [$obj:expr, $selector:ident $(,)?] => ({
-        use $crate::__macro_helpers::{Err, Ok, panic};
         let sel = $crate::sel!($selector);
         let result;
-        match $crate::MessageReceiver::send_message($obj, sel, ()) {
-            Err(s) => panic!("{}", s),
-            Ok(r) => result = r,
-        }
+        result = $crate::MessageReceiver::send_message($obj, sel, ());
         result
     });
     [$obj:expr, $($selector:ident : $argument:expr $(,)?)+] => ({
-        use $crate::__macro_helpers::{Err, Ok, panic};
         let sel = $crate::sel!($($selector :)+);
         let result;
-        match $crate::MessageReceiver::send_message($obj, sel, ($($argument,)+)) {
-            Err(s) => panic!("{}", s),
-            Ok(r) => result = r,
-        }
+        result = $crate::MessageReceiver::send_message($obj, sel, ($($argument,)+));
         result
     });
 }
@@ -669,29 +654,25 @@ macro_rules! msg_send_id {
     [$obj:expr, $selector:ident $(,)?] => ({
         $crate::__msg_send_id_helper!(@verify $selector);
 
-        use $crate::__macro_helpers::{u8, Err, Ok, Option, panic, stringify};
+        // Note: this import means that using these types inside `expr` may
+        // generate an error. We won't (yet) bother with that though.
+        use $crate::__macro_helpers::{u8, Option, stringify};
 
         let sel = $crate::sel!($selector);
         const NAME: &[u8] = stringify!($selector).as_bytes();
         $crate::__msg_send_id_helper!(@get_assert_consts NAME);
         let result: Option<$crate::rc::Id<_, _>>;
-        match <RS as $crate::__macro_helpers::MsgSendId<_, _>>::send_message_id($obj, sel, ()) {
-            Err(s) => panic!("{}", s),
-            Ok(r) => result = r,
-        }
+        result = <RS as $crate::__macro_helpers::MsgSendId<_, _>>::send_message_id($obj, sel, ());
         result
     });
     [$obj:expr, $($selector:ident : $argument:expr),+ $(,)?] => ({
-        use $crate::__macro_helpers::{concat, u8, Err, Ok, Option, panic, stringify};
+        use $crate::__macro_helpers::{concat, u8, Option, stringify};
 
         let sel = $crate::sel!($($selector:)+);
         const NAME: &[u8] = concat!($(stringify!($selector), ':'),+).as_bytes();
         $crate::__msg_send_id_helper!(@get_assert_consts NAME);
         let result: Option<$crate::rc::Id<_, _>>;
-        match <RS as $crate::__macro_helpers::MsgSendId<_, _>>::send_message_id($obj, sel, ($($argument,)+)) {
-            Err(s) => panic!("{}", s),
-            Ok(r) => result = r,
-        }
+        result = <RS as $crate::__macro_helpers::MsgSendId<_, _>>::send_message_id($obj, sel, ($($argument,)+));
         result
     });
 }

objc2/src/message/apple/mod.rs

@@ -1,6 +1,7 @@
-use super::{conditional_try, Encode, MessageArguments, MessageError};
+use super::conditional_try;
 use crate::ffi;
 use crate::runtime::{Class, Imp, Object, Sel};
+use crate::{Encode, MessageArguments};
 
 #[cfg(target_arch = "x86")]
 #[path = "x86.rs"]
@@ -24,11 +25,8 @@ unsafe trait MsgSendFn: Encode {
 }
 
 #[inline]
-pub(crate) unsafe fn send_unverified<A, R>(
-    receiver: *mut Object,
-    sel: Sel,
-    args: A,
-) -> Result<R, MessageError>
+#[track_caller]
+pub(crate) unsafe fn send_unverified<A, R>(receiver: *mut Object, sel: Sel, args: A) -> R
 where
     A: MessageArguments,
     R: Encode,
@@ -38,12 +36,13 @@ where
 }
 
 #[inline]
+#[track_caller]
 pub(crate) unsafe fn send_super_unverified<A, R>(
     receiver: *mut Object,
     superclass: &Class,
     sel: Sel,
     args: A,
-) -> Result<R, MessageError>
+) -> R
 where
     A: MessageArguments,
     R: Encode,

objc2/src/message/gnustep.rs

@@ -1,14 +1,12 @@
 use core::mem;
 
-use super::{conditional_try, Encode, MessageArguments, MessageError};
+use super::conditional_try;
 use crate::ffi;
 use crate::runtime::{Class, Object, Sel};
+use crate::{Encode, MessageArguments};
 
-pub(crate) unsafe fn send_unverified<A, R>(
-    receiver: *mut Object,
-    sel: Sel,
-    args: A,
-) -> Result<R, MessageError>
+#[track_caller]
+pub(crate) unsafe fn send_unverified<A, R>(receiver: *mut Object, sel: Sel, args: A) -> R
 where
     A: MessageArguments,
     R: Encode,
@@ -28,12 +26,13 @@ where
     unsafe { conditional_try(|| A::__invoke(msg_send_fn, receiver, sel, args)) }
 }
 
+#[track_caller]
 pub(crate) unsafe fn send_super_unverified<A, R>(
     receiver: *mut Object,
     superclass: &Class,
     sel: Sel,
     args: A,
-) -> Result<R, MessageError>
+) -> R
 where
     A: MessageArguments,
     R: Encode,