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

[Objc] Add From<ChildClass>, TryFrom<ParentClass>, function ownership updates and Protocol inheritance #1883

Merged
merged 11 commits into from
Sep 16, 2020

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Aug 24, 2020

First off, I don't wanna spend forever trying to build the perfect objective-c bindings because I think that might never end. I think this PR adds a bit to the ergonomics to the generated bindings.

In this PR, we add

  • impl From<Bar> for Foo where Bar is a subclass of Foo in the objective-c. This will be done for all super classes of Baz. The reason why this will be nice is that whatever uses these bindings will want to deal with the generics of stuff. In my case, I plan to create UIKit views using specific types (UILabel, UIButton, UITextInput, etc.) and then turn them into a UIView. If there's strong push back against this feature, I suppose using a Box<dyn UIView> will have to work.
  • Changing unsafe fn methodWithFoo_(self, foo: Foo) to unsafe fn methodWithFoo_(&self, foo: Foo) and removing the Copy as a derived trait. One aspect of this that I've got mixed feelings about is that the existence of &self implies that there should be a &mut self and that's just not something that's all that feasible across the board. With getters/setters it's probably possible to have &self and &mut self respectively. With things like addSubview_, it modifies the instance of the UIView.

I'm also temped to try and move to unsafe fn methodWithFoo_(&self, foo: &Foo) but I'm less interested in that is because those things are commonly actually taking ownership. For example, root_view.setBackgroundColor_(UIColor::redColor()). You don't really need the color after this (though you may need to deallocate it). But as a counter point root_view.addSubview_(my_button), you want to keep track of my_button because you're going to need to run [removeFromSuperview_](https://developer.apple.com/documentation/uikit/uiview/1622421-removefromsuperview?language=objc) and then free the memory (or not depending on your use case).

@scoopr I think you might have some opinions on this.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@scoopr
Copy link
Contributor

scoopr commented Aug 27, 2020

For the ref in self, the change seems sane, but I don't have any strong feelings about it. Is there any visible change in user code except that there is no Copy bound, which I guess would encourage to put clones/retains in the right places?

The From trait I'm less sure about. I guess I would prefer maybe a method something like cast_as<T>(), perhaps like

trait ObjCCast { fn cast_as<T>(); }
impl <T:Deref<Target=objc::runtime::Object>> ObjCCast for T
{
  fn cast_as<T>() -> Option<T> { /* .. */ }
}

where the body would do a isKindOf:T::class() test, and maybe add an unsafe variant that skipped that. But I suppose that is more a matter of taste, but especially in the From trait form I would prefer some kind of a type-check behavior, as that could be called from safe-context.

@simlay
Copy link
Contributor Author

simlay commented Aug 28, 2020

For the ref in self, the change seems sane, but I don't have any strong feelings about it. Is there any visible change in user code except that there is no Copy bound, which I guess would encourage to put clones/retains in the right places?

Yeah. There's a small chance of needing to put a & in places such as my example with uikit-sys. This particular error would be solved if I wasn't importing multiple traits that have an initWithFrame_ function in them. One nice thing about this is if I remove the .clone from the addSubview_ call, it errors giving me a capture variable error and I like that.

The From trait I'm less sure about. I guess I would prefer maybe a method something like cast_as(), perhaps like

For the From trait, I only added upcasting (child to parent only) which, if I understand correctly, is safe to assume you can upcast without danger. Downcasting however is something I've actually not thought about at all. Doing this with a TryFrom and using the isKindOf feature I think could be quite nice.

@scoopr
Copy link
Contributor

scoopr commented Aug 28, 2020

Oh yeah, now I get it. These changes seem fine to me!

* Added TryInto trait implementation from parent to child interfaces.
* Added HashSet for protocols so that the protocol inheritance works as
well.
@simlay
Copy link
Contributor Author

simlay commented Aug 28, 2020

I went ahead and added generation for TryFrom<ParentClass> for ChildClass using the objective-c isKindOfClass feature. The error type is a String in the format: format!("This {} is not an cannot be downcasted to {}", #parent_struct_name, #child_struct_name). I've got mixed feelings about it but don't really know what it should be.

I've gone ahead and tested this out with:

#[test]
fn uiview_downcasting_tests()  {
    use uikit_sys::{
        UIButton,
        INSObject,
        UIView,
        UILabel,
    };
    use std::convert::TryFrom;
    let button = UIButton(unsafe {UIButton::alloc().init()});
    let as_view : UIView = button.into(); 
    let as_button = UIButton::try_from(as_view.clone());
    assert!(as_button.is_ok());
    let as_uilabel = UILabel::try_from(as_view);
    assert!(as_uilabel.is_err());
    println!("AS UILABEL: {:?}", as_uilabel.err())
}

And it worked as intended.

Also, I recently noticed that the impl blocks for some parent protocols were missing so I added the generation of parent protocols. The use of a HashSet is because in the inheritance tree, you can have multiple things conform to the same protocol and rust doesn't like that. I also added a test for this as well.

@simlay simlay marked this pull request as ready for review August 29, 2020 23:58
@simlay simlay changed the title [Objc] Add From<ChildClass> for ParentClass and change functions to borrow over own. [Objc] Add From<ChildClass>, TryFrom<ParentClass>, function ownership updates and Protocol inheritance Aug 30, 2020
@simlay
Copy link
Contributor Author

simlay commented Aug 30, 2020

@emilio would you like me to add entries in the CHANGELOG.md? I'm not sure how I missed this is the past but would gladly it if it makes your life easier.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can, writing changelog entries would be great and save me some time when doing releases, though not huge deal, your call :)

src/codegen/mod.rs Outdated Show resolved Hide resolved
tests/expectations/tests/libclang-3.9/objc_inheritance.rs Outdated Show resolved Hide resolved
src/codegen/mod.rs Outdated Show resolved Hide resolved
src/codegen/mod.rs Outdated Show resolved Hide resolved
@simlay simlay force-pushed the objc-into-parent-objects-and-borrow branch from 111b312 to e4fb4a7 Compare August 31, 2020 21:12
@simlay simlay requested a review from emilio September 11, 2020 05:52
@simlay
Copy link
Contributor Author

simlay commented Sep 16, 2020

@emilio Is there anything else I can do to get this merged?

@simlay
Copy link
Contributor Author

simlay commented Sep 16, 2020

It's unclear to me what the deal is with travis though. One of the two builds finished but the other is "queued".

@emilio
Copy link
Contributor

emilio commented Sep 16, 2020

No, sorry, I just need to get to reviewing it again. Really sorry for the lag :(

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, just one minor change, but I see that's already an issue with Deref implementations (and I guess it doesn't make a lot of sense to use objc with no-std...)

So perhaps not a big deal.

@@ -3962,6 +3964,53 @@ impl CodeGenerator for ObjCInterface {
}
};
result.push(impl_trait);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting kinda gnarly, it may be worth factoring this if let ... { .. } else { .. } to its own function. Follow-up is fine.

parent_struct_name, child_struct_name
);
let try_into_block = quote! {
impl std::convert::TryFrom<#parent_struct> for #class_name {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can't use std if the no_std flag is set, so this should use trait_prefix. Also, probably should use the old style root paths (::std::convert::TryFrom) for consistency with other code and to support Rust 2015.

@emilio emilio merged commit 4f714ab into rust-lang:master Sep 16, 2020
@simlay simlay mentioned this pull request Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants