Virtual inheritance

[HGuillemet]

In the JNI code, the address field of Java objects is interpreted as a pointer to a C++ object using a C-style cast.
This always works when the effective class of the object is the class we are casting to. This works also in most cases where the object class derived. But not always.

First case where this cast is illegal is when the base class B is polymorphic and the derived class D inherits virtually from B:

Example 1:

#include <cstdio>

class B {
  int b = 1;
  public:
    virtual void f() { printf("%d\n", b); }
};

class D: public virtual B {
  int d = 2;
};

If we write in Java: new D().f() we get a segmentation fault.
To prevent this, we would need to replace the C-style cast by a static_cast<B*>(ptr) where ptr is a D*, but we never have a chance to. First time we enter C++ code during this call is in the JNI of method f and at this point the code is specific to B and knows nothing about D.

Example 2, we add this function to Example 1:

void g(B b) {
  b.f();
}

When I call from Java global.g(new D()), on my machine I get 2, which is incorrect. Others may get segmentation fault. Anyway the C-style cast (B*) made by the JNI of g on its argument is illegal.

Another case where C-style cast is illegal is multiple inheritance.

Example 3, we add:

class B2 {
  int b2 = 3;
};

class D2: public B, public B2 {
  int d2 = 4;
};

Here, no segmentation fault nor incorrect output when we call new D2().f() or global.g(new D2()). Java doesn't support multiple inheritance and JavaCPP only keeps the first inheritance relationship. I doubt this is written in any specification, but in practice C-style casts works to up-cast to the first class. JavaCPP also produces automatically an asB2() instance method for D2 that does a static_cast<B2*>.

My suggestions:

1. Drop the Java inheritance in case of C++ virtual inheritance of a polymorphic class. The Java inheritance is just useless and only causes segmentation faults or, worse, hard to diagnose erroneous results. I don't think there is a way to have Java inheritance works here. Is there ?
2. Add an automatic asB() method in this case, just like it's done for multiple inheritance, and also manually by Info in Pytorch presets for Module subclasses.
3. In addition to asB(), we could also automatically remap all member functions of B in D. In example 1, we would add a D.f native method. This would allow the user to do d.f() like in C++ instead of d.asB().f(). Similarly, we could add overloads for all methods taking a B (or one of its superclass) as argument, that take a D instead. This is done manually by Info in Pytorch too, in the specific case of the register_module method.

Suggestions 1 and 2 are important I believe.
3 is less essential, and I'm not sure there is an easy way to implement this in the parser.

[saudet]

Yes, that sounds like the best we can do in the case of virtual inheritance. Plus, it's a feature very rarely used for APIs, so it's not worth spending too much time on it.

[saudet]

BTW, we can already "flatten" hierarchies of classes like that by setting Info.flatten.

[HGuillemet]

Great. I missed that.
So example 1 can be dealed with using an Info("B").flatten().
But this will apply to all subclasses of B, even those not virtually inheriting. And example 2 still doesn't work.

What could be improved is thus:

• automatic flattening without requiring Info
• only when necessary (virtual inheritance and polymorphic superclass)
• automatic overloading/flattening of methods (example 2)

How much change would that need in Parser ?

[saudet]

It doesn't sound like a lot of changes are needed, but we still need to figure out the best places to put them so that it does want we want it do.

[HGuillemet]

I have looked at how flatten works: all text resulting from the parsing of the superclass (besides automatic constructors) is saved in the Info, and the text is copied as is in the subclasses, after a string replacement of the superclass name by the subclass name. Simple but a bit violent. For example for Pytorch Module it doesn't work, probably because the string replacement alters the annotations.

I'm going to try to add something a bit generic to copy declarations of functions from one class to another and use it automatically in the case of virtual inheritance, or on-demand via Info.

Concerning automatic overloading of functions, we need the list of classes virtually inhering the superclass when we parse a function taking the superclass as an argument. But I don't see how we could have such list with the current mechanism where we generate the Java at the same time as the C++ parsing. I guess this will have to wait for the rewrite of the parser and the use of AST.

[HGuillemet]

PR #655 deals with this issue by implementing suggestions 1 and 2 and half of suggestion 3.

However, we could do better and keep the Java inheritance, by:

1. wrapping all native instance methods of B:

native public void f();

with:

native private void _f();
public void f() { asB()._f(); }

2. wrapping all native methods taking a B as argument:

public native void g(B b);

with;

private native void _g(B b);
public void g(B b) { _g(b.asB()); }

This solution avoids the copy of methods or the generation of overloads.

The parser would have to know which classes need this special treatment, which requires a specific Info until the parser is rewritten to analyze an AST and able to know in advance that some classes virtually inherit from B.
The parser could however issue an error or warning when it detects a virtual inheritance against a polymorphic class and the info is not set on the polymorphic class.

[saudet]

This kind of hack works for member methods of that class, but it doesn't work for methods outside that class. We still need a way to cast them around.

[HGuillemet]

If we know a class need this special cast, because we set something line new Info("B").explicitUpcast(), we can add the wrapper for all methods taking an instance of B as parameter, even outside of class B. Or did I miss something ?

[saudet]

I guess we could generate overloads for all methods everywhere, yes...