Soundess issues

[madsmtm]

From a quick skim, I found the following important safety mistakes, that leads to unsound code (unsoundness = a safe abstract which a downstream user can use to violate memory safety without writing unsafe themselves).

Note that I completely understand the reasoning behind these mistakes, most have only been fixed in objc2 very recently!

• arc::Retain<T> allows access to &mut T (through DerefMut), which is not safe for almost all classes.

  use cidre::ns;
  
  let mut s1 = ns::String::with_str("already lowercase");
  let mut s2 = s1.retained();
  let s1: &mut ns::String = &mut *s1;
  let s2: &mut ns::String = &mut *s2;
  // The mutable references s1 and s2 now alias

• Autoreleased references are not bound to a pool, meaning that they can be accessed after they've been reclaimed by the pool.

  use cidre::{ns, objc};
  
  let s = ns::String::with_str("My String");
  let lowercased = objc::autoreleasepool(|| {
      s.lowercased_ar()
      // The newly created string is released and deallocated here
  });
  // But we can still use it here
  println!("{lowercased}");

• Message sending to methods in the new family leaks, since new returns an object with +1 retain count.

  use cidre::mtl;
  let device = mtl::Device::default().unwrap();
  
  // Created with +1 retain count, and retained, so it ends up at +2
  let cmd_queue = device.new_cmd_queue();
  // Retain count decremented only once
  drop(cmd_queue);
  // The CmdQueue is leaked

[yury]

Wow, so many feedback :) Thank you!

I will start with last one:

Message sending to methods in the new family leaks, since new returns an object with +1 retain count.

There is not leak here. cidre does ref count transfer.

I was thinking how to proper model objc_autoreleaseReturnValue/objc_retainAutoreleasedReturnValue.
So I decided to go with _ar suffix to indicate that returned value is autoreleased return value and next function called without _ar suffix and it returns retained autoreleased return value.

cidre/cidre/src/mtl/device.rs

Lines 110 to 114 in c6160b4

	#[objc::msg_send(newCommandQueue)]
	pub fn new_cmd_queue_ar(&self) -> Option<&'ar CmdQueue>;
	#[objc::rar_retain()]
	pub fn new_cmd_queue(&self) -> Option<arc::R<CmdQueue>>;

See #[objc::rar_retain()]

Yes it doubles API's, but user can decide to go with &'ar versions to remove deallocations in critical render loops and do them after render submission.

[madsmtm]

That's true for most selectors, but since the selector newCommandQueue starts with new, it's return value implicitly has +1 retain count, see the clang documentation on retained return values and this section in objc2::msg_send_id!.

[madsmtm]

I.e. you don't need to retain things that is returned by new, rather you have the responsibility to release it when you're done.

[madsmtm]

Btw, the reason I've mostly avoided using autoreleased references (apart from ergonomics) is that they're as far as I could find out, slower than doing the retain/release with objc_retainAutoreleasedReturnValue - I suspect because the cost of putting something into the autorelease pool is fairly high, which can otherwise be avoided with objc_retainAutoreleasedReturnValue?

(Do note that these measurements were made a while ago, and on older hardware, so details may vary on your device).

[yury]

Yep, you right about new. Will fix it. Thank you.

[yury]

About first one.

I wanna to do mut for setters. So may be it is worth it. What do you think?

[madsmtm]

Yeah, I understand, and it is sound for some classes, but not for others. The solution I've gone with is to "classify" ever type as (basically) either mutable or using interior mutability. So e.g. NSView is marked as using interior mutability, while NsMutableString is marked as using normal mutability.

In turn, this allows retain-ing NSView, and allows &mut access to NSMutableString (with the important point (/ restriction) being that there is only ever one retained/owning pointer to a NSMutableString.

See the objc2::mutability module for details.

This is probably not the only option, so feel free to experiment with others!

[madsmtm]

Also, your solution for autoreleased references is not enough, e.g. one could just do the following instead:

use cidre::{ns, objc};

let s = ns::String::with_str("My String");
let lowercased;
objc::ar_pool(|| {
    lowercased = s.lowercased_ar();
    // The newly created string is released and deallocated here
});
// But we can still use it here
println!("{lowercased}");

See objc2::rc::autoreleasepool for further details on what must be disallowed, and on the solution. I've written about this at length before, see SSheldon/rust-objc#103 and the backreferences to that PR.