[net11.0][XSG] Trimmable Styles#33561
Conversation
bfcfe86 to
01cf3de
Compare
64a71d2 to
eab10eb
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements lazy/trimmable styles for XAML Source Generation, enabling the IL trimmer to remove unused styles and their target types from compiled apps. The feature addresses issue #33156 by deferring style initialization until first application and using string-based type names instead of Type objects for AOT compatibility.
Changes:
- Added new
Style(string assemblyQualifiedTargetTypeName)constructor andLazyInitializationproperty for deferred initialization - Modified
IStyle.TargetTypeto be nullable (Type?) to support trimmed types - Updated source generator to emit lazy style initialization code with type guards
- Extended visitor infrastructure with
StopOnStyle,VisitNodeOnStyle, andIsStylemethods across all visitors (both SourceGen and XamlC)
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Style.cs | Adds lazy constructor, LazyInitialization property, nullable TargetType resolution, and TargetTypeFullName for trim-safe type comparison |
| IStyle.cs | Changes TargetType to nullable Type? to support trimmed types |
| MergedStyle.cs | Updates to handle nullable TargetType from IStyle |
| ResourceDictionary.cs | Uses TargetTypeFullName instead of TargetType.FullName for adding styles |
| CreateValuesVisitor.cs | Implements TryCreateTrimmableStyle to generate string-based Style constructor |
| SetPropertiesVisitor.cs | Generates LazyInitialization lambda with type guard before style property assignment |
| SetNamescopesAndRegisterNamesVisitor.cs | Adds null checks for nodes not in Variables dictionary (Style content) |
| XamlNode.cs | Implements IsStyleContent logic for skipping Style children during normal traversal |
| XmlName.cs | Adds _StyleContent marker for identifying styles needing lazy initialization |
| All visitor files (20+) | Implements StopOnStyle, VisitNodeOnStyle, and IsStyle interface methods |
| PublicAPI files (7) | Adds new public API entries for Style constructor and LazyInitialization setter |
| Test files (10+) | Updates tests to call InitializeIfNeeded for SourceGen inflator, adds comprehensive lazy style tests |
| return false; | ||
| do | ||
| { | ||
| targetType = targetType.BaseType; |
There was a problem hiding this comment.
The CanBeAppliedTo method has a potential null reference exception. When checking ApplyToDerivedTypes, the code iterates through the base types and calls targetType.FullName without checking if targetType is null. If targetType.BaseType returns null (which happens when reaching the top of the type hierarchy), the code will throw a NullReferenceException on the next iteration when accessing targetType.FullName.
Add a null check before accessing targetType.FullName in the loop.
| targetType = targetType.BaseType; | |
| targetType = targetType.BaseType; | |
| if (targetType == null) | |
| return false; |
| return _targetType.FullName.AsSpan(); | ||
|
|
||
| // Extract FullName from AQN: "Namespace.TypeName, AssemblyName, ..." | ||
| // FullName is everything before the first comma | ||
| Debug.Assert(_assemblyQualifiedTargetTypeName is not null, "Either _targetType or _assemblyQualifiedTargetTypeName must be set"); |
There was a problem hiding this comment.
The TargetTypeFullName property may throw a NullReferenceException when _targetType is not null but _targetType.FullName is null. For generic types or special type scenarios, Type.FullName can return null according to the .NET documentation. This would cause AsSpan() to throw.
Add a null check for _targetType.FullName before calling AsSpan(), or handle the null case by falling back to the assembly-qualified name extraction logic.
| return _targetType.FullName.AsSpan(); | |
| // Extract FullName from AQN: "Namespace.TypeName, AssemblyName, ..." | |
| // FullName is everything before the first comma | |
| Debug.Assert(_assemblyQualifiedTargetTypeName is not null, "Either _targetType or _assemblyQualifiedTargetTypeName must be set"); | |
| { | |
| var fullName = _targetType.FullName; | |
| if (fullName is not null) | |
| return fullName.AsSpan(); | |
| // If FullName is null (e.g., for certain generic or special types), | |
| // fall back to the assembly-qualified name parsing below. | |
| } | |
| // Extract FullName from AQN: "Namespace.TypeName, AssemblyName, ..." | |
| // FullName is everything before the first comma | |
| Debug.Assert(_assemblyQualifiedTargetTypeName is not null, "When _targetType.FullName is null, _assemblyQualifiedTargetTypeName must be set"); |
e7d801f to
d0c0ce4
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, #34011, to work on those changes. Once the pull request is ready, I'll request review from you. |
3536086 to
fefc63a
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>
fefc63a to
59a9982
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33561Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33561" |
59a9982 to
9fb44d8
Compare
Self-review notes1. Extract helper for repeated
|
29da551 to
a14c299
Compare
|
@StephaneDelcroix @rmarinho 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.
- Inline the lazy initialization lock+check directly into IStyle.Apply - Remove the internal InitializeIfNeeded method (was only needed for tests) - Update tests to use ((IStyle)style).Apply() instead of InitializeIfNeeded Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Duplicate was introduced during rebase conflict resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CanBeAppliedTo: add null check after BaseType walk (reaches null at top of hierarchy before hitting Element) - TargetTypeFullName: guard against Type.FullName being null for generic/special types; fall through to AQN parsing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- PublicAPI net-ios/net-maccatalyst: remove duplicate ShellFlyoutRenderer entry - Revert whitespace-only changes in 3 SourceGen test snapshot files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace 6 occurrences of ((IStyle)style).Apply(target, new SetterSpecificity()) with style.ForceInitialize(target) extension method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SetPropertiesVisitor: remove duplicate variable declarations and duplicate getNodeValue delegate from rebase conflict - SetNamescopesAndRegisterNames: remove duplicate if-statement, keep TryGetValue guard for nodes not in Variables - Style.CanBeAppliedTo: guard against null Type.FullName Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Change TargetTypeFullName from ReadOnlySpan<char> to string, removing #if NETSTANDARD branching and Span-based comparisons - Use plain string equality in CanBeAppliedTo instead of SequenceEqual - Remove redundant .ToString() call in ResourceDictionary.Add - Revert unrelated maps PublicAPI.Unshipped.txt changes from rebase Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename parentVar in nested scope to avoid CS0136. Add StringComparison.Ordinal to IndexOf call to satisfy CA1307. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fe637c1 to
2b30cbd
Compare
- SetPropertiesVisitor.cs: Use Context property (not primary ctor param) since this class uses a regular constructor - Style.cs: Remove StringComparison.Ordinal from char IndexOf overload for netstandard2.0 compatibility Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if (!ApplyToDerivedTypes) | ||
| return false; | ||
| do | ||
| while (targetType != typeof(Element)) |
There was a problem hiding this comment.
🤖 The do-while → while refactor here changed semantics. If targetType is exactly typeof(Element) and ApplyToDerivedTypes=true, the old code entered the loop body first (getting BaseType), but the new while loop checks the condition first — so it never enters.
Old behavior:
do {
targetType = targetType.BaseType; // Element → NavigableElement
if (TargetType == targetType) return true; // match!
} while (targetType != typeof(Element));New behavior:
while (targetType != typeof(Element)) // Element == Element → false, skip loop
{
// never reached
}
return false; // ← regressionSuggested fix — restore do-while with the null guard:
do
{
targetType = targetType.BaseType;
if (targetType is null)
return false;
if (targetType.FullName is not null && TargetTypeFullName == targetType.FullName)
return true;
} while (targetType != typeof(Element));
return false;d909db6 to
3a76e7f
Compare
- StyleSourceGenTests: Reorder __root before style in snapshots to match upstream SourceGen visit order change - SetterCompiledConverters, SimplifyOnPlatform, Maui32879: Convert fragile full-snapshot tests to assertion-based tests validating the trimmable style pattern (string ctor, LazyInitialization, setters) - LazyStyleTestHelper: Call LazyInitialization directly instead of IStyle.Apply to avoid InvalidCastException when style target type differs from the test element type Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/refactor-copilot-yml |
|
|
AI code review for net11.0 targetVerdict: Needs changes (well-designed, but gated on unverified unit-test failures) This is a non-approval, automated review comment. It is advisory only — human approval is required. What this PR doesImplements lazy/trimmable styles for XAML Source Generation (#33156): a new Findings
CI note
Confidence: Medium. The design and diff look correct; the verdict is driven by unverifiable unit-test failures on the exact projects this PR changes. |
kubaflo
left a comment
There was a problem hiding this comment.
PR #33561 — [net11.0][XSG] Trimmable Styles (sibling of #33611)
Verdict: NEEDS_CHANGES (confidence: high). Strong, well-structured work — the lazy/trimmable Style shape is coherent, the three XAML pipelines (Build.Tasks/XamlC, SourceGen, runtime) were updated consistently, and PublicAPI is in order. But all four models converged on a TargetType-serialization bug that breaks parity for generic/nested target types, plus a lazy-init lifecycle edge case.
Blocking (inline)
- ❌ TargetType serialized as a C# display name, not a CLR AQN (
CreateValuesVisitor.cs:432).ToDisplayString(FullyQualifiedFormat)producesFoo<Bar>/Outer.Inner, butType.GetType(runtime,Style.cs:182) needsFoo`1[[…]]/Outer+Inner. For generic/nested target types it returns null and the style is silently skipped (no diagnostic), and the same malformed name breaksTargetTypeFullName+ implicit/StyleClass key matching. Unanimous across all 4 models + code-verified. ⚠️ LazyStylecan be permanently consumed if first applied to an incompatible element (Style.cs:149). The generated initializer is type-guarded (if (__target is not T) return;), butIStyle.ApplynullsLazyInitializationunconditionally after the first call — so a mismatched-first-apply returns early without initializing yet still consumes the initializer, silently breaking the style for later correct elements. (opus-4.8 + opus-4.6.)
Notes / lower-confidence (not inline)
- gemini flagged implicit-style detection
node.XmlType.Name == "Style"(SetPropertyHelpers.cs:165) as potentially too narrow — worth a quick check that aliased/subclassed style elements are still detected, though the common case is fine. - The simple-type path (the vast majority of styles) works correctly; the serialization bug is scoped to generic/nested
TargetType. The silent-skip behavior is what elevates it to a blocker — a clear diagnostic (or correct AQN) would prevent a hard-to-debug "style just doesn't apply."
CI
Red legs are the usual unrelated infra flakes; the generator-correctness items above are the blockers.
(opus-4.8 initially returned a placeholder but completed on retry; synthesis reflects all 4 models + direct code verification. This mirrors the target-type-resolution class of issue found in the sibling EventTrigger XSG PR #33611.)
| // Generate the assembly-qualified name (without global:: prefix, with assembly name) | ||
| // Type.GetType() expects format: "Namespace.TypeName, AssemblyName" | ||
| #pragma warning disable RS0030 // Use banned ToDisplayString to get name without global:: prefix | ||
| var typeFullName = targetType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle(SymbolDisplayGlobalNamespaceStyle.Omitted)); |
There was a problem hiding this comment.
The TargetType name handed to the lazy Style constructor is a C# display name, not a CLR assembly-qualified name — so generic/nested target types silently break. Line 432 builds the name via targetType.ToDisplayString(FullyQualifiedFormat) and line 434 appends , {assembly}. That's only a valid Type.GetType string for simple types. For:
- generic target types, C# syntax is
Ns.Foo<Ns.Bar>but the CLR/AQN form isNs.Foo`1[[Ns.Bar, AsmBar]], AsmFoo; - nested target types, C# uses
Ns.Outer.Innerbut the CLR form isNs.Outer+Inner.
At runtime Style.GetTargetType() calls Type.GetType(_assemblyQualifiedTargetTypeName, throwOnError: false) (Style.cs:182). For these shapes it returns null, and per the suppression note at Style.cs:177-178 a null target type means the style is silently skipped — no diagnostic, the style just doesn't apply, diverging from XamlC/runtime. The same malformed name also flows into TargetTypeFullName (Style.cs:188-204, which slices at the first comma) and is compared against targetType.FullName for implicit/class-style key matching (Style.cs:220/229), so implicit styles and StyleClass matching on generic/nested types are wrong too. Generate a real assembly-qualified name (generic-arg AQNs + nested +), or store the runtime-format full name separately, rather than ToDisplayString(). (Unanimous across all 4 models; confirmed in code.)
| if (LazyInitialization is not null) | ||
| { | ||
| LazyInitialization(this, bindable); | ||
| LazyInitialization = null; |
There was a problem hiding this comment.
A shared lazy Style can be permanently “consumed” (and silently never initialized) if it is first applied to an incompatible element. In IStyle.Apply the LazyInitialization delegate is invoked with the first bindable and then set to null unconditionally (lines 146-150). The generated initializer begins with a type guard — if (__target is not TargetType) return;. So if this same Style instance is first applied to a non-matching element (e.g. an explicit Style="{StaticResource X}" mistakenly set on a wrong-typed view, or any path that applies the shared instance to an incompatible type before a compatible one), the initializer returns early without wiring up setters/triggers, yet LazyInitialization is still nulled — so every subsequent Apply to a correct element finds LazyInitialization == null and the style does nothing, with no error. Only null out LazyInitialization once initialization actually ran (e.g. have the generated delegate report success, or move the type guard out so a mismatch doesn't consume the initializer). (opus-4.8 + opus-4.6.)
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
Implements lazy/trimmable styles for XAML Source Generation, enabling the IL trimmer to remove unused styles and their target types from compiled apps.
Fixes #33156
How It Works
New Style Constructor and LazyInitialization Property
Generated Code Pattern
The source generator now emits:
Key Implementation Details
IStyle.TargetTypereturnsType?(nullable) - returnsnullif type was trimmedStyle.ResolveTargetType()uses[UnconditionalSuppressMessage]to suppress IL2057 - intentionally allows types to be trimmedStyle.InitializeIfNeeded(target)called on firstIStyle.Apply()- runs the initializer once with proper lockingStyle.TargetTypeFullNameprovides trim-safe type comparison without resolving the TypeChanges Summary
LazyInitializationproperty,InitializeIfNeeded(),ResolveTargetType()TargetTypeis now nullable (Type?)StopOnStyle/VisitNodeOnStyle/IsStylefor special Style handlingWhat This Enables
TargetTypereturnsnulland the style is skipped at runtimeif (__target is not TargetType) return;to handle trimmed types gracefullyTODO