madsmtm / objc2

Bindings to Apple's frameworks in Rust
https://docs.rs/objc2/
MIT License
348 stars 40 forks source link

Public `AnyClass::as_ptr` method #502

Closed glowcoil closed 2 weeks ago

glowcoil commented 1 year ago

It would be useful to have a public AnyClass::as_ptr method to get the underlying *const objc_class, analogous to the Sel::as_ptr method.

Some info about my use case: I'm currently working on a windowing library for use in audio plugins (https://github.com/coupler-rs/portlight), and I'm evaluating switching from objc and cocoa to objc2 and icrate (my impression has been very positive so far!). To avoid naming collisions due to the fact that Objective-C has a single global class namespace, I'm currently creating an NSView subclass with a random UUID in the name and then deregistering it when the plugin GUI is destroyed (this is what other plugin frameworks like JUCE do as well). Currently, in order to deregister a class declared using objc2's ClassBuilder API, I would have to use objc_getClass to get the *const objc_class by name and then pass it to objc_disposeClassPair. That's basically fine, but it feels a little silly, and being able to get the *const objc_class directly from an &AnyClass would be nice.

madsmtm commented 1 year ago

Related: https://github.com/madsmtm/objc2/issues/251, where I recently decided to not (safely) support deregistering classes.

You can quite easily get the *mut objc_class you need, it's just a pointer cast away (and this will continue to work, since we need the ABI of &AnyClass to match *const objc_class. I think I've somewhat guaranteed this in the docs, but it's referencing the Objective-C alias Class, which I agree is not entirely clear. In general, most of the types in objc2 have stable and "obvious" ABIs).

let cls: &AnyClass;
let ptr: *const AnyClass = cls; // Coerce reference to pointer
let ptr: *mut objc_class = ptr.cast().cast_mut(); // Cast pointer

But I agree a dedicated method is easier to understand, I'd probably accept a PR to add that (would also be useful on the other runtime types).

Alternatively, we could add a pub unsafe fn deregister(&self) that calls objc_disposeClassPair, to have a place to document the safety guarantees that one must uphold.

glowcoil commented 1 year ago

That makes sense; I was hesitant to use the pointer cast approach since it wasn't clear to me from the docs that that could be relied upon by consumers of the API.

Happy to file a PR to add the as_ptr method. A deregister method could be nice, but it's not obvious to me that it would be sound for it to take a reference as opposed to a pointer.

madsmtm commented 1 year ago

Hmm, you might be right that an unsafe fn deregister(&self) method is kinda problematic, since the lifetime of the class is not tied to anything, and Rust might assume that it is still valid after the method has been called.

But then again, so is deregistering the class in any other way, see e.g. the following:

let cls: &AnyClass = class!("MyClassThatIKnowNoOneIsUsingAnymore");
let ptr: *mut ffi::objc_class = cls.as_ptr();
// SAFETY: The class is never used beyond this point
unsafe { objc_disposeClassPair(ptr) };
// Ooops! The `cls` variable is still technically valid at the end of the scope here!

I can't really remember the rules about reference validity in such a case, but I think that this might still be an active area of research for the unsafe code guidelines.

In any case, whether one does the above or uses an unsafe fn deregister(&self), I think the end result is the same.

madsmtm commented 1 year ago

I think it can somewhat be compared to destroying resources created via. Box::leak? See e.g. this gist, running that under Miri fairly clearly shows that destroying references before their "real" lifetime ends is sound (although of course a huge footgun if you do end up using the reference after it has been destroyed).

madsmtm commented 2 weeks ago

I have merged objc_class and AnyClass in https://github.com/madsmtm/objc2/pull/648, so this should no longer be an issue.

A deregister method might be useful, but I'll defer that to later, since I'm undecided if I'd actually rather move methods away from AnyClass, to make it match the C API more.