diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dceeb357d..76d88b3ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -155,7 +155,7 @@ jobs: # Use --no-fail-fast, except with dinghy TESTARGS: ${{ matrix.dinghy && ' ' || '--no-fail-fast' }} ${{ matrix.test-args }} SOME_FEATURES: ${{ matrix.features || 'malloc,block,exception,foundation' }} - FEATURES: ${{ matrix.features || 'malloc,block,exception,foundation,catch-all,verify_message,uuid' }} + FEATURES: ${{ matrix.features || 'malloc,block,exception,foundation,catch-all,verify,uuid' }} UNSTABLE_FEATURES: ${{ matrix.unstable-features || 'unstable-autoreleasesafe,unstable-c-unwind' }} CMD: cargo diff --git a/crates/objc2-encode/src/encoding_box.rs b/crates/objc2-encode/src/encoding_box.rs index c338d6017..56883bbac 100644 --- a/crates/objc2-encode/src/encoding_box.rs +++ b/crates/objc2-encode/src/encoding_box.rs @@ -105,6 +105,27 @@ impl EncodingBox { Encoding::ULongLong => Self::ULongLong, _ => unreachable!(), }; + + /// Parse and comsume an encoding from the start of a string. + /// + /// This is can be used to parse concatenated encodings, such as those + /// returned by `method_getTypeEncoding`. + /// + /// [`from_str`][Self::from_str] is simpler, use that instead if you can. + pub fn from_start_of_str(s: &mut &str) -> Result { + let mut parser = Parser::new(s); + parser.strip_leading_qualifiers(); + + match parser.parse_encoding() { + Err(err) => Err(ParseError::new(parser, err)), + Ok(encoding) => { + let remaining = parser.remaining(); + *s = remaining; + + Ok(encoding) + } + } + } } /// Same formatting as [`Encoding`]'s `Display` implementation. @@ -213,4 +234,19 @@ mod tests { assert_eq!(expected, actual); assert_eq!(expected.to_string(), "AA{a}"); } + + #[test] + fn parse_part_of_string() { + let mut s = "{a}c"; + + let expected = EncodingBox::Struct("a".into(), None); + let actual = EncodingBox::from_start_of_str(&mut s).unwrap(); + assert_eq!(expected, actual); + + let expected = EncodingBox::Char; + let actual = EncodingBox::from_start_of_str(&mut s).unwrap(); + assert_eq!(expected, actual); + + assert_eq!(s, ""); + } } diff --git a/crates/objc2-encode/src/parse.rs b/crates/objc2-encode/src/parse.rs index 44a8363d4..fce23665e 100644 --- a/crates/objc2-encode/src/parse.rs +++ b/crates/objc2-encode/src/parse.rs @@ -117,6 +117,10 @@ impl<'a> Parser<'a> { } } + pub(crate) fn remaining(&self) -> &'a str { + &self.data[self.split_point..] + } + fn peek(&self) -> Result { self.try_peek().ok_or(ErrorKind::UnexpectedEnd) } diff --git a/crates/objc2/CHANGELOG.md b/crates/objc2/CHANGELOG.md index a44eb8a0b..08720eec0 100644 --- a/crates/objc2/CHANGELOG.md +++ b/crates/objc2/CHANGELOG.md @@ -31,6 +31,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). in methods declared with `extern_methods!`, and let that be the `NSError**` parameter. * Added `#[method_id(...)]` attribute to `extern_methods!`. +* Added `"verify"` feature as a replacement for the `"verify_message"` + feature. ### Changed * Allow other types than `&Class` as the receiver in `msg_send_id!` methods @@ -38,19 +40,26 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). * **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. + the `verify` feature is enabled. * Verify in `declare_class!` that protocols are implemented correctly. * **BREAKING**: Changed the name of the attribute macro in `extern_methods` from `#[sel(...)]` to `#[method(...)]`. * **BREAKING**: Changed `extern_methods!` and `declare_class!` such that associated functions whoose first parameter is called `this`, is treated as instance methods instead of class methods. +* **BREAKING**: Message verification is now enabled by default. Your message + sends might panic with `debug_assertions` enabled if they are detected to + be invalid. Test your code to see if that is the case! ### Fixed * Fixed duplicate selector extraction in `extern_methods!`. * Fixed using `deprecated` attributes in `declare_class!`. * Fixed `cfg` attributes on methods and implementations in `declare_class!`. +### Removed +* **BREAKING**: Removed `"verify_message"` feature. It has been mostly + replaced by `debug_assertions` and the `"verify"` feature. + ## 0.3.0-beta.3 - 2022-09-01 diff --git a/crates/objc2/Cargo.toml b/crates/objc2/Cargo.toml index 8cfd62cc2..58eed05ab 100644 --- a/crates/objc2/Cargo.toml +++ b/crates/objc2/Cargo.toml @@ -34,9 +34,8 @@ exception = ["objc-sys/unstable-exception"] # Wrap every `objc2::msg_send` call in a `@try/@catch` block catch-all = ["exception"] -# Verify type encodings on every message send -# Only intended to be used while debugging! -verify_message = ["malloc"] # TODO: Remove malloc feature here +# Enable all verification steps when debug assertions are enabled. +verify = ["malloc"] # Expose features that require linking to `libc::free`. # diff --git a/crates/objc2/src/__macro_helpers.rs b/crates/objc2/src/__macro_helpers.rs index 3de982292..44aaf7464 100644 --- a/crates/objc2/src/__macro_helpers.rs +++ b/crates/objc2/src/__macro_helpers.rs @@ -1,16 +1,16 @@ -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] use alloc::vec::Vec; use core::ptr; -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] use std::collections::HashSet; use crate::declare::ClassBuilder; -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] use crate::declare::MethodImplementation; use crate::encode::Encode; use crate::message::__TupleExtender; use crate::rc::{Allocated, Id, Ownership, Shared}; -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] use crate::runtime::MethodDescription; use crate::runtime::{Class, Object, Protocol, Sel}; use crate::{Message, MessageArguments, MessageReceiver}; @@ -478,7 +478,7 @@ impl ModuleInfo { impl ClassBuilder { #[doc(hidden)] - #[cfg(feature = "verify_message")] + #[cfg(all(debug_assertions, feature = "verify"))] pub fn __add_protocol_methods<'a, 'b>( &'a mut self, protocol: &'b Protocol, @@ -497,7 +497,7 @@ impl ClassBuilder { } #[doc(hidden)] - #[cfg(not(feature = "verify_message"))] + #[cfg(not(all(debug_assertions, feature = "verify")))] #[inline] pub fn __add_protocol_methods(&mut self, protocol: &Protocol) -> &mut Self { self.add_protocol(protocol); @@ -509,7 +509,7 @@ impl ClassBuilder { /// - Only methods on the protocol are overriden. /// - TODO: The methods have the correct signature. /// - All required methods are overridden. -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] pub struct ClassProtocolMethodsBuilder<'a, 'b> { builder: &'a mut ClassBuilder, protocol: &'b Protocol, @@ -521,7 +521,7 @@ pub struct ClassProtocolMethodsBuilder<'a, 'b> { registered_class_methods: HashSet, } -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] impl ClassProtocolMethodsBuilder<'_, '_> { #[inline] pub unsafe fn add_method(&mut self, sel: Sel, func: F) @@ -579,7 +579,7 @@ impl ClassProtocolMethodsBuilder<'_, '_> { } } -#[cfg(feature = "verify_message")] +#[cfg(all(debug_assertions, feature = "verify"))] impl Drop for ClassProtocolMethodsBuilder<'_, '_> { fn drop(&mut self) { for desc in &self.required_instance_methods { @@ -778,7 +778,7 @@ mod tests { #[test] #[should_panic = "unexpected NULL newMethodOnInstance; receiver was NULL"] - #[cfg(not(feature = "verify_message"))] // Does NULL receiver checks + #[cfg(not(debug_assertions))] // Does NULL receiver checks fn test_new_any_with_null_receiver() { let obj: *const NSObject = ptr::null(); let _obj: Id = unsafe { msg_send_id![obj, newMethodOnInstance] }; @@ -801,7 +801,7 @@ mod tests { #[test] #[should_panic = "failed allocating object"] - #[cfg(not(feature = "verify_message"))] // Does NULL receiver checks + #[cfg(not(debug_assertions))] // Does NULL receiver checks fn test_init_with_null_receiver() { let obj: Option> = None; let _obj: Id = unsafe { msg_send_id![obj, init] }; @@ -823,7 +823,7 @@ mod tests { #[test] #[should_panic = "unexpected NULL description; receiver was NULL"] - #[cfg(not(feature = "verify_message"))] // Does NULL receiver checks + #[cfg(not(debug_assertions))] // Does NULL receiver checks fn test_normal_with_null_receiver() { let obj: *const NSObject = ptr::null(); let _obj: Id = unsafe { msg_send_id![obj, description] }; diff --git a/crates/objc2/src/declare.rs b/crates/objc2/src/declare.rs index 76497bc85..23cc6340f 100644 --- a/crates/objc2/src/declare.rs +++ b/crates/objc2/src/declare.rs @@ -129,8 +129,6 @@ 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}; @@ -260,9 +258,9 @@ fn log2_align_of() -> u8 { /// before registering it. #[derive(Debug)] pub struct ClassBuilder { - // Note: Don't ever construct a &mut Class, since it is possible to get - // this pointer using `Class::classes`! - cls: NonNull, + // Note: Don't ever construct a &mut objc_class, since it is possible to + // get this pointer using `Class::classes`! + cls: NonNull, } #[doc(hidden)] @@ -285,30 +283,27 @@ unsafe impl Sync for ClassBuilder {} impl ClassBuilder { fn as_mut_ptr(&mut self) -> *mut ffi::objc_class { - self.cls.as_ptr().cast() + self.cls.as_ptr() } - #[cfg(feature = "verify_message")] + #[allow(unused)] 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() + // SAFETY: Though the class is not finalized, `class_getSuperclass` is + // still safe to call. + unsafe { Class::superclass_raw(self.cls.as_ptr()) } } - #[cfg(feature = "verify_message")] + #[allow(unused)] fn name(&self) -> &str { // SAFETY: Same as `superclass` - let cls = unsafe { self.cls.as_ref() }; - cls.name() + unsafe { Class::name_raw(self.cls.as_ptr()) } } 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(); let cls = unsafe { ffi::objc_allocateClassPair(super_ptr, name.as_ptr(), 0) }; - NonNull::new(cls.cast()).map(|cls| Self { cls }) + NonNull::new(cls).map(|cls| Self { cls }) } /// Constructs a [`ClassBuilder`] with the given name and superclass. @@ -352,9 +347,8 @@ impl ClassBuilder { /// 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. + /// for example if `debug_assertions` are enabled and the method is + /// overriding another method, we verify that their encodings are equal. /// /// /// # Safety @@ -379,18 +373,12 @@ impl ClassBuilder { // 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 - ) - } + #[cfg(debug_assertions)] + if let Some(superclass) = self.superclass() { + if let Some(method) = superclass.instance_method(sel) { + if let Err(err) = crate::verify::verify_method_signature::(method) + { + panic!("declared invalid method -[{} {sel:?}]: {err}", self.name()) } } } @@ -416,13 +404,7 @@ impl ClassBuilder { /// /// # Panics /// - /// 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. + /// Panics in the same cases as [`add_method`][Self::add_method]. /// /// /// # Safety @@ -446,18 +428,12 @@ impl ClassBuilder { // 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 - ) - } + #[cfg(debug_assertions)] + if let Some(superclass) = self.superclass() { + if let Some(method) = superclass.class_method(sel) { + if let Err(err) = crate::verify::verify_method_signature::(method) + { + panic!("declared invalid method +[{} {sel:?}]: {err}", self.name()) } } } @@ -538,7 +514,7 @@ impl ClassBuilder { // Forget self, otherwise the class will be disposed in drop let mut this = ManuallyDrop::new(self); unsafe { ffi::objc_registerClassPair(this.as_mut_ptr()) }; - unsafe { this.cls.as_ref() } + unsafe { this.cls.cast::().as_ref() } } } @@ -698,8 +674,8 @@ 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" + debug_assertions, + 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(); @@ -716,8 +692,8 @@ mod tests { #[test] #[cfg_attr( - feature = "verify_message", - should_panic = "declared invalid method +[TestClassBuilderInvalidClassMethod classFoo]: expected return to have type code I, but found i" + debug_assertions, + 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(); @@ -768,10 +744,7 @@ mod tests { fn test_in_all_classes() { fn is_present(cls: *const Class) -> bool { // Check whether the class is present in Class::classes() - Class::classes() - .into_iter() - .find(|&item| ptr::eq(cls, *item)) - .is_some() + Class::classes().iter().any(|item| ptr::eq(cls, *item)) } let superclass = test_utils::custom_class(); @@ -779,9 +752,9 @@ mod tests { if cfg!(all(feature = "apple", target_arch = "x86_64")) { // It is IMO a bug that it is present here! - assert!(is_present(builder.cls.as_ptr())); + assert!(is_present(builder.cls.as_ptr().cast())); } else { - assert!(!is_present(builder.cls.as_ptr())); + assert!(!is_present(builder.cls.as_ptr().cast())); } let cls = builder.register(); @@ -850,8 +823,8 @@ mod tests { #[test] #[cfg_attr( - feature = "verify_message", - should_panic = "declared invalid method -[TestDeclareClassInvalidMethod description]: expected return to have type code @, but found v" + debug_assertions, + should_panic = "declared invalid method -[TestDeclareClassInvalidMethod description]: expected return to have type code '@', but found 'v'" )] fn test_declare_class_invalid_method() { declare_class!( @@ -874,7 +847,7 @@ mod tests { #[test] #[cfg_attr( - feature = "verify_message", + all(debug_assertions, feature = "verify"), should_panic = "must implement required protocol method -[NSCopying copyWithZone:]" )] fn test_declare_class_missing_protocol_method() { @@ -895,7 +868,7 @@ mod tests { } #[test] - // #[cfg_attr(feature = "verify_message", should_panic = "...")] + // #[cfg_attr(all(debug_assertions, feature = "verify"), should_panic = "...")] fn test_declare_class_invalid_protocol_method() { declare_class!( struct Custom {} @@ -918,8 +891,10 @@ mod tests { } #[test] - #[cfg(feature = "verify_message")] - #[should_panic = "failed overriding protocol method -[NSCopying someOtherMethod]: method not found"] + #[cfg_attr( + all(debug_assertions, feature = "verify"), + should_panic = "failed overriding protocol method -[NSCopying someOtherMethod]: method not found" + )] fn test_declare_class_extra_protocol_method() { declare_class!( struct Custom {} diff --git a/crates/objc2/src/lib.rs b/crates/objc2/src/lib.rs index 1a67cb5a8..bd0ad3334 100644 --- a/crates/objc2/src/lib.rs +++ b/crates/objc2/src/lib.rs @@ -103,22 +103,15 @@ //! (some Rust types have the same Objective-C encoding, but are not //! equivalent), but it gets us much closer to it! //! -//! To use this functionality, enable the `"verify_message"` cargo feature -//! while debugging. With this feature enabled, encodings are checked every -//! time you send a message, and the message send will panic if they are not +//! When `debug_assertions` are enabled we check the encoding every time you +//! send a message, and the message send will panic if they are not //! equivalent. //! //! To take the example above, if we changed the `hash` method's return type -//! as in the following example, it panics when the feature is enabled: +//! as in the following example, it'll panic if debug assertions are enabled: //! -#![cfg_attr( - all(feature = "apple", feature = "verify_message"), - doc = "```should_panic" -)] -#![cfg_attr( - not(all(feature = "apple", feature = "verify_message")), - doc = "```no_run" -)] +#![cfg_attr(all(feature = "apple", debug_assertions), doc = "```should_panic")] +#![cfg_attr(not(all(feature = "apple", debug_assertions)), doc = "```no_run")] //! # use objc2::{class, msg_send, msg_send_id}; //! # use objc2::rc::{Id, Owned}; //! # use objc2::runtime::Object; @@ -128,8 +121,13 @@ //! # //! // Wrong return type - this is UB! //! let hash1: f32 = unsafe { msg_send![&obj1, hash] }; +//! # +//! # panic!("does not panic in release mode for some reason, so for testing we make it!"); //! ``` //! +//! This library contains further such debug checks, most of which are enabled +//! by default. To enable all of them, use the `"verify"` cargo feature. +//! //! [`objc2-encode`]: objc2_encode //! [`Box`]: std::boxed::Box //! @@ -199,7 +197,6 @@ pub use objc2_encode::{Encode, EncodeArguments, Encoding, RefEncode}; pub use crate::class_type::ClassType; pub use crate::message::{Message, MessageArguments, MessageReceiver}; -#[cfg(feature = "malloc")] pub use crate::verify::VerificationError; #[cfg(feature = "objc2-proc-macros")] @@ -228,7 +225,6 @@ pub mod rc; pub mod runtime; #[cfg(test)] mod test_utils; -#[cfg(feature = "malloc")] mod verify; /// Hacky way to make GNUStep link properly to Foundation while testing. diff --git a/crates/objc2/src/macros.rs b/crates/objc2/src/macros.rs index c90440234..93148e535 100644 --- a/crates/objc2/src/macros.rs +++ b/crates/objc2/src/macros.rs @@ -713,9 +713,8 @@ macro_rules! __class_inner { /// throws an exception. Exceptions may still cause UB until /// `extern "C-unwind"` is stable, see [RFC-2945]. /// -/// Panics if the `"verify_message"` feature is enabled and the Objective-C -/// method's argument's encoding does not match the encoding of the given -/// arguments. This is highly recommended to enable while testing! +/// Panics if `debug_assertions` are enabled and the Objective-C method's +/// encoding does not match the encoding of the given arguments and return. /// /// And panics if the `NSError**` handling functionality described above is /// used, and the error object was unexpectedly `NULL`. diff --git a/crates/objc2/src/macros/declare_class.rs b/crates/objc2/src/macros/declare_class.rs index ae99057ca..bcb35882e 100644 --- a/crates/objc2/src/macros/declare_class.rs +++ b/crates/objc2/src/macros/declare_class.rs @@ -93,10 +93,11 @@ /// 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'. -/// - The `verify_message` feature is enabled, and the required protocol -/// methods are not implemented. +/// - Debug assertions are enabled, and an overriden method's signature is not +/// equal to the one on the superclass. +/// - The `verify` feature and debug assertions are enabled, and the required +/// protocol methods are not implemented. +/// - And possibly more similar cases. /// /// /// # Safety diff --git a/crates/objc2/src/message/mod.rs b/crates/objc2/src/message/mod.rs index 768af743f..7ded2ae47 100644 --- a/crates/objc2/src/message/mod.rs +++ b/crates/objc2/src/message/mod.rs @@ -30,7 +30,7 @@ unsafe fn conditional_try(f: impl FnOnce() -> R) -> R { f() } -#[cfg(feature = "verify_message")] +#[cfg(debug_assertions)] #[track_caller] fn panic_verify(cls: &Class, sel: Sel, err: crate::VerificationError) -> ! { panic!( @@ -177,8 +177,7 @@ pub unsafe trait MessageReceiver: private::Sealed + Sized { R: EncodeConvert, { let this = self.__as_raw_receiver(); - // TODO: Always enable this when `debug_assertions` are on. - #[cfg(feature = "verify_message")] + #[cfg(debug_assertions)] { // SAFETY: Caller ensures only valid or NULL pointers. let this = unsafe { this.as_ref() }; @@ -225,7 +224,7 @@ pub unsafe trait MessageReceiver: private::Sealed + Sized { R: EncodeConvert, { let this = self.__as_raw_receiver(); - #[cfg(feature = "verify_message")] + #[cfg(debug_assertions)] { if this.is_null() { panic!("messsaging {:?} to nil", sel); @@ -581,10 +580,7 @@ mod tests { } #[test] - #[cfg_attr( - feature = "verify_message", - should_panic = "messsaging description to nil" - )] + #[cfg_attr(debug_assertions, should_panic = "messsaging description to nil")] fn test_send_message_nil() { use crate::rc::Shared; diff --git a/crates/objc2/src/rc/test_object.rs b/crates/objc2/src/rc/test_object.rs index 64fb78e07..1ca20747c 100644 --- a/crates/objc2/src/rc/test_object.rs +++ b/crates/objc2/src/rc/test_object.rs @@ -359,11 +359,7 @@ mod tests { // Work around https://github.com/rust-lang/rust-clippy/issues/9737: const IF_AUTORELEASE_NOT_SKIPPED: usize = if cfg!(feature = "gnustep-1-7") { 1 - } else if cfg!(any( - debug_assertions, - feature = "exception", - feature = "verify_message" - )) { + } else if cfg!(any(debug_assertions, feature = "exception")) { 2 } else { 1 diff --git a/crates/objc2/src/runtime.rs b/crates/objc2/src/runtime.rs index cf41f3634..36aab4dc0 100644 --- a/crates/objc2/src/runtime.rs +++ b/crates/objc2/src/runtime.rs @@ -19,14 +19,12 @@ use std::os::raw::c_char; #[cfg(feature = "malloc")] use std::os::raw::c_uint; -use crate::encode::{Encode, Encoding, RefEncode}; +mod method_encoding_iter; + +pub(crate) use self::method_encoding_iter::{EncodingParseError, MethodEncodingIter}; +use crate::encode::{Encode, EncodeArguments, EncodeConvert, Encoding, RefEncode}; use crate::ffi; -#[cfg(feature = "malloc")] -use crate::{ - encode::{EncodeArguments, EncodeConvert}, - verify::{verify_method_signature, Inner}, - VerificationError, -}; +use crate::verify::{verify_method_signature, Inner, VerificationError}; #[doc(inline)] pub use crate::encode::__bool::Bool; @@ -332,8 +330,31 @@ impl Method { } } - // method_getTypeEncoding, efficient version of: - // -> return_type() + sum(argument_type(i) for i in arguments_count()) + /// An iterator over the method's types. + /// + /// It is approximately equivalent to: + /// + /// ```ignore + /// let types = method.types(); + /// assert_eq!(types.next()?, method.return_type()); + /// for i in 0..method.arguments_count() { + /// assert_eq!(types.next()?, method.argument_type(i)?); + /// } + /// assert!(types.next().is_none()); + /// ``` + #[doc(alias = "method_getTypeEncoding")] + pub(crate) fn types(&self) -> MethodEncodingIter<'_> { + // SAFETY: The method pointer is valid and non-null + let cstr = unsafe { ffi::method_getTypeEncoding(self.as_ptr()) }; + if cstr.is_null() { + panic!("method type encoding was NULL"); + } + // SAFETY: `method_getTypeEncoding` returns a C-string, and we just + // checked that it is non-null. + let encoding = unsafe { CStr::from_ptr(cstr) }; + let s = str::from_utf8(encoding.to_bytes()).expect("method type encoding to be UTF-8"); + MethodEncodingIter::new(s) + } /// Returns the number of arguments accepted by self. pub fn arguments_count(&self) -> usize { @@ -387,18 +408,46 @@ impl Class { unsafe { ffi::objc_getClassList(ptr::null_mut(), 0) as usize } } + /// # Safety + /// + /// 1. The class pointer must be valid. + /// 2. The string is unbounded, so the caller must bound it. + pub(crate) unsafe fn name_raw<'a>(ptr: *const ffi::objc_class) -> &'a str { + // SAFETY: Caller ensures that the pointer is valid + let name = unsafe { ffi::class_getName(ptr) }; + if name.is_null() { + panic!("class name was NULL"); + } + // SAFETY: We've checked that the pointer is not NULL, and + // `class_getName` is guaranteed to return a valid C-string. + // + // That the result is properly bounded is checked by the caller. + let name = unsafe { CStr::from_ptr(name) }; + str::from_utf8(name.to_bytes()).unwrap() + } + /// Returns the name of the class. pub fn name(&self) -> &str { - let name = unsafe { CStr::from_ptr(ffi::class_getName(self.as_ptr())) }; - str::from_utf8(name.to_bytes()).unwrap() + // SAFETY: The pointer is valid, and the return is properly bounded + unsafe { Self::name_raw(self.as_ptr()) } + } + + /// # Safety + /// + /// 1. The class pointer must be valid. + /// 2. The caller must bound the lifetime of the returned class. + pub(crate) unsafe fn superclass_raw<'a>(ptr: *const ffi::objc_class) -> Option<&'a Class> { + // SAFETY: Caller ensures that the pointer is valid + let superclass = unsafe { ffi::class_getSuperclass(ptr) }; + let superclass: *const Class = superclass.cast(); + // SAFETY: The result is properly bounded by the caller. + unsafe { superclass.as_ref() } } /// Returns the superclass of self, or [`None`] if self is a root class. pub fn superclass(&self) -> Option<&Class> { - unsafe { - let superclass = ffi::class_getSuperclass(self.as_ptr()); - superclass.cast::().as_ref() - } + // SAFETY: The pointer is valid, and the return is properly bounded + unsafe { Self::superclass_raw(self.as_ptr()) } } /// Returns the metaclass of self. @@ -558,7 +607,6 @@ impl Class { /// let result = cls.verify_sel::<(&Class,), bool>(sel); /// assert!(result.is_ok()); /// ``` - #[cfg(feature = "malloc")] pub fn verify_sel(&self, sel: Sel) -> Result<(), VerificationError> where A: EncodeArguments, @@ -984,7 +1032,7 @@ mod tests { assert!(::ENCODING.equivalent_to_str(&method.return_type())); assert!(Sel::ENCODING.equivalent_to_str(&method.argument_type(1).unwrap())); - assert!(cls.instance_methods().into_iter().any(|m| *m == method)); + assert!(cls.instance_methods().iter().any(|m| *m == method)); } } @@ -1002,7 +1050,7 @@ mod tests { assert!(cls .metaclass() .instance_methods() - .into_iter() + .iter() .any(|m| *m == method)); } } diff --git a/crates/objc2/src/runtime/method_encoding_iter.rs b/crates/objc2/src/runtime/method_encoding_iter.rs new file mode 100644 index 000000000..852ed4440 --- /dev/null +++ b/crates/objc2/src/runtime/method_encoding_iter.rs @@ -0,0 +1,213 @@ +//! Utility for parsing an Objective-C method type encoding. +//! +//! TODO: Move this to `objc2-encode` when more stable. +use core::fmt; +use core::num::ParseIntError; +use core::str; +use std::error::Error; + +use crate::encode::{Encoding, EncodingBox, ParseError}; + +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct MethodEncodingIter<'a> { + s: &'a str, +} + +impl<'a> MethodEncodingIter<'a> { + pub(crate) fn new(s: &'a str) -> Self { + Self { s } + } + + pub(crate) fn extract_return( + &mut self, + ) -> Result<(EncodingBox, Option), EncodingParseError> { + // TODO: Verify stack layout + self.next().ok_or(EncodingParseError::MissingReturn)? + } + + pub(crate) fn verify_receiver(&mut self) -> Result<(), EncodingParseError> { + // TODO: Verify stack layout + let (enc, _stack_layout) = self.next().ok_or(EncodingParseError::MissingReceiver)??; + if !Encoding::Object.equivalent_to_box(&enc) { + return Err(EncodingParseError::InvalidReceiver(enc)); + } + Ok(()) + } + + pub(crate) fn verify_sel(&mut self) -> Result<(), EncodingParseError> { + let (enc, _stack_layout) = self.next().ok_or(EncodingParseError::MissingSel)??; + if !Encoding::Sel.equivalent_to_box(&enc) { + return Err(EncodingParseError::InvalidSel(enc)); + } + Ok(()) + } + + fn extract_encoding(&mut self) -> Result<(EncodingBox, Option), EncodingParseError> { + // See also the following other approaches: + // objrs: https://gitlab.com/objrs/objrs/-/blob/b4f6598696b3fa622e6fddce7aff281770b0a8c2/src/test.rs + // libobjc2: https://github.com/gnustep/libobjc2/blob/v2.1/encoding2.c + // objc4: https://github.com/apple-oss-distributions/objc4/blob/objc4-841.13/runtime/objc-typeencoding.mm + + let encoding = EncodingBox::from_start_of_str(&mut self.s)?; + let stack_layout = parse_stack_layout(&mut self.s)?; + + Ok((encoding, stack_layout)) + } +} + +impl<'a> Iterator for MethodEncodingIter<'a> { + type Item = Result<(EncodingBox, Option), EncodingParseError>; + + fn next(&mut self) -> Option { + if self.s.is_empty() { + return None; + } + Some(self.extract_encoding()) + } +} + +// TODO: Is `isize` correct here? +fn parse_stack_layout(s: &mut &str) -> Result, ParseIntError> { + let rest = s.trim_start_matches(|c: char| c.is_ascii_digit() || c == '-' || c == '+'); + let stack_layout = &s[..s.len() - rest.len()]; + *s = rest; + + if stack_layout.is_empty() { + return Ok(None); + } + stack_layout.parse().map(Some) +} + +#[derive(Debug, PartialEq, Eq, Hash)] +pub(crate) enum EncodingParseError { + ParseError(ParseError), + InvalidStackLayoutInteger, + MissingReturn, + MissingReceiver, + MissingSel, + InvalidReceiver(EncodingBox), + InvalidSel(EncodingBox), +} + +impl From for EncodingParseError { + fn from(e: ParseError) -> Self { + Self::ParseError(e) + } +} + +impl From for EncodingParseError { + fn from(_: ParseIntError) -> Self { + Self::InvalidStackLayoutInteger + } +} + +impl fmt::Display for EncodingParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if !matches!(self, Self::ParseError(_)) { + write!(f, "failed parsing encoding: ")? + } + + match self { + Self::ParseError(e) => write!(f, "{e}")?, + Self::InvalidStackLayoutInteger => write!(f, "invalid integer for stack layout")?, + Self::MissingReturn => write!(f, "return type must be present")?, + Self::MissingReceiver => write!(f, "receiver type must be present")?, + Self::MissingSel => write!(f, "selector type must be present")?, + Self::InvalidReceiver(enc) => { + write!(f, "receiver encoding must be '@', but it was '{enc}'")? + } + Self::InvalidSel(enc) => { + write!(f, "selector encoding must be '@', but it was '{enc}'")? + } + } + write!(f, ". This is likely a bug, please report it!") + } +} + +impl Error for EncodingParseError {} + +#[cfg(test)] +mod tests { + use super::*; + use alloc::boxed::Box; + use alloc::vec; + use alloc::vec::Vec; + + fn assert_stack_layout(mut types: &str, expected: Option, rest: &str) { + let sl = parse_stack_layout(&mut types).unwrap(); + assert_eq!(sl, expected); + assert_eq!(types, rest); + } + + #[test] + fn stack_layout_extract() { + assert_stack_layout("", None, ""); + assert_stack_layout("abc", None, "abc"); + assert_stack_layout("abc12abc", None, "abc12abc"); + assert_stack_layout("0", Some(0), ""); + assert_stack_layout("1abc", Some(1), "abc"); + assert_stack_layout("42def24", Some(42), "def24"); + assert_stack_layout("1234567890xyz", Some(1234567890), "xyz"); + + assert_stack_layout("-1a", Some(-1), "a"); + assert_stack_layout("-1a", Some(-1), "a"); + + // GNU runtime's register parameter hint?? + assert_stack_layout("+1a", Some(1), "a"); + } + + fn assert_encoding_extract(s: &str, expected: &[(EncodingBox, Option)]) { + let actual: Vec<_> = MethodEncodingIter::new(s) + .collect::>() + .unwrap_or_else(|e| panic!("{}", e)); + assert_eq!(&actual, expected); + } + + #[test] + fn parse_bitfield() { + assert_encoding_extract( + "@48@0:8Ad16r^*24{bitfield=b64b1}32i48", + &[ + (EncodingBox::Object, Some(48)), + (EncodingBox::Object, Some(0)), + (EncodingBox::Sel, Some(8)), + (EncodingBox::Atomic(Box::new(EncodingBox::Double)), Some(16)), + ( + EncodingBox::Pointer(Box::new(EncodingBox::String)), + Some(24), + ), + ( + EncodingBox::Struct( + "bitfield".into(), + Some(vec![ + EncodingBox::BitField(64, None), + EncodingBox::BitField(1, None), + ]), + ), + Some(32), + ), + (EncodingBox::Int, Some(48)), + ], + ); + } + + #[test] + fn parse_complex() { + assert_encoding_extract( + "jf16@0:8", + &[ + (EncodingBox::FloatComplex, Some(16)), + (EncodingBox::Object, Some(0)), + (EncodingBox::Sel, Some(8)), + ], + ); + assert_encoding_extract( + "jf@:", + &[ + (EncodingBox::FloatComplex, None), + (EncodingBox::Object, None), + (EncodingBox::Sel, None), + ], + ); + } +} diff --git a/crates/objc2/src/verify.rs b/crates/objc2/src/verify.rs index a9a94f11a..0dd17c5aa 100644 --- a/crates/objc2/src/verify.rs +++ b/crates/objc2/src/verify.rs @@ -1,72 +1,37 @@ use core::fmt; -use core::hash::{Hash, Hasher}; -use malloc_buf::Malloc; +use core::hash::Hash; use std::error::Error; -use crate::encode::{Encode, EncodeArguments, EncodeConvert, Encoding}; -use crate::runtime::{Method, Object, Sel}; - -/// Workaround for `Malloc` not implementing common traits -#[derive(Debug)] -pub(crate) struct MallocEncoding(Malloc); - -// SAFETY: `char*` strings can safely be free'd on other threads. -unsafe impl Send for MallocEncoding {} -unsafe impl Sync for MallocEncoding {} - -impl PartialEq for MallocEncoding { - fn eq(&self, other: &Self) -> bool { - *self.0 == *other.0 - } -} - -impl Eq for MallocEncoding {} - -impl Hash for MallocEncoding { - fn hash(&self, state: &mut H) { - Hash::hash(&*self.0, state) - } -} - -impl fmt::Display for MallocEncoding { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt(&*self.0, f) - } -} +use crate::encode::{Encode, EncodeArguments, EncodeConvert, Encoding, EncodingBox}; +use crate::runtime::{EncodingParseError, Method}; #[derive(Debug, PartialEq, Eq, Hash)] pub(crate) enum Inner { MethodNotFound, - MismatchedReturn(MallocEncoding, Encoding), + EncodingParseError(EncodingParseError), + MismatchedReturn(EncodingBox, Encoding), MismatchedArgumentsCount(usize, usize), - MismatchedArgument(usize, MallocEncoding, Encoding), + MismatchedArgument(usize, EncodingBox, Encoding), } impl fmt::Display for Inner { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::MethodNotFound => { - write!(f, "method not found") - } + Self::MethodNotFound => write!(f, "method not found"), + Self::EncodingParseError(e) => write!(f, "{e}"), Self::MismatchedReturn(expected, actual) => { write!( f, - "expected return to have type code {}, but found {}", - expected, actual + "expected return to have type code '{expected}', but found '{actual}'", ) } Self::MismatchedArgumentsCount(expected, actual) => { - write!( - f, - "expected {} arguments, but {} were given", - expected, actual - ) + write!(f, "expected {expected} arguments, but {actual} were given",) } Self::MismatchedArgument(i, expected, actual) => { write!( f, - "expected argument at index {} to have type code {}, but found {}", - i, expected, actual + "expected argument at index {i} to have type code '{expected}', but found '{actual}'", ) } } @@ -81,10 +46,16 @@ 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 +/// [`Class::verify_sel`]: crate::runtime::Class::verify_sel #[derive(Debug, PartialEq, Eq, Hash)] pub struct VerificationError(Inner); +impl From for VerificationError { + fn from(e: EncodingParseError) -> Self { + Self(Inner::EncodingParseError(e)) + } +} + impl From for VerificationError { fn from(inner: Inner) -> Self { Self(inner) @@ -105,36 +76,44 @@ where A: EncodeArguments, R: EncodeConvert, { + let mut iter = method.types(); + + // TODO: Verify stack layout + let (expected, _stack_layout) = iter.extract_return()?; let actual = R::__Inner::ENCODING; - let expected = method.return_type(); - if !actual.equivalent_to_str(&*expected) { - return Err(Inner::MismatchedReturn(MallocEncoding(expected), actual).into()); + if !actual.equivalent_to_box(&expected) { + return Err(Inner::MismatchedReturn(expected, actual).into()); } - let self_and_cmd = [<*mut Object>::ENCODING, Sel::ENCODING]; - let args = A::ENCODINGS; + iter.verify_receiver()?; + iter.verify_sel()?; - let actual = self_and_cmd.len() + args.len(); - let expected = method.arguments_count(); - if actual != expected { - return Err(Inner::MismatchedArgumentsCount(expected, actual).into()); - } + let actual_count = A::ENCODINGS.len(); - for (i, actual) in self_and_cmd.iter().chain(args).enumerate() { - let expected = method.argument_type(i).unwrap(); - if !actual.equivalent_to_str(&*expected) { - return Err( - Inner::MismatchedArgument(i, MallocEncoding(expected), actual.clone()).into(), - ); + for (i, actual) in A::ENCODINGS.iter().enumerate() { + if let Some(res) = iter.next() { + // TODO: Verify stack layout + let (expected, _stack_layout) = res?; + if !actual.equivalent_to_box(&expected) { + return Err(Inner::MismatchedArgument(i, expected, actual.clone()).into()); + } + } else { + return Err(Inner::MismatchedArgumentsCount(i, actual_count).into()); } } + let remaining = iter.count(); + if remaining != 0 { + return Err(Inner::MismatchedArgumentsCount(actual_count + remaining, actual_count).into()); + } + Ok(()) } #[cfg(test)] mod tests { use super::*; + use crate::runtime::Sel; use crate::sel; use crate::test_utils; use alloc::string::ToString; @@ -165,22 +144,22 @@ mod tests { let err = cls.verify_sel::<(u32,), u64>(sel!(setFoo:)).unwrap_err(); assert_eq!( err.to_string(), - "expected return to have type code v, but found Q" + "expected return to have type code 'v', but found 'Q'" ); // Too many arguments let err = cls.verify_sel::<(u32, i8), ()>(sel!(setFoo:)).unwrap_err(); - assert_eq!(err.to_string(), "expected 3 arguments, but 4 were given"); + assert_eq!(err.to_string(), "expected 1 arguments, but 2 were given"); // Too few arguments let err = cls.verify_sel::<(), ()>(sel!(setFoo:)).unwrap_err(); - assert_eq!(err.to_string(), "expected 3 arguments, but 2 were given"); + assert_eq!(err.to_string(), "expected 1 arguments, but 0 were given"); // Incorrect argument type let err = cls.verify_sel::<(Sel,), ()>(sel!(setFoo:)).unwrap_err(); assert_eq!( err.to_string(), - "expected argument at index 2 to have type code I, but found :" + "expected argument at index 0 to have type code 'I', but found ':'" ); // Metaclass @@ -188,19 +167,19 @@ mod tests { let err = metaclass .verify_sel::<(i32, i32, i32), i32>(sel!(addNumber:toNumber:)) .unwrap_err(); - assert_eq!(err.to_string(), "expected 4 arguments, but 5 were given"); + assert_eq!(err.to_string(), "expected 2 arguments, but 3 were given"); } #[test] - #[cfg(feature = "verify_message")] - #[should_panic = "invalid message send to -[CustomObject foo]: expected return to have type code I, but found i"] + #[cfg(debug_assertions)] + #[should_panic = "invalid message send to -[CustomObject foo]: expected return to have type code 'I', but found 'i'"] fn test_send_message_verified() { let obj = test_utils::custom_object(); let _: i32 = unsafe { crate::msg_send![&obj, foo] }; } #[test] - #[cfg(feature = "verify_message")] + #[cfg(debug_assertions)] #[should_panic = "invalid message send to +[CustomObject abcDef]: method not found"] fn test_send_message_verified_to_class() { let cls = test_utils::custom_class(); diff --git a/crates/objc2/tests/id_retain_autoreleased.rs b/crates/objc2/tests/id_retain_autoreleased.rs index 7ca4061cf..f6ad0ab56 100644 --- a/crates/objc2/tests/id_retain_autoreleased.rs +++ b/crates/objc2/tests/id_retain_autoreleased.rs @@ -53,11 +53,7 @@ fn test_retain_autoreleased() { // the autoreleased value! let expected = if cfg!(feature = "gnustep-1-7") { 1 - } else if cfg!(any( - debug_assertions, - feature = "exception", - feature = "verify_message" - )) { + } else if cfg!(any(debug_assertions, feature = "exception")) { 2 } else { 1 diff --git a/crates/tests/src/exception.rs b/crates/tests/src/exception.rs index 1a162d2fb..3a55d19ab 100644 --- a/crates/tests/src/exception.rs +++ b/crates/tests/src/exception.rs @@ -28,15 +28,11 @@ fn throw_catch_raise_catch() { assert_retain_count(&exc, 2); // TODO: Investigate this! - let extra_retain = if cfg!(all( + let extra_retain = usize::from(cfg!(all( feature = "apple", target_os = "macos", target_arch = "x86" - )) { - 1 - } else { - 0 - }; + ))); let exc = autoreleasepool(|_| { let exc = NSException::into_exception(exc); diff --git a/helper-scripts/test-local.fish b/helper-scripts/test-local.fish index b69c13234..675e9bca7 100755 --- a/helper-scripts/test-local.fish +++ b/helper-scripts/test-local.fish @@ -1,7 +1,7 @@ #!/usr/local/bin/fish # A test script I use to test on my local devices -echo 'Add this to objc2/Cargo.toml: `default = ["exception", "verify_message", "catch-all"]`' +echo 'Add this to objc2/Cargo.toml: `default = ["exception", "verify", "catch-all"]`' read export MACOSX_DEPLOYMENT_TARGET=10.7 @@ -34,5 +34,5 @@ rm -d .cargo export SDKROOT=(pwd)/ideas/MacOSX10.13.sdk export CARGO_BUILD_TARGET=i686-apple-darwin cargo +nightly test -Zbuild-std -cargo +nightly test -Zbuild-std --features malloc,block,exception,catch-all,verify_message,unstable-static-class,unstable-static-sel +cargo +nightly test -Zbuild-std --features malloc,block,exception,catch-all,verify,unstable-static-class,unstable-static-sel cargo +nightly test -Zbuild-std --release