[generator] support covariant return types

[jonathanpeppers]

Java has the ability to define a method, Object getObject() and then
change the return type to String getObject() in an overridden method.
This is not possible in C#, so cases like this cause the generator to
create invalid C# code.

My current thoughts to fix this are:

• Look for “covariant return types” on interfaces and base classes via
  a IsCovariantOverride method
• IsCovariant is held true if: neither are generic, have the same
  method name, have a different return type, and the signature of the
  parameters is the same.
• If a covariant return value is found, override RetVal.FullName
  with the ReturnType of the base method.
• Also need to set a new override/callback, so that the base method's
  RetVal.ToNative can be called. Otherwise, the wrong code is generated
  for the return statement.
• Include test cases for covariance, and hope we have enough test
  coverage to show if this breaks anything

Other changes:

• Removed unused Method owner parameter from ReturnValue type

[jonathanpeppers]

Note: when either #216 or #217 are merged, I will need to rebase the second PR and update the CovariantReturnTypes test.

[jonpryor]

PR #217 was merged, so I guess you need to update the CovariantReturnTypes test.

[jonpryor]

One check to do: in your local xamarin-android tree, update external/Java.Interop to contain this fix, then:

cd src/Mono.Android
mv obj/Debug/android-27/mcw __mcw
xbuild
diff -rup __mcw obj/Debug/android-27/mcw

How reasonable does the diff look? (I fear something will "break", in that the return type is changed in an incompatible manner.)

Additionally, from the xamarin-android topdir:

make run-api-compatibility-tests

Does that report any breakage?

[jonpryor]

This should check for Parameters.Count equality before hitting JavaSignature, to save on extra work.

[jonpryor]

This is wrong.

As per the XML:

<method abstract="false" deprecated="not deprecated" final="false" name="getObject" native="false" return="java.lang.String" static="false" synchronized="false" visibility="public">

This method returns java.lang.String on the Java side. As such, the JNI signature we use for lookup should also be java.lang.String. Specifically, this should be:

const string __id = "getObject.()Ljava/lang/String;";

[jonpryor]

...and this raises a different yet related problem: the [Register] attribute is used as part of Java Callable Wrapper generation; the values present are used for the method overrides.

For example, if we do:

public class Whatever : CovariantInterfaceImplementation {
    public override Java.Lang.Object Object {
        get {return this; /* mwa-ha-ha-ha-ha-ha! */}
    }
}

then the generated Java Callable Wrapper contain:

public class Whatever extends CovariantInterfaceImplementation {
    public java.lang.Object getObject() {
        return n_getObject();
    }
    native java.lang.Object getObject();
}

This will not compile, as the Java CovariantInterfaceImplementation is:

public class CovariantInterfaceImplementation {
    public String getObject() {/*...*/}
}

Object getObject() cannot override a String getObject() method.

What would actually need to happen is that the [Register] attribute needs to contain information for the covariant override:

public virtual unsafe global::Java.Lang.Object Object {
    [Register ("getObject", "()Ljava/lang/String;", "GetGetObjectHandler" /* eh? */)]
    get { ... }
}

This will ensure that the Java Callable Wrapper is sane.

It won't fix the deliberate "think-o" in the above example: it's supposed to be a Java, but the C# compiler doesn't know that, and allows us to return a non-String value. This likely isn't fixable.

Question-to-which-I-don't-know-the-answer: is GetGetObjectHandler going to be correct here? I don't know.

[jonpryor]

For good measure/coverage/"future proofing", you should also try binding:

public class GenericStringImplementation implements GenericInterface<String> {
    public String getObject() {return "";}
}

...just to make sure that does something "reasonable."

[jonathanpeppers]

Well, this example breaks... even without my changes 😞

It looks like there is not an implementation for explicit interfaces on properties, like we have for methods: https://github.com/xamarin/java.interop/blob/79a8e1ee93673aa2f8ef9f2ea8cb9bebdc1394df/tools/generator/Method.cs#L598

We need a second foreach loop here for properties: https://github.com/xamarin/java.interop/blob/461878798f8ec076e2f56c0141aa7b605e911fb0/tools/generator/ClassGen.cs#L474

I think I should redo these covariant tests to just use methods (maybe just have one case for a property). After this gets merged, I can revisit the issues with generic interfaces + properties and make specific test cases for those.

[jonathanpeppers]

How reasonable does the diff look? (I fear something will "break", in that the return type is changed in an incompatible manner.)

There are some APIs that are breaking, one example:

 		// Metadata.xml XPath method reference: path="/api/package[@name='java.nio']/class[@name='CharBuffer']/method[@name='append' and count(parameter)=1 and parameter[1][@type='java.lang.CharSequence']]"
 		[Register ("append", "(Ljava/lang/CharSequence;)Ljava/nio/CharBuffer;", "GetAppend_Ljava_lang_CharSequence_Handler")]
-		public virtual unsafe Java.Nio.CharBuffer Append (Java.Lang.ICharSequence csq)
+		public virtual unsafe Java.Lang.IAppendable Append (Java.Lang.ICharSequence csq)

I need to keep working on this. I tried running make run-api-compatibility-tests before I sent this PR, but I guess I missed a step to clean the obj/Debug/android-*/mcw directories in Mono.Android.

[jonpryor]

This looks good, but before we merge we're going to need to have a xamarin-android-side patch that doesn't cause API compatibility to be broken.

@jonathanpeppers: Would you be able to work on that?

[jonathanpeppers]

@jonpryor I'm going to work on this next.

I found another issue when testing this in xamarin-android, it seems there are some NREs caused by how the types happened to be ordered in the XML.

I added a bunch of ? locally to get past them, but I need to fix it the right way. Hopefully, I can work it all out!

[jonathanpeppers]

Ok I've fixed the NREs, and the current diff for Mono.Android.dll looks reasonable: https://gist.github.com/jonathanpeppers/3a2b49976d6c3a0dacae50e6b656e727

It seems to be about 22 files. I'll look info fixing the API breakage tomorrow.

[jonathanpeppers]

@jonpryor here is the patch that would keep API compatibility (it wasn't that bad): dotnet/android@master...jonathanpeppers:covariant-return-types

1. The diff now only shows changes in obj/Debug/android-27/mcw/api.xml.fixed from the patch.
2. make run-api-compatibility-tests passes now.

[jonpryor]

@atsushieno: Please review these changes, in particular the Mono.Android/metadata changes at: #216 (comment)

[atsushieno]

I'm okay with adding those metadata changes, but the fact that this change required metadata changes scares me - will third party / components developers be suffered from ABI breakages due to this change?

I'd rather like to ask @Redth for that possibility.

[jonathanpeppers]

The thing we have going for us on this change is:

1. Before, you had to do something to work around covariant return types
2. If you just added a metadata fixup to change managedReturn, you should not have to change anything--just the fixup wouldn't be needed anymore

The only issue is if a manual addition is added (like Mono.Android had), you would have to make a change to keep API compatibility.

Would we all feel better if I tried @Redth 's API diff on the repo here before we merge this? https://github.com/xamarin/AndroidSupportComponents

[jonathanpeppers]

I made Redth a build, and he's going to give it a try.

[jonathanpeppers]

Here are Redth's results:

Seems very similar to what I saw running this change on Mono.Android.dll, but alot less changes.

We are thinking the changes on this PR should be good for 15-7.

[atsushieno]

Sounds like it is a breaking change to bindings library developers too. That matches my understanding on making changes to this semantics. If you guys are positive to those changes, feel free to go ahead, but I don't support the idea.

[jonathanpeppers]

@atsushieno can you give some detail on what you would like generator to do instead?

Should generator, create both methods?

Like in the example above, it could generate both:

• public Transition Clone
• public Java.Lang.Object Clone

I'm not sure if this right or wrong/better or worse. What do you think?

I do think #216 and #223 address the most common pain points for developers w/ binding projects.

[jonathanpeppers]

Whoops actually this wouldn't compile in C#:

• public Transition Clone
• public Java.Lang.Object Clone

Can't change the return type without changing method signature in C#...

So another option?

[atsushieno]

I left it as is and let developers decide what to do.

[jonpryor]

My goal for generator is that a developer should be able to provide any .jar to a Library Binding Project and "something useful" should happen.

Something which compiles.

Clearly we're quite far from that goal. :-)

My understanding from prior conversations with Atsushi is that he doesn't fully agree with that goal, and considers "required developer fixup" to be acceptable.

I consider any required developer fixup to be a bug. (And yes, we have many, many, bugs.)

This is clearly a "the perfect is the enemy of good" scenario, and opinions will vary as to which is "the perfect" and which is "good".

---

All that aside, I have an alternate proposal. (Which is, of course, more complicated.)

The only place that fixing up the return type matters is when overriding an abstract method.

If the base class method is not abstract, then we can declare a new method.

If the method comes from an interface, then we can declare the method and explicitly implement it.

For example:

// Java
public abstract class AbstractBase {
    public abstract Object method();
}
public interface Interface {
    java.util.Collection value();
}
public abstract class IntermediateAbstractClass extends AbstractBase implements Interface {
    public AbstractList method() {return null;}
    public abstract AbstractCollection value();
}
public class ConcreteClass extends IntermediateAbstractClass {
    public ArrayList method() {return null;}
    public AbstractList value() {return null;}
}
public class ConcreteInterface implements Interface {
    public AbstractCollection value() {return null;}
}

From the "only abstract overrides need an exact match" rule, we can bind as:

// C# Binding
public abstract class AbstractClass {
    public abstract Object Method();
}
public interface IInterface {
    public Java.Util.ICollection Value();
}
public abstract class IntermediateAbstractClass : AbstractClass, IInterface {
    public override Object Method() {return null;}
    public abstract AbstractCollection Value();
    Java.Util.ICollection IInterface.Value() {return Value();}
}
public class ConcreteClass : IntermediateAbstractClass {
    public new ArrayList Method() {return null;}
    public override AbstractCollection Value() {return null;}
}
public class ConcreteInterface : IInterface {
    public AbstractCollection Value() {return null;}
    Java.Util.ICollection IInterface.Value() {return Value();}
}

With this idea in mind, we can turn to Redth's results:

• Transition.Clone() is a non-abstract method. It thus would not override, and would instead declare a new method. No API change would occur.

All the methods are Clone(), actually, so they'd always be new, not override, unless you encountered a class which made it abstract:

public abstract class RequiresClone {
    public abstract RequiresClone clone();
}

[jonathanpeppers]

I just had a slight seizure... but I think this is the right idea.

The three cases do seem like they need to be handled differently:

1. Abstract members -> breaking change as we see above, change the return type
2. Virtual members -> declare new
3. Interface members -> explicitly implemented

If I change this PR to this logic, only no. 1 would cause breaking changes. Redth's example above, looks like it is no. 2. So it seems we can do this with a lot less breaking than what is currently in this PR.

I have been working on automating something on VSTS to get better reports against what happens with generator changes upstream in xamarin-android, and potentially other components repos. I think its worth a little work to put this in place, so we can actually change generator without as much fear.

[atsushieno]

I'm not going to join such a discussion that 1) the problems that I pointed out are all ignored, and 2) I'm impressed to prefer build failures. Last thing I point out here is "predictability" - if developers cannot predict and logicaly explain why the outcome bindings look like such, that's not a reliable tool.

[jpobst]

I am going to close this as it is likely superseded by proper covariant return type support planned for C# 9.