Allow creating and loading instance variables with the same name

crates/objc2/CHANGELOG.md

@@ -81,6 +81,26 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * Allow cloning `Id<AnyObject>`.
 * **BREAKING**: Restrict message sending to `&mut` references to things that
   implement `IsAllowedMutable`.
+* Allow adding instance variables with the same name on Apple platforms.
+* **BREAKING**: Make loading instance variables robust and sound in the face
+  of instance variables with the same name.
+
+  To read or write the instance variable for an object, you should now use the
+  `load`, `load_ptr` and `load_mut` methods on `Ivar`, instead of the `ivar`,
+  `ivar_ptr` and `ivar_mut` methods on `AnyObject`.
+
+  This _is_ more verbose, but it also ensures that the class for the instance
+  variable you're loading is the same as the one the instance variable you
+  want to access is defined on.
+
+  ```rust
+  // Before
+  let number = unsafe { *obj.ivar::<u32>("number") };
+
+  // After
+  let ivar = cls.instance_variable("number").unwrap();
+  let number = unsafe { *ivar.load::<u32>(&obj) };
+  ```
 
 ### Removed
 * **BREAKING**: Removed `ProtocolType` implementation for `NSObject`.

crates/objc2/Cargo.toml

@@ -108,6 +108,7 @@ objc2-proc-macros = { path = "../objc2-proc-macros", version = "0.1.1", optional
 [dev-dependencies]
 iai = { version = "0.1", git = "https://github.com/madsmtm/iai", branch = "callgrind" }
 static_assertions = "1.1.0"
+memoffset = "0.9.0"
 
 [[bench]]
 name = "autorelease"

crates/objc2/examples/introspection.rs

@@ -49,9 +49,7 @@ fn main() {
 
     println!("NSObject address: {obj:p}");
 
-    // Access an ivar of the object
-    //
-    // As before, you should not rely on the `isa` ivar being available!
-    let isa = unsafe { *obj.ivar::<*const AnyClass>("isa") };
+    // Read an ivar on the object
+    let isa: *const AnyClass = unsafe { *ivar.load(&obj) };
     println!("NSObject isa: {isa:?}");
 }

crates/objc2/src/declare/ivar.rs

@@ -5,7 +5,7 @@ use core::ops::{Deref, DerefMut};
 use core::ptr::{self, NonNull};
 
 use crate::encode::Encode;
-use crate::runtime::{ivar_offset, AnyObject};
+use crate::runtime::AnyObject;
 
 pub(crate) mod private {
     pub trait Sealed {}
@@ -92,9 +92,13 @@ pub unsafe trait IvarType {
     const NAME: &'static str;
 
     #[doc(hidden)]
-    unsafe fn __offset(ptr: NonNull<AnyObject>) -> isize {
-        let obj = unsafe { ptr.as_ref() };
-        ivar_offset(obj.class(), Self::NAME, &Self::Type::ENCODING)
+    unsafe fn __ivar_ptr(ptr: NonNull<AnyObject>) -> NonNull<Self::Type> {
+        // FIXME: This is currently unsound! Looking up the instance variable
+        // dynamically will return the wrong variable if two variables with
+        // the same name exist.
+        let ivar = unsafe { ptr.as_ref() }.lookup_instance_variable_dynamically(Self::NAME);
+        ivar.debug_assert_encoding(&Self::Type::ENCODING);
+        unsafe { ivar.offset_ptr(ptr).cast() }
     }
 }
 
@@ -215,9 +219,7 @@ impl<T: IvarType> Ivar<T> {
         // Note: We technically don't have provenance over the object, nor the
         // ivar, but the object doesn't have provenance over the ivar either,
         // so that is fine.
-        let offset = unsafe { T::__offset(ptr) };
-        // SAFETY: The offset is valid
-        unsafe { AnyObject::ivar_at_offset::<T::Type>(ptr, offset) }
+        unsafe { T::__ivar_ptr(ptr) }
     }
 
     /// Get a mutable pointer to the instance variable.
@@ -238,9 +240,7 @@ impl<T: IvarType> Ivar<T> {
         let ptr: NonNull<AnyObject> = NonNull::from(self).cast();
 
         // SAFETY: Same as `as_inner_ptr`
-        let offset = unsafe { T::__offset(ptr) };
-        // SAFETY: The offset is valid
-        unsafe { AnyObject::ivar_at_offset::<T::Type>(ptr, offset) }
+        unsafe { T::__ivar_ptr(ptr) }
     }
 
     /// Sets the value of the instance variable.

crates/objc2/src/declare/mod.rs

@@ -96,8 +96,9 @@ impl<T> Log2Alignment for T {
 ///     ) -> Option<&mut AnyObject> {
 ///         let this: Option<&mut AnyObject> = msg_send![super(this, NSObject::class()), init];
 ///         this.map(|this| {
+///             let ivar = AnyClass::get("MyNumber").unwrap().instance_variable("_number").unwrap();
 ///             // SAFETY: The ivar is added with the same type above
-///             this.set_ivar::<Cell<u32>>("_number", Cell::new(number));
+///             *ivar.load_mut::<Cell<u32>>(this) = Cell::new(number);
 ///             this
 ///         })
 ///     }
@@ -131,17 +132,19 @@ impl<T> Log2Alignment for T {
 ///
 ///     // Add an Objective-C method for setting the number
 ///     extern "C" fn my_number_set(this: &NSObject, _cmd: Sel, number: u32) {
+///         let ivar = AnyClass::get("MyNumber").unwrap().instance_variable("_number").unwrap();
 ///         // SAFETY: The ivar is added with the same type above
-///         unsafe { this.ivar::<Cell<u32>>("_number") }.set(number);
+///         unsafe { ivar.load::<Cell<u32>>(this) }.set(number);
 ///     }
 ///     unsafe {
 ///         builder.add_method(sel!(setNumber:), my_number_set as extern "C" fn(_, _, _));
 ///     }
 ///
 ///     // Add an Objective-C method for getting the number
 ///     extern "C" fn my_number_get(this: &NSObject, _cmd: Sel) -> u32 {
+///         let ivar = AnyClass::get("MyNumber").unwrap().instance_variable("_number").unwrap();
 ///         // SAFETY: The ivar is added with the same type above
-///         unsafe { this.ivar::<Cell<u32>>("_number") }.get()
+///         unsafe { ivar.load::<Cell<u32>>(this) }.get()
 ///     }
 ///     unsafe {
 ///         builder.add_method(sel!(number), my_number_get as extern "C" fn(_, _) -> _);
@@ -404,25 +407,17 @@ impl ClassBuilder {
         align: u8,
         encoding: &Encoding,
     ) {
-        // `class_addIvar` sadly doesn't check this for us.
-        //
-        // We must _always_ do the check, since there is no way for the user
-        // to statically know if the superclass has a declared instance
-        // variable on it, since that may change if a new version of the
-        // library/framework the class came from is released.
-        if let Some(_ivar) = self
-            .superclass()
-            .and_then(|superclass| superclass.instance_variable(name))
-        {
-            panic!("instance variable {name:?} already exists on a superclass");
-        }
-
         let c_name = CString::new(name).unwrap();
         let encoding = CString::new(encoding.to_string()).unwrap();
 
         // Note: The Objective-C runtime contains functionality to do stuff
         // with "instance variable layouts", but we don't have to touch any of
         // that, it was only used in the garbage-collecting runtime.
+        //
+        // Note: On GNUStep, instance variables cannot have the same name
+        // on subclasses as it has on superclasses.
+        //
+        // See <https://github.com/gnustep/libobjc2/issues/246>
         let success = Bool::from_raw(unsafe {
             ffi::class_addIvar(
                 self.as_mut_ptr(),
@@ -614,12 +609,16 @@ mod tests {
     use std::collections::hash_map::DefaultHasher;
     use std::hash::Hash;
 
+    use memoffset::offset_of;
+
     use super::*;
     use crate::encode::RefEncode;
     use crate::mutability::Immutable;
     use crate::rc::Id;
     use crate::runtime::{NSObject, NSObjectProtocol};
-    use crate::{declare_class, extern_methods, msg_send, test_utils, ClassType, ProtocolType};
+    use crate::{
+        declare_class, extern_methods, msg_send, msg_send_id, test_utils, ClassType, ProtocolType,
+    };
 
     #[test]
     fn test_alignment() {
@@ -945,4 +944,136 @@ mod tests {
         assert!(obj1.is_kind_of::<Custom>());
         assert!(obj1.is_kind_of::<Custom>());
     }
+
+    #[test]
+    #[cfg_attr(
+        feature = "gnuste-1-7",
+        ignore = "ivars cannot have the same name on GNUStep"
+    )]
+    fn test_ivar_sizing() {
+        #[repr(align(16))]
+        struct U128align16([u64; 2]);
+
+        unsafe impl Encode for U128align16 {
+            const ENCODING: Encoding = <[u64; 2]>::ENCODING;
+        }
+
+        let mut superclass =
+            ClassBuilder::new("DeclareClassDuplicateIvarSuperclass", NSObject::class()).unwrap();
+        superclass.add_ivar::<u8>("ivar1");
+        superclass.add_ivar::<U128align16>("ivar2");
+        superclass.add_ivar::<u8>("ivar3");
+        let superclass = superclass.register();
+
+        let mut subclass =
+            ClassBuilder::new("DeclareClassDuplicateIvarSubclass", superclass).unwrap();
+        // Try to overwrite instance variables
+        subclass.add_ivar::<i16>("ivar1");
+        subclass.add_ivar::<usize>("ivar2");
+        subclass.add_ivar::<*const AnyObject>("ivar3");
+        subclass.add_ivar::<usize>("ivar4");
+        let subclass = subclass.register();
+
+        // Test that ivar layout matches that of C
+        //
+        // In particular, ivars are not reordered, though any extra padding on
+        // superclasses are utilized on subclasses.
+        #[repr(C)]
+        struct NSObjectLayout {
+            isa: *const AnyClass,
+        }
+        assert_eq!(
+            NSObject::class().instance_size(),
+            mem::size_of::<NSObjectLayout>(),
+        );
+
+        #[repr(C)]
+        struct SuperLayout {
+            isa: *const AnyClass,
+            ivar1: u8,
+            // Padding (7 on 64bit, 11 on 32bit)
+            ivar2: U128align16,
+            ivar3: u8,
+            // Padding (15 in Rust, 7 on 64bit, 3 on 32bit)
+        }
+        // Class's ivar size is only rounded up to a pointer-sized boundary,
+        // not all the way up to the maximum alignment.
+        //
+        // This is surprising, but actually fine, since Objective-C objects
+        // are never packed closely like Rust structs would be in an array.
+        assert_eq!(
+            superclass.instance_size(),
+            mem::size_of::<SuperLayout>() - 16 + mem::size_of::<*const AnyClass>(),
+        );
+
+        #[repr(C)]
+        struct SubLayout {
+            isa: *const AnyClass,
+            ivar1: u8,
+            // Padding (7 on 64bit, 11 on 32bit)
+            ivar2: U128align16,
+            ivar3: u8,
+            // Padding (1)
+            ivar1_b: i16,
+            // Padding (4)
+            ivar2_b: usize,
+            ivar3_b: *const AnyObject,
+            ivar4: usize,
+        }
+        assert_eq!(subclass.instance_size(), mem::size_of::<SubLayout>());
+
+        let superclass_ivar1 = superclass.instance_variable("ivar1").unwrap();
+        let superclass_ivar2 = superclass.instance_variable("ivar2").unwrap();
+        let superclass_ivar3 = superclass.instance_variable("ivar3").unwrap();
+        let subclass_ivar1 = subclass.instance_variable("ivar1").unwrap();
+        let subclass_ivar2 = subclass.instance_variable("ivar2").unwrap();
+        let subclass_ivar3 = subclass.instance_variable("ivar3").unwrap();
+        let subclass_ivar4 = subclass.instance_variable("ivar4").unwrap();
+
+        // Ensure that duplicate names do not conflict
+        assert_ne!(superclass_ivar1, subclass_ivar1);
+        assert_ne!(superclass_ivar2, subclass_ivar2);
+        assert_ne!(superclass_ivar3, subclass_ivar3);
+
+        // Ensure that all offsets are as expected
+        assert_eq!(
+            superclass_ivar1.offset(),
+            offset_of!(SuperLayout, ivar1) as isize
+        );
+        assert_eq!(
+            superclass_ivar2.offset(),
+            offset_of!(SuperLayout, ivar2) as isize
+        );
+        assert_eq!(
+            superclass_ivar3.offset(),
+            offset_of!(SuperLayout, ivar3) as isize
+        );
+        assert_eq!(
+            subclass_ivar1.offset(),
+            offset_of!(SubLayout, ivar1_b) as isize
+        );
+        assert_eq!(
+            subclass_ivar2.offset(),
+            offset_of!(SubLayout, ivar2_b) as isize
+        );
+        assert_eq!(
+            subclass_ivar3.offset(),
+            offset_of!(SubLayout, ivar3_b) as isize
+        );
+        assert_eq!(
+            subclass_ivar4.offset(),
+            offset_of!(SubLayout, ivar4) as isize
+        );
+
+        // Ensure our ivar loading works correctly
+        let obj: Id<NSObject> = unsafe { msg_send_id![subclass, new] };
+        let ptr = unsafe { *subclass_ivar3.load::<*const AnyObject>(&obj) };
+        assert!(ptr.is_null());
+
+        // Illustration of what goes wrong with the naive approach of loading
+        // the Ivar dynamically; in short, we can't be sure of which instance
+        // variable we're refering to here.
+        //
+        // let ivar = *obj.get_ivar::<u8>("ivar3");
+    }
 }