[generator] Fix NRE by cloning method when copying from base type to derived type.

[jpobst]

Context: generator crash while binding Google Material 1.6.1

Imagine:

• A public class that inherits from a package-private class
• The package-private class exposes a method we would like to expose on the public class

public class MyClass extends BaseClass<Object> 
{
}

class BaseClass<V>
{
  public void DoThing (V obj) { }
}

We will not bind BaseClass because it is package-private, but we would like for its methods to be available for users. In 4ec5d4e, we supported this by copying members from the base class to the public derived class.

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

In general, this is fine. However consider the case above.

DoThing (V obj) is copied to MyClass, which does not have the generic type argument V. When validation runs on the copied method, this is detected, a warning is emitted, the Method is marked as IsValid=false and is removed from the public class.

public class MyClass extends BaseClass<Object> 
{
  public void DoThing (V obj) { }   // What is V?
}

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

This is also fine and expected.

However this Method object is still referenced by the BaseClass model in its Methods collection. This version should be valid, however it has been marked as IsValid=false and the parameter ISymbol has been set to null.

After validation, class Methods collections should only contain IsValid=true Methods, but this is no longer the case. When later logic attempts to use this Method it hits an NRE trying to access the parameter type. An example NRE is below, but guarding the NRE will simply move it to another location.

System.NullReferenceException
  Message=Object reference not set to an instance of an object.
  StackTrace:
   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 the original change says it was doing and make a "copy" of the Method object using a new Clone () method. With this fix, the copied method will be invalidated and removed without corrupting the original object instance.

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.

[jpobst]

Test XA bump to ensure no regressions:
dotnet/android#7773

[jonpryor]

Draft commit message:

Context: 4ec5d4e9d1c6483f41bb84791268d8434ffb5269
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 4ec5d4e9 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 when 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 4ec5d4e9 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.

[jonpryor]

While writing the draft commit message, my thoughts turn to BaseClass<V>.doThing(V): wouldn't this still have a parameter type of V? How is that "resolved" via declaringType.TypeParameters?

Rephrased, I would expect that the clone instance will have clone.Parameters.Count==1, and clone.Parameters[0] equivalent to:

new Parameter (
        name: "value",
        type: "java.lang.Object",
        managedType: "Java.Lang.Object"
        isEnumified: false
);

but I don't see this happening. Should this be happening?

Or is this instead handled by new lines 128-130 (above), wherein we added this.GenericArguments to clone.GenericArguments. Though I'm not certain about that either, as shouldn't GenericArguments be empty as BaseClass<V>.doThing(V) isn't itself generic…

[jonpryor]

Why is this asserting that the method got removed?

OK, this test is asserting that we no longer NRE, that the BG8800 warning is still emitted (implicitly), that public_class has no methods, and base_class.Methods is still sane.

But… shouldn't the "final" fix be a situation in which:

1. public_class.Methods.Count is also 1, and
2. public_class.Methods[0].IsValid is true, and
3. public_class.Methods[0].Parameters[0].JniType is java/lang/Object?

Upon review, it feels like this PR fixes the NRE, but doesn't resolve the underlying scenario allowing publicly accessible members from package-private types to be bound in the derived type.

Is that possible?

[jpobst]

Correct, this PR only fixes the NRE. It does not try to fix the underlying issue of correctly supporting generics when copying methods from a package-private class. I do not believe that is a topic we want to tackle any time soon.