Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] Skip AbstractMethodErrors on DIM (#3458)
Browse files Browse the repository at this point in the history
Context: f96fcf9
Context: https://github.com/jpobst/dimtest
Context: #1939

As of commit f96fcf9, the linker will "fix abstract methods" in
order to preserve binary compatibility.  For example, if a type
built for e.g. `$(TargetFrameworkVersion)`=v2.3 implements a Java
side interface, and the assembly containing that type is included
into an app with `$(TargetFrameworkVersion)`=v10.0 and the Java-side
interface added members between v2.3 and v10, then those "added"/
"missing" interface members will be *inserted* into the type, with
a implementation which throws `Java.Lang.AbstractMethodError`.

Unfortunately, f96fcf9 assumes that *all* interface members are
"abstract" members which need to be implemented (or fixed up).

This is no longer true with [C#8 Default Interface Members][0], as
interface methods can now contain bodies:

	namespace MyNamespace {
	  public IMyInterface {
	    void MyAbstractMethod ();
	    public void MyDefaultMethod ()
	    {
	    }
	  }
	}

Types which implement `IMyInterface` don't need to provide an
implementation for `IMyInterface.MyDefaultMethod()`, which is *fine*:

	// What the developer wrote
	class MyExample : IMyInterface {
	  public void MyAbstractMethod() {}

	  // Does not provide MyDefaultMethod()
	}

Unfortunately, `FixAbstractMethodsStep` didn't know about such
methods (they didn't exist in 2017), so it would insert the "missing"
abstract methods:

	// What the linker writes:
	class MyExample : IMyInterface {
	  public void MyAbstractMethod () {}

	  public void MyDefaultMethod () => throw new Java.Lang.AbstractMethodError ();
	}

Additionally, `FixAbstractMethodsStep` was not constrained to operate
on only Java-related types and interfaces.  It fixes *everything*.

This breaks a "pure C#" scenario in which Java isn't involved at all:

	IMyInterface e = new MyExample ();
	e.MyDefaultMethod ();

The expectation is that this will invoke the
`IMyInterface.MyDefaultMethod()` method.

Instead, it throws an `AbstractMethodError`!

Update `FixAbstractMethodsStep` so that it only processes *`abstract`*
methods, not all interface methods.  This will prevent it from
emitting a `MyExample.MyDefaultMethod()` override and prevent the
`AbstractMethodError` from being thrown.

[0]: https://github.com/dotnet/csharplang/blob/f7952cdddf85316a4beec493a0ecc14fcb3241c8/proposals/csharp-8.0/default-interface-methods.md
  • Loading branch information
jpobst authored and jonpryor committed Aug 8, 2019
1 parent f9e2ede commit 0c4c033
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;

using Mono.Cecil;

Expand Down Expand Up @@ -163,7 +164,7 @@ bool FixAbstractMethods (TypeDefinition type)
if (ifaceDef.HasGenericParameters)
continue;

foreach (var iMethod in ifaceDef.Methods) {
foreach (var iMethod in ifaceDef.Methods.Where (m => m.IsAbstract)) {
bool exists = false;

foreach (var tMethod in typeMethods) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
using System;
using System.Linq;
using Mono.Cecil;
using Mono.Linker;
using MonoDroid.Tuner;
using NUnit.Framework;

namespace Xamarin.Android.Build.Tests
{
public class LinkerTests : BaseTest
{
[Test]
public void FixAbstractMethodsStep_SkipDimMembers ()
{
var step = new FixAbstractMethodsStep ();
var pipeline = new Pipeline ();
var context = new LinkContext (pipeline);

var android = CreateFauxMonoAndroidAssembly ();
var jlo = android.MainModule.GetType ("Java.Lang.Object");

context.Resolver.CacheAssembly (android);

var assm = AssemblyDefinition.CreateAssembly (new AssemblyNameDefinition ("DimTest", new Version ()), "DimTest", ModuleKind.Dll);
var void_type = assm.MainModule.ImportReference (typeof (void));

// Create interface
var iface = new TypeDefinition ("MyNamespace", "IMyInterface", TypeAttributes.Interface);

var abstract_method = new MethodDefinition ("MyAbstractMethod", MethodAttributes.Abstract, void_type);
var default_method = new MethodDefinition ("MyDefaultMethod", MethodAttributes.Public, void_type);

iface.Methods.Add (abstract_method);
iface.Methods.Add (default_method);

assm.MainModule.Types.Add (iface);

// Create implementing class
var impl = new TypeDefinition ("MyNamespace", "MyClass", TypeAttributes.Public, jlo);
impl.Interfaces.Add (new InterfaceImplementation (iface));

assm.MainModule.Types.Add (impl);

context.Resolver.CacheAssembly (assm);

step.Process (context);

// We should have generated an override for MyAbstractMethod
Assert.IsTrue (impl.Methods.Any (m => m.Name == "MyAbstractMethod"));

// We should not have generated an override for MyDefaultMethod
Assert.IsFalse (impl.Methods.Any (m => m.Name == "MyDefaultMethod"));
}

static AssemblyDefinition CreateFauxMonoAndroidAssembly ()
{
var assm = AssemblyDefinition.CreateAssembly (new AssemblyNameDefinition ("Mono.Android", new Version ()), "DimTest", ModuleKind.Dll);
var void_type = assm.MainModule.ImportReference (typeof (void));

// Create fake JLO type
var jlo = new TypeDefinition ("Java.Lang", "Object", TypeAttributes.Public);
assm.MainModule.Types.Add (jlo);

// Create fake Java.Lang.AbstractMethodError type
var ame = new TypeDefinition ("Java.Lang", "AbstractMethodError", TypeAttributes.Public);
ame.Methods.Add (new MethodDefinition (".ctor", MethodAttributes.Public, void_type));
assm.MainModule.Types.Add (ame);

return assm;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
<Compile Include="Tasks\CopyIfChangedTests.cs" />
<Compile Include="Tasks\GetDependenciesTests.cs" />
<Compile Include="Tasks\ResolveMonoAndroidSdksTests.cs" />
<Compile Include="Tasks\LinkerTests.cs" />
<Compile Include="Tasks\ResolveSdksTaskTests.cs" />
<Compile Include="Tasks\AndroidResourceTests.cs" />
<Compile Include="Tasks\ManagedResourceParserTests.cs" />
Expand Down

0 comments on commit 0c4c033

Please sign in to comment.