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

[Java.Base] Begin binding JDK-11 java.base module #909

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Nov 6, 2021

[Java.Base] Begin binding JDK-11 java.base module

Context: #858

What do I want? To be able to use our wonderful Java binding
infrastructure against Desktop Java, not just Android.

At the same time, I don't want "Android-isms" "leaking" into such a
binding. Just Java.Interop, no xamarin-android.

"Take over" the generator --codegen-target=JavaInterop1 format
so that it isn't useful for Xamarin.Android, and is instead usable
for non-Android usage.

This is a work-in-progress, and far from complete. For prototype
purposes, this only binds:

  • java.lang.Object
  • java.lang.Throwable
  • java.lang.Class

The Java.Base binding is only for .NET 6 and above. I'm not
interested in .NET Standard support at this point in time.

Update samples/Hello so that it (1) works, and (2) instantiates the
Java.Lang.Object binding:

dotnet run --project samples/Hello

~~ Binding changes vs. Xamarin.Android ~~

Java arrays are bound as appropriate IList<T>, using the
Java.Interop.Java*Array types as an intermediary. This should help
reduce marshaling logic & overhead, as if the "source" array is a
Java*Array, it doesn't need to be "deep copied". The exception is
C# params arrays, which continue to be bound as arrays, and are
marshaled via an appropriate Java*Array type.

java.io.InputStream isn't bound as System.IO.Stream, etc.

"Java.Interop-style" constructors are used (25de1f3), e.g.

// This
DeclaringType (ref JniObjectReference reference, JniObjectReferenceOptions options);

// Not Xamarin.Android-style
DeclaringType (IntPtr handle, JniHandleOwnership transfer);

"Java.Interop-style" wrapper construction is used, e.g.

// This
var wrapper = JniEnvironment.Runtime.ValueManager.GetValue<DeclaringType>(ref h, JniObjectReferenceOptions.CopyAndDispose);

// Not this
var wrapper = Java.Lang.Object.GetObject<DeclaringType>(handle);

Marshal methods are currently skipped. Java-to-managed invocations
are not currently supported.

~~ TODO: Marshal Methods? ~~

Xamarin.Android uses Java Callable Wrappers + Runtime.register()
to specify which methods to register, via lots of reflection, etc.

For Desktop, JCW's shouldn't have all the methods to register.
Instead, use the jnimarshalmethod-gen-originated strategy of
[JniAddNativeMethodRegistrationAttribute] within the binding, and
then have it use MarshalMethodBuilder to generate the marshal
methods. Need to update MarshalMethodBuilder to look for overrides
in addition to methods with [JavaCallable], which in turn will
require an equivalent to Android.Runtime.RegisterAttribute(…).

Perhaps JniMethodSignatureAttribute(string name, string sig)?

In the meantime, Java.Base will skip all marshal-method logic
plus runtime method generation. Leave that for later.

~~ TODO: Other API Changes? ~~

We should "unify" java.lang.Object and System.Object. Consider
java.lang.Class:

/* partial */ class Class<T> {
    public boolean isInstance(java.lang.Object);
    public java.lang.Object[] getSigners();
}

If we unify java.lang.Object and System.Object, we'd have a
binding of:

partial class Class {
    public bool IsInstance (object value);
    public object[] GetSigners();
}

~~ Open Questions ~~

What's up with java.lang.Class.getAnnotationsByType()?

During an iteration of this PR, I got:

public unsafe Java.Interop.JavaObjectArray<Java.Lang.Object>? GetAnnotationsByType (Java.Lang.Class? annotationClass)
{
    const string __id = "getAnnotationsByType.(Ljava/lang/Class;)[Ljava/lang/annotation/Annotation;";

From __id we see that the Java return type is Annotation[], yet
we bind it as an Object array? Why? How do we fix that to instead
bind it as JavaObjectArray<Java.Lang.Annotations.Annotation>?

Currently, it's differently worse; I don't know why, but __id
is now:

const string __id = "getAnnotationsByType.(Ljava/lang/Class;)[Ljava/lang/Object;";

i.e. the return type is an Object array instead of an Annotation
array, which is wrong, as per javap:

% javap -s java.lang.Class
…
  public <A extends java.lang.annotation.Annotation> A getAnnotation(java.lang.Class<A>);
    descriptor: (Ljava/lang/Class;)Ljava/lang/annotation/Annotation;

@jonpryor jonpryor requested a review from jpobst November 6, 2021 00:22
@jonpryor jonpryor force-pushed the jonp-add-java.base-binding branch 5 times, most recently from 60d2579 to cb8ffbd Compare November 11, 2021 02:39
@jonpryor jonpryor force-pushed the jonp-add-java.base-binding branch 3 times, most recently from 878213f to d6fc7a0 Compare November 16, 2021 21:13
@jonpryor jonpryor force-pushed the jonp-add-java.base-binding branch 4 times, most recently from a480ab1 to 47f6542 Compare November 19, 2021 00:20
@jonpryor
Copy link
Member Author

How do we fix that to instead
bind it as JavaObjectArray<Java.Lang.Annotations.Annotation>?

That's: #669

Still no idea why the JNI signature started using Ljava/lang/Object; instead of Ljava/lang/annotations/Annotation; though.

@jpobst
Copy link
Contributor

jpobst commented Dec 2, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

Let's fix the build warnings that this creates, and then it looks good.

Also, this seems to run fewer tests than main, which is concerning.

  • main - 4,550 tests
  • PR - 4,390 tests

@jonpryor jonpryor force-pushed the jonp-add-java.base-binding branch from 47f6542 to 27ed440 Compare December 5, 2021 22:00
Context: dotnet#858

What do *I* want?  To be able to use our wonderful Java binding
infrastructure against *Desktop Java*, not just Android.

At the same time, I don't want "Android-isms" "leaking" into such a
binding.  *Just* Java.Interop, no xamarin-android.

"Take over" the `generator --codegen-target=JavaInterop1` format
so that it *isn't* useful for Xamarin.Android, and is instead usable
for non-Android usage.

This is a work-in-progress, and *far* from complete.  For prototype
purposes, this *only* binds:

  * `java.lang.Object`
  * `java.lang.Throwable`
  * `java.lang.Class`

The `Java.Base` binding is only for .NET 6 and above.  I'm not
interested in .NET Standard support at this point in time.

Update `samples/Hello` so that it (1) works, and (2) instantiates the
`Java.Lang.Object` binding:

	dotnet run --project samples/Hello

~~ Binding changes vs. Xamarin.Android ~~

Java arrays are bound as appropriate `IList<T>`, using the
`Java.Interop.Java*Array` types as an intermediary.  This should help
reduce marshaling logic & overhead, as if the "source" array is a
`Java*Array`, it doesn't need to be "deep copied".  The exception is
C# `params` arrays, which continue to be bound as arrays, and are
marshaled via an appropriate `Java*Array` type.

`java.io.InputStream` isn't bound as `System.IO.Stream`, etc.

"Java.Interop-style" constructors are used (25de1f3), e.g.

	// This
	DeclaringType (ref JniObjectReference reference, JniObjectReferenceOptions options);

	// Not Xamarin.Android-style
	DeclaringType (IntPtr handle, JniHandleOwnership transfer);

"Java.Interop-style" wrapper construction is used, e.g.

	// This
	var wrapper = JniEnvironment.Runtime.ValueManager.GetValue<DeclaringType>(ref h, JniObjectReferenceOptions.CopyAndDispose);

	// Not this
	var wrapper = Java.Lang.Object.GetObject<DeclaringType>(handle);

Marshal methods are currently skipped.  Java-to-managed invocations
are not currently supported.

~~ TODO: Marshal Methods? ~~

Xamarin.Android uses Java Callable Wrappers + `Runtime.register()`
to specify which methods to register, via lots of reflection, etc.

For Desktop, JCW's shouldn't have all the methods to register.
Instead, use the `jnimarshalmethod-gen`-originated strategy of
`[JniAddNativeMethodRegistrationAttribute]` within the binding, and
then have it use `MarshalMethodBuilder` to generate the marshal
methods.  Need to update `MarshalMethodBuilder` to look for overrides
in addition to methods with [`JavaCallable`], which in turn will
require an equivalent to `Android.Runtime.RegisterAttribute(…)`.

Perhaps `JniMethodSignatureAttribute(string name, string sig)`?

In the meantime, `Java.Base` will skip all marshal-method logic
plus runtime method generation.  Leave that for later.

~~ TODO: Other API Changes? ~~

We should "unify" `java.lang.Object` and `System.Object`.  Consider
`java.lang.Class`:

	/* partial */ class Class<T> {
	    public boolean isInstance(java.lang.Object);
	    public java.lang.Object[] getSigners();
	}

If we unify `java.lang.Object` and `System.Object`, we'd have a
binding of:

	partial class Class {
	    public bool IsInstance (object value);
	    public object[] GetSigners();
	}

~~ Open Questions ~~

What's up with `java.lang.Class.getAnnotationsByType()`?

During an iteration of this PR, I got:

	public unsafe Java.Interop.JavaObjectArray<Java.Lang.Object>? GetAnnotationsByType (Java.Lang.Class? annotationClass)
	{
	    const string __id = "getAnnotationsByType.(Ljava/lang/Class;)[Ljava/lang/annotation/Annotation;";

From `__id` we see that the Java return type is `Annotation[]`, yet
we bind it as an `Object` array?  Why?  How do we fix that to instead
bind it as `JavaObjectArray<Java.Lang.Annotations.Annotation>`?

Currently, it's differently *worse*; I don't know why, but `__id`
is now:

	const string __id = "getAnnotationsByType.(Ljava/lang/Class;)[Ljava/lang/Object;";

i.e. the return type is an `Object` array instead of an `Annotation`
array, which is wrong, as per `javap`:

	% javap -s java.lang.Class
	…
	  public <A extends java.lang.annotation.Annotation> A getAnnotation(java.lang.Class<A>);
	    descriptor: (Ljava/lang/Class;)Ljava/lang/annotation/Annotation;

Fixing unit tests...
@jonpryor jonpryor force-pushed the jonp-add-java.base-binding branch from 27ed440 to 6e47f67 Compare December 5, 2021 22:22
@@ -10,9 +10,9 @@
namespace generatortests
{
[TestFixture]
class JavaInteropCodeGeneratorTests : CodeGeneratorTests
class XAJavaInteropCodeGeneratorTests : CodeGeneratorTests
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this change -- which is basically the removal of the JavaInteropCodeGeneratorTests fixture -- is what's responsible for the drop in the number of unit tests run.

The primary reason for removing it was to reduce the number of failing tests, but maybe it won't be too hard to re-enable this fixture…

    /Users/runner/hostedtoolcache/dotnet/sdk/6.0.100/Microsoft.Common.CurrentVersion.targets(2068,5):
    Warning : The referenced project '../../tools/class-parse.csproj' does not exist.

Oops.  Bad path.

    build-tools\scripts\Prepare.targets(17,5): Warning : Could not get information about JdksRoot path `C:\Program Files\Java`: Not a directory (Parameter 'homePath')

Guess we shouldn't hardcode `C:\Program Files\Java`.
Try using `$(JAVA_HOME_8_X64)` instead.
@jonpryor
Copy link
Member Author

jonpryor commented Dec 6, 2021

@jpobst: all warnings are fixed. More unit tests than current main are now executed, instead of fewer.

@jonpryor jonpryor merged commit bc5bcf4 into dotnet:main Dec 7, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 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