Higher level objective-c support.

[simlay]

I took a whack at doing a bit of #109. There hasn't been much movement on the issue so I wanted some feedback on some of the trait constraints generated.

Here's a summary of the changes:

• Each objective-c interface Foo now has a struct_Foo which wraps the id as mentioned in Objective-C support #109.
• I removed impl interface_Foo for id { all_the_trait_fns } and replaced it with pub trait interface_Foo { all_the_trait_fns }, and added impl interface_Foo for struct_Foo { }. This allows some of the simple inheritance.
• I added trait constraints of Sized + std::ops::Deref + objc::Message, to mimic the objc-foundation crate.
• Added an impl struct_Foo { fn alloc() {...}} for creation of methods.

I still need to add an impl struct_Foo { fn alloc() {...}} for the structs as a convenience method.

Also, I think it might be useful to have things that return a different type from a given function to be of that wrapped type So, when a interface_Foo trait has a property of type Bar, it'd return Bar(id). Thoughts?

I've tested this out with generating bindings for a uikit-sys crate I've been working on and it compiles. That being said, it's still a WIP.

[emilio]

So as you may imagine I am not particularly an objc expert, so this review is mostly superficial.

I think this is not terrible, though it seems a bit weird to have struct_Foo and interface_Foo separated. I'd probably at least just make the struct be Foo, if we're generating wrapped structs, wdyt?

The struct should probably be #[repr(transparent)], or at least some sort of ffi-safe representation.

I have a couple other questions about the code below... It'd probably take me a bit of time to do a proper review, may be useful to try to get @scoopr to take a look if they have the time. Pretty sure their feedback would be much more on-point than mine :)

[emilio]

Thanks for improving the ObjectiveC support btw, it's great! (Just realized that my comment above read a bit negative... sorry, quite late here :)).

[simlay]

I'd probably at least just make the struct be Foo, if we're generating wrapped structs, wdyt?

I was kind of just trying to keep with some of the conventions. Also part of the distinction between the two (struct_Foo and interface_Foo) is because it would be nice to have some of the same structure as the objective-c stuff. Annoyingly, defining struct_Foo as the the wrapper around the id and having interface_Foo as the trait allows for inheritance. I'm not big on the pattern but I'm not sure I really see a way around it. :/

Thanks for improving the ObjectiveC support btw, it's great! (Just realized that my comment above read a bit negative... sorry, quite late here :)).

It wasn't too negative. I really appreciate the feedback. I'm just glad this is in the right direction.

[scoopr]

It was so long ago that I've effectively unloaded all that I remembered about this whole thing. But I'm really glad that there is progress on it again!

Looking at the generated code, there is some Class::get calls done, even if the new code uses the class! macro. I think it would be nice to use the macro everywhere, which I think does some caching.

Also not sure about the struct_Foo naming, but I didn't really try to think it through. But I do agree that wrapping the id is likely needed as dumping all the impls for id is likely not a scalable solution. I think I was going to do that but tried to make it work otherwise first. But does it now go through the whole inheritance chain for every type, doing a impl interface_NSObject for struct_Foo; thing, so that all relevant methods are always available?

I'm also having hard time remembering what the blocker issues were for me to getting the foundation frameworks generated, but I remember being really close and the issues were kind of stupid but I didn't have the knowledge to workaround them.

But if I had my blockers fixed, my next step that I was dreading to think, but I felt was quite necessary, was generating compatible types for frameworks separately. What I mean is, that when there is a foundation-sys crate that has NSString, that the uikit-sys you have could use (or depend on) those types, but I had no idea how to approach that (can I detect which framework the type came from?). That is a separate endeavor but thinking aloud, if you have any thoughts on it.

EDIT: one of my blockers might of been the generics support that I saw you did earlier, nice!

[emilio]

But if I had my blockers fixed, my next step that I was dreading to think, but I felt was quite necessary, was generating compatible types for frameworks separately. What I mean is, that when there is a foundation-sys crate that has NSString, that the uikit-sys you have could use (or depend on) those types, but I had no idea how to approach that (can I detect which framework the type came from?). That is a separate endeavor but thinking aloud, if you have any thoughts on it.

FWIW, the way I usually deal with this kind of stuff from C/C++ at least is to do the equivalent to blacklist NSString, and importing foundation_sys::NSString using raw_lines. Not sure that'd work if you'd need to implement more traits on NSString, but probably it would, as the implemented trait is defined in uikit-sys?

[scoopr]

I did a small test, and I got this to run!

mod foundation;
use std::ffi;

use crate::foundation::NSLog;
use crate::foundation::struct_NSString;
use crate::foundation::NSString_NSStringExtensionMethods;

fn main() {

    let nsstring = unsafe { struct_NSString::stringWithUTF8String_(
            ffi::CString::new("Hello").unwrap().as_ptr()
            ) };

    unsafe { NSLog( nsstring ); }

}

For file generated from the whole Foundation on macos

I think the trait bound for methods is not quite right, I had to comment out the one for stringWithUTF8String_ to get that to compile. Not sure what the bound is trying to do in the first place.

Having all the protocols be separate traits causes us having to use some of the extension protocols, that would be "implementation detail" and transparent when writing objc, which is unfortunate, but I suppose not terribly bad.

Some interesting tidbits from the generated code, which are not really about objc support,

1. Had to blacklist some HFS types, had some conflicting repr/packed stuff
2. The timezone conflict that I see you stumbled upon as well
3. The generated code had some u128 and rust warns that it is not ffi safe
4. Couple of warnings of

#[derive] can't be used on a #[repr(packed)] struct that does not derive Copy (error E0133)

This is really good progress!

[simlay]

What I mean is, that when there is a foundation-sys crate that has NSString, that the uikit-sys you have could use (or depend on) those types, but I had no idea how to approach that (can I detect which framework the type came from?).

I like this idea. As it is now, in my test uikit-sys crate, it generates all the bindings for everything that UIKit depends on all the way up to the NSObject declarations. I guess if we have #303, it'd be much easier to make this work. I think to make that happen, you'd need to specify where a given module subgroup comes from. On the plus side, having a foundation-sys crate and a uikit-sys crate would also allow for much better compile times. It takes a few minutes to compile the crate after all the dependencies have finished :/

Also not sure about the struct_Foo naming, but I didn't really try to think it through.

I'm also not sure how I feel about it. I kinda like Foo_struct and Foo_interface, and Foo_protocol a little bit more.

Some interesting tidbits from the generated code, which are not really about objc support,

1. Had to blacklist some HFS types, had some conflicting repr/packed stuff
2. The timezone conflict that I see you stumbled upon as well
3. The generated code had some u128 and rust warns that it is not ffi safe
4. Couple of warnings of

Yeah, there's a lot of domain specific knowledge around these things. I hope to eventually put them in the bindgen book.

My actual concern is usage of some of the protocol methods on self like this:

This header:

@interface Foo
- (instancetype) initWithFirstNumber:(int)firstNumber;
@end

produces

#[macro_use]
extern crate objc;
#[allow(non_camel_case_types)]
pub type id = *mut objc::runtime::Object;
#[repr(transparent)]
pub struct struct_Foo(pub id);
impl std::ops::Deref for struct_Foo {
    type Target = id;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}
unsafe impl objc::Message for struct_Foo {}
impl struct_Foo {
    pub fn alloc() -> Self {
        Self(unsafe { msg_send!(objc::class!(Foo), alloc) })
    }
}
impl interface_Foo for struct_Foo {}
pub trait interface_Foo: Sized + std::ops::Deref + objc::Message {
    unsafe fn firstNumber(self) -> ::std::os::raw::c_int
    where
        <Self as std::ops::Deref>::Target: objc::Message + Sized,
    {
        msg_send!(self, firstNumber)
    }
}

Something that would consume this would be:

use crate::bindings::{
    struct_Foo,
    interface_Foo,
};
fn main() {
    unsafe {
        let foo = struct_Foo::alloc();
        foo.initWithFirstNumber_(10);
    }
}

I get this error:

error[E0277]: the trait bound `*mut objc::runtime::Object: objc::message::Message` is not satisfied
  --> src/main.rs:21:13
   |
21 |         foo.initWithFirstNumber_(10);
   |             ^^^^^^^^^^^^^^^^^^^^ the trait `objc::message::Message` is not implemented for `*mut objc::runtime::Object`

error: aborting due to previous error

Thoughts?

[emilio]

You're implementing objc::Message on the Foo struct. Why do you require Deref::Target to implement objc::Message? I think removing the method bound would fix it.

[simlay]

You're implementing objc::Message on the Foo struct. Why do you require Deref::Target to implement objc::Message? I think removing the method bound would fix it.

I resolved it by changing the Deref implementation to be return &objc::runtime::Object to be unsafe { &*self.0 }. It fixes the above issue. I'm gonna do more testing to verify that this actually works correctly in the objective-c runtime.

I'm not big on having self.0 of struct_Foo be public. I added it because functions that return instancetype, are just ids, it' means that the initWithBar stuff returns an id, getting back to the struct means struct_Foo(struct_Foo::alloc().initWithBar_(bar)) is the way to get back a Foo. I could add a From<id> for struct_Foo. Thoughts?

Also, in #109, the author mentions having DerefMut as an implemented trait. Is there a specific use case I'm missing? All the mutations happen on the other side of the FFI/objective-c runtime so I don't see the use case.

Also, after messing around with struct_Foo, I think it'd be much better as Foo_struct for the wrapped struct, and Foo_interface, Foo_protocol and Foo_interface_${CATEGORY_NAME} (though, the category name change might not be needed). This would make imports be:

use crate::bindings::{
    Bar_struct,
    Bar_interface,
    Foo_struct,
    Foo_interface,
    Foo_CategoryBaz,
}

Seems a lot cleaner to me. wdyt?

[simlay]

The last commit fixes the above issue I mentioned but now requires blacklisting of objc_object in the binding generator as the struct derives Clone and Copy but objc::runtime::Object doesn't implement Clone or Copy.

[simlay]

After a bit of thought and some testing on my uikit-sys bindings, I changed the names from protocol_ to format!("P{}", protocol_name) and interface_ to format("I{}", interface_name) and made the struct names as the interface names. You can see what an example looks like here: https://github.com/simlay/uikit-sys/blob/3621a0c8300865fa931577db9d15392cf2ef30e2/examples/rect.rs#L83. I like this the most partly because it parallels what objc-foundation does so I think it'll be helpful for people that want to use this similar to objc-foundation.

Also, I've added #[derive(Clone, Copy)] to the structs that are generated. It's not ideal as it forces user to dealloc stuff but for now but I think we should leave that for another PR.

@emilio @scoopr What do you guys think? Shall I fix CI and let's merge this?

[simlay]

@scoopr @emilio bump

[scoopr]

I'm excited this is going forward! I'm sorry that I don't have proper time to look at it for now though. On a cursory glance it looks like a good direction, though. The example you linked seems really clean looking!

Shouldn't the unit tests be updated in the commit, the CI tests would pass, but also it is nicer to review what the expected output is.

If I had the time, one thing I would like to look at is framework like Metal, where everything is just a protocol without concrete interfaces. But that's minor stuff.

I suppose just having the CI go green, this could go in, and followup PRs could then improve it if further issues arise?

[emilio]

Yes, that sounds like a good way forward.

[emilio]

Some documentation about how are people supposed to use this would be great too, but doesn't need to block I guess :)

[simlay]

Some documentation about how are people supposed to use this would be great too, but doesn't need to block I guess :)

Yup. That's the plan. I totally intend to add documentation to the book to use this along with all the annoying things you've gotta blacklist.

My next PRs are going to be:

• Add the parent classes as impl blocks for each struct so that the inheritance matches the objective-c documentation.
• Play with the unsafe blocks and hopefully make the fns be safe with an unsafe block around the msg_send! call.

I'm just trying to keep this PR small as it's already pretty big.

I suppose just having the CI go green, this could go in, and followup PRs could then improve it if further issues arise?

I'm hoping publish a uikit-sys crate (and probably some sub-crates to deal with compile time) with this soon so, I'm sure I'll think of more features/issues and fix them.

Anyway, I updated the tests to fix CI! 🚀

[simlay]

@emilio bump

[emilio]

Thanks, and sorry for the lag (bindgen is not my full-time job).

This looks good to me with a few nits. Mind taking a look at the comment, squashing, and I'm happy to get this merged.

Thanks!

[simlay]

Done and done! I just wanna say thanks @emilio, I'm really glad you maintain this project and keep the quality up when it comes these PRs. I hope I didn't come off pushy earlier. I'm just excited about having more complete objective-c support.

[emilio]

Thank you!