Objective-c inheritance support #1750

simlay · 2020-03-19T01:00:34Z

This PR adds objective-c inheritance support by generating the impl IFoo for Foo { } and impl IBar for Foo { } (see the unit test) which generally match the objective-c inheritance patterns.

I also changed all the unsafe fns do be internal unsafe blocks to mimic that of @SSheldon's objc-foundation crate. I'm actually not sure this is the best way to do it. Would love some comments as to why not.

I've got a branch of the bindgen book that I'm working on as well. I'm just gonna make it a separate PR and link to a uikit-sys crate.

Similar to #1722 I did, I don't have all the versions of clang so I'll fix CI once we discuss this more.

highfive · 2020-03-19T01:00:39Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio

I'm ok with the unsafe change I think.

I think looping over all the items to find the base class is not the way to go though. Does objective-c allow for multiple interfaces with the same name under different C++ namespaces? If so it seems this would break.

Is there any chance this could look more similar to the C++ case?

src/codegen/mod.rs

src/ir/objc.rs

emilio · 2020-03-24T10:22:31Z

src/ir/objc.rs

@@ -179,6 +183,35 @@ impl ObjCInterface {
                    let name = c.spelling();
                    interface.template_names.push(name);
                }
+                CXCursor_ObjCSuperClassRef => {
+                    let items_map = ctx.items();


Looping over all the items means that this has really really bad performance characteristics. Which is pretty unfortunate... Do you really need to do a name lookup? I think Item::from_ty_or_ref would do the right thing if you give it the cursor? See the handling of CXCursor_CXXBaseSpecifier and such.

I tried to get around the need for the name lookup. After a bit of thought and reading through the Base stuff in the handling of CXCursor_CXXBaseSpecifier, I think the thing that would be similar would be something like:

struct Inheritance { inheritance_type: InheritanceType, parent_name: String, } enum InheritanceType { Protocol, Class, }

The part about Class and Protocol in InheritanceType is because the logic from the needle, haystack stuff I took from the protocol logic so it would make sense to fix both of those handlers. I'll think about it more and maybe implement it if it's not too gross. I'm not sure what goes on when you iterate through ctx.items() but I kinda assume it's the whole syntax tree... 🤢

Bleh, I tried this a couple different ways based on the above idea similar to how
CXCursor_CXXBaseSpecifier handling works. The issue is the templates. If we don't need to know if the parent is a template, then we can just use format!("I{}", c.spelling()) stored in the ir for the objc codegen.

I propose that we do one of the two options:

take out template tags. I don't know how you'd use the template things with with the generated bindings anyway so why do we have them?

Deal with the current bad performance - I've tested it with uikit-sys, it takes ~2.5 minutes to build so it's not the end of the world but people will definitely dislike that crate :/

I'm a bit up for doing option 1 and excluding generics support as a whole. @scoopr, do you have opinions on that?

Just to clarify, are you talking generally about generics support for objc code or just when there is a parent class that has generic params?

I would really like that NSArray<Foo> works, but I guess I could be convinced that it is not critical with parent classes. Perhaps stuff like NSMutableArray would look a bit odd?

Yes, the generics desugar to id, so of course everything works without them, so if you must remove them completely, so be it. But I would find that kind of a missed opportunity.

If I had to choose, I would really try to live with bad performance, and perhaps try to optimize it separately if it indeed is that bad. But in the end I guess it is more important to be useful than idealistic.

I tried to understand the code here, but it would take a bit too long to actually ingest what happens here, and why other approaches don't work. Just as random idea, can the iterating of the items be done as a separate pass later, when you know all the needles to search for, so you could do only a single iteration of the items?

Also the objc_template.rs expectation tests shows some method generic params as u64 under libclang-9, though not in libclang-5, so are these types already being dropped from methods (by accident or something else)? The trait has then nevertheless

Just to clarify, are you talking generally about generics support for objc code or just when there is a parent class that has generic params?

Yeah.

Just as random idea, can the iterating of the items be done as a separate pass later, when you know all the needles to search for, so you could do only a single iteration of the items?

Also the objc_template.rs expectation tests shows some method generic params as u64 under libclang-9, though not in libclang-5, so are these types already being dropped from methods (by accident or something else)? The trait has then nevertheless

Yeah, that seems to be an issue with just libclang-9 that probably got merged in #1702. I'm not really sure why that is though. We should file that as a bug and investigate more.

If I had to choose, I would really try to live with bad performance, and perhaps try to optimize it separately if it indeed is that bad. But in the end I guess it is more important to be useful than idealistic.

That's good with me.

@emilio Is there anything else I can do other than fix the tests for this PR to get merged?

I still think looping through all the items here is not acceptable. Can you explain why the partial type approach (what the base classes / pointers / etc does) doesn't work?

src/codegen/mod.rs

emilio · 2020-04-26T17:02:15Z

src/ir/objc.rs

@@ -179,6 +183,35 @@ impl ObjCInterface {
                    let name = c.spelling();
                    interface.template_names.push(name);
                }
+                CXCursor_ObjCSuperClassRef => {
+                    let items_map = ctx.items();


I still think looping through all the items here is not acceptable. Can you explain why the partial type approach (what the base classes / pointers / etc does) doesn't work?

emilio · 2020-04-26T17:02:41Z

src/ir/objc.rs

+                        if Some(needle.as_ref()) == ty.name() {
+                            debug!("Found parent class {:?}", item);
+                            interface.parent_classes.push(id);
+                            for i in class.parent_classes.clone() {


This clones the whole vector unnecessarily.

simlay · 2020-05-04T07:01:11Z

I still think looping through all the items here is not acceptable. Can you explain why the partial type approach (what the base classes / pointers / etc does) doesn't work?

I managed to get Item:: from_ty_or_ref to work in the code generation phase, it gave me a different ItemId which is a TypeKind::ResolvedTypeRef(TypeId) so I had to call ctx.resolve_type twice so that I could traverse up to the parent's parent and such.

I think a similar thing could be done for the CXCursor_ObjCProtocolRef handling. I wouldn't mind adding that as well.

I'm not really sure how I feel about the solution but I really couldn't figure out how to get a parent's parent all the way up the tree.

I also updated the unit tests to be more useful.

emilio

Yeah, this looks much better! Just need to resolve through type refs in codegen to simplify a bit and not have to manually write that loop.

Doing the same for protocols seems fine, but probably separate patch?

src/codegen/mod.rs

emilio · 2020-05-12T15:53:58Z

Thanks for bearing with me!

highfive added the S-awaiting-review label Mar 19, 2020

This was referenced Mar 21, 2020

Initial objective-c docs #1751

Merged

SIGSEGV running rust-bindgen on Objective-C headers #665

Open

emilio requested changes Mar 24, 2020

View reviewed changes

SSheldon reviewed Mar 25, 2020

View reviewed changes

src/codegen/mod.rs Outdated Show resolved Hide resolved

simlay force-pushed the objc-inheritance branch from 57ec0d8 to 26db1e7 Compare April 23, 2020 05:35

simlay requested a review from emilio April 26, 2020 00:00

emilio reviewed Apr 26, 2020

View reviewed changes

simlay requested a review from emilio May 4, 2020 07:01

simlay force-pushed the objc-inheritance branch from 5e71637 to 086a45d Compare May 4, 2020 17:50

emilio reviewed May 8, 2020

View reviewed changes

src/codegen/mod.rs Outdated Show resolved Hide resolved

simlay force-pushed the objc-inheritance branch 2 times, most recently from 0594d09 to b874115 Compare May 8, 2020 20:14

Added inheritance to objective-c support.

1f324ca

simlay force-pushed the objc-inheritance branch from 5e1e65e to 1f324ca Compare May 12, 2020 00:05

simlay requested a review from emilio May 12, 2020 04:36

emilio approved these changes May 12, 2020

View reviewed changes

emilio merged commit fcc1096 into rust-lang:master May 12, 2020

simlay mentioned this pull request May 13, 2020

Objective-c categories aren't included in inheritance traits and impl blocks #1779

Open

simlay mentioned this pull request Aug 18, 2020

Objective-C support #109

Open

simlay mentioned this pull request Sep 19, 2020

iOS Support bevyengine/bevy#87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Objective-c inheritance support #1750

Objective-c inheritance support #1750

simlay commented Mar 19, 2020

highfive commented Mar 19, 2020

emilio left a comment

emilio Mar 24, 2020

simlay Mar 26, 2020

simlay Apr 4, 2020

scoopr Apr 7, 2020

simlay Apr 23, 2020 •

edited

Loading

emilio Apr 26, 2020

emilio Apr 26, 2020

emilio Apr 26, 2020

simlay commented May 4, 2020

emilio left a comment

emilio commented May 12, 2020

Objective-c inheritance support #1750

Objective-c inheritance support #1750

Conversation

simlay commented Mar 19, 2020

highfive commented Mar 19, 2020

emilio left a comment

Choose a reason for hiding this comment

emilio Mar 24, 2020

Choose a reason for hiding this comment

simlay Mar 26, 2020

Choose a reason for hiding this comment

simlay Apr 4, 2020

Choose a reason for hiding this comment

scoopr Apr 7, 2020

Choose a reason for hiding this comment

simlay Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

emilio Apr 26, 2020

Choose a reason for hiding this comment

emilio Apr 26, 2020

Choose a reason for hiding this comment

emilio Apr 26, 2020

Choose a reason for hiding this comment

simlay commented May 4, 2020

emilio left a comment

Choose a reason for hiding this comment

emilio commented May 12, 2020

simlay Apr 23, 2020 •

edited

Loading