microsoft / microsoft/microsoft-ui-xaml

XAML Compiler source generator creates unnecessary null checks for non-nullable objects

Open
#11,487 2 comments 0 reactions 0 assignees View on GitHub
area-Binding area-XamlCompiler bug team-Core
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.