[Trimming] Fix event trigger trimming warnings

[simonrozsival]

Description of Change

This PR fixes trimming warnings related to event triggers. Consider the following example:

<Entry Text="Hello">
    <Entry.Triggers>
        <EventTrigger Event="TextChanged">
            <local:EntryTrigger />
        </EventTrigger>
    </Entry.Triggers>
</Entry>

An app with this markup will produce the following trimming warning:

/.../maui/src/Controls/src/Core/Interactivity/EventTrigger.cs(73): Trim analysis warning IL2072: Microsoft.Maui.Controls.EventTrigger.AttachHandlerTo(BindableObject): 'name' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicEvents' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeEvent(Type,String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/.../MyMauiApp/MyMauiApp.csproj::TargetFramework=net9.0-maccatalyst]

The fix will preserve all public events on BindableObjects. This is not the optimal solution and we could consider adding a generic EventTrigger<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicEvents)] T> variant of EventTrigger to preserve events only on types used with event triggers and improve app size.

[jonathanpeppers]

Could this instead be:

Suggested change

	static readonly MethodInfo s_handlerinfo = typeof(EventTrigger).GetMethod(nameof(OnEventTriggered), BindingFlags.Instance | BindingFlags.NonPublic, binder: null, types: new Type[] { typeof(object), typeof(EventArgs) }, modifiers: null);
	static readonly MethodInfo s_handlerinfo = new EventHandler(OnEventTriggered).Method;

Then s_handlerinfo can't be null (if we had NRT warnings in this file).

Would the OnEventTriggered method still be preserved?

[simonrozsival]

That looks interesting, I haven't thought of that. I'll give it a try.

[simonrozsival]

So, it turns out we cannot do exactly what you suggested because OnEventTriggered is an instance method, but I think we don't need the static s_handlerinfo at all.

[simonrozsival]

/cc @vitek-karas

[jonathanpeppers]

But overall, if this solves a trimming warning, I think it is a step better than what we had before. 👍

[jonathanpeppers]

Reading this class, trying to understand why so much System.Reflection usage in the original code...

I was wondering why it is not:

EventHandler _handlerdelegate;
//...
_handlerdelegate = OnEventTriggered;

But I guess what they are doing here is trying to make EventHandler<T> of any type work?

[simonrozsival]

Yes, I had the same idea, but it didn't work with generic event handlers.

[vitek-karas]

Agree with everything - not a fan of so much reflection, but this change should fix the trimming parts.
Can you please check if this produces AOT warnings? The CreateDelegate call is something I would be careful about in that context.

[simonrozsival]

@vitek-karas I don't get any AOT warnings related to EventTrigger.

The irony in this case is that we need to use CreateDelegate to match the event args type of event, but then we completely ignore the event args value 😄 Isn't there some (reflection-free) .NET hack to subscribe to an event without receiving the event args?

[vitek-karas]

@MichalStrehovsky I can't remember the rules around CreateDelegate, is this something which might cause problems in AOT?

[MichalStrehovsky]

This is fine for AOT. If we can invoke a method, we can also create a delegate to it with reflection.

Having to create a delegate just so we can get a MethodInfo from it is unfortunate. Looks like this would benefit from a C# methodof keyword or - since we're unlikely to get methodof - dotnet/runtime#94975.