Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Mar 20, 2023

Fixes: #810

When a new abstract method is added to an existing class, that is a breaking change that we do not wish to inflict upon our users. Instead, we will change it to a "normal" method that catches a Java NoSuchMethodError if the abstract method is not implemented. If the method does not exist, we throw a AbstractMethodError instead.

This is accomplished by using metadata to set compatVirtualMethod on a method to true.

Example:

API-34 adds:

public abstract Java.Nio.ByteBuffer Slice (int index, int length);

Setting compatVirtualMethod to true:

<attr path="/api/package[@name='java.nio']/class[@name='ByteBuffer']/method[@name='slice']" name="compatVirtualMethod">true</attr>

Changes the method to:

public virtual unsafe Java.Nio.ByteBuffer Slice (int index, int length)
{
  const string __id = "slice.(II)Ljava/nio/ByteBuffer;";
  try {
    JniArgumentValue* __args = stackalloc JniArgumentValue [2];
    __args [0] = new JniArgumentValue (index);
    __args [1] = new JniArgumentValue (length);
    var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
    return global::Java.Lang.Object.GetObject<Java.Nio.ByteBuffer> (__rm.Handle, JniHandleOwnership.TransferLocalRef)!;
  }
  catch (Java.Lang.NoSuchMethodError) {  // This "catch" block is added when 'compatVirtualMethod' is 'true'
    throw new Java.Lang.AbstractMethodError (__id);
  } finally {
  }
}

This also works for interface types.

@jpobst jpobst marked this pull request as ready for review March 21, 2023 13:51
@jonpryor
Copy link
Contributor

Draft commit message:

Context: https://github.com/xamarin/java.interop/issues/810
Context: https://github.com/xamarin/xamarin-android/commit/f96fcf93e157472072576bcc0a8698302899e8cf
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: https://github.com/xamarin/xamarin-android/commit/1f4c8be98f4aa8cd76cc4d17ead8c2709d802edb

.NET and Java have different behaviors around ABI compatibility when
adding new `abstract` methods across 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, given:

	// Lib.dll
	public class LibBase {
	}

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

[.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 to do so, e.g. updating `LibBase` to have a new method
`public abstract void M();` *without* recompiling `App.exe`, 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.

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 -- xamarin/xamarin-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 relies on
[Default Interface Methods][3] to maintain API and ABI compatibility.

Previously, newly introduced `abstract` methods would be *manually*
supported by using `<remove-node/>` to remove the new `abstract`
method, and adding altered `generator` output such that instead of
the default `generator` output of:

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

we would add a `partial` class containing:

	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
automatically translate `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 {
	        }
	    }
	}

[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)

@jonpryor
Copy link
Contributor

Also, once this is merged we should update our docs to mention compatVirtualMethod: https://github.com/xamarin/java.interop/wiki/Metadata-Cheat-Sheet

@jonpryor jonpryor merged commit 53bfb4a into main Mar 22, 2023
@jonpryor jonpryor deleted the compat-virtual-method branch March 22, 2023 23:36
jpobst added a commit that referenced this pull request Jun 27, 2023
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)

(cherry picked from commit 53bfb4a)
jonpryor pushed a commit that referenced this pull request Jun 29, 2023
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)

(cherry picked from commit 53bfb4a)
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metadata support for //method/@compatVirtualMethod

3 participants