From 6a18fdb81b2e29147ec9913dd0c7c2a167204a4e Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Sep 2022 16:05:57 +0200 Subject: [PATCH 1/8] Test overriding class method --- objc2/src/message/mod.rs | 18 ++++++++++++++++++ objc2/src/test_utils.rs | 6 ++++++ 2 files changed, 24 insertions(+) diff --git a/objc2/src/message/mod.rs b/objc2/src/message/mod.rs index ca3b820d8..7753300a6 100644 --- a/objc2/src/message/mod.rs +++ b/objc2/src/message/mod.rs @@ -532,6 +532,24 @@ mod tests { } } + #[test] + #[cfg_attr( + feature = "gnustep-1-7", + ignore = "GNUStep deadlocks here for some reason" + )] + fn test_send_message_class_super() { + let cls = test_utils::custom_subclass(); + let superclass = test_utils::custom_class(); + unsafe { + let foo: u32 = msg_send![super(cls, superclass.metaclass()), classFoo]; + assert_eq!(foo, 7); + + // The subclass is overriden to return + 2 + let foo: u32 = msg_send![cls, classFoo]; + assert_eq!(foo, 9); + } + } + #[test] fn test_send_message_manuallydrop() { let obj = ManuallyDrop::new(test_utils::custom_object()); diff --git a/objc2/src/test_utils.rs b/objc2/src/test_utils.rs index b0edf6211..a05c99719 100644 --- a/objc2/src/test_utils.rs +++ b/objc2/src/test_utils.rs @@ -231,9 +231,15 @@ pub(crate) fn custom_subclass() -> &'static Class { foo + 2 } + extern "C" fn custom_subclass_class_method(_cls: &Class, _cmd: Sel) -> u32 { + 9 + } + unsafe { let get_foo: extern "C" fn(_, _) -> _ = custom_subclass_get_foo; builder.add_method(sel!(foo), get_foo); + let class_method: extern "C" fn(_, _) -> _ = custom_subclass_class_method; + builder.add_class_method(sel!(classFoo), class_method); } builder.register(); From 009c7932051e2bd61f47110dd4e00aa591024a5b Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 21 Sep 2022 00:16:43 +0200 Subject: [PATCH 2/8] Add `Class::class_method` --- objc2/CHANGELOG.md | 1 + objc2/src/runtime.rs | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md index a74e88b04..4efd29849 100644 --- a/objc2/CHANGELOG.md +++ b/objc2/CHANGELOG.md @@ -23,6 +23,7 @@ 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`. ### Changed * Allow other types than `&Class` as the receiver in `msg_send_id!` methods diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs index 49c58561c..666609d8c 100644 --- a/objc2/src/runtime.rs +++ b/objc2/src/runtime.rs @@ -379,7 +379,17 @@ impl Class { } } - // fn class_method(&self, sel: Sel) -> Option<&Method>; + /// Returns a specified class method for self, or [`None`] if self and + /// its superclasses do not contain a class method with the specified + /// selector. + /// + /// Same as `cls.metaclass().class_method()`. + pub fn class_method(&self, sel: Sel) -> Option<&Method> { + unsafe { + let method = ffi::class_getClassMethod(self.as_ptr(), sel.as_ptr()); + method.cast::().as_ref() + } + } /// Returns the ivar for a specified instance variable of self, or /// [`None`] if self has no ivar with the given name. @@ -881,7 +891,7 @@ mod tests { } #[test] - fn test_method() { + fn test_instance_method() { let cls = test_utils::custom_class(); let sel = Sel::register("foo"); let method = cls.instance_method(sel).unwrap(); @@ -892,8 +902,26 @@ mod tests { assert!(::ENCODING.equivalent_to_str(&method.return_type())); assert!(Sel::ENCODING.equivalent_to_str(&method.argument_type(1).unwrap())); - let methods = cls.instance_methods(); - assert!(methods.len() > 0); + assert!(cls.instance_methods().into_iter().any(|m| *m == method)); + } + } + + #[test] + fn test_class_method() { + let cls = test_utils::custom_class(); + let method = cls.class_method(sel!(classFoo)).unwrap(); + assert_eq!(method.name().name(), "classFoo"); + assert_eq!(method.arguments_count(), 2); + #[cfg(feature = "malloc")] + { + assert!(::ENCODING.equivalent_to_str(&method.return_type())); + assert!(Sel::ENCODING.equivalent_to_str(&method.argument_type(1).unwrap())); + + assert!(cls + .metaclass() + .instance_methods() + .into_iter() + .any(|m| *m == method)); } } From 2829f4ebdcb4324a1dffe0bc8ede8e9dfd573df4 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Sep 2022 16:07:59 +0200 Subject: [PATCH 3/8] Verify signature of overridden methods --- objc2/CHANGELOG.md | 2 + objc2/src/declare.rs | 114 +++++++++++++++++++++++++++++++++++++++++-- objc2/src/runtime.rs | 5 +- objc2/src/verify.rs | 15 +++--- 4 files changed, 121 insertions(+), 15 deletions(-) diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md index 4efd29849..81894c01b 100644 --- a/objc2/CHANGELOG.md +++ b/objc2/CHANGELOG.md @@ -30,6 +30,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). of the `new` family. * **BREAKING**: Changed the `Allocated` struct to be used as `Allocated` instead of `Id, O>`. +* Verify the message signature of overriden methods when declaring classes if + the `verify_message` feature is enabled. ## 0.3.0-beta.3 - 2022-09-01 diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs index 30b05087c..11f4e16c8 100644 --- a/objc2/src/declare.rs +++ b/objc2/src/declare.rs @@ -127,6 +127,8 @@ use crate::encode::{Encode, EncodeArguments, Encoding, RefEncode}; use crate::ffi; use crate::runtime::{Bool, Class, Imp, Object, Protocol, Sel}; use crate::sel; +#[cfg(feature = "verify_message")] +use crate::verify::verify_method_signature; use crate::Message; pub use ivar::{InnerIvarType, Ivar, IvarType}; @@ -284,6 +286,22 @@ impl ClassBuilder { self.cls.as_ptr().cast() } + #[cfg(feature = "verify_message")] + fn superclass(&self) -> Option<&Class> { + // SAFETY: Unsure if this is safe to do before the class is finalized, + // but since we only do it when `verify_message` is enabled, it should + // be fine. + let cls = unsafe { self.cls.as_ref() }; + cls.superclass() + } + + #[cfg(feature = "verify_message")] + fn name(&self) -> &str { + // SAFETY: Same as `superclass` + let cls = unsafe { self.cls.as_ref() }; + cls.name() + } + fn with_superclass(name: &str, superclass: Option<&Class>) -> Option { let name = CString::new(name).unwrap(); let super_ptr = superclass.map_or(ptr::null(), |c| c).cast(); @@ -325,10 +343,17 @@ impl ClassBuilder { /// Adds a method with the given name and implementation. /// + /// /// # Panics /// - /// Panics if the method wasn't sucessfully added or if the selector and - /// function take different numbers of arguments. + /// Panics if the method wasn't sucessfully added (e.g. a method with that + /// name already exists). + /// + /// May also panic if the method was detected to be invalid in some way; + /// for example with the `verify_message` feature enabled, if the method + /// is overriding another method, we verify that their encodings are + /// equal. + /// /// /// # Safety /// @@ -350,6 +375,24 @@ impl ClassBuilder { encs.len(), ); + // Verify that, if the method is present on the superclass, that the + // encoding is correct. + #[cfg(feature = "verify_message")] + { + if let Some(superclass) = self.superclass() { + if let Some(method) = superclass.instance_method(sel) { + if let Err(err) = verify_method_signature::(method) { + panic!( + "declared invalid method -[{} {:?}]: {}", + self.name(), + sel, + err + ) + } + } + } + } + let types = method_type_encoding(&F::Ret::ENCODING, encs); let success = Bool::from_raw(unsafe { ffi::class_addMethod( @@ -368,10 +411,17 @@ impl ClassBuilder { /// Adds a class method with the given name and implementation. /// + /// /// # Panics /// - /// Panics if the method wasn't sucessfully added or if the selector and - /// function take different numbers of arguments. + /// Panics if the method wasn't sucessfully added (e.g. a method with that + /// name already exists). + /// + /// May also panic if the method was detected to be invalid in some way; + /// for example with the `verify_message` feature enabled, if the method + /// is overriding another method, we verify that their encodings are + /// equal. + /// /// /// # Safety /// @@ -392,6 +442,24 @@ impl ClassBuilder { encs.len(), ); + // Verify that, if the method is present on the superclass, that the + // encoding is correct. + #[cfg(feature = "verify_message")] + { + if let Some(superclass) = self.superclass() { + if let Some(method) = superclass.class_method(sel) { + if let Err(err) = verify_method_signature::(method) { + panic!( + "declared invalid method +[{} {:?}]: {}", + self.name(), + sel, + err + ) + } + } + } + } + let types = method_type_encoding(&F::Ret::ENCODING, encs); let success = Bool::from_raw(unsafe { ffi::class_addMethod( @@ -617,6 +685,44 @@ mod tests { } } + #[test] + #[cfg_attr( + feature = "verify_message", + should_panic = "declared invalid method -[TestClassBuilderInvalidMethod foo]: expected return to have type code I, but found i" + )] + fn invalid_method() { + let cls = test_utils::custom_class(); + let builder = ClassBuilder::new("TestClassBuilderInvalidMethod", cls).unwrap(); + let mut builder = ManuallyDrop::new(builder); + + extern "C" fn foo(_this: &Object, _cmd: Sel) -> i32 { + 0 + } + + unsafe { + builder.add_method(sel!(foo), foo as extern "C" fn(_, _) -> _); + } + } + + #[test] + #[cfg_attr( + feature = "verify_message", + should_panic = "declared invalid method +[TestClassBuilderInvalidClassMethod classFoo]: expected return to have type code I, but found i" + )] + fn invalid_class_method() { + let cls = test_utils::custom_class(); + let builder = ClassBuilder::new("TestClassBuilderInvalidClassMethod", cls).unwrap(); + let mut builder = ManuallyDrop::new(builder); + + extern "C" fn class_foo(_cls: &Class, _cmd: Sel) -> i32 { + 0 + } + + unsafe { + builder.add_class_method(sel!(classFoo), class_foo as extern "C" fn(_, _) -> _); + } + } + #[test] #[should_panic = "Failed to add protocol NSObject"] fn duplicate_protocol() { diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs index 666609d8c..0e12e364c 100644 --- a/objc2/src/runtime.rs +++ b/objc2/src/runtime.rs @@ -22,7 +22,7 @@ use crate::ffi; #[cfg(feature = "malloc")] use crate::{ encode::{EncodeArguments, EncodeConvert}, - verify::verify_message_signature, + verify::{verify_method_signature, Inner}, VerificationError, }; @@ -514,7 +514,8 @@ impl Class { A: EncodeArguments, R: EncodeConvert, { - verify_message_signature::(self, sel) + let method = self.instance_method(sel).ok_or(Inner::MethodNotFound)?; + verify_method_signature::(method) } } diff --git a/objc2/src/verify.rs b/objc2/src/verify.rs index 6ad172572..897f24fe8 100644 --- a/objc2/src/verify.rs +++ b/objc2/src/verify.rs @@ -4,11 +4,11 @@ use malloc_buf::Malloc; use std::error::Error; use crate::encode::{Encode, EncodeArguments, EncodeConvert, Encoding}; -use crate::runtime::{Class, Object, Sel}; +use crate::runtime::{Method, Object, Sel}; /// Workaround for `Malloc` not implementing common traits #[derive(Debug)] -struct MallocEncoding(Malloc); +pub(crate) struct MallocEncoding(Malloc); // SAFETY: `char*` strings can safely be free'd on other threads. unsafe impl Send for MallocEncoding {} @@ -35,7 +35,7 @@ impl fmt::Display for MallocEncoding { } #[derive(Debug, PartialEq, Eq, Hash)] -enum Inner { +pub(crate) enum Inner { MethodNotFound, MismatchedReturn(MallocEncoding, Encoding), MismatchedArgumentsCount(usize, usize), @@ -80,6 +80,8 @@ impl fmt::Display for Inner { /// /// This implements [`Error`], and a description of the error can be retrieved /// using [`fmt::Display`]. +/// +/// [`Class::verify_sel`]: crate::Class::verify_sel #[derive(Debug, PartialEq, Eq, Hash)] pub struct VerificationError(Inner); @@ -98,16 +100,11 @@ impl fmt::Display for VerificationError { impl Error for VerificationError {} -pub(crate) fn verify_message_signature(cls: &Class, sel: Sel) -> Result<(), VerificationError> +pub(crate) fn verify_method_signature(method: &Method) -> Result<(), VerificationError> where A: EncodeArguments, R: EncodeConvert, { - let method = match cls.instance_method(sel) { - Some(method) => method, - None => return Err(Inner::MethodNotFound.into()), - }; - let actual = R::__Inner::ENCODING; let expected = method.return_type(); if !actual.equivalent_to_str(&*expected) { From 46e8faf79152131b04a455ba6519034ce8524abc Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 20 Sep 2022 22:26:04 +0200 Subject: [PATCH 4/8] Refactor class declaration a bit --- objc2/src/macros/declare_class.rs | 96 ++++++++++++++++++------------- 1 file changed, 57 insertions(+), 39 deletions(-) diff --git a/objc2/src/macros/declare_class.rs b/objc2/src/macros/declare_class.rs index f6f51d443..3917db2ed 100644 --- a/objc2/src/macros/declare_class.rs +++ b/objc2/src/macros/declare_class.rs @@ -1,11 +1,15 @@ #[doc(hidden)] #[macro_export] -macro_rules! __inner_declare_class { - {@rewrite_methods @($($output:tt)*)} => {}; +macro_rules! __declare_class_rewrite_methods { { - // Unsafe variant - @rewrite_methods - @($($output:tt)*) + @($($macro:tt)*) + @($($macro_arguments:tt)*) + } => {}; + + // Unsafe variant + { + @($($macro:tt)*) + @($($macro_arguments:tt)*) $(#[$($m:tt)*])* unsafe fn $name:ident($($args:tt)*) $(-> $ret:ty)? $body:block @@ -13,12 +17,12 @@ macro_rules! __inner_declare_class { $($rest:tt)* } => { $crate::__rewrite_self_arg! { - ($crate::__inner_declare_class) + ($($macro)*) ($($args)*) // Split the function into parts, and send the arguments down to // be used later on - @$($output)* + @($($macro_arguments)*) @($(#[$($m)*])*) @(unsafe extern "C") @($name) @@ -29,17 +33,18 @@ macro_rules! __inner_declare_class { // Will add @(args_rest) } - $crate::__inner_declare_class! { - @rewrite_methods - @($($output)*) + $crate::__declare_class_rewrite_methods! { + @($($macro)*) + @($($macro_arguments)*) $($rest)* } }; + + // Safe variant { - // Safe variant - @rewrite_methods - @($($output:tt)*) + @($($macro:tt)*) + @($($macro_arguments:tt)*) $(#[$($m:tt)*])* fn $name:ident($($args:tt)*) $(-> $ret:ty)? $body:block @@ -47,9 +52,10 @@ macro_rules! __inner_declare_class { $($rest:tt)* } => { $crate::__rewrite_self_arg! { - ($crate::__inner_declare_class) + ($($macro)*) ($($args)*) - @$($output)* + + @($($macro_arguments)*) @($(#[$($m)*])*) @(extern "C") @($name) @@ -59,16 +65,20 @@ macro_rules! __inner_declare_class { // Same as above } - $crate::__inner_declare_class! { - @rewrite_methods - @($($output)*) + $crate::__declare_class_rewrite_methods! { + @($($macro)*) + @($($macro_arguments)*) $($rest)* } }; +} +#[doc(hidden)] +#[macro_export] +macro_rules! __declare_class_method_out { { - @method_out + @() // No arguments needed @($(#[$($m:tt)*])*) @($($qualifiers:tt)*) @($name:ident) @@ -79,11 +89,11 @@ macro_rules! __inner_declare_class { @($($args_rest:tt)*) } => { $crate::__fn_args! { - ($crate::__inner_declare_class) + ($crate::__declare_class_method_out) ($($args_rest)*,) () () - @method_out_inner + @inner @($(#[$($m)*])*) @($($qualifiers)*) @($name) @@ -98,7 +108,7 @@ macro_rules! __inner_declare_class { // No return type { - @method_out_inner + @inner @($(#[$($m:tt)*])*) @($($qualifiers:tt)*) @($name:ident) @@ -118,9 +128,10 @@ macro_rules! __inner_declare_class { }) } }; + // With return type { - @method_out_inner + @inner @($(#[$($m:tt)*])*) @($($qualifiers:tt)*) @($name:ident) @@ -140,9 +151,14 @@ macro_rules! __inner_declare_class { }) } }; +} +#[doc(hidden)] +#[macro_export] +macro_rules! __declare_class_register_out { + // Class method { - @register_out($builder:ident) + @($builder:ident) @($(#[$($m:tt)*])*) @($($qualifiers:tt)*) @($name:ident) @@ -155,7 +171,7 @@ macro_rules! __inner_declare_class { $builder.add_class_method( $crate::__attribute_helper! { @extract_sel - ($crate::__inner_declare_class) + ($crate::__declare_class_register_out) ($(#[$($m)*])*) @call_sel }, @@ -164,8 +180,10 @@ macro_rules! __inner_declare_class { }, ); }; + + // Instance method { - @register_out($builder:ident) + @($builder:ident) @($(#[$($m:tt)*])*) @($($qualifiers:tt)*) @($name:ident) @@ -178,7 +196,7 @@ macro_rules! __inner_declare_class { $builder.add_method( $crate::__attribute_helper! { @extract_sel - ($crate::__inner_declare_class) + ($crate::__declare_class_register_out) ($(#[$($m)*])*) @call_sel }, @@ -721,9 +739,9 @@ macro_rules! __declare_class_methods { ) => { $(#[$m])* impl $for { - $crate::__inner_declare_class! { - @rewrite_methods - @(method_out) + $crate::__declare_class_rewrite_methods! { + @($crate::__declare_class_method_out) + @() $($methods)* } } @@ -746,9 +764,9 @@ macro_rules! __declare_class_methods { ) => { $(#[$m])* impl $for { - $crate::__inner_declare_class! { - @rewrite_methods - @(method_out) + $crate::__declare_class_rewrite_methods! { + @($crate::__declare_class_method_out) + @() $($methods)* } } @@ -783,9 +801,9 @@ macro_rules! __declare_class_methods { // SAFETY: Upheld by caller unsafe { - $crate::__inner_declare_class! { - @rewrite_methods - @(register_out($builder)) + $crate::__declare_class_rewrite_methods! { + @($crate::__declare_class_register_out) + @($builder) $($methods)* } @@ -809,9 +827,9 @@ macro_rules! __declare_class_methods { ) => { // SAFETY: Upheld by caller unsafe { - $crate::__inner_declare_class! { - @rewrite_methods - @(register_out($builder)) + $crate::__declare_class_rewrite_methods! { + @($crate::__declare_class_register_out) + @($builder) $($methods)* } From 2ca500fb2585c4ce1b6af96b7e6ca56ce6238dae Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Sep 2022 16:10:36 +0200 Subject: [PATCH 5/8] Document and test declare_class! panic cases --- objc2/src/declare.rs | 80 ++++++++++++++++++++++++++++++- objc2/src/macros/declare_class.rs | 12 +++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs index 11f4e16c8..e94f457c5 100644 --- a/objc2/src/declare.rs +++ b/objc2/src/declare.rs @@ -644,8 +644,9 @@ impl ProtocolBuilder { #[cfg(test)] mod tests { use super::*; - use crate::msg_send; + use crate::foundation::{NSObject, NSZone}; use crate::test_utils; + use crate::{declare_class, msg_send, ClassType}; #[test] fn test_classbuilder_duplicate() { @@ -790,4 +791,81 @@ mod tests { let result: u32 = unsafe { msg_send![cls, classFoo] }; assert_eq!(result, 7); } + + #[test] + #[should_panic = "could not create new class TestDeclareClassDuplicate. Perhaps a class with that name already exists?"] + fn test_declare_class_duplicate() { + declare_class!( + struct Custom1 {} + + unsafe impl ClassType for Custom1 { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassDuplicate"; + } + ); + + declare_class!( + struct Custom2 {} + + unsafe impl ClassType for Custom2 { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassDuplicate"; + } + ); + + let _cls = Custom1::class(); + // Should panic + let _cls = Custom2::class(); + } + + #[test] + #[should_panic = "could not find protocol NotAProtocolName"] + fn test_declare_class_protocol_not_found() { + declare_class!( + struct Custom {} + + unsafe impl ClassType for Custom { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassProtocolNotFound"; + } + + // Implementing this should work + unsafe impl Protocol for Custom { + #[sel(copyWithZone:)] + #[allow(unreachable_code)] + fn copy_with_zone(&self, _zone: *const NSZone) -> *mut Self { + unimplemented!() + } + } + + // But this should fail + unsafe impl Protocol for Custom {} + ); + + let _cls = Custom::class(); + } + + #[test] + #[cfg_attr( + feature = "verify_message", + should_panic = "declared invalid method -[TestDeclareClassInvalidMethod description]: expected return to have type code @, but found v" + )] + fn test_declare_class_invalid_method() { + declare_class!( + struct Custom {} + + unsafe impl ClassType for Custom { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassInvalidMethod"; + } + + unsafe impl Custom { + // Override `description` with a bad return type + #[sel(description)] + fn description(&self) {} + } + ); + + let _cls = Custom::class(); + } } diff --git a/objc2/src/macros/declare_class.rs b/objc2/src/macros/declare_class.rs index 3917db2ed..edb98bdc3 100644 --- a/objc2/src/macros/declare_class.rs +++ b/objc2/src/macros/declare_class.rs @@ -377,6 +377,16 @@ macro_rules! __fn_args { /// definition to make things easier to read. /// /// +/// # Panics +/// +/// The implemented `ClassType::class` method may panic in a few cases, such +/// as if: +/// - A class with the name specified with `const NAME` already exists. +/// - One of the +/// - The `verify_message` feature is enabled, and an overriden method's +/// signature is not equal to the superclass'. +/// +/// /// # Safety /// /// Using this macro requires writing a few `unsafe` markers: @@ -800,6 +810,7 @@ macro_rules! __declare_class_methods { ); // SAFETY: Upheld by caller + #[allow(unused_unsafe)] unsafe { $crate::__declare_class_rewrite_methods! { @($crate::__declare_class_register_out) @@ -826,6 +837,7 @@ macro_rules! __declare_class_methods { $($rest:tt)* ) => { // SAFETY: Upheld by caller + #[allow(unused_unsafe)] unsafe { $crate::__declare_class_rewrite_methods! { @($crate::__declare_class_register_out) From 3e0e03e076a81e70c8d3dbf7cefe37b3f5584a2e Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Sep 2022 21:49:51 +0200 Subject: [PATCH 6/8] Verify that protocols are implemented correctly in declare_class! --- objc2/CHANGELOG.md | 1 + objc2/src/__macro_helpers.rs | 140 +++++++++++++++++++++++++++++- objc2/src/declare.rs | 73 ++++++++++++++++ objc2/src/macros/declare_class.rs | 11 ++- objc2/src/runtime.rs | 84 +++++++++++++++++- 5 files changed, 303 insertions(+), 6 deletions(-) diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md index 81894c01b..eb3cc22b9 100644 --- a/objc2/CHANGELOG.md +++ b/objc2/CHANGELOG.md @@ -32,6 +32,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). instead of `Id, O>`. * Verify the message signature of overriden methods when declaring classes if the `verify_message` feature is enabled. +* Verify in `declare_class!` that protocols are implemented correctly. ## 0.3.0-beta.3 - 2022-09-01 diff --git a/objc2/src/__macro_helpers.rs b/objc2/src/__macro_helpers.rs index 2fe14ef01..675067052 100644 --- a/objc2/src/__macro_helpers.rs +++ b/objc2/src/__macro_helpers.rs @@ -1,6 +1,16 @@ +#[cfg(feature = "verify_message")] +use alloc::vec::Vec; +#[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::runtime::{Class, Object, Sel}; +#[cfg(feature = "verify_message")] +use crate::runtime::MethodDescription; +use crate::runtime::{Class, Object, Protocol, Sel}; use crate::{Message, MessageArguments, MessageReceiver}; pub use crate::cache::CachedClass; @@ -405,6 +415,134 @@ impl ModuleInfo { } } +impl ClassBuilder { + #[doc(hidden)] + #[cfg(feature = "verify_message")] + pub fn __add_protocol_methods<'a, 'b>( + &'a mut self, + protocol: &'b Protocol, + ) -> ClassProtocolMethodsBuilder<'a, 'b> { + self.add_protocol(protocol); + ClassProtocolMethodsBuilder { + builder: self, + protocol, + required_instance_methods: protocol.method_descriptions(true), + optional_instance_methods: protocol.method_descriptions(false), + registered_instance_methods: HashSet::new(), + required_class_methods: protocol.class_method_descriptions(true), + optional_class_methods: protocol.class_method_descriptions(false), + registered_class_methods: HashSet::new(), + } + } + + #[doc(hidden)] + #[cfg(not(feature = "verify_message"))] + #[inline] + pub fn __add_protocol_methods(&mut self, protocol: &Protocol) -> &mut Self { + self.add_protocol(protocol); + self + } +} + +/// Helper for ensuring that: +/// - Only methods on the protocol are overriden. +/// - TODO: The methods have the correct signature. +/// - All required methods are overridden. +#[cfg(feature = "verify_message")] +pub struct ClassProtocolMethodsBuilder<'a, 'b> { + builder: &'a mut ClassBuilder, + protocol: &'b Protocol, + required_instance_methods: Vec, + optional_instance_methods: Vec, + registered_instance_methods: HashSet, + required_class_methods: Vec, + optional_class_methods: Vec, + registered_class_methods: HashSet, +} + +#[cfg(feature = "verify_message")] +impl ClassProtocolMethodsBuilder<'_, '_> { + #[inline] + pub unsafe fn add_method(&mut self, sel: Sel, func: F) + where + T: Message + ?Sized, + F: MethodImplementation, + { + let _types = self + .required_instance_methods + .iter() + .chain(&self.optional_instance_methods) + .find(|desc| desc.sel == sel) + .map(|desc| desc.types) + .unwrap_or_else(|| { + panic!( + "failed overriding protocol method -[{} {:?}]: method not found", + self.protocol.name(), + sel + ) + }); + + // SAFETY: Checked by caller + unsafe { self.builder.add_method(sel, func) }; + + if !self.registered_instance_methods.insert(sel) { + unreachable!("already added") + } + } + + #[inline] + pub unsafe fn add_class_method(&mut self, sel: Sel, func: F) + where + F: MethodImplementation, + { + let _types = self + .required_class_methods + .iter() + .chain(&self.optional_class_methods) + .find(|desc| desc.sel == sel) + .map(|desc| desc.types) + .unwrap_or_else(|| { + panic!( + "failed overriding protocol method +[{} {:?}]: method not found", + self.protocol.name(), + sel + ) + }); + + // SAFETY: Checked by caller + unsafe { self.builder.add_class_method(sel, func) }; + + if !self.registered_class_methods.insert(sel) { + unreachable!("already added") + } + } +} + +#[cfg(feature = "verify_message")] +impl Drop for ClassProtocolMethodsBuilder<'_, '_> { + fn drop(&mut self) { + for desc in &self.required_instance_methods { + if !self.registered_instance_methods.contains(&desc.sel) { + panic!( + "must implement required protocol method -[{} {:?}]", + self.protocol.name(), + desc.sel + ) + } + } + + for desc in &self.required_class_methods { + if !self.registered_class_methods.contains(&desc.sel) { + panic!( + "must implement required protocol method +[{} {:?}]", + self.protocol.name(), + desc.sel + ) + } + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs index e94f457c5..cbaad73cc 100644 --- a/objc2/src/declare.rs +++ b/objc2/src/declare.rs @@ -868,4 +868,77 @@ mod tests { let _cls = Custom::class(); } + + #[test] + #[cfg_attr( + feature = "verify_message", + should_panic = "must implement required protocol method -[NSCopying copyWithZone:]" + )] + fn test_declare_class_missing_protocol_method() { + declare_class!( + struct Custom {} + + unsafe impl ClassType for Custom { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassMissingProtocolMethod"; + } + + unsafe impl Protocol for Custom { + // Missing required method + } + ); + + let _cls = Custom::class(); + } + + #[test] + // #[cfg_attr(feature = "verify_message", should_panic = "...")] + fn test_declare_class_invalid_protocol_method() { + declare_class!( + struct Custom {} + + unsafe impl ClassType for Custom { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassInvalidProtocolMethod"; + } + + unsafe impl Protocol for Custom { + // Override with a bad return type + #[sel(copyWithZone:)] + fn copy_with_zone(&self, _zone: *const NSZone) -> u8 { + 42 + } + } + ); + + let _cls = Custom::class(); + } + + #[test] + #[cfg(feature = "verify_message")] + #[should_panic = "failed overriding protocol method -[NSCopying someOtherMethod]: method not found"] + fn test_declare_class_extra_protocol_method() { + declare_class!( + struct Custom {} + + unsafe impl ClassType for Custom { + type Super = NSObject; + const NAME: &'static str = "TestDeclareClassExtraProtocolMethod"; + } + + unsafe impl Protocol for Custom { + #[sel(copyWithZone:)] + #[allow(unreachable_code)] + fn copy_with_zone(&self, _zone: *const NSZone) -> *mut Self { + unimplemented!() + } + + // This doesn't exist on the protocol + #[sel(someOtherMethod)] + fn some_other_method(&self) {} + } + ); + + let _cls = Custom::class(); + } } diff --git a/objc2/src/macros/declare_class.rs b/objc2/src/macros/declare_class.rs index edb98bdc3..3720f4155 100644 --- a/objc2/src/macros/declare_class.rs +++ b/objc2/src/macros/declare_class.rs @@ -385,6 +385,8 @@ macro_rules! __fn_args { /// - One of the /// - The `verify_message` feature is enabled, and an overriden method's /// signature is not equal to the superclass'. +/// - The `verify_message` feature is enabled, and the required protocol +/// methods are not implemented. /// /// /// # Safety @@ -800,7 +802,8 @@ macro_rules! __declare_class_methods { $($rest:tt)* ) => { // Implement protocol - $builder.add_protocol( + #[allow(unused_mut)] + let mut protocol_builder = $builder.__add_protocol_methods( $crate::runtime::Protocol::get(stringify!($protocol)).unwrap_or_else(|| { $crate::__macro_helpers::panic!( "could not find protocol {}", @@ -814,12 +817,16 @@ macro_rules! __declare_class_methods { unsafe { $crate::__declare_class_rewrite_methods! { @($crate::__declare_class_register_out) - @($builder) + @(protocol_builder) $($methods)* } } + // Finished declaring protocol; get error message if any + #[allow(clippy::drop_ref)] + drop(protocol_builder); + $crate::__declare_class_methods!( @register_out($builder) $($rest)* diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs index 0e12e364c..91510cca5 100644 --- a/objc2/src/runtime.rs +++ b/objc2/src/runtime.rs @@ -3,6 +3,8 @@ //! For more information on foreign functions, see Apple's documentation: //! +#[cfg(feature = "malloc")] +use alloc::vec::Vec; #[cfg(doc)] use core::cell::UnsafeCell; use core::fmt; @@ -252,6 +254,28 @@ unsafe impl Send for Ivar {} impl UnwindSafe for Ivar {} impl RefUnwindSafe for Ivar {} +#[cfg_attr(not(feature = "malloc"), allow(dead_code))] +#[derive(Debug, PartialEq, Eq, Hash)] +pub(crate) struct MethodDescription { + pub(crate) sel: Sel, + pub(crate) types: &'static str, +} + +impl MethodDescription { + #[cfg_attr(not(feature = "malloc"), allow(dead_code))] + pub(crate) unsafe fn from_raw(raw: ffi::objc_method_description) -> Option { + // SAFETY: Sel::from_ptr checks for NULL, rest is checked by caller. + let sel = unsafe { Sel::from_ptr(raw.name) }?; + if raw.types.is_null() { + return None; + } + // SAFETY: We've checked that the pointer is not NULL, rest is checked + // by caller. + let types = unsafe { CStr::from_ptr(raw.types) }.to_str().unwrap(); + Some(Self { sel, types }) + } +} + impl Method { pub(crate) fn as_ptr(&self) -> *const ffi::objc_method { let ptr: *const Self = self; @@ -590,6 +614,38 @@ impl Protocol { let name = unsafe { CStr::from_ptr(ffi::protocol_getName(self.as_ptr())) }; str::from_utf8(name.to_bytes()).unwrap() } + + #[cfg(feature = "malloc")] + fn method_descriptions_inner(&self, required: bool, instance: bool) -> Vec { + let mut count: c_uint = 0; + let descriptions = unsafe { + ffi::protocol_copyMethodDescriptionList( + self.as_ptr(), + Bool::new(required).as_raw(), + Bool::new(instance).as_raw(), + &mut count, + ) + }; + let descriptions = unsafe { Malloc::from_array(descriptions, count as usize) }; + descriptions + .iter() + .map(|desc| { + unsafe { MethodDescription::from_raw(*desc) }.expect("invalid method description") + }) + .collect() + } + + #[cfg(feature = "malloc")] + #[allow(dead_code)] + pub(crate) fn method_descriptions(&self, required: bool) -> Vec { + self.method_descriptions_inner(required, true) + } + + #[cfg(feature = "malloc")] + #[allow(dead_code)] + pub(crate) fn class_method_descriptions(&self, required: bool) -> Vec { + self.method_descriptions_inner(required, false) + } } impl PartialEq for Protocol { @@ -844,9 +900,8 @@ mod tests { use alloc::format; use alloc::string::ToString; - use super::{Bool, Class, Imp, Ivar, Method, Object, Protocol, Sel}; + use super::*; use crate::test_utils; - use crate::Encode; use crate::{msg_send, sel}; #[test] @@ -969,8 +1024,31 @@ mod tests { assert_eq!(proto.name(), "CustomProtocol"); let class = test_utils::custom_class(); assert!(class.conforms_to(proto)); + #[cfg(feature = "malloc")] - assert!(class.adopted_protocols().len() > 0); + { + // The selectors are broken somehow on GNUStep < 2.0 + if cfg!(any(not(feature = "gnustep-1-7"), feature = "gnustep-2-0")) { + let desc = MethodDescription { + sel: sel!(setBar:), + types: "v@:i", + }; + assert_eq!(&proto.method_descriptions(true), &[desc]); + let desc = MethodDescription { + sel: sel!(getName), + types: "*@:", + }; + assert_eq!(&proto.method_descriptions(false), &[desc]); + let desc = MethodDescription { + sel: sel!(addNumber:toNumber:), + types: "i@:ii", + }; + assert_eq!(&proto.class_method_descriptions(true), &[desc]); + } + assert_eq!(&proto.class_method_descriptions(false), &[]); + + assert!(class.adopted_protocols().iter().any(|p| *p == proto)); + } } #[test] From 0413621b4047c3a1cb413af2ed54b54ba0155ea6 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Sep 2022 19:38:02 +0200 Subject: [PATCH 7/8] Fix dropping ClassBuilder on GNUStep --- objc2/src/declare.rs | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs index cbaad73cc..ecec66dae 100644 --- a/objc2/src/declare.rs +++ b/objc2/src/declare.rs @@ -542,6 +542,17 @@ impl ClassBuilder { impl Drop for ClassBuilder { fn drop(&mut self) { + // Disposing un-registered classes doesn't work properly on GNUStep, + // so we register the class before disposing it. + // + // Doing it this way is _technically_ a race-condition, since other + // code could read e.g. `Class::classes()` and then pick the class + // before it got disposed - but let's not worry about that for now. + #[cfg(feature = "gnustep-1-7")] + unsafe { + ffi::objc_registerClassPair(self.as_mut_ptr()); + } + unsafe { ffi::objc_disposeClassPair(self.as_mut_ptr()) } } } @@ -661,9 +672,7 @@ mod tests { #[should_panic = "Failed to add ivar xyz"] fn duplicate_ivar() { let cls = test_utils::custom_class(); - let builder = ClassBuilder::new("TestClassBuilderDuplicateIvar", cls).unwrap(); - // ManuallyDrop to work around GNUStep issue - let mut builder = ManuallyDrop::new(builder); + let mut builder = ClassBuilder::new("TestClassBuilderDuplicateIvar", cls).unwrap(); builder.add_ivar::("xyz"); // Should panic: @@ -674,8 +683,7 @@ mod tests { #[should_panic = "Failed to add method xyz"] fn duplicate_method() { let cls = test_utils::custom_class(); - let builder = ClassBuilder::new("TestClassBuilderDuplicateMethod", cls).unwrap(); - let mut builder = ManuallyDrop::new(builder); + let mut builder = ClassBuilder::new("TestClassBuilderDuplicateMethod", cls).unwrap(); extern "C" fn xyz(_this: &Object, _cmd: Sel) {} @@ -693,8 +701,7 @@ mod tests { )] fn invalid_method() { let cls = test_utils::custom_class(); - let builder = ClassBuilder::new("TestClassBuilderInvalidMethod", cls).unwrap(); - let mut builder = ManuallyDrop::new(builder); + let mut builder = ClassBuilder::new("TestClassBuilderInvalidMethod", cls).unwrap(); extern "C" fn foo(_this: &Object, _cmd: Sel) -> i32 { 0 @@ -712,8 +719,7 @@ mod tests { )] fn invalid_class_method() { let cls = test_utils::custom_class(); - let builder = ClassBuilder::new("TestClassBuilderInvalidClassMethod", cls).unwrap(); - let mut builder = ManuallyDrop::new(builder); + let mut builder = ClassBuilder::new("TestClassBuilderInvalidClassMethod", cls).unwrap(); extern "C" fn class_foo(_cls: &Class, _cmd: Sel) -> i32 { 0 @@ -728,8 +734,7 @@ mod tests { #[should_panic = "Failed to add protocol NSObject"] fn duplicate_protocol() { let cls = test_utils::custom_class(); - let builder = ClassBuilder::new("TestClassBuilderDuplicateProtocol", cls).unwrap(); - let mut builder = ManuallyDrop::new(builder); + let mut builder = ClassBuilder::new("TestClassBuilderDuplicateProtocol", cls).unwrap(); let protocol = Protocol::get("NSObject").unwrap(); @@ -739,10 +744,6 @@ mod tests { } #[test] - #[cfg_attr( - feature = "gnustep-1-7", - ignore = "Dropping ClassBuilder has weird threading side effects on GNUStep" - )] fn test_classbuilder_drop() { let cls = test_utils::custom_class(); let builder = ClassBuilder::new("TestClassBuilderDrop", cls).unwrap(); From 0fa47bc019059d265ac2cd4e7a0cd98e4bc73faf Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Sep 2022 19:59:06 +0200 Subject: [PATCH 8/8] Fix PartialEq and Hash impl of Sel on GNUStep --- objc2/src/runtime.rs | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs index 91510cca5..68642b14b 100644 --- a/objc2/src/runtime.rs +++ b/objc2/src/runtime.rs @@ -59,8 +59,7 @@ pub const NO: ffi::BOOL = ffi::NO; /// [`sel!`]: crate::sel /// [interned string]: https://en.wikipedia.org/wiki/String_interning #[repr(transparent)] -// ffi::sel_isEqual is just pointer comparison, so we just generate PartialEq -#[derive(Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Copy, Clone)] #[doc(alias = "SEL")] pub struct Sel { ptr: NonNull, @@ -196,6 +195,33 @@ impl Sel { } } +// `ffi::sel_isEqual` is just pointer comparison on Apple (the documentation +// explicitly notes this); so as an optimization, let's do that as well! +#[cfg(feature = "apple")] +standard_pointer_impls!(Sel); + +// GNUStep implements "typed" selectors, which means their pointer values +// sometimes differ; so let's use the runtime-provided `sel_isEqual`. +#[cfg(not(feature = "apple"))] +impl PartialEq for Sel { + #[inline] + fn eq(&self, other: &Self) -> bool { + unsafe { Bool::from_raw(ffi::sel_isEqual(self.as_ptr(), other.as_ptr())).as_bool() } + } +} + +#[cfg(not(feature = "apple"))] +impl Eq for Sel {} + +#[cfg(not(feature = "apple"))] +impl hash::Hash for Sel { + #[inline] + fn hash(&self, state: &mut H) { + // Note: We hash the name instead of the pointer + self.name().hash(state) + } +} + // SAFETY: `Sel` is FFI compatible, and the encoding is of course `Sel`. unsafe impl Encode for Sel { const ENCODING: Encoding = Encoding::Sel;