java.interop icon indicating copy to clipboard operation
java.interop copied to clipboard

Error with public class exposing inherited protected class methods with covariant return types

Open jpobst opened this issue 2 years ago • 3 comments
trafficstars

Imagine we have the following Java:

public abstract class ClassA {
  protected abstract class ChildClass {
    public abstract ClassA createThing ();
  }
}

public class ClassB extends ClassA {
  public class ChildClass extends ClassA.ChildClass {
    @Override
    public ClassB createThing () { ... }
  }
}

There are 2 interesting things to "fix" when translating this to C#:

  1. public class ClassB.ChildClass inherits protected class ClassA.ChildClass, which C# doesn't allow
  2. ClassB.ChildClass.createThing () implements the abstract method ClassA.ChildClass.createThing (), but it has a covariant return type

The way we fix (1) is:

  • Make ClassB.ChildClass inherit from Object instead of ClassA.ChildClass
  • Copy members from ClassA.ChildClass to ClassB.ChildClass

If we didn't have the covariant return type (2), we would detect that ClassB.ChildClass.createThing () implements ClassA.ChildClass.createThing () and we would generate a virtual method.

However, because of the covariant return type, we cannot make the match and end up generating both a virtual and an abstract createThing ():

public abstract partial class ClassA : Java.Lang.Object {
  protected internal abstract partial class ChildClass : Java.Lang.Object {
    [Register ("createThing", "()Lexample/ClassA;", "GetCreateThingHandler")]
    public abstract ClassA CreateThing ();
  }
}

public partial class ClassB : ClassA {
  public new partial class ChildClass : Java.Lang.Object {
    [Register ("createThing", "()Lexample/ClassB;", "GetCreateThingHandler")]
    public virtual unsafe ClassB CreateThing () { ... }

    [Register ("createThing", "()Lexample/ClassA;", "GetCreateThingHandler")]
    public abstract ClassA CreateThing ();
  }
}

This fails with:

CS0513 'ClassB.ChildClass.CreateThing()' is abstract but it is contained in non-abstract type 

as well as several duplicate member errors.

jpobst avatar May 23 '23 17:05 jpobst

Additional context:

This was found while enabling DIM for 4 packages in https://github.com/xamarin/GooglePlayServicesComponents/pull/779. An interesting aspect to investigate is that these instances do not appear to have any relation to interfaces or default methods.

jpobst avatar May 25 '23 14:05 jpobst

Paraphrasing my replies from Discord…

Conceptual issue: the original sample code is inconsistent. The Java code has getThing() methods, while the binding code instead has DoThing() methods. These should be made consistent, and should probably just become createThing()/CreateThing(), as getThing() would become a property when bound, which just complicates discussion.

Discussion:

I think the "problem" is:

  • Copy members from ClassA.ChildClass to ClassB.ChildClass

While this needs to be done, we can't "just copy" all members from the base into the derived type. We need to copy only non-overridden/"hidden" members.

See also:

So we are attempting to filter, but the filter is not quite right.

Here might be the crux of the problem:

  • https://github.com/xamarin/java.interop/blob/93c50fef9eca85110977ee47ddaa053f2ed47d59/tools/generator/Java.Interop.Tools.Generator.ObjectModel/Method.cs#L229

Method.Matches() doesn't take covariant return types into consideration! It only supports exact matches. Consequently, Method.Matches() doesn't believe that ClassB.ChildClass.createThing() overrides ClassA.ChildClass.createThing(), because the return types don't match. (ClassB != ClassA.)

Finally, note that as per 4ec5d4e9, the binding of ClassB.ChildClass cannot inherit from ClassA.ChildClass, as ClassA.ChildClass isn't public. As such:

  • ClassB.ChildClass should inherit Java.Lang.Object
  • ClassB.ChildClass.CreateThing() should be virtual, not abstract

jonpryor avatar May 26 '23 00:05 jonpryor

The Java code has getThing() methods, while the binding code instead has DoThing() methods

Updated issue description to standardize on createThing.

jpobst avatar May 30 '23 15:05 jpobst