Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[generator] Fix StackOverflow when copying DIM via private interfaces (…
…#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
- Loading branch information
