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

Emit [DynamicDependency] to preserve Invokers #1263

Open
Tracked by #1192
jonpryor opened this issue Sep 30, 2024 · 8 comments
Open
Tracked by #1192

Emit [DynamicDependency] to preserve Invokers #1263

jonpryor opened this issue Sep 30, 2024 · 8 comments
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Milestone

Comments

@jonpryor
Copy link
Member

In the current .NET for Android world order, the .NET for Android workload adds custom linker steps to preserve the *Invoker types when a bound Java type is preserved.

In a possible future NativeAOT world order, there are no custom linker steps. We thus must use the linker-supported custom attributes to cause types to be preserved.

Given:

partial interface IMyInterface : IJavaPeerable {
    // …
}
partial class IMyInterfaceInvoker : Java.Lang.Object, IMyInterface {
    // …
}

We should place [DynamicDependency] on the interface (and/or abstract class) declaration so that the linker knows to preserve the Invoker:

[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(IMyInterfaceInvoker))]
partial interface IMyInterface : IJavaPeerable {
    // …
}
partial class IMyInterfaceInvoker : Java.Lang.Object, IMyInterface {
    // …
}
@jonpryor jonpryor added this to the .NET 10 milestone Sep 30, 2024
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Oct 1, 2024
@jpobst jpobst changed the title Emit [DynamicDependency] to preserver Invokers Emit [DynamicDependency] to preserve Invokers Oct 14, 2024
@jpobst
Copy link
Contributor

jpobst commented Dec 3, 2024

[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(IMyInterfaceInvoker))]
partial interface IMyInterface : IJavaPeerable {
    // …
}
partial class IMyInterfaceInvoker : Java.Lang.Object, IMyInterface {
    // …
}

This is not valid, as [DynamicDependency] can only be placed on a Constructor/Field/Method.

Is there another way to accomplish this? Or do we need to add this attribute to every type member? 🤮

@vitek-karas
Copy link
Member

How do we get from the interface to the invoker at runtime?
What do we do with the invoker once we have it?

My general answer will always be: "Don't use DynamicDependency. Instead figure out how we use reflection and adapt to that, ideally removing the reflection and if not possible make it more declarative"

@jonpryor
Copy link
Member Author

jonpryor commented Dec 4, 2024

@vitek-karas: please see this for a reasonable overview: 1adb796

Consider the Java java.lang.Runnable interface:

package java.lang;
public interface Runnable {
    void run ();
}

This is bound as:

package Java.Lang;
public interface IRunnable : IJavaPeerable {
    void Run ();
}

Now, assume a Java API + corresponding binding which returns a
Runnable instance:

package example;
public class Whatever {
    public static Runnable createRunnable();
}

You can invoke IRunnable.Run() on the return value:

IRunnable r = Whatever.CreateRunnable();
r.Run();

but how does that work?

How it works is via convention and Reflection.

How do we get from the interface to the invoker at runtime?

We append "Invoker" to the abstract class or interface name, and resolve it from the same assembly:

What do we do with the invoker once we have it?

We instantiate it:

ideally removing the reflection and if not possible make it more declarative"

I'm not sure I understand what that means. My guess is that you're suggesting we e.g. update [Register] to contain a Type directly referencing the Invoker type, e.g.

partial class RegisterAttribute : Attribute {
    [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
    public Type? InvokerType {get; set;}
}

which would allow:

[Register (, InvokerType=typeof(IRunnableInvoker))]
public partial interface IRunnable {
}

internal partial class IRunnableInvoker {
}

@jonpryor
Copy link
Member Author

jonpryor commented Dec 4, 2024

The InvokerType property is apparently the right idea: https://discord.com/channels/732297728826277939/732297837953679412/1313852503070212146

@jonpryor
Copy link
Member Author

jonpryor commented Dec 4, 2024

@jpobst: okay, so this got more complicated. :-(

Firstly, we need to update our attributes:

namespace Java.Interop {
    // In java-interop
    public partial class JniTypeSignatureAttribute {
        [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
        public Type? InvokerType {get; set;}
    }
}

// in dotnet/android
namespace Android.Runtime {
    partial class RegisterAttribute : Attribute {
        [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
        public Type? InvokerType {get; set;}
    }
}

Then we need to update generator to include InvokerType=typeof(WhateverInvoker) in the [JniTypeSignature] and [Register] output.

Then we need to update Java.Interop.dll & Mono.Android.dll to now look for and use InvokerType instead/in addition to the "append Invoker" convention we currently use.

Java.Interop.dll could probably be updated to only support InvokerType.

@jpobst: open question: should we update [Register] at all? Or just have XAJavaInterop1 start emitting [JniTypeSignature]?

@vitek-karas
Copy link
Member

I would also consider not using All unless we absolutely have to. All is kind of weird in that it will keep really everything about the type, so for example if the type implements an interface it will force all the methods on that interface to be kept (for everybody) and so on. It seems this is consumed by the CreateProxy which only needs all constructors.

@jonpryor
Copy link
Member Author

jonpryor commented Dec 4, 2024

@vitek-karas wrote:

for example if the type implements an interface it will force all the methods on that interface to be kept (for everybody) and so on.

I think that's mostly what we actually want/need. (See also.) For interfaces, all interface members must be preserved, always, because Java might call them, and we can't know whether or not that'll actually happen.

Abstract classes are more difficult, as we do want the linker to be able to remove non-overridden methods. However, if user code overrides a Java method, that must be preserved, especially when the only callers of that method are from Java.

You're thus immediately right that a new InvokerType property should constrain to PublicConstructors | NonPublicConstructors, but I believe that won't be enough, and I'm not sure what "enough" would be.

@jpobst
Copy link
Contributor

jpobst commented Dec 5, 2024

@jpobst: open question: should we update [Register] at all? Or just have XAJavaInterop1 start emitting [JniTypeSignature]?

Given that we already know there are issues with using [JniTypeSignature] for XAJavaInterop1 and that .NET 10 is likely going to have a lot of risk due to the other major changes we plan on making, I think my preference would be to update [Register] and not make too many changes at one.

If switching to [JniTypeSignature] is something we deem important in the .NET 10 timeframe we should probably tackle it separately.

jonpryor added a commit that referenced this issue Dec 5, 2024
Context: #1263
Context: #1263 (comment)

We want the default trimmer infrastructure to be able to automatically
preserve the `*Invoker` types which are required for interacting with
`abstract` classes and interfaces.

The most straightforward way to do this is to add a new `InvokerType`
property to `JniTypeSignatureAttribute` (and eventually
`RegisterAttribute`):

	partial class JniTypeSignatureAttribute {
	    [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors |
	        DynamicallyAccessedMemberTypes.NonPublicConstructors)]
	    public Type? InvokerType {get; set;}
	}

Update `generator` so that `generator --codegen-target=JavaInterop1`
output sets this new property on abstract classes and interfaces:

	namespace Java.Lang {

	    [Java.Interop.JniTypeSignatureAttribute("java/lang/Runnable", GenerateJavaPeer=false, InvokerType=typeof(Java.Lang.IRunnableInvoker))]
	    public partial interface IRunnable {
	    }
	    internal partial class IRunnableInvoker {
	    }

	    [Java.Interop.JniTypeSignatureAttribute("java/lang/Number", GenerateJavaPeer=false, InvokerType=typeof(Java.Lang.NumberInvoker))]
	    public abstract partial class Number {
	    }
	    internal partial class NumberInvoker {
	    }
	}

This allows the default trimmer to automatically preserve the
`*Invoker` type and constructors.

Update `Java.Interop.JniRuntime.JniValueManager` to no longer look
for `*Invoker` types "by string", and instead require use of the
`JniTypeSignatureAttribute.InvokerType` property.

Update unit tests and expected output so that everything works.

Note: XAJavaInterop1 output is impacted, but it shouldn't matter, as
it changes the `RegisterAttribute` `connector` on interfaces,
which isn't *used* by anything:

	// old and busted
	[Register ("xamarin/test/NotificationCompatBase$Action$Factory", "", "Xamarin.Test.NotificationCompatBase/Action/IFactoryInvoker")]

	// new hawtness
	[Register ("xamarin/test/NotificationCompatBase$Action$Factory", "", "Xamarin.Test.NotificationCompatBase.Action.IFactoryInvoker")]

Further note that the old value is non-sensical.
jonpryor added a commit that referenced this issue Dec 9, 2024
Context: #1263
Context: #1263 (comment)

We want the default trimmer infrastructure to be able to automatically
preserve the `*Invoker` types which are required for interacting with
`abstract` classes and interfaces.

The most straightforward way to do this is to add a new `InvokerType`
property to `JniTypeSignatureAttribute` (and eventually
`RegisterAttribute`):

	partial class JniTypeSignatureAttribute {
	    [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors |
	        DynamicallyAccessedMemberTypes.NonPublicConstructors)]
	    public Type? InvokerType {get; set;}
	}

Update `generator` so that `generator --codegen-target=JavaInterop1`
output sets this new property on abstract classes and interfaces:

	namespace Java.Lang {

	    [Java.Interop.JniTypeSignatureAttribute("java/lang/Runnable", GenerateJavaPeer=false, InvokerType=typeof(Java.Lang.IRunnableInvoker))]
	    public partial interface IRunnable {
	    }
	    internal partial class IRunnableInvoker {
	    }

	    [Java.Interop.JniTypeSignatureAttribute("java/lang/Number", GenerateJavaPeer=false, InvokerType=typeof(Java.Lang.NumberInvoker))]
	    public abstract partial class Number {
	    }
	    internal partial class NumberInvoker {
	    }
	}

This allows the default trimmer to automatically preserve the
`*Invoker` type and constructors.

Update `Java.Interop.JniRuntime.JniValueManager` to no longer look
for `*Invoker` types "by string", and instead require use of the
`JniTypeSignatureAttribute.InvokerType` property.

Update unit tests and expected output so that everything works.
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.)
Projects
None yet
Development

No branches or pull requests

3 participants