diff --git a/Cargo.lock b/Cargo.lock index 112412047..d5c16c02c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -34,6 +34,12 @@ dependencies = [ "winapi", ] +[[package]] +name = "autocfg" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" + [[package]] name = "basic-toml" version = "0.1.4" @@ -259,6 +265,15 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2dffe52ecf27772e601905b7522cb4ef790d2cc203488bbd0e2fe85fcb74566d" +[[package]] +name = "memoffset" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a634b1c61a95585bd15607c6ab0c4e5b226e695ff2800ba0cdccddf208c406c" +dependencies = [ + "autocfg", +] + [[package]] name = "nu-ansi-term" version = "0.46.0" @@ -282,6 +297,7 @@ version = "0.4.1" dependencies = [ "iai", "malloc_buf", + "memoffset", "objc-sys", "objc2-encode", "objc2-proc-macros", diff --git a/crates/objc2/CHANGELOG.md b/crates/objc2/CHANGELOG.md index 204f7bbfa..26211ea27 100644 --- a/crates/objc2/CHANGELOG.md +++ b/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`. * **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::("number") }; + + // After + let ivar = cls.instance_variable("number").unwrap(); + let number = unsafe { *ivar.load::(&obj) }; + ``` ### Removed * **BREAKING**: Removed `ProtocolType` implementation for `NSObject`. diff --git a/crates/objc2/Cargo.toml b/crates/objc2/Cargo.toml index 96dc18391..17c3ae6ee 100644 --- a/crates/objc2/Cargo.toml +++ b/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" diff --git a/crates/objc2/examples/introspection.rs b/crates/objc2/examples/introspection.rs index 66f62c3f9..a7c8109b0 100644 --- a/crates/objc2/examples/introspection.rs +++ b/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:?}"); } diff --git a/crates/objc2/src/declare/ivar.rs b/crates/objc2/src/declare/ivar.rs index d9dbedd74..88c802f97 100644 --- a/crates/objc2/src/declare/ivar.rs +++ b/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) -> isize { - let obj = unsafe { ptr.as_ref() }; - ivar_offset(obj.class(), Self::NAME, &Self::Type::ENCODING) + unsafe fn __ivar_ptr(ptr: NonNull) -> NonNull { + // 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 Ivar { // 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::(ptr, offset) } + unsafe { T::__ivar_ptr(ptr) } } /// Get a mutable pointer to the instance variable. @@ -238,9 +240,7 @@ impl Ivar { let ptr: NonNull = 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::(ptr, offset) } + unsafe { T::__ivar_ptr(ptr) } } /// Sets the value of the instance variable. diff --git a/crates/objc2/src/declare/mod.rs b/crates/objc2/src/declare/mod.rs index 490da7310..4a5efbdf2 100644 --- a/crates/objc2/src/declare/mod.rs +++ b/crates/objc2/src/declare/mod.rs @@ -96,8 +96,9 @@ impl 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::>("_number", Cell::new(number)); +/// *ivar.load_mut::>(this) = Cell::new(number); /// this /// }) /// } @@ -131,8 +132,9 @@ impl 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::>("_number") }.set(number); +/// unsafe { ivar.load::>(this) }.set(number); /// } /// unsafe { /// builder.add_method(sel!(setNumber:), my_number_set as extern "C" fn(_, _, _)); @@ -140,8 +142,9 @@ impl Log2Alignment for T { /// /// // 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::>("_number") }.get() +/// unsafe { ivar.load::>(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 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::()); assert!(obj1.is_kind_of::()); } + + #[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::("ivar1"); + superclass.add_ivar::("ivar2"); + superclass.add_ivar::("ivar3"); + let superclass = superclass.register(); + + let mut subclass = + ClassBuilder::new("DeclareClassDuplicateIvarSubclass", superclass).unwrap(); + // Try to overwrite instance variables + subclass.add_ivar::("ivar1"); + subclass.add_ivar::("ivar2"); + subclass.add_ivar::<*const AnyObject>("ivar3"); + subclass.add_ivar::("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::(), + ); + + #[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::() - 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::()); + + 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 = 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::("ivar3"); + } } diff --git a/crates/objc2/src/runtime/mod.rs b/crates/objc2/src/runtime/mod.rs index f30f3b496..c4187181c 100644 --- a/crates/objc2/src/runtime/mod.rs +++ b/crates/objc2/src/runtime/mod.rs @@ -332,6 +332,143 @@ impl Ivar { let encoding = unsafe { CStr::from_ptr(ffi::ivar_getTypeEncoding(self.as_ptr())) }; str::from_utf8(encoding.to_bytes()).unwrap() } + + #[inline] + pub(crate) fn debug_assert_encoding(&self, _expected: &Encoding) { + #[cfg(debug_assertions)] + { + let encoding = self.type_encoding(); + assert!( + _expected.equivalent_to_str(encoding), + "wrong encoding. Tried to retrieve ivar with encoding {encoding}, but the encoding of the given type was {_expected}", + ); + } + } + + /// Offset an object pointer to get a pointer to the ivar. + /// + /// This is kept non-generic to help monomorphization. + /// + /// + /// # Safety + /// + /// The ivar must be valid for the given object pointer. + #[inline] + pub(crate) unsafe fn offset_ptr(&self, ptr: NonNull) -> NonNull { + // `offset` is given in bytes, so we convert to `u8` and back to `T` + let ptr: NonNull = ptr.cast(); + let ptr: *mut u8 = ptr.as_ptr(); + // SAFETY: The offset is valid + let ptr: *mut u8 = unsafe { ptr.offset(self.offset()) }; + // SAFETY: The offset operation is guaranteed to not end up computing + // a NULL pointer. + let ptr: NonNull = unsafe { NonNull::new_unchecked(ptr) }; + ptr + } + + /// Returns a pointer to the instance variable / ivar with the given name. + /// + /// This is similar to [`UnsafeCell::get`], see that for more information + /// on what is and isn't safe to do. + /// + /// Usually you will have defined the instance variable yourself with + /// [`ClassBuilder::add_ivar`], the type of the ivar `T` must match the + /// type used in that. + /// + /// Attempting to access or modify private implementation details of a + /// class that you do no control using this is not supported, and may + /// invoke undefined behaviour. + /// + /// Library implementors are strongly encouraged to expose a safe + /// interface to the ivar. + /// + /// [`UnsafeCell::get`]: core::cell::UnsafeCell::get + /// [`ClassBuilder::add_ivar`]: crate::declare::ClassBuilder::add_ivar + /// + /// + /// # Panics + /// + /// May panic if the object has no ivar with the given name. May also + /// panic if the type encoding of the ivar differs from the type encoding + /// of `T`. + /// + /// This should purely seen as help while debugging and is not guaranteed + /// (e.g. it may be disabled when `debug_assertions` are off). + /// + /// + /// # Safety + /// + /// The object must have the given instance variable on it, and it must be + /// of type `T`. Any invariants that the object have assumed about the + /// value of the instance variable must not be violated. + /// + /// No thread syncronization is done on accesses to the variable, so you + /// must ensure that any access to the returned pointer do not cause data + /// races, and that Rust's mutability rules are not otherwise violated. + #[inline] + pub unsafe fn load_ptr(&self, obj: &AnyObject) -> *mut T { + self.debug_assert_encoding(&T::ENCODING); + + let ptr = NonNull::from(obj); + // SAFETY: That the ivar is valid is ensured by the caller + let ptr = unsafe { self.offset_ptr(ptr) }; + let ptr: NonNull = ptr.cast(); + + // Safe as *mut T because `self` is `UnsafeCell` + ptr.as_ptr() + } + + /// Returns a reference to the instance variable with the given name. + /// + /// See [`Ivar::load_ptr`] for more information, including on when + /// this panics. + /// + /// + /// # Safety + /// + /// The object must have the given instance variable on it, and it must be + /// of type `T`. + /// + /// Note that an object can have multiple instance variables with the same + /// name; TODO + /// + /// No thread syncronization is done, so you must ensure that no other + /// thread is concurrently mutating the variable. This requirement can be + /// considered upheld if all mutation happens through + /// [`Ivar::load_mut`] (since that takes `&mut self`). + #[inline] + pub unsafe fn load<'obj, T: Encode>(&self, obj: &'obj AnyObject) -> &'obj T { + // SAFETY: That the ivar is valid as `&T` is ensured by the caller, + // and the reference is properly bound to the object. + unsafe { self.load_ptr::(obj).as_ref().unwrap_unchecked() } + } + + /// Returns a mutable reference to the ivar with the given name. + /// + /// See [`Ivar::load_ptr`] for more information, including on when + /// this panics. + /// + /// + /// # Safety + /// + /// The object must have an instance variable with the given name, and it + /// must be of type `T`. + /// + /// This access happens through `&mut Self`, which means we know it to be + /// the only reference, hence you do not need to do any work to ensure + /// that data races do not happen. + #[inline] + pub unsafe fn load_mut<'obj, T: Encode>(&self, obj: &'obj mut AnyObject) -> &'obj mut T { + self.debug_assert_encoding(&T::ENCODING); + + let ptr = NonNull::from(obj); + // SAFETY: That the ivar is valid is ensured by the caller + let ptr = unsafe { self.offset_ptr(ptr) }; + let mut ptr: NonNull = ptr.cast(); + + // SAFETY: That the ivar is valid as `&mut T` is ensured by the caller + unsafe { ptr.as_mut() } + } } standard_pointer_impls!(Ivar); @@ -710,6 +847,9 @@ impl AnyClass { /// Returns the ivar for a specified instance variable of self, or /// [`None`] if self has no ivar with the given name. + /// + /// If the instance variable was not found on the specified class, the + /// superclasses are searched. #[doc(alias = "class_getInstanceVariable")] pub fn instance_variable(&self, name: &str) -> Option<&Ivar> { let name = CString::new(name).unwrap(); @@ -998,20 +1138,6 @@ impl fmt::Display for AnyProtocol { } } -pub(crate) fn ivar_offset(cls: &AnyClass, name: &str, expected: &Encoding) -> isize { - match cls.instance_variable(name) { - Some(ivar) => { - let encoding = ivar.type_encoding(); - assert!( - expected.equivalent_to_str(encoding), - "wrong encoding. Tried to retrieve ivar with encoding {encoding}, but the encoding of the given type was {expected}", - ); - ivar.offset() - } - None => panic!("ivar {name} not found on class {cls}"), - } -} - /// An Objective-C object. /// /// This is slightly different from [`NSObject`] in that it may represent an @@ -1113,157 +1239,41 @@ impl AnyObject { old_cls } - /// Offset an object pointer to get a pointer to an ivar. - /// - /// - /// # Safety - /// - /// The offset must be valid for the given type. - #[inline] - pub(crate) unsafe fn ivar_at_offset(ptr: NonNull, offset: isize) -> NonNull { - // `offset` is given in bytes, so we convert to `u8` and back to `T` - let ptr: NonNull = ptr.cast(); - let ptr: *mut u8 = ptr.as_ptr(); - // SAFETY: The offset is valid - let ptr: *mut u8 = unsafe { ptr.offset(offset) }; - // SAFETY: The offset operation is guaranteed to not end up computing - // a NULL pointer. - let ptr: NonNull = unsafe { NonNull::new_unchecked(ptr) }; - let ptr: NonNull = ptr.cast(); - ptr - } - - /// Returns a pointer to the instance variable / ivar with the given name. - /// - /// This is similar to [`UnsafeCell::get`], see that for more information - /// on what is and isn't safe to do. - /// - /// Usually you will have defined the instance variable yourself with - /// [`ClassBuilder::add_ivar`], the type of the ivar `T` must match the - /// type used in that. - /// - /// Attempting to access or modify private implementation details of a - /// class that you do no control using this is not supported, and may - /// invoke undefined behaviour. - /// - /// Library implementors are strongly encouraged to expose a safe - /// interface to the ivar. - /// - /// [`UnsafeCell::get`]: core::cell::UnsafeCell::get - /// [`ClassBuilder::add_ivar`]: crate::declare::ClassBuilder::add_ivar - /// - /// - /// # Panics - /// - /// May panic if the object has no ivar with the given name. May also - /// panic if the type encoding of the ivar differs from the type encoding - /// of `T`. - /// - /// This should purely seen as help while debugging and is not guaranteed - /// (e.g. it may be disabled when `debug_assertions` are off). - /// - /// - /// # Safety - /// - /// The object must have an instance variable with the given name, and it - /// must be of type `T`. Any invariants that the object have assumed about - /// the value of the instance variable must not be violated. - /// - /// No thread syncronization is done on accesses to the variable, so you - /// must ensure that any access to the returned pointer do not cause data - /// races, and that Rust's mutability rules are not otherwise violated. - pub unsafe fn ivar_ptr(&self, name: &str) -> *mut T { - let offset = ivar_offset(self.class(), name, &T::ENCODING); - - let ptr = NonNull::from(self); - // SAFETY: The offset is valid - let ptr = unsafe { Self::ivar_at_offset::(ptr, offset) }; - - // Safe as *mut T because `self` is `UnsafeCell` - ptr.as_ptr() + pub(crate) fn lookup_instance_variable_dynamically(&self, name: &str) -> &'static Ivar { + let cls = self.class(); + cls.instance_variable(name) + .unwrap_or_else(|| panic!("ivar {name} not found on class {cls}")) } - /// Returns a reference to the instance variable with the given name. - /// - /// See [`AnyObject::ivar_ptr`] for more information, including on when - /// this panics. + /// Use [`Ivar::load`] instead. /// /// /// # Safety /// /// The object must have an instance variable with the given name, and it - /// must be of type `T`. + /// must be of type `T`. /// - /// No thread syncronization is done, so you must ensure that no other - /// thread is concurrently mutating the variable. This requirement can be - /// considered upheld if all mutation happens through - /// [`AnyObject::ivar_mut`] (since that takes `&mut self`). - pub unsafe fn ivar(&self, name: &str) -> &T { - // SAFETY: Upheld by caller. - unsafe { self.ivar_ptr::(name).as_ref().unwrap_unchecked() } - } - - /// Use [`AnyObject::ivar`] instead. - /// - /// - /// # Safety - /// - /// See [`AnyObject::ivar`]. - #[deprecated = "Use `AnyObject::ivar` instead."] + /// See [`Ivar::load`] for details surrounding this. + #[deprecated = "this is difficult to use correctly, use `Ivar::load` instead."] pub unsafe fn get_ivar(&self, name: &str) -> &T { + let ivar = self.lookup_instance_variable_dynamically(name); // SAFETY: Upheld by caller - unsafe { self.ivar::(name) } + unsafe { ivar.load::(self) } } - /// Returns a mutable reference to the ivar with the given name. - /// - /// See [`AnyObject::ivar_ptr`] for more information, including on when - /// this panics. + /// Use [`Ivar::load_mut`] instead. /// /// /// # Safety /// - /// The object must have an instance variable with the given name, and it - /// must be of type `T`. + /// Same as [`Ivar::load_mut`]. /// - /// This access happens through `&mut self`, which means we know it to be - /// the only reference, hence you do not need to do any work to ensure - /// that data races do not happen. - pub unsafe fn ivar_mut(&mut self, name: &str) -> &mut T { - let offset = ivar_offset(self.class(), name, &T::ENCODING); - - let ptr = NonNull::from(self); - // SAFETY: The offset is valid - let mut ptr = unsafe { Self::ivar_at_offset::(ptr, offset) }; - - // SAFETY: - unsafe { ptr.as_mut() } - } - - /// Use [`AnyObject::ivar_mut`] instead. - /// - /// - /// # Safety - /// - /// Same as [`AnyObject::ivar_mut`]. - #[deprecated = "Use `AnyObject::ivar_mut` instead."] + /// TODO + #[deprecated = "this is difficult to use correctly, use `Ivar::load_mut` instead."] pub unsafe fn get_mut_ivar(&mut self, name: &str) -> &mut T { + let ivar = self.lookup_instance_variable_dynamically(name); // SAFETY: Upheld by caller - unsafe { self.ivar_mut::(name) } - } - - /// Sets the value of the ivar with the given name. - /// - /// This is a shorthand for [`AnyObject::ivar_mut`], see that for more - /// information. - /// - /// - /// # Safety - /// - /// Same as [`AnyObject::ivar_mut`]. - pub unsafe fn set_ivar(&mut self, name: &str, value: T) { - // SAFETY: Invariants upheld by caller - unsafe { *self.ivar_mut::(name) = value }; + unsafe { ivar.load_mut::(self) } } // objc_setAssociatedObject @@ -1475,27 +1485,32 @@ mod tests { #[test] fn test_object() { let mut obj = test_utils::custom_object(); - assert_eq!(obj.class(), test_utils::custom_class()); + let cls = test_utils::custom_class(); + assert_eq!(obj.class(), cls); - let result = unsafe { - obj.set_ivar::("_foo", 4); - *obj.ivar::("_foo") - }; + let ivar = cls.instance_variable("_foo").unwrap(); + + unsafe { *ivar.load_mut::(&mut obj) = 4 }; + let result = unsafe { *ivar.load::(&obj) }; assert_eq!(result, 4); } #[test] - #[should_panic = "ivar unknown not found on class CustomObject"] fn test_object_ivar_unknown() { - let obj = test_utils::custom_object(); - let _ = unsafe { *obj.ivar::("unknown") }; + let cls = test_utils::custom_class(); + assert_eq!(cls.instance_variable("unknown"), None); } #[test] - #[should_panic = "wrong encoding. Tried to retrieve ivar with encoding I, but the encoding of the given type was C"] + #[cfg_attr( + debug_assertions, + should_panic = "wrong encoding. Tried to retrieve ivar with encoding I, but the encoding of the given type was C" + )] fn test_object_ivar_wrong_type() { let obj = test_utils::custom_object(); - let _ = unsafe { *obj.ivar::("_foo") }; + let cls = test_utils::custom_class(); + let ivar = cls.instance_variable("_foo").unwrap(); + let _ = unsafe { *ivar.load::(&obj) }; } #[test] diff --git a/crates/objc2/src/test_utils.rs b/crates/objc2/src/test_utils.rs index 6b0e964b2..efd66a82d 100644 --- a/crates/objc2/src/test_utils.rs +++ b/crates/objc2/src/test_utils.rs @@ -93,15 +93,18 @@ pub(crate) fn custom_class() -> &'static AnyClass { } extern "C" fn custom_obj_set_foo(this: &mut AnyObject, _cmd: Sel, foo: u32) { - unsafe { this.set_ivar::("_foo", foo) } + let ivar = this.class().instance_variable("_foo").unwrap(); + unsafe { *ivar.load_mut::(this) = foo } } extern "C" fn custom_obj_get_foo(this: &AnyObject, _cmd: Sel) -> u32 { - unsafe { *this.ivar::("_foo") } + let ivar = this.class().instance_variable("_foo").unwrap(); + unsafe { *ivar.load::(this) } } extern "C" fn custom_obj_get_foo_reference(this: &AnyObject, _cmd: Sel) -> &u32 { - unsafe { this.ivar::("_foo") } + let ivar = this.class().instance_variable("_foo").unwrap(); + unsafe { ivar.load::(this) } } extern "C" fn custom_obj_get_struct(_this: &AnyObject, _cmd: Sel) -> CustomStruct { @@ -118,9 +121,8 @@ pub(crate) fn custom_class() -> &'static AnyClass { } extern "C" fn custom_obj_set_bar(this: &mut AnyObject, _cmd: Sel, bar: u32) { - unsafe { - this.set_ivar::("_foo", bar); - } + let ivar = this.class().instance_variable("_bar").unwrap(); + unsafe { *ivar.load_mut::(this) = bar } } extern "C" fn custom_obj_add_number_to_number( diff --git a/crates/objc2/tests/declare_class.rs b/crates/objc2/tests/declare_class.rs index 329a500f6..08a9d0622 100644 --- a/crates/objc2/tests/declare_class.rs +++ b/crates/objc2/tests/declare_class.rs @@ -2,7 +2,7 @@ use core::ptr::{self, NonNull}; use objc2::declare::IvarEncode; -use objc2::mutability::{Immutable, Mutable}; +use objc2::mutability::Immutable; use objc2::rc::Id; use objc2::runtime::NSObject; use objc2::{declare_class, extern_methods, sel, ClassType}; @@ -388,71 +388,6 @@ fn test_duplicate_ivar() { let _ = DeclareClassDuplicateIvar::class(); } -#[test] -#[should_panic = "instance variable \"ivar\" already exists on a superclass"] -fn test_subclass_duplicate_ivar() { - declare_class!( - struct Cls { - ivar_superclass: IvarEncode, - } - - mod ivars; - - unsafe impl ClassType for Cls { - type Super = NSObject; - type Mutability = Mutable; - const NAME: &'static str = "DeclareClassDuplicateIvarSuperclass"; - } - ); - - declare_class!( - struct SubCls { - ivar_subclass: IvarEncode, - } - - mod ivars_subclass; - - unsafe impl ClassType for SubCls { - type Super = Cls; - type Mutability = Mutable; - const NAME: &'static str = "DeclareClassDuplicateIvarSubclass"; - } - ); - - extern_methods!( - unsafe impl SubCls { - #[method_id(new)] - fn new() -> Id; - } - ); - - let _ = SubCls::class(); - - // The rest is to show what would go wrong if it didn't panic - - assert_eq!(Cls::class().instance_size(), 16); - assert_eq!(SubCls::class().instance_size(), 16); - - let mut obj = SubCls::new(); - - // Zero-initialized - assert_eq!(*obj.ivar_superclass, 0); - assert_eq!(*obj.ivar_subclass, 0); - - *obj.ivar_subclass = 2; - - assert_eq!(*obj.ivar_superclass, 2); - assert_eq!(*obj.ivar_subclass, 2); - - *obj.ivar_superclass = 3; - - assert_eq!(*obj.ivar_superclass, 3); - assert_eq!(*obj.ivar_subclass, 3); - - let ivar_dynamically = unsafe { obj.ivar::("ivar") }; - assert_eq!(*ivar_dynamically, 3); -} - declare_class!( #[derive(Debug)] struct OutParam; diff --git a/crates/test-assembly/crates/test_declare_class/expected/apple-aarch64.s b/crates/test-assembly/crates/test_declare_class/expected/apple-aarch64.s index 5650ba07e..00cce80b9 100644 --- a/crates/test-assembly/crates/test_declare_class/expected/apple-aarch64.s +++ b/crates/test-assembly/crates/test_declare_class/expected/apple-aarch64.s @@ -33,18 +33,18 @@ Lloh5: Lloh6: add x1, x1, l_anon.[ID].11@PAGEOFF Lloh7: - adrp x5, l_anon.[ID].12@PAGE + adrp x5, l_anon.[ID].14@PAGE Lloh8: - add x5, x5, l_anon.[ID].12@PAGEOFF + add x5, x5, l_anon.[ID].14@PAGEOFF mov x0, sp mov w2, #4 mov w3, #1 mov w4, #0 bl SYM(objc2::declare::ClassBuilder::add_ivar_inner_mono::GENERATED_ID, 0) Lloh9: - adrp x1, l_anon.[ID].13@PAGE + adrp x1, l_anon.[ID].12@PAGE Lloh10: - add x1, x1, l_anon.[ID].13@PAGEOFF + add x1, x1, l_anon.[ID].12@PAGEOFF Lloh11: adrp x19, l_anon.[ID].2@PAGE Lloh12: @@ -383,30 +383,22 @@ _access_ivars: add x29, sp, #32 bl _get_obj mov x19, x0 - bl _object_getClass Lloh88: adrp x1, l_anon.[ID].11@PAGE Lloh89: add x1, x1, l_anon.[ID].11@PAGEOFF -Lloh90: - adrp x3, l_anon.[ID].12@PAGE -Lloh91: - add x3, x3, l_anon.[ID].12@PAGEOFF mov w2, #4 - bl SYM(objc2::runtime::ivar_offset::GENERATED_ID, 0) + bl SYM(objc2::runtime::AnyObject::lookup_instance_variable_dynamically::GENERATED_ID, 0) + bl _ivar_getOffset ldrb w20, [x19, x0] +Lloh90: + adrp x1, l_anon.[ID].12@PAGE +Lloh91: + add x1, x1, l_anon.[ID].12@PAGEOFF mov x0, x19 - bl _object_getClass -Lloh92: - adrp x1, l_anon.[ID].13@PAGE -Lloh93: - add x1, x1, l_anon.[ID].13@PAGEOFF -Lloh94: - adrp x3, l_anon.[ID].2@PAGE -Lloh95: - add x3, x3, l_anon.[ID].2@PAGEOFF mov w2, #4 - bl SYM(objc2::runtime::ivar_offset::GENERATED_ID, 0) + bl SYM(objc2::runtime::AnyObject::lookup_instance_variable_dynamically::GENERATED_ID, 0) + bl _ivar_getOffset ldr x21, [x19, x0] mov x0, x19 bl _objc_release @@ -416,8 +408,6 @@ Lloh95: ldp x20, x19, [sp, #16] ldp x22, x21, [sp], #48 ret - .loh AdrpAdd Lloh94, Lloh95 - .loh AdrpAdd Lloh92, Lloh93 .loh AdrpAdd Lloh90, Lloh91 .loh AdrpAdd Lloh88, Lloh89 @@ -427,16 +417,16 @@ SYM(::class::REGISTER_CLASS, 0)@PAGE -Lloh97: +Lloh93: add x8, x8, SYM(::class::REGISTER_CLASS, 0)@PAGEOFF ldapr x8, [x8] cmp x8, #3 b.ne LBB7_3 -Lloh98: +Lloh94: adrp x0, l_anon.[ID].16@PAGE -Lloh99: +Lloh95: add x0, x0, l_anon.[ID].16@PAGEOFF mov w1, #15 bl SYM(objc2::runtime::AnyClass::get::GENERATED_ID, 0) @@ -450,47 +440,47 @@ LBB7_3: strb w8, [sp, #7] add x8, sp, #7 str x8, [sp, #8] -Lloh100: +Lloh96: adrp x0, SYM(::class::REGISTER_CLASS, 0)@PAGE -Lloh101: +Lloh97: add x0, x0, SYM(::class::REGISTER_CLASS, 0)@PAGEOFF -Lloh102: +Lloh98: adrp x3, l_anon.[ID].0@PAGE -Lloh103: +Lloh99: add x3, x3, l_anon.[ID].0@PAGEOFF -Lloh104: +Lloh100: adrp x4, l_anon.[ID].15@PAGE -Lloh105: +Lloh101: add x4, x4, l_anon.[ID].15@PAGEOFF add x2, sp, #8 mov w1, #0 bl SYM(std::sys_common::once::queue::Once::call::GENERATED_ID, 0) -Lloh106: +Lloh102: adrp x0, l_anon.[ID].16@PAGE -Lloh107: +Lloh103: add x0, x0, l_anon.[ID].16@PAGEOFF mov w1, #15 bl SYM(objc2::runtime::AnyClass::get::GENERATED_ID, 0) cbnz x0, LBB7_2 LBB7_4: -Lloh108: +Lloh104: adrp x0, l_anon.[ID].8@PAGE -Lloh109: +Lloh105: add x0, x0, l_anon.[ID].8@PAGEOFF -Lloh110: +Lloh106: adrp x2, l_anon.[ID].15@PAGE -Lloh111: +Lloh107: add x2, x2, l_anon.[ID].15@PAGEOFF mov w1, #43 bl SYM(core::panicking::panic::GENERATED_ID, 0) - .loh AdrpAdd Lloh96, Lloh97 + .loh AdrpAdd Lloh92, Lloh93 + .loh AdrpAdd Lloh94, Lloh95 + .loh AdrpAdd Lloh102, Lloh103 + .loh AdrpAdd Lloh100, Lloh101 .loh AdrpAdd Lloh98, Lloh99 + .loh AdrpAdd Lloh96, Lloh97 .loh AdrpAdd Lloh106, Lloh107 .loh AdrpAdd Lloh104, Lloh105 - .loh AdrpAdd Lloh102, Lloh103 - .loh AdrpAdd Lloh100, Lloh101 - .loh AdrpAdd Lloh110, Lloh111 - .loh AdrpAdd Lloh108, Lloh109 .p2align 2 SYM(::class::{closure#0}::__objc2_dealloc, 0): @@ -500,26 +490,22 @@ SYM(::call_once::< &c_int { - unsafe { self.inner.ivar("var1") } + let ivar = Self::class().instance_variable("var1").unwrap(); + unsafe { ivar.load(&self.inner) } } fn var1_ivar_mut(&mut self) -> &mut c_int { - unsafe { self.inner.ivar_mut("var1") } + let ivar = Self::class().instance_variable("var1").unwrap(); + unsafe { ivar.load_mut(&mut self.inner) } } fn add_to_ivar1(&mut self, number: c_int) { @@ -154,11 +156,13 @@ impl MyTestObject { } fn var2_ivar(&self) -> &Bool { - unsafe { self.inner.ivar("var2") } + let ivar = Self::class().instance_variable("var2").unwrap(); + unsafe { ivar.load(&self.inner) } } fn var2_ivar_mut(&mut self) -> &mut Bool { - unsafe { self.inner.ivar_mut("var2") } + let ivar = Self::class().instance_variable("var2").unwrap(); + unsafe { ivar.load_mut(&mut self.inner) } } fn var3(&self) -> *mut AnyObject { @@ -170,11 +174,13 @@ impl MyTestObject { } fn var3_ivar(&self) -> &*mut AnyObject { - unsafe { self.inner.ivar("var3") } + let ivar = Self::class().instance_variable("var3").unwrap(); + unsafe { ivar.load(&self.inner) } } fn var3_ivar_mut(&mut self) -> &mut *mut AnyObject { - unsafe { self.inner.ivar_mut("var3") } + let ivar = Self::class().instance_variable("var3").unwrap(); + unsafe { ivar.load_mut(&mut self.inner) } } }