Rework verification feature selection

- Always only on `debug_assertions`
- Most of it is enabled by default
- The rest can be enabled with the `"verify"` feature

.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
 

crates/objc2/CHANGELOG.md

@@ -38,19 +38,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<T>`
  instead of `Id<Allocated<T>, 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
 

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`.
 #

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<Sel>,
 }
 
-#[cfg(feature = "verify_message")]
+#[cfg(all(debug_assertions, feature = "verify"))]
 impl ClassProtocolMethodsBuilder<'_, '_> {
  #[inline]
  pub unsafe fn add_method<T, F>(&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<Object, Shared> = 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<Allocated<RcTestObject>> = None;
  let _obj: Id<RcTestObject, Owned> = 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<NSString, Shared> = unsafe { msg_send_id![obj, description] };

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<T>() -> 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<Class>,
+ // Note: Don't ever construct a &mut objc_class, since it is possible to
+ // get this pointer using `Class::classes`!
+ cls: NonNull<ffi::objc_class>,
 }
 
 #[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<Self> {
  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::<F::Args, F::Ret>(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::<F::Args, F::Ret>(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::<F::Args, F::Ret>(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::<F::Args, F::Ret>(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::<Class>().as_ref() }
  }
 }
 
@@ -698,7 +674,7 @@ mod tests {
 
  #[test]
  #[cfg_attr(
- feature = "verify_message",
+ debug_assertions,
  should_panic = "declared invalid method -[TestClassBuilderInvalidMethod foo]: expected return to have type code 'I', but found 'i'"
  )]
  fn invalid_method() {
@@ -716,7 +692,7 @@ mod tests {
 
  #[test]
  #[cfg_attr(
- feature = "verify_message",
+ debug_assertions,
  should_panic = "declared invalid method +[TestClassBuilderInvalidClassMethod classFoo]: expected return to have type code 'I', but found 'i'"
  )]
  fn invalid_class_method() {
@@ -779,9 +755,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,7 +826,7 @@ mod tests {
 
  #[test]
  #[cfg_attr(
- feature = "verify_message",
+ debug_assertions,
  should_panic = "declared invalid method -[TestDeclareClassInvalidMethod description]: expected return to have type code '@', but found 'v'"
  )]
  fn test_declare_class_invalid_method() {
@@ -874,7 +850,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 +871,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 +894,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 {}