Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[generator] Allow 'managedOverride' metadata to support 'abstract' methods. #1086

Merged
merged 4 commits into from
Mar 7, 2023

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Feb 28, 2023

Context: dotnet/android#7796

Imagine the following construct, which is new in android.jar API-34:

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

public abstract class Buffer  {
  public abstract Buffer duplicate ();
}

Generating the subclass's abstract method causes:

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

C# supports this construct if we make the subclass's abstract method with override:

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

Theoretically we can tell generator how to fix this thanks to our addition of the managedOverride metadata property:

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

However, managedOverride was not written to support abstract methods. This commit fixes that issue so that using managedOverride to create an override abstract method is possible.

Method Invokers

This gets us halfway there, but there is another issue: method invokers for both the class and base class methods will be generated:

[global::Android.Runtime.Register ("java/nio/CharBuffer", DoNotGenerateAcw=true)]
internal partial class CharBufferInvoker : CharBuffer {
	[Register ("duplicate", "()Ljava/nio/CharBuffer;", "")]
	public override sealed unsafe CharBuffer Duplicate () { ... }

	[Register ("duplicate", "()Ljava/nio/Buffer;", "")]
	public override sealed unsafe Buffer Duplicate () { ... }
}

This creates 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 add a skipMethodInvokers metadata to 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>

This tells generator to skip generating the Buffer Buffer.duplicate () invoker when generating the CharBuffer type. Multiple invokers to skip can be specified as a comma or space separated list.

@jpobst jpobst force-pushed the abstract-override branch from ff8a56b to 1dd91b5 Compare March 2, 2023 20:48
@@ -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 [] { ',' }))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think space could make fore a "more readable" representation.

@jpobst jpobst marked this pull request as ready for review March 6, 2023 15:39
@@ -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 [] { ',', ' ' }))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also add \n and \r in there as well.

@jonpryor jonpryor merged commit 73ebad2 into main Mar 7, 2023
@jonpryor jonpryor deleted the abstract-override branch March 7, 2023 01:19
jonpryor pushed a commit to dotnet/android that referenced this pull request Mar 8, 2023
Changes: dotnet/java-interop@77800dd...73ebad2

  * dotnet/java-interop@73ebad24: [generator] Support Buffer.duplicate() binding in API-34 (dotnet/java-interop#1086)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
jpobst added a commit that referenced this pull request Jun 27, 2023
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

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

(cherry picked from commit 73ebad2)
@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.

2 participants