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

How is downcasting expected to work #658

Open
Jasper-Bekkers opened this issue Oct 1, 2024 · 11 comments
Open

How is downcasting expected to work #658

Jasper-Bekkers opened this issue Oct 1, 2024 · 11 comments
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request

Comments

@Jasper-Bekkers
Copy link

I'm currently on the latest commit, trying to create a MTLAllocation from a MTLHeap can I can't figure it out.

Technically a ProtocolObject<dyn MTLHeap>, I seem to hit this problem every so often and need increasingly arcane methods of casting these down. Similar for Retained objects. Both would be extremely welcome to have a cast function that would convert from U to T where U:T.

@Jasper-Bekkers
Copy link
Author

ProtocolObject::<dyn MTLEvent>::from_retained or ProtocolObject::<dyn MTLEvent>::from_ref

@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels Oct 1, 2024
@madsmtm
Copy link
Owner

madsmtm commented Oct 1, 2024

Yup, that's the ones.

I'll re-open this though, and use it to track finding a nicer way of doing these conversions. I'm considering AsRef and/or Into implementations, though not sure if coherence will allow us to add them?

@madsmtm madsmtm reopened this Oct 1, 2024
@Jasper-Bekkers
Copy link
Author

Potentially just having better method names for these would help, I ultimately found them on Retained::downcast (which would be a word I'd look for when casting).

@MarijnS95
Copy link

I do wonder how this protocol type checking works. We have U: T where MTLHeap and MTLResource are known to implement the MTLAllocation protocol. But it's a new protocol that showed up in the XCode 16 beta, and is only available on MacOS 15 onwards (https://developer.apple.com/documentation/metal/mtlallocation) whereas these protocols (MTLHeap and MTLResource) are already available on older releases.

On those older releases MTLAllocation doesn't exist, so there's no backwards-compatible given way to upcast to a "higher" protocol in the trait chain, unless this happens in a fallible manner?

@madsmtm
Copy link
Owner

madsmtm commented Oct 3, 2024

I do wonder how this protocol type checking works. We have U: T where MTLHeap and MTLResource are known to implement the MTLAllocation protocol. But it's a new protocol that showed up in the XCode 16 beta, and is only available on MacOS 15 onwards (https://developer.apple.com/documentation/metal/mtlallocation) whereas these protocols (MTLHeap and MTLResource) are already available on older releases.

On those older releases MTLAllocation doesn't exist, so there's no backwards-compatible given way to upcast to a "higher" protocol in the trait chain, unless this happens in a fallible manner?

To start with, upcasting to a protocol (e.g. ProtocolObject<dyn MTLResource> -> ProtocolObject<dyn MTLAllocation> or NSString -> ProtocolObject<dyn NSCopying>) with from_retained / from_ref exist purely in the type-system, and don't actually do anything at runtime.
Downcasting protocols (e.g. NSObject -> ProtocolObject<dyn MTLAllocation>) isn't yet implemented, but would have to be a runtime check, and yes, that would fail on older macOS versions.

To more specifically answer your question, upcasting to ProtocolObject<dyn MTLAllocation> is safe/sound in older versions, as protocols are mostly a concept that exists in the type-system to make it safer than passing around AnyObject. That said, if you actually call any methods on the protocol, such as the allocatedSize, that will of course not work in older versions.

@MarijnS95
Copy link

That said, if you actually call any methods on the protocol, such as the allocatedSize, that will of course not work in older versions.

That's where the crux lies; this purely appears to be a getter which likely does not need to be unsafe. So what would actually happen if we did this on MacOS 14?

Hence, if upcasting (as expected) doesn't do anything at runtime:

  • We cannot do runtime checking to return an Option on older MacOS?
  • Hence casting helpers (i.e. based on ProtocolObject::from_ref()) must be unsafe because unlike conventional trait/object hierarchies, it isn't guaranteed to implement the parent that we upcast to.

Just wanted to make sure an Into for these things probably wouldn't fly.

@madsmtm
Copy link
Owner

madsmtm commented Oct 3, 2024

That said, if you actually call any methods on the protocol, such as the allocatedSize, that will of course not work in older versions.

That's where the crux lies; this purely appears to be a getter which likely does not need to be unsafe. So what would actually happen if we did this on MacOS 14?

Same thing as happens elsewhere if you call a method on an object that isn't available; the runtime sees that there's no selector with that name, and you crash (safely, so not UB). So it would be perfectly fine for us to mark it as safe.

If you want to call the method with a check at runtime first, you need to do the availability checking yourself:

We cannot do runtime checking to return an Option on older MacOS?

We could, conformsToProtocol exists for that kind of thing, but I'd argue it's unnecessary, as again, the ProtocolObject<dyn T> is literally just AnyObject that allows access to the methods on the protocol.

Besides, it'd be overly restrictive, there's many cases where Apple has introduced a protocol into an API, in a later version than the API itself was introduced.

  • Hence casting helpers (i.e. based on ProtocolObject::from_ref()) must be unsafe because unlike conventional trait/object hierarchies, it isn't guaranteed to implement the parent that we upcast to.

Just wanted to make sure an Into for these things probably wouldn't fly.

No, as outlined above, the safety of an Into impl would not be affected by the runtime availability of the API.

@MarijnS95
Copy link

Not the safety, but the usefulness. Assuming a developer is on the latest (Beta) version of MacOS and relies on an infallible .into() to go from MTLHeap to MTLAllocation, only to see it consistently crash (not UB, but still, a crash) on older end-user machines seems strange to me.

@madsmtm
Copy link
Owner

madsmtm commented Oct 3, 2024

Not the safety, but the usefulness. Assuming a developer is on the latest (Beta) version of MacOS and relies on an infallible .into() to go from MTLHeap to MTLAllocation, only to see it consistently crash (not UB, but still, a crash) on older end-user machines seems strange to me.

Yeah, that's a fair point. Again, the Into conversion itself wouldn't fail, but method calls would.

I once considered making method calls return an Option everywhere that they might be unavailable, but I think that would be prohibitively annoying, and the user would likely just end up doing a bunch of .unwrap()s. Besides, this idea wouldn't work well with deployment targets, as users that opt in to newer versions would then still have to do the checks, and the API would need to break if we raised the minimum supported version.

The way Swift and Objective-C solves this whole ordeal is with compiler warnings, which we can't do as we're just a library. I am thinking of ways to work around this (see again #266), but it's fairly difficult, and my current ideas would still require testing at runtime.

(I'm not sure how my tone sounds here, I fear I may sound a little dismissive because I feel pretty certain in the chosen solution at this point (i.e. do nothing), but wanted to state that I'm glad to have this discussion (!), and I am open to try changing my viewpoint)

@MarijnS95
Copy link

I align exactly with the chosen solution, i.e. do nothing. Specifically because, while the original issue description may make it sound like this is an ordinary U: T inheritance chain that one might find in a trait hierarchy, C++ inheritance hierarchy, or COM hierarchy, where upcasting is infallible, that appears to not the case in Objective-C (as I learned before catching up to this issue).

One might hence expect any upcasting-like API like the proposed Into conversion to fail (by means of a panic or better yet, TryInto) or otherwise have callable methods; not for the conversion to a specific protocol to succeed (and "own" an instance of that protocol object) only to fail when any of its methods are called.

I'll leave it to you to consider the best API to describe this, just glad that we're on the same page when it comes to these "inherited protocols" to really be optional.

@Jasper-Bekkers
Copy link
Author

For what it's worth, for me this issue was mostly about the discoverability of a casting API (either direction), not so much about the possibility of being able to.

The only casting api I could find at the time was on Retained, while I needed to casting a ProtocolObject. I had seen the from_ref and from_retained apis before but never connected the dots that these were for casting.

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

No branches or pull requests

3 participants