Cherry-pick ae1c875 for objc2 v0.6.4

Author: madsmtm

crates/objc2/CHANGELOG.md

@@ -9,6 +9,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Added
 * Added support for the unstable `darwin_objc` feature.
 
+## Changed
+* **BREAKING** (very slightly): `define_class!` now rejects non-static and
+  non-unique class names.
+
+## Fixed
+* Allow classes created with `define_class!` to be used in multiple shared
+  dynamic libraries in the same process.
+
 
 ## [0.6.3] - 2025-10-04
 [0.6.3]: https://github.com/madsmtm/objc2/compare/objc2-0.6.2...objc2-0.6.3

crates/objc2/src/__macro_helpers/define_class.rs

@@ -1,6 +1,6 @@
-use alloc::ffi::CString;
 #[cfg(debug_assertions)]
 use alloc::vec::Vec;
+use core::ffi::CStr;
 use core::marker::PhantomData;
 use core::panic::{RefUnwindSafe, UnwindSafe};
 #[cfg(debug_assertions)]
@@ -15,7 +15,10 @@ use crate::runtime::{
 use crate::runtime::{AnyProtocol, MethodDescription};
 use crate::{AnyThread, ClassType, DefinedClass, Message, ProtocolType};
 
-use super::defined_ivars::{register_with_ivars, setup_dealloc};
+use super::defined_ivars::{
+    drop_flag_offset, ivar_drop_flag_names, ivars_offset, register_drop_flag, register_ivars,
+    setup_dealloc,
+};
 use super::{CopyFamily, InitFamily, MutableCopyFamily, NewFamily, NoneFamily};
 
 /// Helper for determining auto traits of defined classes.
@@ -184,42 +187,132 @@ impl<T: Message> MaybeOptionRetained for Option<Retained<T>> {
     }
 }
 
-#[derive(Debug)]
-pub struct ClassBuilderHelper<T: ?Sized> {
-    builder: ClassBuilder,
-    p: PhantomData<T>,
+/// Convert a class name with a trailing NUL byte to a `CStr`, at `const`.
+#[track_caller]
+pub const fn class_c_name(name: &str) -> &CStr {
+    let bytes = name.as_bytes();
+    // Workaround for `from_bytes_with_nul` not being `const` in MSRV.
+    let mut i = 0;
+    while i < bytes.len() - 1 {
+        if bytes[i] == 0 {
+            panic!("class name must not contain interior NUL bytes");
+        }
+        i += 1;
+    }
+    if let Ok(c_name) = CStr::from_bytes_until_nul(bytes) {
+        c_name
+    } else {
+        unreachable!()
+    }
 }
 
-// Outlined for code size
+// Kept separate for code size.
 #[track_caller]
-fn create_builder(name: &str, superclass: &AnyClass) -> ClassBuilder {
-    let c_name = CString::new(name).expect("class name must be UTF-8");
-    match ClassBuilder::new(&c_name, superclass) {
-        Some(builder) => builder,
-        None => panic!(
-            "could not create new class {name}. Perhaps a class with that name already exists?"
-        ),
-    }
+fn class_not_present(c_name: &CStr) -> ! {
+    panic!("could not create new class {c_name:?}, though there was no other class with that name")
 }
 
-impl<T: DefinedClass> ClassBuilderHelper<T> {
-    #[inline]
-    #[track_caller]
-    #[allow(clippy::new_without_default)]
-    pub fn new() -> Self
-    where
-        T::Super: ClassType,
-    {
-        let mut builder = create_builder(T::NAME, <T::Super as ClassType>::class());
+#[track_caller]
+fn class_not_unique(c_name: &CStr) -> ! {
+    panic!("could not create new class {c_name:?}, perhaps a class with that name already exists?")
+}
 
-        setup_dealloc::<T>(&mut builder);
+#[inline]
+#[track_caller]
+#[allow(clippy::new_without_default)]
+pub fn define_class<T: DefinedClass>(
+    c_name: &CStr,
+    name_is_auto_generated: bool,
+    register_impls: impl FnOnce(&mut ClassBuilderHelper<T>),
+) -> (&'static AnyClass, isize, isize)
+where
+    T::Super: ClassType,
+{
+    let (ivar_name, drop_flag_name) = ivar_drop_flag_names::<T>();
 
-        Self {
+    let superclass = <T::Super as ClassType>::class();
+    let cls = if let Some(builder) = ClassBuilder::new(c_name, superclass) {
+        let mut this = ClassBuilderHelper {
             builder,
             p: PhantomData,
+        };
+
+        setup_dealloc::<T>(&mut this.builder);
+
+        register_impls(&mut this);
+
+        register_ivars::<T>(&mut this.builder, &ivar_name);
+        register_drop_flag::<T>(&mut this.builder, &drop_flag_name);
+
+        this.builder.register()
+    } else {
+        // When loading two dynamic libraries that both use a class from some
+        // shared (static) Rust library, the dynamic linker will duplicate the
+        // statics that `define_class!` defines.
+        //
+        // For most statics that people create, this is the desired behaviour.
+        //
+        // In our case though, there is only a single Objective-C runtime with
+        // a single list of classes, and thus those two dynamic libraries end
+        // up trying to register the same class multiple times.
+        //
+        // To support such use-cases, we assume that the existing class is the
+        // one that we want, and don't try to declare it ourselves.
+        //
+        // This is **sound** within the context of a single linker invocation,
+        // since we ensure in `define_class!` with `#[extern_name = ...]` that
+        // the class name is unique.
+        //
+        // It is **unsound** in the case above of multiple dynamic libraries,
+        // since we cannot guarantee that the name actually comes from the
+        // same piece of code. The dynamic linker already does this same merge
+        // operation though, so we will consider this a non-issue (i.e. the
+        // same problem already exists in Objective-C w. dynamic libraries).
+        //
+        // See <https://github.com/rust-windowing/raw-window-metal/issues/29>
+        // for more details on the use-case.
+        //
+        // NOTE: We _could_ also solve this by autogenerating a class name
+        // based on the address of a static - but in the future, we would like
+        // to generate fully static classes, and then such a solution wouldn't
+        // be possible.
+        //
+        // ---
+        //
+        // We only do this by default when we've auto-generated the name,
+        // since here we'll be reasonably sure that it's unique to that
+        // specific piece of code.
+        //
+        // In the future, we might be able to relax this to also work with
+        // user-specified names, though we'd have to somehow ensure that the
+        // name isn't something crazy like "NSObject".
+        //
+        // We also have an (intentionally undocumented) workaround env var in
+        // case this becomes a problem for users in the future.
+        let overridden = option_env!("UNSAFE_OBJC2_ALLOW_CLASS_OVERRIDE") == Some("1");
+        if name_is_auto_generated || overridden {
+            AnyClass::get(c_name).unwrap_or_else(|| class_not_present(c_name))
+        } else {
+            class_not_unique(c_name)
         }
-    }
+    };
+
+    // Pass class and offsets back to allow storing them in statics for faster
+    // subsequent access.
+    (
+        cls,
+        ivars_offset::<T>(cls, &ivar_name),
+        drop_flag_offset::<T>(cls, &drop_flag_name),
+    )
+}
 
+#[derive(Debug)]
+pub struct ClassBuilderHelper<T: ?Sized> {
+    builder: ClassBuilder,
+    p: PhantomData<T>,
+}
+
+impl<T: DefinedClass> ClassBuilderHelper<T> {
     #[inline]
     pub fn add_protocol_methods<P>(&mut self) -> ClassProtocolMethodsBuilder<'_, T>
     where
@@ -279,11 +372,6 @@ impl<T: DefinedClass> ClassBuilderHelper<T> {
         // SAFETY: Checked by caller
         unsafe { self.builder.add_class_method(sel, func) }
     }
-
-    #[inline]
-    pub fn register(self) -> (&'static AnyClass, isize, isize) {
-        register_with_ivars::<T>(self.builder)
-    }
 }
 
 /// Helper for ensuring that:

crates/objc2/src/__macro_helpers/defined_ivars.rs

@@ -234,35 +234,34 @@ where
     }
 }
 
-/// Register the class, and get the ivar offsets.
 #[inline]
-pub(crate) fn register_with_ivars<T: DefinedClass>(
-    mut builder: ClassBuilder,
-) -> (&'static AnyClass, isize, isize) {
-    let (ivar_name, drop_flag_name): (Cow<'static, CStr>, Cow<'static, CStr>) = {
-        if cfg!(feature = "gnustep-1-7") {
-            // GNUStep does not support a subclass having an ivar with the
-            // same name as a superclass, so let's use the class name as the
-            // ivar name to ensure uniqueness.
+pub(crate) fn ivar_drop_flag_names<T: DefinedClass>() -> (Cow<'static, CStr>, Cow<'static, CStr>) {
+    if cfg!(feature = "gnustep-1-7") {
+        // GNUStep does not support a subclass having an ivar with the
+        // same name as a superclass, so let's use the class name as the
+        // ivar name to ensure uniqueness.
+        (
+            CString::new(format!("{}_ivars", T::NAME)).unwrap().into(),
+            CString::new(format!("{}_drop_flag", T::NAME))
+                .unwrap()
+                .into(),
+        )
+    } else {
+        // SAFETY: The byte slices are NUL-terminated, and do not contain
+        // interior NUL bytes.
+        // TODO: Use `c"my_str"` syntax once in MSRV
+        unsafe {
             (
-                CString::new(format!("{}_ivars", T::NAME)).unwrap().into(),
-                CString::new(format!("{}_drop_flag", T::NAME))
-                    .unwrap()
-                    .into(),
+                CStr::from_bytes_with_nul_unchecked(b"ivars\0").into(),
+                CStr::from_bytes_with_nul_unchecked(b"drop_flag\0").into(),
             )
-        } else {
-            // SAFETY: The byte slices are NUL-terminated, and do not contain
-            // interior NUL bytes.
-            // TODO: Use `c"my_str"` syntax once in MSRV
-            unsafe {
-                (
-                    CStr::from_bytes_with_nul_unchecked(b"ivars\0").into(),
-                    CStr::from_bytes_with_nul_unchecked(b"drop_flag\0").into(),
-                )
-            }
         }
-    };
+    }
+}
 
+/// Register the ivars.
+#[inline]
+pub(crate) fn register_ivars<T: DefinedClass>(builder: &mut ClassBuilder, ivars_name: &CStr) {
     if T::HAS_IVARS {
         // TODO: Consider not adding a encoding - Swift doesn't do it.
         let ivar_encoding = Encoding::Array(
@@ -276,25 +275,33 @@ pub(crate) fn register_with_ivars<T: DefinedClass>(
                 alignment => panic!("unsupported alignment {alignment} for `{}::Ivars`", T::NAME),
             },
         );
-        unsafe { builder.add_ivar_inner::<T::Ivars>(&ivar_name, &ivar_encoding) };
+        unsafe { builder.add_ivar_inner::<T::Ivars>(ivars_name, &ivar_encoding) };
     }
+}
 
+/// Register the drop flag ivar.
+#[inline]
+pub(crate) fn register_drop_flag<T: DefinedClass>(
+    builder: &mut ClassBuilder,
+    drop_flag_name: &CStr,
+) {
     if T::HAS_DROP_FLAG {
         // TODO: Maybe we can reuse the drop flag when subclassing an already
         // defined class?
-        builder.add_ivar::<DropFlag>(&drop_flag_name);
+        builder.add_ivar::<DropFlag>(drop_flag_name);
     }
+}
 
-    let cls = builder.register();
-
-    let ivars_offset = if T::HAS_IVARS {
+#[inline]
+pub(crate) fn ivars_offset<T: DefinedClass>(cls: &AnyClass, ivars_name: &CStr) -> isize {
+    if T::HAS_IVARS {
         // Monomorphized error handling
         // Intentionally not #[track_caller], we expect this error to never occur
         fn get_ivar_failed() -> ! {
             unreachable!("failed retrieving instance variable on newly defined class")
         }
 
-        cls.instance_variable(&ivar_name)
+        cls.instance_variable(ivars_name)
             .unwrap_or_else(|| get_ivar_failed())
             .offset()
     } else {
@@ -303,16 +310,19 @@ pub(crate) fn register_with_ivars<T: DefinedClass>(
         // This is fine, since any reads here will only be via zero-sized
         // ivars, where the actual pointer doesn't matter.
         0
-    };
+    }
+}
 
-    let drop_flag_offset = if T::HAS_DROP_FLAG {
+#[inline]
+pub(crate) fn drop_flag_offset<T: DefinedClass>(cls: &AnyClass, drop_flag_name: &CStr) -> isize {
+    if T::HAS_DROP_FLAG {
         // Monomorphized error handling
         // Intentionally not #[track_caller], we expect this error to never occur
         fn get_drop_flag_failed() -> ! {
             unreachable!("failed retrieving drop flag instance variable on newly defined class")
         }
 
-        cls.instance_variable(&drop_flag_name)
+        cls.instance_variable(drop_flag_name)
             .unwrap_or_else(|| get_drop_flag_failed())
             .offset()
     } else {
@@ -321,9 +331,7 @@ pub(crate) fn register_with_ivars<T: DefinedClass>(
         // This is fine, since the drop flag is never actually used in the
         // cases where it was not added.
         0
-    };
-
-    (cls, ivars_offset, drop_flag_offset)
+    }
 }
 
 /// # Safety

crates/objc2/src/__macro_helpers/mod.rs

@@ -3,6 +3,7 @@ pub use core::cell::UnsafeCell;
 pub use core::cmp::{Eq, PartialEq};
 pub use core::convert::AsRef;
 pub use core::default::Default;
+pub use core::ffi::CStr;
 pub use core::fmt;
 pub use core::hash::{Hash, Hasher};
 pub use core::marker::{PhantomData, Sized};
@@ -38,8 +39,8 @@ pub use self::class::{DoesNotImplDrop, MainThreadOnlyDoesNotImplSendSync, ValidT
 pub use self::common_selectors::{alloc_sel, dealloc_sel, init_sel, new_sel};
 pub use self::convert::{ConvertArgument, ConvertArguments, ConvertReturn, TupleExtender};
 pub use self::define_class::{
-    ClassBuilderHelper, ClassProtocolMethodsBuilder, MaybeOptionRetained, MessageReceiveRetained,
-    RetainedReturnValue, ThreadKindAutoTraits,
+    class_c_name, define_class, ClassBuilderHelper, ClassProtocolMethodsBuilder,
+    MaybeOptionRetained, MessageReceiveRetained, RetainedReturnValue, ThreadKindAutoTraits,
 };
 pub use self::defined_ivars::DefinedIvarsHelper;
 pub use self::image_info::ImageInfo;