[generator] Add support for 'compatVirtualMethod'. (#1088)

Fixes: #810

Context: dotnet/android@f96fcf9
Context: https://learn.microsoft.com/en-us/dotnet/core/compatibility/categories#binary-compatibility
Context: https://github.com/xamarin/xamarin-android/blob/f3592b3c42674f2161c14d1f4246083a85fe17ab/Documentation/workflow/mono-android-api-compatibility.md
Context: dotnet/android@1f4c8be

.NET and Java have different behaviors around ABI compatibility when
adding new `abstract` methods to types which cross module boundaries.
(Within a module boundary, adding a new `abstract` method will be an
*API* break and the compiler will not compile the code.)

For example, consider "version 1":

	// Lib.dll; version 1
	public abstract partial class LibBase {
	}

	// App.exe
	class MyType : LibBase {
	}
	// …
	Console.WriteLine (new MyType().ToString ());

Then for "version 2" we update `LibBase` in `Lib.dll` to:

	// Lib.dll; version 2
	public abstract partial class LibBase {
	    public abstract void M();
	}

*We **do not** rebuild `App.exe`*.

What Happens™?

[.NET says][0] Don't Do That™:

> * ❌ DISALLOWED: Adding an abstract member to a public type that has
> accessible (public or protected) constructors and that is not sealed

If you attempt this anyway, then Bad Things™ happen; a
`TypeLoadException` is thrown when attempting to instantiate `MyType`:

	Unhandled exception. System.TypeLoadException: Method 'M' in type 'MyType' from assembly
	  'App, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.

This happens even if the new `abstract` method is not invoked.

(MonoVM is -- was? -- worse; it would flat out *abort* the process!)

Java, meanwhile [*says* that it breaks compatibility][1]:

> Changing a method that is not declared `abstract` to be declared
> `abstract` will break compatibility with pre-existing binaries that
> previously invoked the method, causing an `AbstractMethodError`.

but the *form* of the runtime behavior differs significantly from
.NET: In Java, *so long as* the new `abstract` is not invoked, runtime
behavior doesn't change.  If the new `abstract` method *is* invoked,
an `AbstractMethodError` is thrown.

In Java, adding `abstract` methods is thus "safe" (-ish): existing
subclasses or interface implementations don't need to be recompiled,
and if someone *does* invoke the method, `AbstractMethodError` is
thrown, which can be caught and handled, if necessary.

Consequently, when we look at the history of Android, it is not
unusual for new `abstract` methods to be added over time!
@jonpryor's "favorite" example is [`android.database.Cursor`][2],
which added new `abstract` methods in:

  * API-11 (`getType()`)
  * API-19 (`getNotificationUri()`)
  * API-23 (`setExtras()`)
  * API-29 (`getNotificationUris()`, `setNotificationUris()`)

In "Classic" Xamarin.Android, we "solved" this problem via:

 1. "Not caring"; each new Android API would correspond to a new
    `$(TargetFrameworkVersion)`, which would be a new/different version
    of `Mono.Android.dll`.  Added `abstract` methods would be added,
    which could result in *API* breakage if/when a project changed
    their `$(TargetFrameworkVersion)`.

 2. *ABI* compatibility was preserved by way of a post-build linker
    step -- dotnet/android@f96fcf93 -- which would look for
    "missing" `abstract` methods and *add* them with Cecil.
    The added method would simply throw `new AbstractMethodError()`.

.NET Android still has the post-build linker step, but did not adopt
the "Classic" Xamarin.Android approach of having a separate binding
assembly per API level.  Instead, .NET Android:

  * For new `abstract` methods in classes, the `abstract` method
    would be turned into a `virtual` method which throws
    `AbstractMethodError`.

  * For new non-default methods in interfaces, 
    [Default Interface Methods][3] would be used to to maintain API
    and ABI compatibility.  The default interface method would throw
    `AbstractMethodError`.

However, this was done manually; see dotnet/android@1f4c8be9.
`<remove-node/>` was used to *remove* the new `abstract` method, and
a `partial` class declaration was added which contained `generator`
output for the "original" `abstract` method declaration, manually
patched up to instead introduce a `virtual` method.  The default
`generator` output of:

	public partial class CellInfo {
	    public abstract Android.Telephony.CellIdentity CellIdentity {
	        [Register ("getCellIdentity", "()Landroid/telephony/CellIdentity;", "GetGetCellIdentityHandler", ApiSince = 30)]	
	        get;
	    }
	    // …plus marshal method invocation infrastructure…
	}

would be manually altered to become:

	public partial class CellInfo {
	    public unsafe virtual Android.Telephony.CellIdentity CellIdentity {
	        [Register ("getCellIdentity", "()Landroid/telephony/CellIdentity;", "GetGetCellIdentityHandler", ApiSince = 30)]	
	        get {
	            const string __id = "getCellIdentity.()Landroid/telephony/CellIdentity;";
	            try {
	                var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, null);
	                return Object.GetObject<CellIdentity> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
		    } catch (Java.Lang.NoSuchMethodError) {
	                throw new Java.Lang.AbstractMethodError (__id);
		    }
	        }
	    }
	    // Plus all related "marshal method glue code"
	}

This is cumbersome and error prone.

Improve on this process by adding support for new `compatVirtualMethod`
metadata, a boolean value which can be applied to `//method`:

	<attr
	    api-since="34"
	    path="/api/package[@name='java.nio']/class[@name='ByteBuffer']/method[@name='slice' and count(parameter)=2]"
	>true</attr>

which updates the [`ByteBuffer.slice(int, int`)][4] method -- a newly
introduced `abstract` method in API-34 -- so that it will emit a
`virtual` method instead of an `abstract` method, and the `virtual`
method body translates `NoSuchMethodError` into `AbstractMethodError`:

	partial class ByteBuffer {
	    [Register (…)]
	    public virtual unsafe Slice (int index, int length)
	    {
	        const string __id = "slice.(II)Ljava/nio/ByteBuffer;";
	        try {
	            return …
	        }
	        catch (NoSuchMethodError) {
	            throw new AbstractMethodError (__id);
	        }
	        finally {
	        }
	    }
	}

This allows us to "more reasonably" maintain ABI & API compatibility,
without all that pesky manual fixup.

[0]: https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules
[1]: https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.16
[2]: https://developer.android.com/reference/android/database/Cursor
[3]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/default-interface-methods
[4]: https://developer.android.com/reference/java/nio/ByteBuffer#slice(int,%20int)

Author: jpobst

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

@@ -352,6 +352,30 @@ public void SkipInvokerMethodsMetadata ()
 			Assert.False (writer.ToString ().Contains ("public abstract Com.Xamarin.Android.MyBaseClass DoStuff ();"), $"was: `{writer}`");
 		}
 
+		[Test]
+		public void CompatVirtualMethod_Class ()
+		{
+			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='com.xamarin.android' jni-name='com/xamarin/android'>
+			    <class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
+			      <method abstract='true' deprecated='not deprecated' final='true' name='DoStuff' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public' compatVirtualMethod='true'></method>
+			    </class>
+			  </package>
+			</api>";
+
+			var gens = ParseApiDefinition (xml);
+			var klass = gens.Single (g => g.Name == "MyClass");
+
+			generator.Context.ContextTypes.Push (klass);
+			generator.WriteType (klass, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
+			generator.Context.ContextTypes.Pop ();
+
+			Assert.True (writer.ToString ().NormalizeLineEndings ().Contains ("catch (Java.Lang.NoSuchMethodError) { throw new Java.Lang.AbstractMethodError (__id); }".NormalizeLineEndings ()), $"was: `{writer}`");
+		}
+
 		[Test]
 		public void WriteDuplicateInterfaceEventArgs ()
 		{

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

@@ -490,5 +490,29 @@ public void WriteInterfaceFieldAsDimProperty ()
 
 			AssertTargetedExpected (nameof (WriteInterfaceFieldAsDimProperty), writer.ToString ());
 		}
+
+		[Test]
+		public void CompatVirtualMethod_Interface ()
+		{
+			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='com.xamarin.android' jni-name='com/xamarin/android'>
+			    <interface abstract='true' deprecated='not deprecated' final='false' name='Cursor' static='false' visibility='public' jni-signature='Landroid/database/Cursor;'>
+			      <method abstract='true' deprecated='not deprecated' final='false' name='getNotificationUri' jni-signature='()Ljava/lang/Object;' bridge='false' native='false' return='java.lang.Object' jni-return='Ljava/lang/Object;' static='false' synchronized='false' synthetic='false' visibility='public' compatVirtualMethod='true' />
+			    </interface>
+			  </package>
+			</api>";
+
+			var gens = ParseApiDefinition (xml);
+			var klass = gens.Single (g => g.Name == "ICursor");
+
+			generator.Context.ContextTypes.Push (klass);
+			generator.WriteType (klass, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
+			generator.Context.ContextTypes.Pop ();
+
+			Assert.True (writer.ToString ().NormalizeLineEndings ().Contains ("catch (Java.Lang.NoSuchMethodError) { throw new Java.Lang.AbstractMethodError (__id); }".NormalizeLineEndings ()), $"was: `{writer}`");
+		}
 	}
 }

tools/generator/Java.Interop.Tools.Generator.Importers/XmlApiImporter.cs

@@ -371,6 +371,7 @@ public static Method CreateMethod (GenBase declaringType, XElement elem, CodeGen
 				GenericArguments = elem.GenericArguments (),
 				IsAbstract = elem.XGetAttribute ("abstract") == "true",
 				IsAcw = true,
+				IsCompatVirtualMethod = elem.XGetAttribute ("compatVirtualMethod") == "true",
 				IsFinal = elem.XGetAttribute ("final") == "true",
 				IsReturnEnumified = elem.Attribute ("enumReturn") != null,
 				IsStatic = elem.XGetAttribute ("static") == "true",
@@ -384,6 +385,10 @@ public static Method CreateMethod (GenBase declaringType, XElement elem, CodeGen
 				Visibility = elem.Visibility ()
 			};
 
+			// CompatVirtualMethods aren't abstract
+			if (method.IsCompatVirtualMethod)
+				method.IsAbstract = false;
+
 			method.IsVirtual = !method.IsStatic && elem.XGetAttribute ("final") == "false";
 
 			if (elem.Attribute ("managedName") != null)

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

@@ -19,6 +19,7 @@ public Method (GenBase declaringType) : base (declaringType)
 		public bool GenerateAsyncWrapper { get; set; }
 		public bool GenerateDispatchingSetter { get; set; }
 		public bool IsAbstract { get; set; }
+		public bool IsCompatVirtualMethod { get; set; }
 		public bool IsFinal { get; set; }
 		public bool IsInterfaceDefaultMethod { get; set; }
 		public Method OverriddenInterfaceMethod { get; set; }

tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs

@@ -266,6 +266,12 @@ public static void AddMethodBody (List<string> body, Method method, CodeGenerati
 				body.Add ($"\treturn {method.RetVal.ReturnCast}{method.RetVal.FromNative (opt, r, true) + opt.GetNullForgiveness (method.RetVal)};");
 			}
 
+			if (method.IsCompatVirtualMethod) {
+				body.Add ("}");
+				body.Add ("catch (Java.Lang.NoSuchMethodError) {");
+				body.Add ("	throw new Java.Lang.AbstractMethodError (__id);");
+			}
+
 			body.Add ("} finally {");
 
 			foreach (string cleanup in method.Parameters.GetCallCleanup (opt))