Skip to content

Commit

Permalink
[generator] Support Buffer.duplicate() binding in API-34 (#1086)
Browse files Browse the repository at this point in the history
Context: dotnet/android#7796
Context: 22d5687
Context: 5a0e37e

Imagine the following covariant-return-type -using construct, which
is new in API-34 `android.jar`:

	// Java
	public abstract class Buffer  {
	    public abstract Buffer duplicate ();
	}
	public abstract class CharBuffer extends Buffer {
	    public abstract CharBuffer duplicate ();
	}

By default this is bound as:

	// C#
	[Register(…)]
	public abstract partial class Buffer {
	    [Register(…)]
	    public abstract Buffer Duplicate ();
	}
	[Register(…)]
	public abstract partial class CharBuffer : Buffer {
	    [Register(…)]
	    public abstract CharBuffer Duplicate ();
	}

Alas, this results in [error CS0533][0]:

	error CS0533: 'CharBuffer.Duplicate()' hides inherited abstract member 'Buffer.Duplicate()'

C# supports this semantic if we use [`abstract override`][1], similar
to [reabstraction of Default Interface Methods][2] in 22d5687:

	public abstract class CharBuffer : Buffer {
	    public override abstract CharBuffer Duplicate ();
	}

Theoretically, we can tell `generator` how to fix this via the
`//attr[@name='managedOverride']` metadata property (5a0e37e):

	<attr path="/api/package[@name='java.nio']/class[@name='CharBuffer']/method[@name='duplicate']"
	    name="managedOverride">override</attr>

However, `//attr[@name='managedOverride']` was not written to support
updating`abstract` methods.

Fix that oversight so that using `//attr[@name='managedOverride']`
can be used to emit `override abstract`:

	[Register(…)]
	public abstract partial class CharBuffer : Buffer {
	    [Register(…)]
	    public override abstract CharBuffer Duplicate ();
	}

~~ Method Invokers ~~

This gets us halfway there, but there is another issue: method invokers
for both the class and base class methods are *also* emitted into the
`*Invoker` subclass:

	partial class BufferInvoker : CharBuffer {
	    public override sealed unsafe Buffer Duplicate() {…}
	    // so far so good…
	}
	partial class CharBufferInvoker : CharBuffer {
	    [Register ("duplicate", "()Ljava/nio/Buffer;", "")]
	    public override sealed unsafe Buffer     Duplicate() {…}

	    [Register ("duplicate", "()Ljava/nio/CharBuffer;", "")]
	    public override sealed unsafe CharBuffer Duplicate() {…}
	    // uh oh
	}

This results in the following error:

	error CS0111: Type 'CharBufferInvoker' already defines a member called 'Duplicate' with the same parameter types
	error CS0508: 'CharBufferInvoker.Duplicate()': return type must be 'CharBuffer' to match overridden member 'CharBuffer.Duplicate()'

This perhaps(?) could be something `generator` could detect and
prevent, but the feasibility and cost is unknown and expected to be
high.  Since this is a very rare case, we will fix it with metadata,
however we have never had metadata to apply to invokers before.

We have decided to support `//attr[@name='skipInvokerMethods']`
metadata on the `class` that the invoker class would be created for:

	<attr path="/api/package[@name='java.nio']/class[@name='CharBuffer']"
	    name="skipInvokerMethods">java/nio/Buffer.duplicate()Ljava/nio/Buffer;</attr>

The value is a comma, space, and newline-separated list of
"JNI signature-like" values consisting of:

 1. The simplified JNI name of the declaring class that contains the
    the invoker method to skip, e.g. `java/nio/Buffer`
 2. `.`
 3. The Java name of the invoker method to skip, e.g. `duplicate`.
 4. The JNI method signature of the method to skip, e.g.
   `()Ljava/nio/Buffer;`

The above `java/nio/Buffer.duplicate()Ljava/nio/Buffer;` value tells
`generator` to skip generating the `Buffer Buffer.duplicate ()` invoker
method when generating the `CharBufferInvoker` type.  Multiple invokers
to skip can be specified as a comma or space separated list.

[0]: https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0533
[1]: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/classes#1467-abstract-methods
[2]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/default-interface-methods#reabstraction
  • Loading branch information
jpobst authored Mar 7, 2023
1 parent 77800dd commit 73ebad2
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 7 deletions.
3 changes: 2 additions & 1 deletion src/Xamarin.SourceWriter/Models/MethodWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ public virtual void WriteSignature (CodeWriter writer)

if (IsOverride)
writer.Write ("override ");
else if (IsVirtual)

if (IsVirtual)
writer.Write ("virtual ");
else if (IsAbstract)
writer.Write ("abstract ");
Expand Down
54 changes: 54 additions & 0 deletions tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,30 @@ public void ManagedOverrideMethod_Override ()
Assert.True (writer.ToString ().Contains ("public override unsafe int DoStuff ()"), $"was: `{writer.ToString ()}`");
}

[Test]
public void ManagedOverrideAbstractMethod_Override ()
{
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' managedOverride='override'></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 ().Contains ("public override abstract int DoStuff ();"), $"was: `{writer}`");
}

[Test]
public void ManagedOverrideMethod_None ()
{
Expand Down Expand Up @@ -298,6 +322,36 @@ public void ManagedOverrideInterfaceProperty_Reabstract ()
Assert.True (writer.ToString ().Contains ("abstract int Name {"), $"was: `{writer}`");
}

[Test]
public void SkipInvokerMethodsMetadata ()
{
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='true' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyBaseClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyBaseClass;'>
<method abstract='true' deprecated='not deprecated' final='false' name='doStuff' jni-signature='()Lcom/xamarin/android/MyBaseClass;' bridge='false' native='false' return='com.xamarin.android.MyBaseClass' jni-return='Lcom/xamarin/android/MyBaseClass;' static='false' synchronized='false' synthetic='false' visibility='public'></method>
</class>
<class abstract='true' deprecated='not deprecated' extends='com.xamarin.android.MyBaseClass' extends-generic-aware='com.xamarin.android.MyBaseClass' jni-extends='Lcom/xamarin/android/MyBaseClass;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;' skipInvokerMethods='com/xamarin/android/MyBaseClass.doStuff()Lcom/xamarin/android/MyBaseClass;'>
<method abstract='true' deprecated='not deprecated' final='false' name='doStuff' jni-signature='()Lcom/xamarin/android/MyClass;' bridge='false' native='false' return='com.xamarin.android.MyClass' jni-return='Lcom/xamarin/android/MyClass;' static='false' synchronized='false' synthetic='false' visibility='public' managedOverride='override' return-not-null='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 ();

// `override abstract` causes both invoker methods to get generated. We use metadata
// to suppress the base class's method to prevent a conflict.
Assert.True (writer.ToString ().Contains ("public override abstract Com.Xamarin.Android.MyClass DoStuff ();"), $"was: `{writer}`");
Assert.False (writer.ToString ().Contains ("public abstract Com.Xamarin.Android.MyBaseClass DoStuff ();"), $"was: `{writer}`");
}

[Test]
public void WriteDuplicateInterfaceEventArgs ()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ public static ClassGen CreateClass (XElement pkg, XElement elem, CodeGenerationO
!options.SupportNestedInterfaceTypes
};

if (elem.Attribute ("skipInvokerMethods")?.Value is string skip)
foreach (var m in skip.Split (new char [] { ',', ' ', '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries))
klass.SkippedInvokerMethods.Add (m);

FillApiSince (klass, pkg, elem);
SetLineInfo (klass, elem, options);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace MonoDroid.Generation
public class ClassGen : GenBase
{
bool fill_explicit_implementation_started;
HashSet<string> skipped_invoker_methods;

public List<Ctor> Ctors { get; private set; } = new List<Ctor> ();

Expand Down Expand Up @@ -355,6 +356,8 @@ public override void ResetValidation ()
base.ResetValidation ();
}

public HashSet<string> SkippedInvokerMethods => skipped_invoker_methods ??= new HashSet<string> ();

public override string ToNative (CodeGenerationOptions opt, string varname, Dictionary<string, string> mappings = null)
{
if (opt.CodeGenerationTarget == CodeGenerationTarget.JavaInterop1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public BoundMethodAbstractDeclaration (GenBase gen, Method method, CodeGeneratio

NewFirst = true;

if (method.ManagedOverride?.ToLowerInvariant () == "override")
IsOverride = true;

if (opt.CodeGenerationTarget != CodeGenerationTarget.JavaInterop1) {
method_callback = new MethodCallback (impl, method, opt, null, method.IsReturnCharSequence);
}
Expand Down
15 changes: 9 additions & 6 deletions tools/generator/SourceWriters/ClassInvokerClass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,21 +64,21 @@ public ClassInvokerClass (ClassGen klass, CodeGenerationOptions opt)
Properties.Add (new ThresholdTypeGetter ());
}

AddMemberInvokers (klass, opt, new HashSet<string> ());
AddMemberInvokers (klass, opt, new HashSet<string> (), klass.SkippedInvokerMethods);
}

void AddMemberInvokers (ClassGen klass, CodeGenerationOptions opt, HashSet<string> members)
void AddMemberInvokers (ClassGen klass, CodeGenerationOptions opt, HashSet<string> members, HashSet<string> skipInvokers)
{
AddPropertyInvokers (klass, klass.Properties, members, opt);
AddMethodInvokers (klass, klass.Methods, members, null, opt);
AddMethodInvokers (klass, klass.Methods, members, skipInvokers, null, opt);

foreach (var iface in klass.GetAllDerivedInterfaces ()) {
AddPropertyInvokers (klass, iface.Properties.Where (p => !klass.ContainsProperty (p.Name, false, false)), members, opt);
AddMethodInvokers (klass, iface.Methods.Where (m => (opt.SupportDefaultInterfaceMethods || !m.IsInterfaceDefaultMethod) && !klass.ContainsMethod (m, false, false) && !klass.IsCovariantMethod (m) && !klass.ExplicitlyImplementedInterfaceMethods.Contains (m.GetSignature ())), members, iface, opt);
AddMethodInvokers (klass, iface.Methods.Where (m => (opt.SupportDefaultInterfaceMethods || !m.IsInterfaceDefaultMethod) && !klass.ContainsMethod (m, false, false) && !klass.IsCovariantMethod (m) && !klass.ExplicitlyImplementedInterfaceMethods.Contains (m.GetSignature ())), members, skipInvokers, iface, opt);
}

if (klass.BaseGen != null && klass.BaseGen.FullName != "Java.Lang.Object")
AddMemberInvokers (klass.BaseGen, opt, members);
AddMemberInvokers (klass.BaseGen, opt, members, skipInvokers);
}

void AddPropertyInvokers (ClassGen klass, IEnumerable<Property> properties, HashSet<string> members, CodeGenerationOptions opt)
Expand All @@ -100,9 +100,12 @@ void AddPropertyInvokers (ClassGen klass, IEnumerable<Property> properties, Hash
}
}

void AddMethodInvokers (ClassGen klass, IEnumerable<Method> methods, HashSet<string> members, InterfaceGen gen, CodeGenerationOptions opt)
void AddMethodInvokers (ClassGen klass, IEnumerable<Method> methods, HashSet<string> members, HashSet<string> skipInvokers, InterfaceGen gen, CodeGenerationOptions opt)
{
foreach (var m in methods) {
if (skipInvokers.Contains ($"{m.DeclaringType.RawJniName}.{m.JavaName}{m.JniSignature}"))
continue;

var sig = m.GetSignature ();

if (members.Contains (sig))
Expand Down

0 comments on commit 73ebad2

Please sign in to comment.