microsoft / microsoft/microsoft-ui-xaml
XAML Compiler source generator creates unnecessary null checks for non-nullable objects
- Dominant language
- C++
- Stars
- 8.4k
- Forks
- 942
- Avg merge
- 2d 7h
- Merged PRs (30d)
- 105
Description
### Describe the bug
Unnecessary null checks are created by the XAML compiler. When i define a non-nullable variable, the source generated code still adds null checks.
### Why is this important?
Because it is completely unnecessary and removing them can both reduce the total LOCs in the source generated files and improve the performance.
The same thing happens for value type constants that are [clearly](https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0283?f1url=%3FappId%3Droslyn%26k%3Dk(CS0283)) non-nullable and are bound via `x:Bind` to the UI elements. They go through a method in the source generated code where they are checked for null.
* Possible argument 1: "nullability is just a hint - you can still set non-nullable fields to null if you want (you'll get warnings, but you can)"
* Answer: That's true but if i do that it's my mistake, i don't want thousands or tens of thousands of lines for null checking in case i accidentally set null to a value. I do want things to fail and throw when i make such mistake, otherwise they fail silently and we get undefined behavior, wondering why something isn't working.
* Possible argument 2: There are also a ton of libraries that haven't adopted the new nullability features.
* Answer: Okay, what better way than Microsoft's XAML setting the correct example for others to follow.
### Steps to reproduce the bug
Create a page, such as this
```csharp
internal sealed partial class ASR : Page
{
private ASRVM ViewModel => ViewModelProvider.ASRVM;
internal ASR() => InitializeComponent();
}
```
Observe this in the source generated code
```csharp
this.obj10 = global::WinRT.CastExtensions.As(target);
this.obj10RefreshRequested = (global::Microsoft.UI.Xaml.Controls.RefreshContainer p0, global::Microsoft.UI.Xaml.Controls.RefreshRequestedEventArgs p1) =>
{
if (this.dataRoot != null)
{
if (this.dataRoot.ViewModel != null)
{
this.dataRoot.ViewModel.RetrieveLatest();
}
}
};
```
### Actual behavior
This happens which is completely useless, unnecessary code and removing it can improve performance
```csharp
if (this.dataRoot.ViewModel != null)
```
### Expected behavior
This is the expected behavior
```csharp
this.obj10 = global::WinRT.CastExtensions.As(target);
this.obj10RefreshRequested = (global::Microsoft.UI.Xaml.Controls.RefreshContainer p0, global::Microsoft.UI.Xaml.Controls.RefreshRequestedEventArgs p1) =>
{
if (this.dataRoot != null)
{
this.dataRoot.ViewModel.RetrieveLatest();
}
};
```
### Screenshots
_No response_
### NuGet package version
2.3.2
### Windows version
Windows 11 (25H2): Build 26200
### Additional context
_No response_
Contributor guide
Research direction
Start with the C# page and ViewModel binding shown in the reproduction, then inspect the generated code containing the RefreshContainer handler and nested null checks. Compare the generated output with the expected behavior, preserving required checks while omitting checks for non-nullable objects and clearly non-nullable value-type constants; done when the reproduction no longer emits those redundant checks.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- compilers, desktop
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100