Fix Xaml compilation generating an empty InitializeComponent method when App contains a single item #8853
Conversation
…hen App contains a single item. Fixes dotnet#4538 Fixes dotnet#4457 Fixes dotnet#2926 Fixes dotnet#2543 Fixes dotnet#6198
dotnet-policy-service
bot
requested review from
dipeshmsft and
singhashish-wpf
dotnet-policy-service
bot
added
PR
| @@ -517,7 +517,7 @@ public override void WriteElementEnd(XamlElementEndNode xamlEndObjectNode) | |||
| xmlReader.MoveToAttribute(attrName); | |||
| } | |||
| } | |||
| else if (!_compiler.IsBamlNeeded && !_compiler.ProcessingRootContext && _compiler.IsCompilingEntryPointClass && xmlReader.Depth > 0) | |||
There was a problem hiding this comment.
This code is called too early to call _compiler.ProcessingRootContext.
Basically, there are two steps involved here:
- Reading a Xaml element.
- Parsing the Xaml element.
_compiler.ProcessingRootContext will only be set in step 2 but this code here is ran in step 1. This means that we get its value before it was set. Since both steps are ran for each Xaml element, when we have 2 or more element we execute this code here with _compiler.ProcessingRootContext initialized.
We can remove the check to _compiler.ProcessingRootContext because it was already superfluous because the check here ProcessedRootElement:
basically does the same check as
_compiler.ProcessingRootContext but ProcessedRootElement is set in step 1 instead of step 2.
I hope my explanation was clear enough to explain my reasoning. Feel free to ask question and I'll try to answer though the Xaml compilation process is very complex and I might not be able to properly explain it. It makes sense in my head 😄.
There was a problem hiding this comment.
Thanks for your contribution @ThomasGoulet73 , this change simplifies the code by removing a redundant check to _compiler.ProcessingRootContext, which is called too early in the XAML compilation process to have its value set. The existing check to ProcessedRootElement already serves the same purpose and is set earlier in the process, making the _compiler.ProcessingRootContext check unnecessary. This improves code clarity and avoids potential confusion.
Possibly you can add this in the description as well for better understanding.
There was a problem hiding this comment.
@anjali-wpf I found it easier to comment on the changes on this specific line. I added a link to this comment in the description to improve clarity.
Fixes #2543
Fixes #2926
Fixes #4457
Fixes #4538
Fixes #6198
Description
There were many bug reports of different type that were all caused by the same bug. Xaml compilation generating an empty InitializeComponent method when App contains a single item.
Some context:
When generating App.InitializeComponent for a Xaml where there is only the StartupUri property, it will skip loading Baml in App.InitializeComponent and only set the StartupUri property in the generated code for performance reasons. There is currently a bug when detecting if the StartupUri property is the only property that would be loaded from the Baml.
See #8853 (comment) for an explanation of the changes in this PR.
Customer Impact
Should fix issues linked at the top (Might be more since the bug does not always surface the same way)
Regression
Yes and no, the bug was not present in non-SDK style .Net Framework projects.
Testing
Manual testing using the samples provided in the issues and manual testing with my own samples with multiple Xaml combination.
Risk
Low to medium. Worst case scenario this PR could cause existing project that were not loading Baml to now load Baml which could cause slight perf regression but I wasn't able to cause this in my testing.
Microsoft Reviewers: Open in CodeFlow