-
Notifications
You must be signed in to change notification settings - Fork 53
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
output should avoid mentioning non-public
types
#1269
Labels
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
Comments
jonpryor
pushed a commit
that referenced
this issue
Oct 21, 2024
…#1261 Context: dotnet/android-libraries#988 Context: 9e0a469 Context: 1adb796 Context: #1183 Context: #1269 Imagine you bind the following Java interfaces: // Java package io.grpc; interface Configurator { default void configureChannelBuilder(ManagedChannelBuilder<?> channelBuilder) {} } public interface InternalConfigurator extends Configurator { } Because the `Configurator` interface is package-protected, its method is copied into the `public` `InternalConfigurator` interface via `generator`'s `FixupAccessModifiers()` code. Later, we look for the "base" method that the copied `InternalConfigurator.configureChannelBuilder()` overrides, which is `Configurator.configureChannelBuilder`. However, because we simply added the same method instance to two types, this method reference is actually pointing to itself. Eventually we hit a StackOverflowException when trying to retrieve the [overridden method's declaring type][0]. Fix this by _cloning_ the method when we "copy" it, rather than having two types reference the same `Method` instance. …***then*** we get to *testing* this behavior: during code review, @jonpryor noticed this change: diff --git a/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs b/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs index 3ea50e6bd..83d01f24a 100644 --- a/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs +++ b/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs @@ -46,7 +46,7 @@ public unsafe void BaseMethod () { const string __id = "baseMethod.()V"; try { - _members_xamarin_test_BaseInterface.InstanceMethods.InvokeAbstractVoidMethod (__id, this, null); + _members_xamarin_test_ExtendedInterface.InstanceMethods.InvokeAbstractVoidMethod (__id, this, null); } finally { } } and went "hmm". Recall and consider 1adb796: if we invoke the interface method on the "wrong" declaring type, things break. This is why 1adb796 updated interface invokers to only invoke methods upon their declared interfaces. The concern comes around *non-`public`* interface method invocation: /* package */ interface PackagePrivateInterface { void m(); } public interface PublicInterface extends PackagePrivateInterface { } With this commit, attempts to invoke `m()` are made upon `PublicInterface`, not `PackagePrivateInterface`. Is this a problem? Begin partially implementing #1183, adding a new `build-tools/Java.Interop.Sdk` (not quite a) "project", which has an `Sdk.props` and `Sdk.targets` file, which combined add support for: * A `@(JavaCompile)` build action, for Java source. * A `@(JavaReference)` build action, for Java libraries. * As a pre-Compile step, files with `%(JavaCompile.Bind)`=True or `%(JavaReference.Bind)`=True will be automatically bound, via `class-parse` + `generator`. * As a post-Compile step, the assembly will be processed with `jcw-gen` to generate Java Callable Wrappers, which will be compiled into `$(AssemblyName).jar`. Then, update `tests/Java.Base-Tests` to use this new functionality, adding a test case to hit the "invoke method declared in a private interface upon the public interface" scenario. Result: it works! Of partial interest is `IInterfaceMethodInheritanceInvoker`: internal partial class IInterfaceMethodInheritanceInvoker : global::Java.Lang.Object, IInterfaceMethodInheritance { static readonly JniPeerMembers _members_net_dot_jni_test_BaseInterface = new JniPeerMembers ("net/dot/jni/test/BaseInterface", typeof (IInterfaceMethodInheritanceInvoker)); static readonly JniPeerMembers _members_net_dot_jni_test_InterfaceMethodInheritance = new JniPeerMembers ("net/dot/jni/test/InterfaceMethodInheritance", typeof (IInterfaceMethodInheritanceInvoker)); static readonly JniPeerMembers _members_net_dot_jni_test_InternalInterface = new JniPeerMembers ("net/dot/jni/test/InternalInterface", typeof (IInterfaceMethodInheritanceInvoker)); static readonly JniPeerMembers _members_net_dot_jni_test_PublicInterface = new JniPeerMembers ("net/dot/jni/test/PublicInterface", typeof (IInterfaceMethodInheritanceInvoker)); } Of these four fields, two are for internal types: `_members_net_dot_jni_test_BaseInterface` and `_members_net_dot_jni_test_InternalInterface`. Fortunately those types aren't otherwise used. Of concern, though, is that the constructor invocation *does* result in a `JNIEnv::FindClass()` invocation, meaning these bindings would look up (ostensibly) "private" types, which could change! This presents a compatibility concern: if (when?) those type names change, then the generated bindings will break. TODO: * #1269: Update `generator` output to *not* emit the `static readonly JniPeerMembers` fields for internal types. Finally, update `jnimarshalmethod-gen` to support resolving types from the active assembly For reasons I'm not going to investigate, if an interface type is defined in the assembly that `jnimarshalmethod-gen` is processing, `Assembly.Location` will be the empty string, which causes an error: % dotnet bin/Release-net8.0/jnimarshalmethod-gen.dll bin/TestRelease-net8.0/Java.Base-Tests.dll -v -v --keeptemp … error JM4006: jnimarshalmethod-gen: Unable to process assembly 'bin/TestRelease-net8.0/Java.Base-Tests.dll' Name can not be empty System.ArgumentException: Name can not be empty at Mono.Cecil.AssemblyNameReference.Parse(String fullName) at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(String fullName, ReaderParameters parameters) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 261 at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(String fullName) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 256 at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.GetAssembly(String fileName) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 251 at Xamarin.Android.Tools.JniMarshalMethodGenerator.Extensions.NeedsMarshalMethod(MethodDefinition md, DirectoryAssemblyResolver resolver, TypeDefinitionCache cache, MethodInfo method, String& name, String& methodName, String& signature) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 790 at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly(String path) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 538 at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies(List`1 assemblies) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 285 Update `App.NeedsMarshalMethod()` so that when the assembly Location is not present, `DirectoryAssemblyResolver.Resolve(assemblyName)` is instead used. This prevents the `ArgumentException`. [0]: https://github.com/dotnet/java-interop/blob/9d997232f0ce3ca6a5f788b1cdb0fb9b2f978c84/tools/generator/SourceWriters/BoundMethod.cs#L95-L100
jpobst
added
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
labels
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
Context: #1261
#1261 describes a scenario in which public interfaces inherit package-private types:
The above is part of the
tests/Java.Base-Tests
build. AfterJava.Base-Tests
is built, the following file is created:tests/Java.Base-Tests/obj/$(Configuration)-net8.0/_ji/mcw/Net.Dot.Jni.Test.IInterfaceMethodInheritance.cs
which contains:
The concern is that the above snippet mentions two non-public types:
net/dot/jni/test/BaseInterface
andnet/dot/jni/test/InternalInterface
. If (when?) these types "go away" in the future -- they're not public, and thus could be removed or renamed at any point in time -- then the type initializer forIInterfaceMethodInheritanceInvoker
will throw an exception, as e.g.new JniPeerMembers ("net/dot/jni/test/BaseInterface", …)
will throw an exception becausenet/dot/jni/test/BaseInterface
will not be found.The text was updated successfully, but these errors were encountered: