Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add downcasting capability based on isKindOfClass: #474

Merged
merged 13 commits into from
Sep 18, 2024

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Jul 26, 2023

Add downcasting capability based on isKindOfClass:.

@cynecx cynecx changed the title Add NSObjectProtocol::is_member_of and some related "downcasting" helpers Add NSObjectProtocol::is_member_of and some related "downcasting" helpers Jul 26, 2023
crates/objc2/src/runtime/nsobject.rs Outdated Show resolved Hide resolved
crates/objc2/src/runtime/nsobject.rs Outdated Show resolved Hide resolved
crates/objc2/src/runtime/nsobject.rs Outdated Show resolved Hide resolved
@madsmtm
Copy link
Owner

madsmtm commented Jul 27, 2023

Another danger is generic types! E.g. there are no safeguards to prevent you from getting NSArray<NSString> from e.g. NSArray<NSData>. The related restriction to 'static types you've made so far is a good idea.

Also wanted to say that I agree that this feature is useful, and would like to get it somehow, it just needs a bit more thinking to make sure it's sound!

@cynecx
Copy link
Contributor Author

cynecx commented Jul 27, 2023

Another danger is generic types!

😅 I have to admit I've started working with this crate since yesterday and didn't even realize that generics were a thing here.

There is one option where one could encode the "genericity" in ClassType and simply disallow such casts when the destination class contains any generics:

pub trait impl(self) Genericity {}

pub struct Generic(!);
impl Genericity for Generic {}

pub struct NotGeneric(!);
impl Genericity for NotGeneric {}

pub unsafe trait ClassType: Message {
    ...

    type Genericity: Genericity;
}

impl AnyObject {
    fn 🚲<T>(&self) -> Option<&T>
    where
        Self: 'static,
        T: ClassType<Genericity = NotGeneric> + 'static,
    {
        if self.class() == T::class() { // Or isKindOfClass
            // SAFETY: self is a direct member of the specified (`T`) class.
            // Therefore casting should be safe.
            Some(unsafe { &*(self as *const Self).cast::<T>() })
        } else {
            None
        }
    }
}

@madsmtm
Copy link
Owner

madsmtm commented Jul 28, 2023

one could encode the "genericity" in ClassType

Hmm... Yeah, that would go a long way in ensuring things are sound!

Although this does exclude us from doing NSObject -> NSArray<AnyObject>, which would be nice to support, so perhaps another approach would be better? Maybe we could add a new trait BikeshedDowncastAble which is implemented automatically in extern_class! and declare_class!, but for generic types we only implement it with the generic parameter being AnyObject?

Also, I'm considering whether it would be possible to not restrict this to NSObjectProtocol, such that you could also use it on AnyObject? Would be prudent to research what Swift does under the hood for their type casting support.

@madsmtm
Copy link
Owner

madsmtm commented Jul 28, 2023

Something that would help my confidence in the soundness of whatever implementation we end up with, maybe we can assemble a list of allowed and disallowed cases (that could be added as (UI) tests)?

@cynecx
Copy link
Contributor Author

cynecx commented Jul 28, 2023

Also, I'm considering whether it would be possible to not restrict this to NSObjectProtocol, such that you could also use it on AnyObject?

I think it might be possible. The objc runtime exposes an internal api called objc_opt_isKindOfClass:

Digging further it seems like NSObject's isKindOfClass implementation is also very similar to objc_opt_isKindOfClass one (And in some cases it just passes through the call to NSObject::isKindOfClass).

However, I am not quite sure about "how internal" that api is since it doesn't have an underscore like many other internal but yet exposed objc runtime apis. But it seems like llvm does an optimization where it transforms [obj isKindOfClass] calls to objc_opt_isKindOfClass calls (https://lldb.llvm.org/cpp_reference/AppleObjCTrampolineHandler_8cpp_source.html). Perhaps that's enough for us to consider using this.

Also if we were to use this, we'd also have to bump our min. MACOSX_DEPLOYMENT_TARGET/... requirements since this is only available since macos 10.15, ... Or we'd have to put this behind a feature flag.

@madsmtm
Copy link
Owner

madsmtm commented Jul 28, 2023

I think objc_opt_isKindOfClass is just an optimization, and yeah, I am not inclined to bump the deployment target requirement just yet.

The problem with calling isKindOfClass: on AnyObject though is that if the object does not subclass NSObject, then the method may not be available! But maybe we can just re-implement the loop ourselves in Rust?

I tried checking what Swift does, it seems like an expression like obj as? NSString (where obj: AnyObject) uses the runtime function swift_dynamicCastObjCClass, which doesn't actually check whether isKindOfClass: is implemented. So maybe we don't really need to bother, since custom root classes are expected to be so rare anyhow? Besides, it's not unsound, it just throws an exception.

@cynecx cynecx marked this pull request as draft August 15, 2023 00:24
@cynecx cynecx changed the title Add NSObjectProtocol::is_member_of and some related "downcasting" helpers Add downcasting capability based on isKindOfClass: Aug 15, 2023
@cynecx
Copy link
Contributor Author

cynecx commented Aug 15, 2023

Converted to a draft because it's probably still missing something (among others, ui-tests might be worthwhile, renaming downcasting -> cast?).

@madsmtm
Copy link
Owner

madsmtm commented Sep 22, 2023

I tried the following Swift code:

import Foundation

let testConversionArray: AnyObject = NSArray.init(objects: NSString(), NSObject())

if let inner = testConversionArray as? [NSString] {
    print("is NSString array", inner[0].length)
} else if let inner = testConversionArray as? [NSObject] {
    print("is NSObject array", inner[0])
} else {
    print("not array")
}

And it is able to detect that the array is actually a NSObject and not an NSString array.

The way that works is by checking every element of the array though, which is probably unacceptable performance-wise in Rust (I'd expect a downcast to be very cheap).

So in Rust users will probably have to do the cast more explicitly:

let array: &AnyObject = ...;
let array: &NSArray<AnyObject> = array.downcast()?;
for item in array {
    let item: &NSString = item.downcast()?;
    // Use item here.
}

Tl;dr: I still think we should disallow casts on generic items, except for when the generics are set to AnyObject.

@madsmtm
Copy link
Owner

madsmtm commented Jan 16, 2024

Returning back to this:

Decision: We shouldn't really care about AnyObject not being guaranteed to support isKindOfClass:. If feasible, we can try to insert extra checks (as is done in current impl), perhaps only with debug_assertions enabled, but it's not like it's unsound if we don't, the runtime is just gonna throw an exception (actually done by CoreFoundation, if that is not linked then we'll get an abort).

Decision: I think the name "downcast" makes sense, that is also Swift's terminology, and matches the equivalent Rust concept of <Box<dyn Any>>::downcast.

Decision: We also want some sort of Id::downcast<U: DowncastTarget>(this: Id<T>) -> Result<Id<U>, Id<T>>, though unsure of the exact bound here yet.

Unsure how downcasting from ProtocolObject should work? ProtocolObject::downcast? Or should downcast be a method on Message? Can we downcast to a ProtocolObject (probably doable with conformsToProtocol:), or should that be handled by something else?

Regarding mutability: An explicit design decision in objc2 is that once you type-erase a mutable object, you are allowed to do normal reference-counting with it (that's also an implicit assumption in Objective-C, e.g. classes are allowed to pass NSString in their public API, while actually giving access to a NSMutableString). I feel this still needs more thought (and tests) before I'm confident that the way we do this is sound.

Also unsure if we could add some kind of downcast_mut? Where could it be used? Is &mut NSString -> &mut NSMutableString fine? Or is it only possible for mutable to mutable, e.g. &mut NSMutableData -> &mut NSPurgeableData?

I also still have to consider generics some more. Likely we'll be implementing the trait for NSArray<AnyObject>, but that still makes it hard to access e.g. a type-erased NSMutableArray<NSMutableString> - But then again, that's probably how it should be, as after that object is type-erased, there is no way to know if it's not shared elsewhere.


I'll try to make a collection of examples and UI tests in one of the coming days to move forwards here.

@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels Jan 16, 2024
@madsmtm madsmtm marked this pull request as ready for review September 18, 2024 16:12
@madsmtm
Copy link
Owner

madsmtm commented Sep 18, 2024

I've finally been able to come back to this properly, I've waited so long because I wanted to remove mutability first, I felt it was the only real way I could feasibly ensure that this was sound.

I have pushed my commits to your branch, hope you don't mind ;)

And thank you again for starting on this work wayy back then!

@madsmtm madsmtm merged commit 5780702 into madsmtm:master Sep 18, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants