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, generator] Bind all of package java.lang #966

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Mar 24, 2022

Context: bc5bcf4

Bind all classes and interfaces in the java.lang package.

Alter Java array binding, so that instead of IList<T>, we get
"direct" Java arrays, e.g.

namespace Java.Lang {
    partial class Character {
        // Previous/Xamarin.Android-like
        public static int CodePointAt (IList<char>? a, int index);

        // New/Desktop
        public static int CodePointAt (JavaCharArray? a, int index);
    }
}

Rationale: it allows for more efficient JVM :: .NET array copying,
by making copies explicit (to the dev), not implicit. We can
add an implicit conversion from e.g. IEnumerable<char> to
JavaCharArray in the future, if deemed useful.

This also impacts method return types, properties, and fields.

Bind the java.lang.module package in the namespace
Java.Lang.Modules. This is to avoid a type/namespace conflict
with java.lang.Module, bound as Java.Lang.Module.

Continue updating generator to remove "Android-isms".

Update Java.Base.csproj to ignore warning CS0108:

Java.Lang.Reflect.IAnnotatedArrayType.cs(15,45):
warning CS0108: 'IAnnotatedArrayType.AnnotatedOwnerType' hides inherited member
'IAnnotatedType.AnnotatedOwnerType'. Use the new keyword if hiding was intended.

The problem here is that we have:

public partial interface IAnnotatedType {
    // Contains default interface method
    virtual unsafe IAnnotatedType? AnnotatedOwnerType => …;
}
public partial interface IAnnotatedArrayType : IAnnotatedType {
    // Contains *no* method body; re-abstracted
    IAnnotatedType? AnnotatedOwnerType {get;}
}

TODO: figure out how to properly fix this. managedOverride
metadata (5a0e37e) doesn't seem useful to "re-abstract" a default
interface member.

Update Java.Base.targets to use generator --global. This is so
that java.lang.System can be bound as Java.Lang.System without
causing various C# compilation errors due to type lookup.
(Compare to Xamarin.Android's Java.Lang.JavaSystem, which got a
Java* prefix to avoid these compilation errors.)

Update JavaInteropCodeGeneratorTests.CreateOptions() so that C#
features such as default interface methods and nested interface types
are enabled within the unit tests.

TODO:

  • When generator --codegen-target=JavaInterop1 is used, all the
    language features should also be enabled by default.

  • Certain Java Annotation-related types aren't bound in
    JavaInterop1, vs. XAJavaInterop1. Revisit this.

  • "Revisit" use of JNIEnv.ToLocalJniHandle() in Xamarin.Android
    bindings, and it's outright removal in JavaInterop1 bindings.
    @jonpryor thinks the JNIENv.ToLocalJniHandle(v) was introduced
    "in case" v would be collected by the GC "during" a JNI call.
    Use of GC.KeepAlive() (1f21f38, da73d6a), would be a better
    solution, but also requires auditing generator output.

  • Bind the rest of java.base.jmod (bc5bcf4).

@jonpryor jonpryor requested a review from jpobst March 24, 2022 01:14
@jonpryor jonpryor force-pushed the jonp-more-java.base-fixes branch 2 times, most recently from 9f7a4be to b37ae0c Compare March 24, 2022 14:07
@jonpryor
Copy link
Member Author

So… strings: how should we bind them? There's no backward compatibility requirement with Java.Base.dll, so we don't need to be consistent with Xamarin.Android (which binds java.lang.String as System.String). This PR alters things so that java.lang.String is bound as Java.Lang.String.

Consider java.lang.String.compareTo(java.lang.String):

class String {
    public int compareTo​(String anotherString);
}

In Xamarin.Android, this is bound as:

partial class String {
    public int CompareTo (string anotherString);
}

In this PR, it is bound as:

partial class String {
    public int CompareTo (Java.Lang.String? anotherString);
}

This PR also adds an implicit conversion from string to Java.Lang.String, so "normal" usage works:

var s = new Java.Lang.String("value");
s.CompareTo("another value");

but this implicit conversion means extra GC String instances will be created.

Pros of binding java.lang.String as Java.Lang.String

Object identity is preserved. If there is a Java API which requires that you use exact string instances and not string equality, then we need to bind java.lang.String as Java.Lang.String.

  • Note: we've never encountered a bug related to this in Xamarin.Android. This is thus only theoretical.

Cons of binding java.lang.String as Java.Lang.String

The "simple" case of s.CompareTo("constant") either (1) isn't possible (unless you have an implicit conversion, as done in this PR), or (2) results in extra String instances that contribute to GC times.

In a "java.lang.String is bound as Java.Lang.String world order", s.CompareTo("constant") can be improved by providing method overloads which accept string. Xamarin.Android already does this for ICharSequence parameter types; extending this to also handle Java.Lang.String shouldn't be too complicated (famous last words), but is still a generator complication.

Part of the complication eluded to above is around virtual methods: the string overload shouldn't be virtual, which means it would need to create a temporary Java.Lang.String instance anyway; we'll be able to dispose it, but it'll still need to be created. Consider java.util.ResourceBundle.containsKey():

partial class ResourceBundle {
    // Xamarin.Android
    public virtual bool ContainsKey (string? key);

    // Java.Base?
    public virtual bool ContainsKey (Java.Lang.String? key);

    public bool ContainsKey (string? key)
    {
        using var jkey = key == null ? null : new Java.Lang.String (key);
        return ContainsKey (jkey);
    }
}

If we don't have a virtual method in play, we can instead overload by hitting JNI directly:

partial class String {
    public int CompareTo (Java.Lang.String? anotherString);

    public int CompareTo (string? anotherString)
    {
        const string __id = "…";
        return _members.InstanceMethods.InvokeAbstractInt32Method (__id, this,);
    }
}

This would avoid a Java.Lang.String temporary, but at the cost of generator complexity.

Consistency: commit bc5bcf4 had suggested a possible binding strategy change of:

TODO: Other Binding Changes?

We should eventually "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 JavaObjectArray<object> GetSigners();
    }

"Removing" java.lang.Object in lieu of System.Object has been a 12+ year old request. For consistency, if we bind java.lang.Object as System.Object, then we should bind java.lang.String as System.String.

Likewise, if we bind java.lang.String as Java.Lang.String, we should not bind java.lang.Object as System.Object.

We could likely get much of the desired benefit of "unifying" Java.Lang.Object with System.Object by providing an implicit conversion from System.Object to Java.Lang.Object:

namespace Java.Lang {
    partial class Object {
        public static implicit operator Object? (object? value) =>}
}

I'm not sure if either/any of these are good ideas.

The "nice" thing about binding java.lang.String as Java.Lang.String is that it makes some bits of generator more consistent: we can consistently emit v?.PeerReference ?? default or new JniObjectReference(v) for all reference type parameters. Internal consistency is nice!

This in turn suggests that "unifying" java.lang.Object with System.Object might not be a good idea either.

@jonpryor
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jpobst
Copy link
Contributor

jpobst commented Mar 24, 2022

A downside of binding as Java.Lang.String is that the implicit cast is not discoverable. When using IntelliSense, there is no way to know that you can use a string here:

image

Additionally, for return values, you are not going to see methods available on string, only methods available for Java.Lang.String:

image

Calling, for example, Java.Lang.String.ToUpperCase () instead of string.ToUpper () is probably not desired, as it incurs a Java roundtrip.

@jonpryor
Copy link
Member Author

@jpobst reminded me that I'm not personally very good with how things look like with IDE-colored glasses. Those are two separate scenarios -- parameters vs. return types -- but the underlying issue is the same: how the IDE presents options is not necessarily ideal.

generator-emitted auto string overloads can help with parameters. It can't help with return types.

@jonpryor
Copy link
Member Author

I previously pondered:

We could likely get much of the desired benefit of "unifying" Java.Lang.Object with System.Object by providing an implicit conversion from System.Object to Java.Lang.Object

After additional thought, that wouldn't be good, as it wouldn't be bidirectional. Consider:

var list = new Java.Util.ArrayList ();
list.Add (new List<int> { 1, 2, 3, 4});

This can be "made to work" by updating Java.Lang.Object to have an implicit conversion from System.Object. However, what would one want to do with list?

var items = (List<int>) list.Get (0); // boom

If ArrayList.Get(int) returns Java.Lang.Object, then the cast to (List<int>) will fail with an InvalidCastException, which is entirely unexpected. This can't be fixed without altering C# to support generic conversion operators, or possibly by inserting an "intermediate" cast (with appropriate "glue"):

var items = (List<int>) (object) list.Get (0); // *maybe* works as expected?

Thus, updating Java.Lang.Object to have an implicit conversion from object is a Bad Idea™.

The original idea of "unifying" Java.Lang.Object with System.Object can be "made to work", a'la:

partial class ArrayList {
    public object? Get (int index)
    {
        const string __id = "get.(I)Ljava/lang/Object;";
        try {
            JniArgumentValue* __args = stackalloc JniArgumentValue [1];
            __args [0] = new JniArgumentValue (index);
            var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
            return global::Java.Interop.JniEnvironment.Runtime.ValueManager.GetValue<object> (ref __rm, JniObjectReferenceOptions.CopyAndDispose);;
        } finally {
        }
    }
}

…because Runtime.ValueManager.GetValue<object>(…) will "unbox" any "boxed" values.

Which taken together suggests two things to me:

  1. We should bind java.lang.String as System.String, a'la Xamarin.Android, and
  2. We should "unify" java.lang.Object and System.Object. Such unification must be part of the actual bindings, and not "higher level" in e.g. Java.Lang.Object bindings.

@jpobst
Copy link
Contributor

jpobst commented Mar 24, 2022

We should "unify" java.lang.Object and System.Object.

I think one of the biggest sticking points for users is the need to inherit JLO to implement a bound Java interface, a la:

public class MyListener : Java.Lang.Object, IOnChangeListener { ... }

Would we be able to get around this requirement?

@jonpryor
Copy link
Member Author

@jpobst asked:

Would we be able to get around this requirement?

Yes and no.

See also: #17 (which is closed for some reason?!)

The "problem" is that all interfaces now implement IJavaPeerable, which has 12 required members. The easiest way to do this is to subclass Java.Lang.Object. We also need Java Callable Wrapper support, which currently excludes Java.Interop.JavaObject, but that can be fixed.

That said, it should be possible to (eventually) make IJavaPeerable implementable by "mere mortals", in part by documenting how to do so, and by extending jcw-gen so that we don't constrain to just Java.Lang.Object subclasses.

"Unifying" java.lang.Object with System.Object is not directly related to your "can IJavaPeerable be implemented?" question, though; IIRC the primary motivation for such "unification" was the above ArrayList example, so that you wouldn't need to create a class Holder : Java.LangObject {} class to stuff "arbitrary" types which are referenced only by Java code. We can support that. We can also "allow" people to implement IJavaPeerable themselves, so long as they do it "correctly" (which is quite the constraint!).

Context: bc5bcf4

Bind all classes and interfaces in the `java.lang` package.

Alter Java array binding, so that instead of `IList<T>`, we get
"direct" Java arrays, e.g.

	namespace Java.Lang {
	    partial class Character {
	        // Previous/Xamarin.Android-like
	        public static int CodePointAt (IList<char>? a, int index);

	        // New/Desktop
	        public static int CodePointAt (JavaCharArray? a, int index);
	    }
	}

Rationale: it *allows* for more efficient JVM :: .NET array copying,
by making copies explicit (to the dev), not implicit.  We can
add an implicit conversion from e.g. `IEnumerable<char>` to
`JavaCharArray` in the future, if deemed useful.

This also impacts method return types, properties, and fields.

Bind the `java.lang.module` package in the namespace
`Java.Lang.Modules`.  This is to avoid a type/namespace conflict
with `java.lang.Module`, bound as `Java.Lang.Module`.

Continue updating `generator` to remove "Android-isms".

Update `Java.Base.csproj` to ignore [warning CS0108][0]:

	Java.Lang.Reflect.IAnnotatedArrayType.cs(15,45):
	warning CS0108: 'IAnnotatedArrayType.AnnotatedOwnerType' hides inherited member
	'IAnnotatedType.AnnotatedOwnerType'. Use the new keyword if hiding was intended.

The problem here is that we have:

	public partial interface IAnnotatedType {
	    // Contains default interface method
	    virtual unsafe IAnnotatedType? AnnotatedOwnerType => …;
	}
	public partial interface IAnnotatedArrayType : IAnnotatedType {
	    // Contains *no* method body; re-abstracted
	    IAnnotatedType? AnnotatedOwnerType {get;}
	}

TODO: figure out how to properly fix this.  `managedOverride`
metadata (5a0e37e) doesn't seem useful to "re-abstract" a default
interface member.

Update `Java.Base.targets` to use `generator --global`.  This is so
that `java.lang.System` can be bound as `Java.Lang.System` without
causing various C# compilation errors due to type lookup.
(Compare to Xamarin.Android's `Java.Lang.JavaSystem`, which got a
`Java*` prefix to avoid these compilation errors.)

Update `JavaInteropCodeGeneratorTests.CreateOptions()` so that C#
features such as default interface methods and nested interface types
are enabled within the unit tests.

TODO:

  * When `generator --codegen-target=JavaInterop1` is used, all the
    language features should also be enabled by default.

  * Certain Java Annotation-related types aren't bound in
    JavaInterop1, vs. XAJavaInterop1.  Revisit this.

  * "Revisit" use of `JNIEnv.ToLocalJniHandle()` in Xamarin.Android
    bindings, and it's outright removal in JavaInterop1 bindings.
    @jonpryor *thinks* the `JNIENv.ToLocalJniHandle(v)` was introduced
    "in case" `v` would be collected by the GC "during" a JNI call.
    Use of `GC.KeepAlive()` (1f21f38, da73d6a), would be a better
    solution, but also requires auditing `generator` output.

  * Bind the rest of `java.base.jmod` (bc5bcf4).

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0108
@jonpryor jonpryor force-pushed the jonp-more-java.base-fixes branch from b37ae0c to 9b7edbc Compare March 28, 2022 18:08
@jonpryor jonpryor merged commit a65d6fb into dotnet:main Mar 29, 2022
@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