Add initial Objective C support #443

scoopr · 2017-01-25T20:17:43Z

Hi! I made an attempt to support parsing objective c headers. This is very much incomplete,
and likely not quite yet ready for merging, but I thought I'd share the progress so far.

Comments appreciated, I'm still very much a newbie in just about everything related to this change (rust as a language, rust ast, libclang, rust-bindgen), and there were many parts of code I wasn't quite sure about.

Commit message:
It parses interfaces and protocol but ignores base classes,
methods that don’t have arguments, the arguments are currently ignored.
Also you can pass objc class instances to C functions.

Next steps are inheritance/base classes, method signatures,
properties, categories.
Then check with system headers what is missing.

scoopr · 2017-01-25T20:34:31Z

The failure is due to me hardwiring the use objc; etc. for my testing.. I suppose the raw_line option is meant for that.

emilio

Woot, this is great! Thanks for working on this :)

It looks quite good! As you mentioned, it still needs a bunch of work though.

I'm totally fine with landing this incrementally, but with some constraints:

First, it needs docs (also, CI is failing because we force documentation).
Second, it needs tests (this is something old bindgen really lacked when I rewrote it, and it's definitely necessary to avoid falling in a spiral of bugs and regressions).

Again, this is awesome! Thanks a lot for working on this :)

emilio · 2017-01-25T21:05:55Z

src/codegen/mod.rs

@@ -2074,6 +2091,18 @@ impl ToRustTy for FunctionSig {
            // [1]: http://c0x.coding-guidelines.com/6.7.5.3.html
            let arg_ty = if let TypeKind::Array(t, _) = *arg_ty.canonical_type(ctx).kind() {


Instead of calling canonical_type two times, let's use a match expression.

emilio · 2017-01-25T21:06:34Z

src/codegen/mod.rs

+                   result: &mut CodegenResult<'a>,
+                   _whitelisted_items: &ItemSet,
+                   _: &Item) {
+


nit: whitespace should go before attempting to merge.

Please remove this newline

emilio · 2017-01-25T21:06:54Z

src/codegen/mod.rs

+        for method in &self.methods {
+            let method_name = ctx.rust_ident(&method.name);
+
+            let body = quote_stmt!(ctx.ext_cx(),msg_send![self, $method_name])


nit: space after ,.

emilio · 2017-01-25T21:08:14Z

src/ir/comp.rs

-                        Ok(item) if ctx.resolve_item(item).kind().is_function() => item,
+                    let signature =
+                        match Item::parse(cur, Some(potential_id), ctx) {
+                            Ok(item) if ctx.resolve_item(item)


could you leave formatting patches in a different commit? That will make this much more pleasurable to review :)

oops, I thought I cleaned them all. I have the editor set up on formatting on save, and I manually cleaned up rest of these.

emilio · 2017-01-25T21:12:28Z

src/ir/context.rs

@@ -1070,6 +1071,7 @@ impl<'ctx> BindgenContext<'ctx> {
                let name = item.canonical_path(self)[1..].join("::");
                debug!("whitelisted_items: testing {:?}", name);
                match *item.kind() {
+                    ItemKind::ObjCInterface(_) => true, // TODO:


Any particular reason you have a full ItemKind for this?

Does an interface have the same characteristics as, let's say, a struct in C? I mean, can you forward-declare them, point to them, etc?

If so, you probably want to just fold it into Type instead, since you'd share a lot of code for detecting duplicates, going to the definition of the type, etc.

(Edit: after reviewing the rest of the commit I've seen you've also added TypeKind::ObjCInterface, you might want to just use ItemKind::Type if there isn't a better reason to have a full ItemKind).

Frankly, I wasn't sure, and I think I just looked at the try_parse! and thought ‘thats what I need’.

Forward declaration is a bit weird, using @class Foo;, this gives a cursor type CXCursor_ObjCClassRef, have not looked at that more closely.

One other interesting bit is protocols, those are for extending more methods for existing interface. Those come in as CXCursor_ObjCCategoryDecl, so I guess they could be handled completely separate (and generate their own trait)

emilio · 2017-01-25T21:15:27Z

src/ir/objc.rs

+
+        match cursor.kind() {
+            CXCursor_ObjCInterfaceDecl |
+            CXCursor_ObjCProtocolDecl => {


Hmmm, does libclang return a specific CXTypeKind for these (you can see this printing cursor.cur_type())? If so, then merging it into Type is probably the best path forward, otherwise, it'll probably need a bit of thinking to get it right.

It does, so I did that.

One annoying hack was that protocols were not types, so I forced it to think they are interfaces.

What kind of type do they report?

emilio · 2017-01-25T21:16:36Z

src/ir/ty.rs

+                /*                let referenced = location.unwrap().referenced().unwrap();
+                let referenced_ty = referenced.cur_type();
+                let declaration = referenced_ty.declaration();
+                println!("referenced={:?}", referenced);


Heh, welcome to the joy of debugging libclang :)

got me with my pants down :)

emilio · 2017-01-25T21:17:31Z

src/ir/ty.rs

+                println!("cur type ={:?}", location.unwrap().cur_type());
+                println!("objc encoding ={:?}", ty.objc_encoding());*/
+
+                let inner = Item::from_ty_or_ref(ty.pointee_type().unwrap(),


This pretty much shares the body with CXType_Pointer, reference, et. al, right? If so, let's just fold it there when you're done with debugging it :)

emilio · 2017-01-25T21:19:40Z

src/ir/ty.rs

@@ -869,6 +874,60 @@ impl Type {
                                           parent_id,
                                           ctx);
            }
+            CXType_ObjCInterface => {


Yeah, probably you don't want a new itemid for each Interface you parse, and I don't see any deduplication going on, so probably you should make this the actual container of your data (and use cursor.definition() to properly go to the actual definition, just in case you find a forward declaration before). You probably need to do roughly the same thing we do for CXType_Record (which is a plain struct), just storing most of the specific interface-related data you want.

emilio · 2017-01-25T21:21:33Z

src/lib.rs

@@ -697,6 +697,12 @@ impl<'ctx> Bindings<'ctx> {
            try!(writer.write("\n".as_bytes()));
        }

+        // TODO:
+
+        try!(writer.write("use objc;\n\n".as_bytes()));


Instead of printing this here we should use something like what we do to insert the __BindgenUnionField, etc. See CodegenResult::saw_union and similar methods.

scoopr · 2017-01-27T01:44:09Z

I'm still missing documentation and testing.
How should I make it run some of the headers in -x objective-c? I suppose I could just name them with .m suffix, or add somehow add the -x flag when header_name.containts("objc")

emilio · 2017-01-27T01:48:22Z

Right now we rely on the automatic language detection for most of the tests, but you can annotate a test with // bindgen-flags:, and they'll get passed to the harness.

So it seems you either want m files (that's fine by me), or otherwise do something like what we do in tests/headers/dash_language.h, which does // bindgen-flags: -- -x c++.

emilio · 2017-01-27T01:50:06Z

src/codegen/mod.rs

-            let arg_ty = if let TypeKind::Array(t, _) = *arg_ty.canonical_type(ctx).kind() {
+
+
+            let arg_ty = match *arg_ty.canonical_type(ctx).kind()


nit: indentation and spacing is quite odd in all this block right now, not sure if intended :)

huh, I wonder how atom/rustfmt missed that o_O

emilio · 2017-01-27T01:53:48Z

src/codegen/mod.rs

@@ -2062,6 +2077,13 @@ impl ToRustTy for Type {
                let ident = ctx.rust_ident(&name);
                quote_ty!(ctx.ext_cx(), $ident)
            }
+
+            TypeKind::ObjCInterface(..) => {
+                let name = item.canonical_name(ctx);


This will be enough if you can't mix objective-c with namespaces. If you can, you need to handle them somehow (hint: you could use utils::build_templated_path, but pass always an empty vec![] of template arguments).

clang will give an actual error if interface embedded in namespace. And to even get that error, it needs to be run with -x objective-c++ mode

scoopr · 2017-01-28T23:43:44Z

Heres my latest, with some rudimentary tests and docs.
Also fixed some objc pointers in structs and global variables to emit the 'id' type there as well.

I'm using the id here to mimic the cocoa-rs convention, thought I think some have suggested trying struct Foo(id) type of wrapping of the type, and impl the traits on those types. But I think that kind of experimentation is ok to day later when it otherwise works.

bors-servo · 2017-01-29T10:14:30Z

☔ The latest upstream changes (presumably #456) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2017-01-29T10:29:32Z

You need to add the objc dependency to the tests/expectations directory, so they compile.

scoopr · 2017-01-29T22:10:03Z

Quite, it didn't seem quite as simple.
Either I did something wrong that I don't understand, or things really are this complicated, but I ended up adding a bindgen flag, that toggles the generated prologue from emitting either use objc; or #[macro_use] extern crate objc;, as the generated tests were run individually, and adding the extern crate to the tests lib.rs didn't seem work.

scoopr · 2017-01-29T22:18:03Z

Ehh, botched rebase, I was sure I had run the tests one more time. Sorry for the noise

scoopr · 2017-01-29T22:36:15Z

ngggh. of course, it needs some cfg guards

I'm not quite sure what is the best option. Should I always put in the generated objc prologue #[cfg(bindgen_objc)]? target_os="macos" would seem too harsh, as in theory this could be used on other platforms as well, even if rare. Guarding just the tests is one option too, I guess, but not sure how to implement that

scoopr · 2017-01-30T21:46:09Z

Woo, finally a passing commit! :D

I opted to add cfg(target_os="macos") to the objc tests. It seemed to be the simplest change, though I'm not sure the .contains("objc_") is good enough toggle for it. I guess this also means that the travis config would need a osx target as well, to have those tests be exercised.

bors-servo · 2017-01-30T22:38:19Z

☔ The latest upstream changes (presumably #461) made this pull request unmergeable. Please resolve the merge conflicts.

emilio

Nice! I think I'm fine with this, mostly nits :)

emilio · 2017-01-30T22:40:59Z

tests/tests.rs

@@ -98,10 +98,17 @@ fn create_bindgen_builder(header: &PathBuf)
    let header_str = try!(header.to_str()
        .ok_or(Error::new(ErrorKind::Other, "Invalid header file name")));

+    let objc_guard;
+    if header_str.contains("objc_") {


instead of this, let's add a new line to the test configuration (see the if line.contains(bindgen-flags check above). // bindgen-osx-only maybe?

Ooh, that makes so much more sense, obvious even.

emilio · 2017-01-30T22:42:31Z

src/ir/objc.rs

+
+impl ObjCInstanceMethod {
+    fn new(name: &str) -> ObjCInstanceMethod {
+


please remove this initial newline.

emilio · 2017-01-30T22:42:47Z

src/ir/objc.rs

+    fn new(name: &str) -> ObjCInstanceMethod {
+
+        let split_name: Vec<&str> = name.split(':').collect();
+


This may also go away if you want :)

emilio · 2017-01-30T22:43:58Z

src/ir/objc.rs

+pub struct ObjCInstanceMethod {
+    /// The original method selector name
+    /// like, dataWithBytes:length:
+    pub name: String,


instead of making these public, could/should we make them private and add accessors?

emilio · 2017-01-30T22:44:10Z

src/ir/objc.rs

+    pub name: String,
+
+    /// List of the methods defined in this interfae
+    pub methods: Vec<ObjCInstanceMethod>,


instead of making these public, could/should we make them private and add accessors?

emilio · 2017-01-30T22:46:27Z

src/codegen/mod.rs

+                .unsafe_()
+                .fn_decl()
+                .self_()
+                .build(ast::SelfKind::Value(ast::Mutability::Immutable))//ast::SelfKind::Region(None, ast::Mutability::Mutable))


remove or reword the comment please :)

emilio · 2017-01-30T22:46:45Z

src/codegen/mod.rs

-            let arg_ty = if let TypeKind::Array(t, _) = *arg_ty.canonical_type(ctx).kind() {
+
+
+            let arg_ty = match *arg_ty.canonical_type(ctx).kind() {


please fix the alignment in this block.

Damnit, I fixed this once already, but somehow rustfmt got really confused in this block :(

emilio · 2017-01-30T22:47:05Z

src/codegen/mod.rs

@@ -2142,10 +2162,24 @@ impl ToRustTy for FunctionSig {
            //     the array type derivation.
            //
            // [1]: http://c0x.coding-guidelines.com/6.7.5.3.html
-            let arg_ty = if let TypeKind::Array(t, _) = *arg_ty.canonical_type(ctx).kind() {
+


please remove the whitespace between the comment and the let.

emilio · 2017-01-30T22:48:42Z

src/codegen/mod.rs

+            .with_items(trait_items)
+            .build();
+
+        let ty_for_impl = aster::AstBuilder::new()


this is the same as: quote_ty!(ctx.ext_cx(), id), which I think is clearer.

emilio · 2017-01-30T22:49:37Z

src/codegen/mod.rs

@@ -2109,6 +2125,10 @@ impl ToRustTy for Type {
                let ident = ctx.rust_ident(&name);
                quote_ty!(ctx.ext_cx(), $ident)
            }
+            TypeKind::ObjCInterface(..) => {
+                let ident = ctx.rust_ident("id");
+                quote_ty!(ctx.ext_cx(), $ident)


just quote_ty!(ctx.ext_cx(), id) should work.

Ah didn't realise that, lot cleaner for sure!

emilio

Two little little nits more, an this is ready to go IMO :)

@fitzgen any concern with this?

emilio · 2017-01-31T09:42:33Z

tests/tests.rs

@@ -87,6 +87,12 @@ fn create_bindgen_builder(header: &PathBuf)
        } else if line.contains("bindgen-unstable") &&
                  cfg!(feature = "llvm_stable") {
            return Ok(None);
+        } else if line.contains("bindgen-osx-only") {


You need to use take(3) above now :)

emilio · 2017-01-31T09:43:14Z

src/codegen/mod.rs

+                    let inner = ctx.resolve_item(inner);
+                    let inner_ty = inner.expect_type();
+                    if let TypeKind::ObjCInterface(_) = *inner_ty.canonical_type(ctx).kind() {
+                        aster::ty::TyBuilder::new().id("id")
            } else {


Wow, I guess rustfmt really likes to mess with this assignment?

that or I forgot add the chunk, nevertheless, I've disabled rustfmt, and hopefully it now makes sense

It parses interfaces and protocol but ignores base classes, and their methods which don’t have arguments, the method signature is currently ignored. Also you can pass objc class instances to C functions. Next steps are inheritance/base classes, method signatures, properties, categories. Then check with system headers what is missing.

emilio · 2017-01-31T22:48:34Z

Looks good to me! I've opened #464 to re-enable testing on OSX, after that lands this is good to go \o/

emilio · 2017-02-02T17:37:54Z

@bors-servo r+

Thanks for this! :)

bors-servo · 2017-02-02T17:37:55Z

📌 Commit 4736263 has been approved by emilio

bors-servo · 2017-02-02T17:37:58Z

⌛ Testing commit 4736263 with merge c1aaa6a...

Add initial Objective C support Hi! I made an attempt to support parsing objective c headers. This is very much incomplete, and likely not quite yet ready for merging, but I thought I'd share the progress so far. Comments appreciated, I'm still very much a newbie in just about everything related to this change (rust as a language, rust ast, libclang, rust-bindgen), and there were many parts of code I wasn't quite sure about. Commit message: It parses interfaces and protocol but ignores base classes, methods that don’t have arguments, the arguments are currently ignored. Also you can pass objc class instances to C functions. Next steps are inheritance/base classes, method signatures, properties, categories. Then check with system headers what is missing.

bors-servo · 2017-02-02T19:13:32Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing c1aaa6a to master...

emilio reviewed Jan 25, 2017

View reviewed changes

scoopr force-pushed the objc branch from 3ffa01d to 761f075 Compare January 27, 2017 01:25

highfive added the S-awaiting-review label Jan 27, 2017

emilio reviewed Jan 27, 2017

View reviewed changes

scoopr force-pushed the objc branch from 761f075 to bf75b0b Compare January 28, 2017 23:30

highfive added the S-needs-rebase label Jan 29, 2017

scoopr force-pushed the objc branch from bf75b0b to 1273da9 Compare January 29, 2017 22:06

scoopr force-pushed the objc branch from 1273da9 to 6ca3259 Compare January 29, 2017 22:17

scoopr force-pushed the objc branch from 6ca3259 to 8b892f5 Compare January 30, 2017 21:15

highfive added the S-needs-rebase label Jan 30, 2017

emilio reviewed Jan 30, 2017

View reviewed changes

scoopr force-pushed the objc branch from 8b892f5 to ea1f397 Compare January 30, 2017 23:36

emilio reviewed Jan 31, 2017

View reviewed changes

scoopr force-pushed the objc branch from ea1f397 to 4736263 Compare January 31, 2017 10:24

highfive assigned emilio Feb 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Feb 2, 2017

bors-servo merged commit 4736263 into rust-lang:master Feb 2, 2017

highfive removed the S-awaiting-merge label Feb 2, 2017

bors-servo mentioned this pull request Feb 2, 2017

Extract and parse unexporsed attributes #459

Closed

		@@ -2074,6 +2091,18 @@ impl ToRustTy for FunctionSig {
		// [1]: http://c0x.coding-guidelines.com/6.7.5.3.html
		let arg_ty = if let TypeKind::Array(t, _) = *arg_ty.canonical_type(ctx).kind() {

		let arg_ty = if let TypeKind::Array(t, _) = *arg_ty.canonical_type(ctx).kind() {


		let arg_ty = match *arg_ty.canonical_type(ctx).kind()


		impl ObjCInstanceMethod {
		fn new(name: &str) -> ObjCInstanceMethod {

		fn new(name: &str) -> ObjCInstanceMethod {

		let split_name: Vec<&str> = name.split(':').collect();

Add initial Objective C support #443

Add initial Objective C support #443

Conversation

scoopr commented Jan 25, 2017

scoopr commented Jan 25, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoopr commented Jan 27, 2017

emilio commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoopr commented Jan 28, 2017

bors-servo commented Jan 29, 2017

emilio commented Jan 29, 2017

scoopr commented Jan 29, 2017

scoopr commented Jan 29, 2017

scoopr commented Jan 29, 2017 • edited Loading

scoopr commented Jan 30, 2017

bors-servo commented Jan 30, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jan 31, 2017

emilio commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

scoopr commented Jan 29, 2017 •

edited

Loading