Djinni Interface Inheritance

[fostah]

This is a feature I've been working on for quite a while, even further back than the commit history associated with this pull request. We started refactoring our iOS and Android apps to share a C++ layer back in November. Djinni was a great choice to minimize a lot of the cross platform boiler plate, but inheritance was required for our app's architecture. This pull request is the outcome of the efforts to fulfill that requirement.

This feature is still a work in progress. What I have here works well -- it's used extensively in the latest release of the our app -- but it's not a complete feature. There's rough spots that I don't like and issues we're aware of. With that said, we thought it was important to start a dialog and hopefully start a process of getting this integrated into Djinni master.

I appreciate any feedback you have and look forward to fleshing this out so others can benefit from it.

Djinni Interface Inheritance Syntax

The syntax to inherit from another interface is pretty simple. Here's a quick example:

foo = interface +c {
    foo_func(): string;
}

bar = interface extends foo +c {
    bar_func(): string;
}

As you would expect, a bar implementation will need to provide implementations for both the foo's foo_func and its own bar_func. Note that it's not required to implement interfaces in C++, but it is the most functionally complete use case at this point (see Type Slicing for details).

Interface Inheritance, NOT Implementation Inheritance

One recurring misunderstanding that we've run into while adopting this new feature on our team is that this is not implementation inheritance. Djinni allows you to define interfaces. With these changes, an inheritance hierarchy of interfaces can be built. However, there's no implementation inheritance provided here.

Using the interfaces provided in Djinni Interface Inheritance Syntax, when you implement foo and bar they will not share an ancestry. Here's a quick and ugly visual of the hierarchy:

foo ----> bar ---> bar_impl
    \
     ---> foo_impl     

To get around this, we've been leveraging something we refer to as inheritance-through-encapsulation. Basically, every subclass implementation has a member variable of the super interface's implementation. Then for the methods it wants to inherit from the super implementation, it just makes a call to the super's method. This crufts up the implementation a bit, but it results in an accurate simulation of a class hierarchy.

Type Slicing

The biggest issue I ran into while implementing this was object types getting sliced when creating the object representation in a different languages. The issue is that Djinni lazily caches objects the first time they need to be converted to another language. This means, for example (again using the interfaces from Djinni Interface Inheritance Syntax), that when you pass a bar object into a method that takes a foo object, you need to make sure a sub object is created and cached in the other languages.

This was a pretty difficult issue for me to get around. And, I only got around it going from C++ to Objective-C or Java. Coming from Objective-C or Java to C++ will still result in a sliced type (see the subclass casting test in the Interface Inheritance tests for more details). And I'm really not that happy with the workaround I came up with for C++ to Objc/Java. It basically involves the C++ generated interfaces carrying around the name's of the Objc and Java proxy classes, and the cache utilities leveraging those names when creating proxy objects.

I think a better approach is possible, if Djinni were to move away from lazy caching the proxy objects to caching the proxy objects when the actual objects are created. That's the only time where Djinni has easy access to all the type information that's needed. However, this will be a rather large change to Djinni, that I just didn't have the time to tackle.

[smarx]

Automated message from Dropbox CLA bot

@fostah, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here.

[smarx]

Automated message from Dropbox CLA bot

@fostah, thanks for signing the CLA!

[fostah]

The jniProxyClassName and objcProxyClassName are now generated for each interface to workaround type slicing when creating the Java and the Objective-C proxies. I would love to come up with a better approach to this, but I think it will require a large change to Djinni's proxy caching. This was a "good enough" solution for our needs.

[artwyman]

Hi, @fostah! Great to see someone taking a stab at this long-requested feature, and using it for a real use case. I don't have the time to do a deep review yet, but will try to get to it when I can. It's probably better for me to understand more about your design and the problems you solved before that, though, so here's some commentary on your description:

1. If I'm correctly understanding you, I'd say "interface inheritance not implementation inheritance" rather than "class inheritance". In C++ at least, the "interfaces" are actually abstract base classes, so there's class inheritance going on, right? But I think you're right that inheriting implementation code is out of the scope of Djinni. Your approach seems like a fine one. I think you might get away with using multiple inheritance too if you make the implementation inheritance private? I'm not 100% sure whether that would fully avoid the issue of multiple distinct foo subobjects when casting.

2. I'm not sure I understand the slicing problem you describe, since I'm not sure what kind of subobject you're talking about needing to create and cache (is this related to the implementation subobjects you described for the issue above?). This sounds similar to an issue we fixed which also involved multiple interfaces on the same object. The issue I was aware of came up when ObjC objects implemented more than one Djinni interface (with no inheritance relationship between them), wwhich is possible for +o interfaces which are generated as a protocol. That was resolved by making the typeid of desired interface one of the arguments to ProxyCache::get(). The same logic should apply to all languages. If that fix is working as expected I'd expect the proxying to function correctly if you pass your bar object as a foo, with the caveat that I'd expect separately-marshalled foo and bar proxies to be distinct objects. The object identity property provided by the proxy cache would only hold for objects which crossed the boundary as the same type, not objects cast to superclasses after that point. Were you seeing something different?

[fostah]

Hey @artwyman. Thanks for your feedback, and sorry for the slow response. I was able to carve out some time last week to put this pull request together, meaning I've got a lot of other work to catch up on this week :)

I have some feedback for your first piece of commentary, but I need some more time to dig into the second one.

First, I agree with your change in terminology. Implementation inheritance is a better term for what I was describing. I've updated the pull request comment.

I gave the private inheritance you suggested a try, but as you guessed, I ran into some casting issues. However, this lead me to virtual inheritance, which seems like a promising solution. If a C++ generated sub-interface was declared as public virtual, and the base-interface's implementation was also declared as public virtual, then the sub-interface's implementation could subclass both. I only quickly came up with a proof-of-concept for this approach. So, I need to spend some more time verifying it. But, it would be a nice enhancement to the feature if it works out as I described. And, it would be a pretty easy change to Djinni to support it.

[mknejp]

Before introducing virtual inheritance I suggest looking into possible alternatives. Virtual inheritance throws a spanner into the normally expected construction sequence. Since so far the generated C++ classes do not have data members this is more or less OK, but should that ever change (e.g. for data binding) then we're in for some nasty surprises. There are techniques to linearize multiple inheritance hierarchies with templates.

[fostah]

Thanks for the heads up @mknejp. I need to absorb a bit more information on linearizing class hierarchies, but what I quickly read seems to suggest that this technique would be an implementation detail, not something that Djinni would be able to generate for you. Does that sound correct to you, @mknejp?

If that's the case, leaving the implementation how it currently is -- without virtual inheritance -- might make more sense. And Djinni's documentation and examples can be updated to provide implementation suggestions for interface hierarchies.

[artwyman]

I'd stay as far away from virtual inheritance as possible, since it adds a lot more complexity, and puts extra burden on implementation classes. For sharing implementation, I think a mix-in template might be a reasonable approach, as Miro suggests. Or using virtual inheritance within your implementation is up to you to decide, but I wouldn't want to add it to the generated code in a way which would enforce it on all users.

[fostah]

Understood. I'm not sure virtual inheritance would be possible without having the sub-interfaces declared as such. But, there seems to be at least two other implementation approaches that will suffice. So, I agree that it makes the most sense to stay away from virtual inheritance in the generated code.

[artwyman]

Just to be clear @fostah I think you own the next step here, since you plan some changes after the discussion above. Is that correct, or do you need a deeper review of what's here?

[fostah]

Hey @artwyman. Thanks for checking in. You're correct. I have some pending investigation and possible actions to take before proceeding. I've just been side tracked with other work the last week so. I'll get back to this as soon as I can.

[artwyman]

No rush, just wanted to make sure I knew what's in my own queue.

[steipete]

@fostah Would love to see support for that as well. Can we help?

[artwyman]

Invoking the code-review process now that it's available, and assigning back to the author who owns the next step here. See prior comments for specifics.

[rashidc]

Thank you for sharing your excellent work, @fostah . Our company would like to use Djinni, but inheritance is a necessity.
Just to check in, are there still plans to field the merge conflicts?

Also I have a basic question on this feature's functionality, just to make sure that I understand how much inheritance it covers. Expanding your earlier example to:

foo = interface +c {
    foo_func(): string;
    overridden_func(): string;
}
bar = interface extends foo +c {
    bar_func(): string;
    overridden_func(): string;
}
zap = interface +c {
    zap_func(my_param:foo);
}

Does this pull request allow one to pass a "bar" type to the "zap" interface's "zap_func" ? And if so, in the c++ implementation would calling "my_param"s "overridden_func" properly call that of the "bar" interface?

Thank you again for your help!

[rashidc]

I managed to answer my own question: For the example given in my previous post, that functionality of inheritance is supported.

Thank you for your helpful pull request!

[niparx]

is this feature dead?

[artwyman]

It's probably safe to assume this PR isn't going to go forward soon. I'm guessing @fostah got too busy/distracted. If someone else (including but not limited to the author) wanted to pick it up and run with it, though, that would be welcome. There were some issues to resolve which might deserve a fresh look, but the CLA was signed so the code in this PR is fine to incorporate or borrow from license-wise.

[ghost]

I addressed the merge conflicts in another branch available here: https://github.com/MikeNachbaur/djinni/tree/InterfaceInheritance

I've created a PR down into the original author's branch (iRobotCorporation#2) in the event @fostah is able to continue this work. I don't have a solution for the original issues that prevented this PR from continuing however, and I don't believe I have the C++ expertise necessary to carry on. But hopefully someone else can continue this work a bit more readily now that the merge conflicts have been unblocked.

[andrewfunston]

@artwyman I'm looking at this a bit with @MikeNachbaur - so to get this the last mile what are the blockers?

I've just started playing with this new branch. It seemed like you believe "type slicing" isn't an issue (since it was similar to the pure objc +o multiple inheritance issue) but @fostah believed it was? That is - the happy path is c++ -> <java/objc> but <java/objc> -> c++ didn't work for pushing through inherited classes? I'm trying to get together a minimal test to understand what's missing, will report back as I play with it a bit, but if you have anything to add please do!

[artwyman]

I just came back to look at this after not having enough time for a while. Honestly I think this has enough history far enough back that reconstructing it all isn't going to be productive if the original author isn't involved. @andrewfunston or @MikeNachbaur if you want to take a stab at this feature, I'd suggest making a new PR with your own proposal (based on this one or not as you prefer) and me or @xianwen can take a fresh look.

[4brunu]

@MikeNachbaur @andrewfunston Did you make progress on this feature? If you need, I can try to help. Thanks 👍