[generator] fix for NRE in FixupAccessModifiers (#299)

Context: 4ec5d4e

In improving our support for "package-private" classes, we added a
check to compare if two methods have the same signature. The
expression was of the form:

	m.Name == baseMethod.Name && m.Parameters.JavaSignature == baseMethod.Parameters.JavaSignature

The problem with this is that the `JavaSignature` property can throw
a `NullReferenceException` if one of the parameter types is unknown,
or only becomes valid later on in generator's process:

	System.NullReferenceException: Object reference not set to an instance of an object
	  at MonoDroid.Generation.Parameter.get_JavaType ()
	  at MonoDroid.Generation.ParameterList.get_JavaSignature ()
	  at MonoDroid.Generation.ClassGen+<>c__DisplayClass29_0.<FixupAccessModifiers>b__0 (MonoDroid.Generation.Method m)
	  at System.Linq.Enumerable.TryGetFirst[TSource] (System.Collections.Generic.IEnumerable`1[T] source, System.Func`2[T,TResult] predicate, System.Boolean& found)
	  at System.Linq.Enumerable.FirstOrDefault[TSource] (System.Collections.Generic.IEnumerable`1[T] source, System.Func`2[T,TResult] predicate)
	  at MonoDroid.Generation.ClassGen.FixupAccessModifiers ()
	  at Xamarin.Android.Binder.CodeGenerator.Validate (System.Collections.Generic.List`1[T] gens, MonoDroid.Generation.CodeGenerationOptions opt)
	  at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver resolver)
	  at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options)
	  at Xamarin.Android.Binder.CodeGenerator.Main (System.String[] args)


Instead of using `JavaSignature`, we need to rely *only* on the
original Java information, and loop over the parameters individually.

To fix this:

  - Create a `bool Matches (MethodBase other)` method in `MethodBase`
  - Override `Matches()` in `Method`, also check the return type
  - Add some unit tests for these scenarios for both `XmlMethod` and
    `ManagedMethod`

Author: jonathanpeppers

tools/generator/ClassGen.cs

@@ -246,7 +246,7 @@ public override void FixupAccessModifiers ()
 				if (baseClass != null && RawVisibility == "public" && baseClass.RawVisibility != "public") {
 					//Skip the BaseType and copy over any "missing" methods
 					foreach (var baseMethod in baseClass.Methods) {
-						var method = Methods.FirstOrDefault (m => m.Name == baseMethod.Name && m.Parameters.JavaSignature == baseMethod.Parameters.JavaSignature);
+						var method = Methods.FirstOrDefault (m => m.Matches (baseMethod));
 						if (method == null)
 							Methods.Add (baseMethod);
 					}

tools/generator/Method.cs

@@ -387,6 +387,22 @@ public string ReturnType {
 			get { return RetVal.FullName; }
 		}
 
+		public override bool Matches (MethodBase other)
+		{
+			bool ret = base.Matches (other);
+			if (!ret)
+				return ret;
+
+			var otherMethod = other as Method;
+			if (otherMethod == null)
+				return false;
+
+			if (RetVal.RawJavaType != otherMethod.RetVal.RawJavaType)
+				return false;
+
+			return true;
+		}
+
 		public string GetMetadataXPathReference (GenBase declaringType)
 		{
 			return string.Format ("{0}/method[@name='{1}'{2}]", declaringType.MetadataXPathReference, JavaName, Parameters.GetMethodXPathPredicate ());

tools/generator/MethodBase.cs

@@ -176,6 +176,22 @@ public string Visibility {
 		public bool IsValid { get; private set; }
 		public string Annotation { get; internal set; }
 
+		public virtual bool Matches (MethodBase other)
+		{
+			if (Name != other.Name)
+				return false;
+
+			if (Parameters.Count != other.Parameters.Count)
+				return false;
+
+			for (int i = 0; i < Parameters.Count; i++) {
+				if (Parameters [i].RawNativeType != other.Parameters [i].RawNativeType)
+					return false;
+			}
+
+			return true;
+		}
+
 		public bool Validate (CodeGenerationOptions opt, GenericParameterDefinitionList type_params)
 		{
 			opt.ContextMethod = this;

tools/generator/Tests/Unit-Tests/ManagedTests.cs

@@ -19,6 +19,12 @@ public void Bar () { }
 		[Register ("barWithParams", "(ZID)Ljava/lang/String;", "")]
 		public string BarWithParams (bool a, int b, double c) => string.Empty;
 
+		[Register ("unknownTypes", "(Lmy/package/foo/Unknown;)V", "")]
+		public void UnknownTypes (object unknown) { }
+
+		[Register ("unknownTypes", "(Lmy/package/foo/Unknown;)Lmy/package/foo/Unknown;", "")]
+		public object UnknownTypesReturn (object unknown) => null;
+
 		[Register ("value")]
 		public const int Value = 1234;
 	}
@@ -85,6 +91,31 @@ public void Method ()
 			Assert.IsNull (method.Deprecated);
 		}
 
+		[Test]
+		public void Method_Matches_True ()
+		{
+			var type = module.GetType ("Com.Mypackage.Foo");
+			var @class = new ManagedClassGen (type);
+			var unknownTypes = type.Methods.First (m => m.Name == "UnknownTypes");
+			var methodA = new ManagedMethod (@class, unknownTypes);
+			var methodB = new ManagedMethod (@class, unknownTypes);
+			Assert.IsTrue (methodA.Matches (methodB), "Methods should match!");
+		}
+
+		[Test]
+		public void Method_Matches_False ()
+		{
+			var type = module.GetType ("Com.Mypackage.Foo");
+			var @class = new ManagedClassGen (type);
+			var unknownTypesA = type.Methods.First (m => m.Name == "UnknownTypes");
+			var unknownTypesB = type.Methods.First (m => m.Name == "UnknownTypesReturn");
+			unknownTypesB.Name = "UnknownTypes";
+			var methodA = new ManagedMethod (@class, unknownTypesA);
+			var methodB = new ManagedMethod (@class, unknownTypesB);
+			//Everything the same besides return type
+			Assert.IsFalse (methodA.Matches (methodB), "Methods should not match!");
+		}
+
 		[Test]
 		public void MethodWithParameters ()
 		{

tools/generator/Tests/Unit-Tests/XmlTests.cs

@@ -34,6 +34,12 @@ public void SetUp ()
 				<parameter name=""b"" type=""int"" />
 				<parameter name=""c"" type=""double"" />
 			</method>
+			<method abstract=""false"" deprecated=""not deprecated"" final=""false"" name=""unknownTypes"" native=""false"" return=""void"" static=""false"" synchronized=""false"" visibility=""public"" managedReturn=""System.Void"">
+				<parameter name=""unknown"" type=""my.package.Unknown"" />
+			</method>
+			<method abstract=""false"" deprecated=""not deprecated"" final=""false"" name=""unknownTypesReturn"" native=""false"" return=""my.package.Unknown"" static=""false"" synchronized=""false"" visibility=""public"" managedReturn=""System.Object"">
+				<parameter name=""unknown"" type=""my.package.Unknown"" />
+			</method>
 	  		<field deprecated=""not deprecated"" final=""true"" name=""value"" static=""true"" transient=""false"" type=""int"" type-generic-aware=""int"" visibility=""public"" volatile=""false"" value=""1234"" />
 		</class>
 		<interface abstract=""true"" deprecated=""not deprecated"" final=""false"" name=""service"" static=""false"" visibility=""public"" />
@@ -87,6 +93,31 @@ public void Method ()
 			Assert.IsNull (method.Deprecated);
 		}
 
+		[Test]
+		public void Method_Matches_True ()
+		{
+			var element = package.Element ("class");
+			var @class = new XmlClassGen (package, element);
+			var unknownTypes = element.Elements ("method").Where (e => e.Attribute ("name").Value == "unknownTypes").First ();
+			var methodA = new XmlMethod (@class, unknownTypes);
+			var methodB = new XmlMethod (@class, unknownTypes);
+			Assert.IsTrue (methodA.Matches (methodB), "Methods should match!");
+		}
+
+		[Test]
+		public void Method_Matches_False ()
+		{
+			var element = package.Element ("class");
+			var @class = new XmlClassGen (package, element);
+			var unknownTypesA = element.Elements ("method").Where (e => e.Attribute ("name").Value == "unknownTypes").First ();
+			var unknownTypesB = element.Elements ("method").Where (e => e.Attribute ("name").Value == "unknownTypesReturn").First ();
+			unknownTypesB.Attribute ("name").Value = "unknownTypes";
+			var methodA = new XmlMethod (@class, unknownTypesA);
+			var methodB = new XmlMethod (@class, unknownTypesB);
+			//Everything the same besides return type
+			Assert.IsFalse (methodA.Matches (methodB), "Methods should not match!");
+		}
+
 		[Test]
 		public void MethodWithParameters ()
 		{