[generator] deep clone methods to avoid NREs (#1080)

Context: 4ec5d4e
Context: #1083
Context: `generator` crash while binding Google Material 1.6.1

Imagine:

 1. A `public` class that inherits from a *package-private* class, and
 2. The *package-private* class contains `public` or `protected`
    members which should be exposed on (1).

For example:

	/* package-private*/ class BaseClass<V> {
	    public void doThing(V value) {}
	}

	public class MyClass extends BaseClass<Object> {
	}

We won't bind `BaseClass<V>` as it is *package-private*, but we want
(need) it's publicly accessible members to be available to users of
the `public` type `MyClass`; that is, *in Java*,
`MyClass.doThing(Object)` will be an accessible method, and our
binding of `MyClass` should likewise have a `MyClass.DoThing(Object)`
method binding.

In 4ec5d4e we supported this scenario by copying members from the
*package-private* base class into the derived class.

Or at least, that's what the commit says it does.  It does not actually
create a *copy* of the `Method`; rather, it simply adds the existing
`Method` instance to the public derived class, meaning that the same
`Method` instance is referenced by both the base & derived types!

In general, this is fine.  However consider `BaseClass<V>`, above:
the derived type `MyClass` doesn't have a generic type argument `V`,
and thus "copying" `BaseClass<V>.doThing(V)` as-is into `MyClass`
is a non-sequitur; when we later `Validate()` the `Method` instance,
this incongruity is detected and a warning is emitted:

	warning BG8800: Unknown parameter type 'V' for member 'MyClass.DoThing(V)'.

As part of validation, the `Method` instance is also marked as
invalid, by setting the `MethodBase.IsValid` property to `false`, and
the instance is removed from `GenBase.Methods` for `MyClass`.

Unfortunately that's not the end of it, because the `Method` instance
is shared and thus is also in the `GenBase.Methods` collection for
`BaseClass<V>`.  This instance is expected/assumed to be valid, but
was invalidated by `MyClass` validation.

`generator` assumes that after validation, everything within
`GenBase.Methods` is still valid.  This assumption is no longer true
for `BaseClass<V>`.  The result is that if we later attempt to
process `BaseClass<V>`, things blow up when trying to use the parameter
types on `BaseClass<V>.doThing(V)`:

	System.NullReferenceException: Object reference not set to an instance of an object.
	   at MonoDroid.Generation.Parameter.get_JniType() in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\Parameter.cs:line 103
	   at MonoDroid.Generation.ParameterList.get_JniSignature() in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\ParameterList.cs:line 203
	   at MonoDroid.Generation.Method.get_JniSignature() in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\Method.cs:line 210
	   at MonoDroid.Generation.GenBase.ContainsMethod(Method method, Boolean check_ifaces, Boolean check_base_ifaces) in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\GenBase.cs:line 225
	   at MonoDroid.Generation.GenBase.ContainsMethod(Method method, Boolean check_ifaces) in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\GenBase.cs:line 208
	   at MonoDroid.Generation.GenBase.FixupMethodOverrides(CodeGenerationOptions opt) in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\GenBase.cs:line 341


The fix is "simple": do what 4ec5d4e said it was doing and make a
*copy* of the `Method` instance using a new `Clone()` method.  With
this fix, the copied method will be invalidated and removed without
corrupting the original `Method` instance.

*Note*: As seen in the unit test changes, the XPath comments are
updated to point to the public type rather than the original package-
private type.  While this isn't "correct", neither was the previous
XPath specification.  After discussion, we have decided that the
needed change is to not add XPath comments on "created" methods (that
is, API that is created by `generator` and is not part of the Java
public API).  However, the cost of doing this change is higher than we
currently wish to spend, so we will live with this minor issue for now.

TODO: #1083 suggests a "complete fix" which allows
`BaseClass<V>.doThing(V)` to be bound as `MyClass.DoThing(Object)`.

Author: jpobst

tests/generator-Tests/Unit-Tests/CodeGeneratorTestBase.cs

@@ -71,6 +71,9 @@ protected List<GenBase> ParseApiDefinition (string xml)
 			foreach (var gen in gens)
 				options.SymbolTable.AddType (gen);
 
+			foreach (var gen in gens)
+				gen.FixupAccessModifiers (options);
+
 			foreach (var gen in gens)
 				gen.Validate (options, new GenericParameterDefinitionList (), generator.Context);
 

tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs

@@ -827,6 +827,55 @@ public void DontWarnIfNestedTypeNameMatchesNamespace ()
 			Assert.False (sb.ToString ().Contains ("warning BG8403"));
 		}
 
+
+		[Test]
+		public void AvoidNREOnInvalidBaseMethod ()
+		{
+			// We copy methods from the package-private base class to the public class, however
+			// the copied method is not valid because it doesn't understand the generic argument
+			// type. The method is remove from the public class, but we need to ensure the
+			// base class method still exists and IsValid.
+			var xml = @"<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;' />
+			  </package>
+
+			  <package name='android.view' jni-name='android/view'>
+			    <class abstract='false' deprecated='not deprecated' extends='java.lang.Object' final='false' name='View' static='false' visibility='public' jni-signature='Landroid/view/View;' />
+			  </package>
+
+			  <package name='com.google.android.material.behavior' jni-name='com/google/android/material/behavior'>
+			    <class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='ViewOffsetBehavior' static='false' visibility='' jni-signature='Lcom/google/android/material/appbar/ViewOffsetBehavior;'>
+			      <typeParameters>
+				<typeParameter name='V' classBound='android.view.View' jni-classBound='Landroid/view/View;'>
+				  <genericConstraints>
+				    <genericConstraint type='android.view.View' />
+				  </genericConstraints>
+				</typeParameter>
+			      </typeParameters>
+			      <method abstract='false' deprecated='not deprecated' final='false' name='layoutChild' jni-signature='(Landroid/view/View;)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='protected'>
+				<parameter name='child' type='V' jni-type='TV;' not-null='true' />
+			      </method>
+			    </class>
+    
+			    <class abstract='false' deprecated='not deprecated' extends='com.google.android.material.behavior.ViewOffsetBehavior' extends-generic-aware='com.google.android.material.behavior.ViewOffsetBehavior&lt;android.view.View&gt;' jni-extends='Lcom/google/android/material/appbar/ViewOffsetBehavior;' final='false' name='Behavior' static='false' visibility='public' jni-signature='Lcom/google/android/material/appbar/Behavior;'>
+			    </class>
+			  </package>
+			</api>";
+
+			var gens = ParseApiDefinition (xml);
+
+			var public_class = gens.Single (g => g.Name == "Behavior");
+			var base_class = gens.Single (g => g.Name == "ViewOffsetBehavior");
+
+			// Method got removed
+			Assert.AreEqual (0, public_class.Methods.Count);
+
+			// Method still exists and is valid
+			Assert.AreEqual (1, base_class.Methods.Count);
+			Assert.AreEqual (true, base_class.Methods [0].IsValid);
+		}
+
 		static string StripRegisterAttributes (string str)
 		{
 			// It is hard to test if the [Obsolete] is on the setter/etc due to the [Register], so remove all [Register]s

tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.PublicFinalClass.cs

@@ -40,7 +40,7 @@ public unsafe void PublicMethod ()
 			}
 		}
 
-		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassB']/method[@name='packageMethodB' and count(parameter)=0]"
+		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodB' and count(parameter)=0]"
 		[global::Java.Interop.JniMethodSignature ("packageMethodB", "()V")]
 		public unsafe void PackageMethodB ()
 		{
@@ -51,7 +51,7 @@ public unsafe void PackageMethodB ()
 			}
 		}
 
-		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassA']/method[@name='packageMethodA' and count(parameter)=0]"
+		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodA' and count(parameter)=0]"
 		[global::Java.Interop.JniMethodSignature ("packageMethodA", "()V")]
 		public unsafe void PackageMethodA ()
 		{

tests/generator-Tests/expected.xaji/AccessModifiers/Xamarin.Test.PublicFinalClass.cs

@@ -57,7 +57,7 @@ public unsafe void PublicMethod ()
 			}
 		}
 
-		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassB']/method[@name='packageMethodB' and count(parameter)=0]"
+		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodB' and count(parameter)=0]"
 		[Register ("packageMethodB", "()V", "")]
 		public unsafe void PackageMethodB ()
 		{
@@ -68,7 +68,7 @@ public unsafe void PackageMethodB ()
 			}
 		}
 
-		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassA']/method[@name='packageMethodA' and count(parameter)=0]"
+		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodA' and count(parameter)=0]"
 		[Register ("packageMethodA", "()V", "")]
 		public unsafe void PackageMethodA ()
 		{

tests/generator-Tests/expected/AccessModifiers/Xamarin.Test.PublicFinalClass.cs

@@ -49,7 +49,7 @@ public unsafe void PublicMethod ()
 		}
 
 		static IntPtr id_packageMethodB;
-		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassB']/method[@name='packageMethodB' and count(parameter)=0]"
+		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodB' and count(parameter)=0]"
 		[Register ("packageMethodB", "()V", "")]
 		public unsafe void PackageMethodB ()
 		{
@@ -62,7 +62,7 @@ public unsafe void PackageMethodB ()
 		}
 
 		static IntPtr id_packageMethodA;
-		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PackageClassA']/method[@name='packageMethodA' and count(parameter)=0]"
+		// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='PublicFinalClass']/method[@name='packageMethodA' and count(parameter)=0]"
 		[Register ("packageMethodA", "()V", "")]
 		public unsafe void PackageMethodA ()
 		{

tools/generator/Java.Interop.Tools.Generator.ObjectModel/ClassGen.cs

@@ -63,7 +63,7 @@ public override void FixupAccessModifiers (CodeGenerationOptions opt)
 					foreach (var baseMethod in baseClass.Methods) {
 						var method = Methods.FirstOrDefault (m => m.Matches (baseMethod));
 						if (method == null)
-							Methods.Add (baseMethod);
+							Methods.Add (baseMethod.Clone (this));
 					}
 					BaseType = baseClass.BaseType;
 				} else {

tools/generator/Java.Interop.Tools.Generator.ObjectModel/GenericParameterDefinition.cs

@@ -49,6 +49,11 @@ public bool Validate (CodeGenerationOptions opt, GenericParameterDefinitionList
 			validated = is_valid = true;
 			return true;
 		}
+
+		public GenericParameterDefinition Clone ()
+		{
+			return new GenericParameterDefinition (Name, ConstraintExpressions);
+		}
 	}
 
 	public class GenericParameterDefinitionList : List<GenericParameterDefinition>

tools/generator/Java.Interop.Tools.Generator.ObjectModel/Method.cs

@@ -107,6 +107,57 @@ internal string CalculateEventName (Func<string, bool> checkNameDuplicate)
 
 		public bool CanHaveStringOverload => IsReturnCharSequence || Parameters.HasCharSequence;
 
+		public Method Clone (GenBase declaringType)
+		{
+			var clone = new Method (declaringType);
+
+			// MethodBase
+			clone.Annotation = Annotation;
+			clone.ApiAvailableSince = ApiAvailableSince;
+			clone.AssemblyName = AssemblyName;
+			clone.Deprecated = Deprecated;
+			clone.DeprecatedSince = DeprecatedSince;
+			clone.IsAcw = IsAcw;
+			clone.Name = Name;
+			clone.Visibility = Visibility;
+			clone.LineNumber = LineNumber;
+			clone.LinePosition = LinePosition;
+			clone.SourceFile = SourceFile;
+			clone.JavadocInfo = JavadocInfo;
+
+			if (GenericArguments != null)
+				foreach (var ga in GenericArguments)
+					clone.GenericArguments.Add (ga.Clone ());
+
+			foreach (var p in Parameters)
+				clone.Parameters.Add (p.Clone ());
+
+			// Method
+			clone.ArgsType = ArgsType;
+			clone.CustomAttributes = CustomAttributes;
+			clone.EventName = EventName;
+			clone.GenerateAsyncWrapper = GenerateAsyncWrapper;
+			clone.GenerateDispatchingSetter = GenerateDispatchingSetter;
+			clone.IsAbstract = IsAbstract;
+			clone.IsFinal = IsFinal;
+			clone.IsInterfaceDefaultMethod = IsInterfaceDefaultMethod;
+			clone.OverriddenInterfaceMethod = OverriddenInterfaceMethod;
+			clone.IsReturnEnumified = IsReturnEnumified;
+			clone.IsStatic = IsStatic;
+			clone.IsVirtual = IsVirtual;
+			clone.JavaName = JavaName;
+			clone.ManagedOverride = ManagedOverride;
+			clone.ManagedReturn = ManagedReturn;
+			clone.PropertyNameOverride = PropertyNameOverride;
+			clone.Return = Return;
+			clone.ReturnNotNull = ReturnNotNull;
+			clone.RetVal = RetVal.Clone (clone);
+			clone.SourceApiLevel = SourceApiLevel;
+			clone.ExplicitInterface = ExplicitInterface;
+
+			return clone;
+		}
+
 		public string ConnectorName => $"Get{Name}{IDSignature}Handler";
 
 		public string EscapedCallbackName => IdentifierValidator.CreateValidIdentifier ($"cb_{JavaName}{IDSignature}", true);

tools/generator/Java.Interop.Tools.Generator.ObjectModel/Parameter.cs

@@ -32,7 +32,12 @@ internal Parameter (string name, string type, string managedType, bool isEnumifi
 			this.is_enumified = isEnumified;
 			NotNull = notNull;
 		}
-		
+
+		public Parameter Clone ()
+		{
+			return new Parameter (name, type, managed_type, is_enumified, rawtype, NotNull);
+		}
+
 		public string GetCall (CodeGenerationOptions opt)
 		{
 			var rgm = sym as IRequireGenericMarshal;

tools/generator/Java.Interop.Tools.Generator.ObjectModel/ReturnValue.cs

@@ -28,6 +28,11 @@ public ReturnValue (Method owner, string java_type, string managed_type, bool is
 
 		public string CallMethodPrefix => TypeNameUtilities.GetCallPrefix (sym);
 
+		public ReturnValue Clone (Method owner)
+		{
+			return new ReturnValue (owner, java_type, managed_type, is_enumified, NotNull);
+		}
+
 		public string DefaultValue {
 			get { return sym.DefaultValue; }
 		}