[generator] Fix NRE from return type not getting set for abstract methods with generics.

[jpobst]

Fixes: dotnet/android#5921

For the following generic-laden Java:

public interface MyInterface<R> {
  R Next (int count);
}

public abstract class MyClass<T> implements MyInterface<T> {
  public T Next (int count);
}

We need to generate an explicit method declaration:

Java.Lang.Object Com.Example.IMyInterface.Next (int count)
{
  throw new NotImplementedException ();
}

However when we did the generator refactor this code code path was not tested, and the code short-circuits, bypassing setting the ReturnType and Parameters collection.

Attempting to generate the method results in an NRE:

ReturnType.WriteTypeReference (writer);

   at Xamarin.SourceWriter.MethodWriter.WriteReturnType(CodeWriter writer) in C:\code\xamarin-android-backport\external\Java.Interop\src\Xamarin.SourceWriter\Models\MethodWriter.cs:line 154

This commit ensures the ReturnType and Parameters collection is set before the method returns.

Pre-refactor code: https://github.com/xamarin/java.interop/blob/main/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/CodeGenerator.cs#L1175-L1181

[jonpryor]

@jpobst: I'm confused. If I extract the XML within WriteBoundMethodAbstractDeclarationWithGenericReturn(), I'm able to bind it using java.interop/main without any error:

$ mono bin/Debug/generator.exe -o ji-834.main ~/ji-834.xml --codegen-target=XAJavaInterop1
# no error

Additionally, the generated C# source doesn't contain explicitly implemented interface members either; there is no Com.Example.RangeIterator.Next() method:

$ grep -r Com.Example.RangeIterator.Next ji-834.main
# no output

(Why was I bothering in the first place? For the commit message, to attempt to describe why we were explicitly implementing a method in the first place, to help "backfill" historical knowledge. On a related note, the original PR message doesn't quite make sense: the "set-up" mentions MyInterface and MyClass, while the generated code is for RangeIterator (?!). Additionally, RangeIterator is being explicitly implemented, but classes can't be explicitly implemented, that's an interface member feature.)

I thus can't be sure that the fix here is an actual fix, given that the test here doesn't trigger the exception mentioned in dotnet/android#5921.

[jpobst]

Interesting, if you revert the change to BoundMethodAbstractDeclaration.cs the unit test will fail with the specified exception. However I may have cut down the test case too much that it is no longer self-contained.

Here's the original api.xml that causes the problem on 16.9.

api.xml.fixed.txt

[jpobst]

It looks like the test fails while real generator does not because we do not use the full pipeline in our tests. Specifically, our tests do not call gen.FixupExplicitImplementation () which eliminates the explicitly implemented method in real generator. Thus real generator never hits the code path that causes the NRE.

[jonpryor]

@jpobst wrote

Thus real generator never hits the code path that causes the NRE.

This sounds like we don't yet have a repro for the a code path which caused "real generator" to NRE then. :-(

[jpobst]

The code path that crashes is simply trying to generate any BoundMethodAbstractDeclaration where method.RetVal.IsGeneric && gen != null. How we get to that code path isn't really relevant. I feel like the unit test is sufficient for demonstrating that the crash in that path has been fixed.