Disallow &mut self receivers in declare_class! when not mutable

These are clearly unsound, and even though the error messages are pretty terrible, it is still better to disallow them.
madsmtm · Apr 20, 2023 · 3c14a78 · 3c14a78
1 parent f6d9aba
commit 3c14a78
Show file tree

Hide file tree

Showing 5 changed files with 147 additions and 18 deletions.
diff --git a/crates/objc2/src/declare/mod.rs b/crates/objc2/src/declare/mod.rs
@@ -20,7 +20,7 @@
 //!
 //! use objc2::declare::ClassBuilder;
 //! use objc2::rc::Id;
-//! use objc2::runtime::{Class, NSObject, Sel};
+//! use objc2::runtime::{Class, Object, NSObject, Sel};
 //! use objc2::{sel, msg_send, msg_send_id, ClassType};
 //!
 //! fn register_class() -> &'static Class {
@@ -32,12 +32,16 @@
 //!     builder.add_ivar::<Cell<u32>>("_number");
 //!
 //!     // Add an Objective-C method for initializing an instance with a number
+//!     //
+//!     // We "cheat" a bit here, and use `Object` instead of `NSObject`,
+//!     // since only the former is allowed to be a mutable receiver (which is
+//!     // always safe in `init` methods, but not in others).
 //!     unsafe extern "C" fn init_with_number(
-//!         this: &mut NSObject,
+//!         this: &mut Object,
 //!         _cmd: Sel,
 //!         number: u32,
-//!     ) -> Option<&mut NSObject> {
-//!         let this: Option<&mut NSObject> = msg_send![super(this, NSObject::class()), init];
+//!     ) -> Option<&mut Object> {
+//!         let this: Option<&mut Object> = msg_send![super(this, NSObject::class()), init];
 //!         this.map(|this| {
 //!             // SAFETY: The ivar is added with the same type above
 //!             this.set_ivar::<Cell<u32>>("_number", Cell::new(number));
@@ -130,6 +134,7 @@ use std::ffi::CString;
 use crate::encode::__unstable::{EncodeArguments, EncodeReturn};
 use crate::encode::{Encode, Encoding, RefEncode};
 use crate::ffi;
+use crate::mutability::IsMutable;
 use crate::rc::Allocated;
 use crate::runtime::{Bool, Class, Imp, Object, Protocol, Sel};
 use crate::sel;
@@ -161,17 +166,17 @@ pub trait MethodImplementation: private::Sealed {
 }
 
 macro_rules! method_decl_impl {
-    (@<$($l:lifetime),*> T, $r:ident, $f:ty, $($t:ident),*) => {
+    (@<$($l:lifetime),*> T: $t_bound:ident, $r:ident, $f:ty, $($t:ident),*) => {
         impl<$($l,)* T, $r, $($t),*> private::Sealed for $f
         where
-            T: Message + ?Sized,
+            T: ?Sized + $t_bound,
             $r: EncodeReturn,
             $($t: Encode,)*
         {}
 
         impl<$($l,)* T, $r, $($t),*> MethodImplementation for $f
         where
-            T: Message + ?Sized,
+            T: ?Sized + $t_bound,
             $r: EncodeReturn,
             $($t: Encode,)*
         {
@@ -184,7 +189,7 @@ macro_rules! method_decl_impl {
             }
         }
     };
-    (@<$($l:lifetime),*> Class, $r:ident, $f:ty, $($t:ident),*) => {
+    (@<$($l:lifetime),*> $callee:ident, $r:ident, $f:ty, $($t:ident),*) => {
         impl<$($l,)* $r, $($t),*> private::Sealed for $f
         where
             $r: EncodeReturn,
@@ -196,7 +201,7 @@ macro_rules! method_decl_impl {
             $r: EncodeReturn,
             $($t: Encode,)*
         {
-            type Callee = Class;
+            type Callee = $callee;
             type Ret = $r;
             type Args = ($($t,)*);
 
@@ -209,14 +214,14 @@ macro_rules! method_decl_impl {
         #[doc(hidden)]
         impl<T, $($t),*> private::Sealed for $f
         where
-            T: Message + ?Sized,
+            T: ?Sized + Message,
             $($t: Encode,)*
         {}
 
         #[doc(hidden)]
         impl<T, $($t),*> MethodImplementation for $f
         where
-            T: Message + ?Sized,
+            T: ?Sized + Message,
             $($t: Encode,)*
         {
             type Callee = T;
@@ -237,12 +242,15 @@ macro_rules! method_decl_impl {
         }
     };
     (# $abi:literal; $($t:ident),*) => {
-        method_decl_impl!(@<'a> T, R, extern $abi fn(&'a T, Sel $(, $t)*) -> R, $($t),*);
-        method_decl_impl!(@<'a> T, R, extern $abi fn(&'a mut T, Sel $(, $t)*) -> R, $($t),*);
-        method_decl_impl!(@<> T, R, unsafe extern $abi fn(*const T, Sel $(, $t)*) -> R, $($t),*);
-        method_decl_impl!(@<> T, R, unsafe extern $abi fn(*mut T, Sel $(, $t)*) -> R, $($t),*);
-        method_decl_impl!(@<'a> T, R, unsafe extern $abi fn(&'a T, Sel $(, $t)*) -> R, $($t),*);
-        method_decl_impl!(@<'a> T, R, unsafe extern $abi fn(&'a mut T, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<'a> T: Message, R, extern $abi fn(&'a T, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<'a> T: IsMutable, R, extern $abi fn(&'a mut T, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<> T: Message, R, unsafe extern $abi fn(*const T, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<> T: Message, R, unsafe extern $abi fn(*mut T, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<'a> T: Message, R, unsafe extern $abi fn(&'a T, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<'a> T: IsMutable, R, unsafe extern $abi fn(&'a mut T, Sel $(, $t)*) -> R, $($t),*);
+
+        method_decl_impl!(@<'a> Object, R, extern $abi fn(&'a mut Object, Sel $(, $t)*) -> R, $($t),*);
+        method_decl_impl!(@<'a> Object, R, unsafe extern $abi fn(&'a mut Object, Sel $(, $t)*) -> R, $($t),*);
 
         method_decl_impl!(@<'a> Class, R, extern $abi fn(&'a Class, Sel $(, $t)*) -> R, $($t),*);
         method_decl_impl!(@<> Class, R, unsafe extern $abi fn(*const Class, Sel $(, $t)*) -> R, $($t),*);

diff --git a/crates/objc2/src/macros/declare_class.rs b/crates/objc2/src/macros/declare_class.rs
@@ -80,6 +80,13 @@
 /// your method will be registered as an instance method, and if you don't it
 /// will be registered as a class method.
 ///
+/// On instance methods, you can freely choose between different types of
+/// receivers, e.g. `&self`, `this: *const Self`, `&mut self`, and so on. Note
+/// though that using raw pointers requires the function to be `unsafe`, and
+/// using `&mut self` requires the class' mutability to be [`IsMutable`].
+/// If you require mutating your class' instance variables, consider using
+/// [`Cell`] or similar instead.
+///
 /// The desired selector can be specified using the `#[method(my:selector:)]`
 /// or `#[method_id(my:selector:)]` attribute, similar to the
 /// [`extern_methods!`] macro.
@@ -107,6 +114,8 @@
 ///
 /// ["associated functions"]: https://doc.rust-lang.org/reference/items/associated-items.html#methods
 /// ["methods"]: https://doc.rust-lang.org/reference/items/associated-items.html#methods
+/// [`IsMutable`]: crate::mutability::IsMutable
+/// [`Cell`]: core::cell::Cell
 /// [open an issue]: https://github.com/madsmtm/objc2/issues/new
 /// [`msg_send!`]: crate::msg_send
 /// [`runtime::Bool`]: crate::runtime::Bool

diff --git a/crates/objc2/src/rc/test_object.rs b/crates/objc2/src/rc/test_object.rs
@@ -99,7 +99,7 @@ declare_class!(
         }
 
         #[method(init)]
-        fn init(this: &mut Self) -> *mut Self {
+        unsafe fn init(this: *mut Self) -> *mut Self {
             TEST_DATA.with(|data| data.borrow_mut().init += 1);
             unsafe { msg_send![super(this), init] }
         }

diff --git a/crates/test-ui/ui/declare_class_mut_self_not_mutable.rs b/crates/test-ui/ui/declare_class_mut_self_not_mutable.rs
@@ -0,0 +1,32 @@
+use objc2::rc::Id;
+use objc2::runtime::NSObject;
+use objc2::{declare_class, mutability, ClassType};
+
+declare_class!(
+    struct CustomObject;
+
+    unsafe impl ClassType for CustomObject {
+        type Super = NSObject;
+        type Mutability = mutability::InteriorMutable;
+        const NAME: &'static str = "CustomObject";
+    }
+
+    unsafe impl CustomObject {
+        #[method(initTest)]
+        fn initTest(&mut self) -> &mut Self {
+            unimplemented!()
+        }
+
+        #[method(testMethod)]
+        fn test_method(&mut self) {
+            unimplemented!()
+        }
+
+        #[method_id(testMethodId)]
+        fn test_method_id(&mut self) -> Id<Self> {
+            unimplemented!()
+        }
+    }
+);
+
+fn main() {}
diff --git a/crates/test-ui/ui/declare_class_mut_self_not_mutable.stderr b/crates/test-ui/ui/declare_class_mut_self_not_mutable.stderr
@@ -0,0 +1,80 @@
+error[E0277]: the trait bound `InteriorMutable: mutability::private::MutabilityIsMutable` is not satisfied
+ --> ui/declare_class_mut_self_not_mutable.rs
+  |
+  | / declare_class!(
+  | |     struct CustomObject;
+  | |
+  | |     unsafe impl ClassType for CustomObject {
+... |
+  | |     }
+  | | );
+  | | ^
+  | | |
+  | |_the trait `mutability::private::MutabilityIsMutable` is not implemented for `InteriorMutable`
+  |   required by a bound introduced by this call
+  |
+  = help: the following other types implement trait `mutability::private::MutabilityIsMutable`:
+            Mutable
+            MutableWithImmutableSuperclass<IS>
+  = note: required for `CustomObject` to implement `IsMutable`
+  = note: required for `extern "C" fn(&mut CustomObject, objc2::runtime::Sel) -> &mut CustomObject` to implement `MethodImplementation`
+note: required by a bound in `ClassBuilder::add_method`
+ --> $WORKSPACE/crates/objc2/src/declare/mod.rs
+  |
+  |         F: MethodImplementation<Callee = T>,
+  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `ClassBuilder::add_method`
+  = note: this error originates in the macro `$crate::__declare_class_register_out` which comes from the expansion of the macro `declare_class` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `InteriorMutable: mutability::private::MutabilityIsMutable` is not satisfied
+ --> ui/declare_class_mut_self_not_mutable.rs
+  |
+  | / declare_class!(
+  | |     struct CustomObject;
+  | |
+  | |     unsafe impl ClassType for CustomObject {
+... |
+  | |     }
+  | | );
+  | | ^
+  | | |
+  | |_the trait `mutability::private::MutabilityIsMutable` is not implemented for `InteriorMutable`
+  |   required by a bound introduced by this call
+  |
+  = help: the following other types implement trait `mutability::private::MutabilityIsMutable`:
+            Mutable
+            MutableWithImmutableSuperclass<IS>
+  = note: required for `CustomObject` to implement `IsMutable`
+  = note: required for `extern "C" fn(&mut CustomObject, objc2::runtime::Sel)` to implement `MethodImplementation`
+note: required by a bound in `ClassBuilder::add_method`
+ --> $WORKSPACE/crates/objc2/src/declare/mod.rs
+  |
+  |         F: MethodImplementation<Callee = T>,
+  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `ClassBuilder::add_method`
+  = note: this error originates in the macro `$crate::__declare_class_register_out` which comes from the expansion of the macro `declare_class` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `InteriorMutable: mutability::private::MutabilityIsMutable` is not satisfied
+ --> ui/declare_class_mut_self_not_mutable.rs
+  |
+  | / declare_class!(
+  | |     struct CustomObject;
+  | |
+  | |     unsafe impl ClassType for CustomObject {
+... |
+  | |     }
+  | | );
+  | | ^
+  | | |
+  | |_the trait `mutability::private::MutabilityIsMutable` is not implemented for `InteriorMutable`
+  |   required by a bound introduced by this call
+  |
+  = help: the following other types implement trait `mutability::private::MutabilityIsMutable`:
+            Mutable
+            MutableWithImmutableSuperclass<IS>
+  = note: required for `CustomObject` to implement `IsMutable`
+  = note: required for `extern "C" fn(&mut CustomObject, objc2::runtime::Sel) -> __IdReturnValue` to implement `MethodImplementation`
+note: required by a bound in `ClassBuilder::add_method`
+ --> $WORKSPACE/crates/objc2/src/declare/mod.rs
+  |
+  |         F: MethodImplementation<Callee = T>,
+  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `ClassBuilder::add_method`
+  = note: this error originates in the macro `$crate::__declare_class_register_out` which comes from the expansion of the macro `declare_class` (in Nightly builds, run with -Z macro-backtrace for more info)