[net11.0][XSG] Trimmable EventTrigger#33611
Conversation
7bf12d8 to
3b691d2
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces AOT-safe and trimming-safe alternatives for EventTrigger by adding factory methods that use compile-time event subscription instead of reflection. The implementation uses a strategy pattern to support both the legacy reflection-based approach (for backward compatibility) and new AOT-safe static strategies.
Changes:
- Added two
EventTrigger.Create<T>()factory methods for AOT-safe event subscription (one forEventHandlerand one forEventHandler<TEventArgs>) - Integrated source generator support to automatically emit AOT-safe code when EventTriggers are used in XAML
- Introduced strategy pattern with
IEventSubscriptionStrategyinterface to support both reflection-based and static event subscription - Marked the parameterless constructor with
[RequiresUnreferencedCode]and[RequiresDynamicCode]attributes - Updated all platform-specific PublicAPI.Unshipped.txt files
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/src/Core/Interactivity/EventTrigger.cs | Core implementation: added factory methods, strategy pattern, and refactored event subscription logic |
| src/Controls/src/SourceGen/EventTriggerValueProvider.cs | New source generator value provider for emitting AOT-safe EventTrigger.Create calls |
| src/Controls/src/SourceGen/Visitors/CreateValuesVisitor.cs | Integration point for EventTriggerValueProvider in the visitor pattern |
| src/Controls/src/SourceGen/NodeSGExtensions.cs | Registered EventTriggerValueProvider as a known markup value provider |
| src/Controls/tests/Xaml.UnitTests/Issues/Maui33591.xaml | XAML test file with EventTriggers on Button, Entry, and Switch |
| src/Controls/tests/Xaml.UnitTests/Issues/Maui33591.xaml.cs | Tests for XAML inflation, control creation, and EventTrigger property validation |
| src/Controls/tests/SourceGen.UnitTests/Maui33591SourceGenTests.cs | Snapshot test verifying source generator produces correct AOT-safe code |
| src/Controls/tests/Core.UnitTests/Triggers/EventTriggerBaseTests.cs | Unit tests for factory methods, event binding, and lifecycle management |
| src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt | Public API surface additions for all platforms (7 files) |
| var eventSymbols = targetType.GetAllEvents(eventName, context).ToList(); | ||
| if (eventSymbols.Count > 0) | ||
| { | ||
| var eventSymbol = eventSymbols.First(); |
There was a problem hiding this comment.
The event lookup logic only checks if eventSymbols.Count is greater than 0 but always uses the first event found. If multiple events with the same name exist (e.g., through inheritance or explicit interface implementation), this could select the wrong event. Consider adding validation or documentation about this behavior, or use a more specific selection strategy.
| private static ITypeSymbol? FindTargetType(ElementNode eventTriggerNode, SourceGenContext context) | ||
| { | ||
| INode? current = eventTriggerNode; | ||
|
|
||
| while (current != null) | ||
| { | ||
| current = current.Parent; | ||
|
|
||
| // Skip ListNodes (they're the collection wrapper) | ||
| if (current is ListNode listNode) | ||
| current = listNode.Parent; | ||
|
|
||
| // Found an ElementNode - this should be our target | ||
| if (current is ElementNode parentElement && parentElement != eventTriggerNode) | ||
| return parentElement.XmlType.GetTypeSymbol(context); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The FindTargetType method could return null if the EventTrigger is not properly nested in the XAML tree. When this happens, the fallback code at line 96-105 is used, but there's no logging or diagnostic information to help developers understand why the AOT-safe path wasn't taken. Consider adding a diagnostic message when targetType is null to aid in debugging XAML structure issues.
77bc8e8 to
ec764d4
Compare
|
/rebase |
|
@copilot this PR needs a proper rebase with conflict resolution. open a PR which addresses this issue. make sure it is up to date with the base net11.0 branch. |
|
@simonrozsival I've opened a new pull request, #34012, to work on those changes. Once the pull request is ready, I'll request review from you. |
f07cb44 to
6afe2be
Compare
…hanges These changes belong in separate PRs: - #33611 (CSS StyleSheet trimming) - #33561 (EventTrigger trimming) - #33160 (HybridWebView trimming) Reverted files to their main branch state, keeping only the [ElementHandler] attribute addition on HybridWebView. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33611Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33611" |
0ed7c65 to
bbea0c2
Compare
bbea0c2 to
dfd63be
Compare
|
/azp run maui-pr-uitests,maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
1c471b3 to
ccf071a
Compare
|
@StephaneDelcroix @jfversluis could you please have a look at this PR and let me know what you think about it? could we merge this into net11.0 soon? |
Squashed for clean rebase.
Duplicate was introduced during rebase conflict resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When the source generator cannot determine the target type for an EventTrigger (e.g., when used inside Style.Triggers or a ResourceDictionary), it falls back to reflection-based EventTrigger. This new warning (MAUIX2016) informs developers that the trigger won't be AOT-safe and suggests ensuring the EventTrigger is nested directly inside an element's Triggers collection. The event lookup using .First() is consistent with the existing ConnectEvent pattern in SetPropertyHelpers.cs and matches the runtime GetRuntimeEvent behavior (most-derived event wins). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Document why .First() is correct for event lookup (most-derived wins, matching GetRuntimeEvent behavior and ConnectEvent in SetPropertyHelpers) - Add Maui33591_ShadowedEvent test: DerivedButton shadows Button.Clicked with a new event, verifies EventTrigger subscribes to the derived one Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leftover entries without the eventName parameter from an earlier iteration of the API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ccf071a to
d6d75fe
Compare
|
/azp run maui-pr-devicetests, maui-pr-uitests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/review -b feature/refactor-copilot-yml |
|
|
AI code review for net11.0 targetVerdict: Needs changes (red CI on consumer builds points to a likely PR-caused issue) Independent review (diff-first, then reconciled with the PR narrative). This is not an approval — a human still needs to sign off. What the PR doesMakes Findings
CI
Confidence: medium-high that the red consumer builds are PR-caused (generator error/emit); please verify the exact diagnostic in the failing Build Sample App log. |
kubaflo
left a comment
There was a problem hiding this comment.
PR #33611 — [net11.0][XSG] Trimmable EventTrigger (issue #33591)
Verdict: NEEDS_CHANGES (confidence: high). Great direction — making EventTrigger AOT/trim-safe by resolving the event at compile time and emitting a strongly-typed EventTrigger.Create<…> subscription (no reflection), with new MAUIX2016/2017 diagnostics and shadowed-event test coverage. But two generator edge cases break compilation/builds for valid XAML — found with strong agreement across the 3 models that completed (gpt-5.5 + gemini NEEDS_CHANGES, opus-4.6 NEEDS_DISCUSSION) and confirmed in code.
Blocking (inline)
- ❌ Custom-delegate events → uncompilable generated code (line 84-93). The path is chosen purely on whether the second event param is
EventArgs, not on whether the event's delegate is actuallyEventHandler/EventHandler<T>. An event using a custom(object, TArgs)delegate emitstarget.Event += EventHandler<TArgs>, which doesn't convert to the custom delegate type → CS error in generated code. Only take the strongly-typed factory path for realEventHandler/EventHandler<T>events; otherwise fall back to the runtimeEventTrigger. - ❌
EventTriggerinside<Style>→ build-breakingMAUIX2016error (line 157).FindTargetTyperesolves the parent asStyle(not theTargetType), the event isn't found, andMAUIX2016isErrorseverity — so valid style-trigger XAML that works at runtime now fails the build (even though the runtime fallback is still emitted). Special-caseStyleto use itsTargetType, or downgradeMAUIX2016to a warning since a working fallback is always generated.
Strengths
The shadowed/new event handling is correct (most-derived-first member walk, matching GetRuntimeEvent), the diagnostics are wired with AnalyzerReleases.Unshipped.md + resx, and the EventHandler/EventHandler common path is clean. Both issues above are localized to the target-type/event-type resolution and are straightforward to gate.
CI
Not the blocker here — the red legs are the usual unrelated infra flakes; the generator-correctness issues above are. Happy to re-review once the two edge cases fall back to the runtime path instead of emitting broken code / a hard error.
(Note: opus-4.8 timed out without producing output this run; synthesis is based on the other 3 models + direct code verification.)
| if (isGenericEventHandler) | ||
| { | ||
| var eventArgsTypeName = eventArgsType.ToFQDisplayString(); | ||
| writer.WriteLine($"global::Microsoft.Maui.Controls.EventTrigger.Create<{targetTypeName}, {eventArgsTypeName}>(\"{eventName}\", static (target, handler) => target.{eventName} += handler, static (target, handler) => target.{eventName} -= handler);"); |
There was a problem hiding this comment.
Custom-delegate events generate uncompilable code. The decision at line 84-86 sets isGenericEventHandler = !eventArgsType.Equals(EventArgs) — it only inspects the event's second parameter type, never whether the event's delegate type is actually System.EventHandler / System.EventHandler<T>. So for an event declared with a custom delegate that happens to have the (object, TArgs) shape — e.g. public delegate void MyHandler(object s, MyArgs e); event MyHandler Foo; — the generator emits:
EventTrigger.Create<Target, MyArgs>("Foo", static (target, handler) => target.Foo += handler, ...);where handler is EventHandler<MyArgs>. But target.Foo is of type MyHandler, and MyHandler += EventHandler<MyArgs> has no implicit conversion → the generated code doesn't compile (the same applies to the non-generic branch at line 97 for custom (object, EventArgs) delegates). The runtime EventTrigger handles these via Delegate.CreateDelegate; the generator should only take the strongly-typed factory path when eventSymbol.Type is exactly System.EventHandler or System.EventHandler<T>, and otherwise fall back to the runtime new EventTrigger { Event = ... } (as it already does when the target type can't be resolved). (3-model consensus: gpt-5.5, gemini, opus-4.6 — confirmed in code.)
| current = listNode.Parent; | ||
|
|
||
| // Found an ElementNode - this should be our target | ||
| if (current is ElementNode parentElement && parentElement != eventTriggerNode) |
There was a problem hiding this comment.
EventTrigger inside a <Style> now produces a build-breaking error for valid XAML. FindTargetType returns the parent ElementNode's type (line 157-158). For a style trigger:
<Style TargetType="Button"><Style.Triggers><EventTrigger Event="Clicked" .../></Style.Triggers></Style>the parent element is <Style>, so targetType resolves to Microsoft.Maui.Controls.Style. Clicked doesn't exist on Style, so GetAllEvents returns empty and the code calls ReportEventNotFound → MAUIX2016, which is DiagnosticSeverity.Error (Descriptors.cs:341). The runtime fallback new EventTrigger { Event = "Clicked" } is still emitted (line 122) and would work at runtime — but because MAUIX2016 is an error, the build fails for XAML that compiles and runs today. Please either special-case Style parents to resolve against their TargetType, or — since a working runtime fallback is already emitted whenever the target/event can't be resolved — make MAUIX2016 a warning (like the MAUIX2017 target-not-resolved case at line 349) rather than an error. (3-model consensus; severity confirmed in Descriptors.cs.)
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
This PR makes
EventTriggerAOT-safe and trimming-safe by introducing factory methods that use compile-time event subscription instead of reflection.Fixes #33591
Changes
New Factory Methods
Added two new
EventTrigger.Create()factory methods that allow AOT-safe event subscription:Source Generator Integration
The XAML source generator now emits
EventTrigger.Create<T>()calls instead ofnew EventTrigger()when the target type can be determined at compile time:Before (reflection-based, not AOT-safe):
After (AOT-safe):
Architecture
IEventSubscriptionStrategyinterfaceReflectionStrategy- Used by parameterless constructor (marked with[RequiresUnreferencedCode])StaticStrategy<TBindable>andStaticStrategy<TBindable, TEventArgs>- Used by factory methods (fully trimming-safe)EventTriggerclass remains sealed and backwards compatibleBackwards Compatibility
new EventTrigger()continues to work (with trimming warnings)Eventproperty is still set for all EventTrigger instancesKnown Limitations
The AOT-safe code generation only works when the source generator can determine the target type at compile time. This works for the standard usage pattern:
Edge cases that fall back to reflection-based EventTrigger:
EventTriggerdefined insideStyle.Triggers(the parent element isStyle, not the actual target type)EventTriggerdefined in aResourceDictionaryand applied dynamicallyBindableObjectThese edge cases will continue to work at runtime but will not be AOT-safe. The source generator emits a warning (MAUIX2015) when it cannot determine the target type and falls back to reflection.
Testing