[generator] Fix NRE from return type not consistently set (#834)

Fixes: dotnet/android#5921 Context: #834 (comment) Context: https://github.com/xamarin/java.interop/blob/100fffc1dc416f543293d0509870138d9d4f1669/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/CodeGenerator.cs#L1175-L1181 Consider the following Java type declaration: package com.example; public interface FlowIterator<R> { R next(); public static class RangeIterator<T extends Comparable<T>> implements FlowIterator<T> { public T next() {return null;} } } This used to work in d16-7 -- not sure when exactly we broke it -- but it now throws a `NullReferenceException` when using `generator --codegen-target=XAJavaInterop1`, when emitting the nested `FlowIteratorRangerIterator` type: $ mono bin/Debug/generator.exe -o yyy ji-834-fixed.xml --codegen-target=XAJavaInterop1 … Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object at Xamarin.SourceWriter.MethodWriter.WriteReturnType (Xamarin.SourceWriter.CodeWriter writer) at Xamarin.SourceWriter.MethodWriter.WriteSignature (Xamarin.SourceWriter.CodeWriter writer) at Xamarin.SourceWriter.MethodWriter.Write (Xamarin.SourceWriter.CodeWriter writer) Fix the `NullReferenceException` by ensuring that the `ReturnType` property and `Parameters` collection are set before the `BoundMethodAbstractDeclaration` constructor completes.
dotnet · May 13, 2021 · 131c149 · 131c149
1 parent 100fffc
commit 131c149
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 2 deletions.
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs b/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
@@ -1,9 +1,11 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using generator.SourceWriters;
 using MonoDroid.Generation;
 using NUnit.Framework;
 using Xamarin.Android.Binder;
+using Xamarin.SourceWriter;
 
 namespace generatortests
 {
@@ -1271,5 +1273,55 @@ public void WriteClassInternalBase ()
 			Assert.True (result.Contains ("internal static new IntPtr class_ref".NormalizeLineEndings ()));
 			Assert.False (result.Contains ("internal static IntPtr class_ref".NormalizeLineEndings ()));
 		}
+
+		[Test]
+		public void WriteBoundMethodAbstractDeclarationWithGenericReturn ()
+		{
+			// Fix a case where the ReturnType of a class method implementing a generic interface method
+			// that has a generic parameter type return (like T) wasn't getting set, resulting in a NRE.
+			var gens = ParseApiDefinition (@"<api>
+			  <package name='java.lang' jni-name='java/lang'>
+			    <class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
+			    <interface abstract='false' deprecated='not deprecated' final='false' name='Comparable' static='false' visibility='public' jni-signature='Ljava/lang/Double;' />
+			  </package>
+
+			  <package name='com.example' jni-name='com/example'>
+			    <interface abstract='true' deprecated='not deprecated' final='false' name='FlowIterator' static='false' visibility='public' jni-signature='Lcom/example/FlowIterator;'>
+			      <typeParameters>
+			        <typeParameter name='R' classBound='java.lang.Object' jni-classBound='Ljava/lang/Object;'></typeParameter>
+			      </typeParameters>
+			      <method abstract='true' deprecated='not deprecated' final='false' name='next' jni-signature='()Ljava/lang/Object;' bridge='false' native='false' return='R' jni-return='TR;' static='false' synchronized='false' synthetic='false' visibility='public' />
+			    </interface>
+			    <class abstract='true' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='FlowIterator.RangeIterator' static='true' visibility='public' jni-signature='Lcom/example/FlowIterator$RangeIterator;'>
+			      <implements name='com.example.FlowIterator' name-generic-aware='com.example.FlowIterator&lt;T&gt;' jni-type='Lcom/example/FlowIterator&lt;TT;&gt;;'></implements>
+			      <typeParameters>
+			        <typeParameter name='T' interfaceBounds='java.lang.Comparable&lt;T&gt;' jni-interfaceBounds='Ljava/lang/Comparable&lt;TT;&gt;;'>
+			          <genericConstraints>
+			            <genericConstraint type='java.lang.Comparable&lt;T&gt;'></genericConstraint>
+			          </genericConstraints>
+			        </typeParameter>
+			      </typeParameters>
+			      <method abstract='false' deprecated='not deprecated' final='false' name='next' jni-signature='()Ljava/lang/Comparable;' bridge='false' native='false' return='T' jni-return='TT;' static='false' synchronized='false' synthetic='false' visibility='public' />
+			    </class>
+			  </package>
+			</api>");
+
+			var declIface = gens.OfType<InterfaceGen> ().Single (c => c.Name == "IFlowIterator");
+			var declClass = declIface.NestedTypes.OfType<ClassGen> ().Single (c => c.Name == "FlowIteratorRangeIterator");
+			var method = declClass.Methods.Single ();
+
+			var bmad = new BoundMethodAbstractDeclaration (declClass, method, options, null);
+			var source_writer = new CodeWriter (writer);
+
+			bmad.Write (source_writer);
+
+			var expected = @"Java.Lang.Object Com.Example.FlowIteratorRangeIterator.Next ()
+					{
+					  throw new NotImplementedException ();
+					}";
+
+			// Ignore nullable operator so this test works on all generators  ;)
+			Assert.AreEqual (expected.NormalizeLineEndings (), writer.ToString ().NormalizeLineEndings ().Replace ("?", ""));
+		}
 	}
 }
diff --git a/tools/generator/SourceWriters/BoundMethodAbstractDeclaration.cs b/tools/generator/SourceWriters/BoundMethodAbstractDeclaration.cs
@@ -19,6 +19,9 @@ public BoundMethodAbstractDeclaration (GenBase gen, Method method, CodeGeneratio
 			this.method = method;
 			this.opt = opt;
 
+			ReturnType = new TypeReferenceWriter (opt.GetTypeReferenceName (method.RetVal));
+			this.AddMethodParameters (method.Parameters, opt);
+
 			if (method.RetVal.IsGeneric && gen != null) {
 				Name = method.Name;
 				ExplicitInterfaceImplementation = opt.GetOutputName (gen.FullName);
@@ -35,7 +38,6 @@ public BoundMethodAbstractDeclaration (GenBase gen, Method method, CodeGeneratio
 			IsAbstract = true;
 			IsShadow = impl.RequiresNew (method.Name, method);
 			SetVisibility (method.Visibility);
-			ReturnType = new TypeReferenceWriter (opt.GetTypeReferenceName (method.RetVal));
 
 			NewFirst = true;
 
@@ -51,7 +53,6 @@ public BoundMethodAbstractDeclaration (GenBase gen, Method method, CodeGeneratio
 			Attributes.Add (new RegisterAttr (method.JavaName, method.JniSignature, method.ConnectorName, additionalProperties: method.AdditionalAttributeString ()));
 
 			SourceWriterExtensions.AddMethodCustomAttributes (Attributes, method);
-			this.AddMethodParameters (method.Parameters, opt);
 		}
 
 		public override void Write (CodeWriter writer)