[XSG] simple support for required#30928
Merged
jfversluis merged 1 commit intonet10.0from Aug 27, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for C# 11's required properties and fields in XAML Source Generation (XSG). It enables the XAML compiler to detect required members on types and ensure they are properly initialized during object creation, while providing appropriate error messages when required properties are missing.
- Adds detection and validation of
requiredproperties and fields during XAML compilation - Implements object initialization syntax to set required members during construction
- Provides diagnostic error reporting when required properties are not found in XAML
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Controls/src/Xaml/XamlNode.cs | Adds helper extension methods for dictionary lookups by property name |
| src/Controls/src/SourceGen/Visitors/CreateValuesVisitor.cs | Implements required property detection, validation, and object initialization syntax generation |
Comments suppressed due to low confidence (1)
src/Controls/src/SourceGen/Visitors/CreateValuesVisitor.cs:195
- [nitpick] The variable name
requiredPropertiesAndFieldsis redundant since it's already clear from the type and context. Consider shortening torequiredMembersfor better readability.
List<(string name, ISymbol propOrField, ITypeSymbol propType, string propValue)> requiredPropertiesAndFields = [];
Comment on lines
+208
to
+209
| else if (!node.Properties.TryGetValue(req.Name, out propNode, out propXmlName)) | ||
| Context.ReportDiagnostic(Diagnostic.Create(Descriptors.XamlParserError, LocationCreate(Context.FilePath!, node, req.Name), $"Required field '{req.Name}' not found in {type}")); |
There was a problem hiding this comment.
The diagnostic error is reported but execution continues, potentially causing null reference exceptions when propNode is used later. Consider adding a continue statement or early return after reporting the diagnostic.
Suggested change
| else if (!node.Properties.TryGetValue(req.Name, out propNode, out propXmlName)) | |
| Context.ReportDiagnostic(Diagnostic.Create(Descriptors.XamlParserError, LocationCreate(Context.FilePath!, node, req.Name), $"Required field '{req.Name}' not found in {type}")); | |
| else if (!node.Properties.TryGetValue(req.Name, out propNode, out propXmlName)) | |
| { | |
| Context.ReportDiagnostic(Diagnostic.Create(Descriptors.XamlParserError, LocationCreate(Context.FilePath!, node, req.Name), $"Required field '{req.Name}' not found in {type}")); | |
| continue; | |
| } |
Comment on lines
+215
to
+226
| var visitor = new SetPropertiesVisitor(Context); | ||
| var children = node.Properties.Values.ToList(); | ||
| children.AddRange(node.CollectionItems); | ||
| foreach (var cn in children) | ||
| { | ||
| if (cn is not ElementNode en) | ||
| continue; | ||
| foreach (var n in en.Properties.Values.ToList()) | ||
| n.Accept(visitor, cn); | ||
| foreach (var n in en.CollectionItems) | ||
| n.Accept(visitor, cn); | ||
| } |
There was a problem hiding this comment.
A new SetPropertiesVisitor is created inside the loop for each required property, but the visitor traversal logic appears to be the same for all properties. Consider moving the visitor creation and traversal outside the loop to avoid redundant work.
Suggested change
| var visitor = new SetPropertiesVisitor(Context); | |
| var children = node.Properties.Values.ToList(); | |
| children.AddRange(node.CollectionItems); | |
| foreach (var cn in children) | |
| { | |
| if (cn is not ElementNode en) | |
| continue; | |
| foreach (var n in en.Properties.Values.ToList()) | |
| n.Accept(visitor, cn); | |
| foreach (var n in en.CollectionItems) | |
| n.Accept(visitor, cn); | |
| } |
| foreach (var (name, propOrField, propType, propValue) in requiredPropertiesAndFields) | ||
| Writer.WriteLine($"{name} = ({propType.ToFQDisplayString()}){propValue},"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The indentation and spacing around the closing brace appears inconsistent with the surrounding code. The else statement on line 286 should be properly aligned.
Suggested change
| } | |
| } |
689165d to
b5c727c
Compare
b54a2f5 to
7ffd7a9
Compare
simonrozsival
previously approved these changes
Aug 25, 2025
Member
simonrozsival
left a comment
There was a problem hiding this comment.
I think this is good to go as is. We might want to consider unifying the inflation of the required fields and properties values with the constructor or factory method parameters values, esp. if we try to allow more complicated constructs in the values (such as StaticResourceExtension).
support for simple values for required props and fields. as we do not build a depency tree, complex constructs (like referencing a staticresource) might not work as the info isn't available. The general advice is to _not_ use `required` fields and props, or assign them in the most direct manner - fixes #30491
7ffd7a9 to
2de51bf
Compare
jfversluis
approved these changes
Aug 27, 2025
MartyIX
reviewed
Aug 27, 2025
Comment on lines
+22
to
+52
| .WithAdditionalSource( | ||
| """ | ||
| using System; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace Microsoft.Maui.Controls.Xaml.UnitTests; | ||
|
|
||
| [XamlProcessing(XamlInflator.SourceGen)] | ||
| public partial class Required : ContentPage | ||
| { | ||
| public RequiredRandomSelector Selector { get; set; } | ||
| public RequiredPerson Person { get; set; } | ||
| public Required() => InitializeComponent(); | ||
| } | ||
|
|
||
| public class RequiredRandomSelector : DataTemplateSelector | ||
| { | ||
| public /*required*/ Controls.DataTemplate Template1 { get; set; } | ||
| public required Controls.DataTemplate Template2 { get; set; } | ||
|
|
||
| protected override Controls.DataTemplate OnSelectTemplate(object item, BindableObject container) => new Random().Next(2) == 0 ? Template1 : Template2; | ||
| } | ||
|
|
||
| public class RequiredPerson | ||
| { | ||
| public required string Name { get; set; } | ||
|
|
||
| public override string ToString() | ||
| => Name; | ||
| } | ||
| """) |
Contributor
There was a problem hiding this comment.
This can be aligned to the right and it will behave the same way as now:
Suggested change
| .WithAdditionalSource( | |
| """ | |
| using System; | |
| using NUnit.Framework; | |
| namespace Microsoft.Maui.Controls.Xaml.UnitTests; | |
| [XamlProcessing(XamlInflator.SourceGen)] | |
| public partial class Required : ContentPage | |
| { | |
| public RequiredRandomSelector Selector { get; set; } | |
| public RequiredPerson Person { get; set; } | |
| public Required() => InitializeComponent(); | |
| } | |
| public class RequiredRandomSelector : DataTemplateSelector | |
| { | |
| public /*required*/ Controls.DataTemplate Template1 { get; set; } | |
| public required Controls.DataTemplate Template2 { get; set; } | |
| protected override Controls.DataTemplate OnSelectTemplate(object item, BindableObject container) => new Random().Next(2) == 0 ? Template1 : Template2; | |
| } | |
| public class RequiredPerson | |
| { | |
| public required string Name { get; set; } | |
| public override string ToString() | |
| => Name; | |
| } | |
| """) | |
| .WithAdditionalSource( | |
| """ | |
| using System; | |
| using NUnit.Framework; | |
| namespace Microsoft.Maui.Controls.Xaml.UnitTests; | |
| [XamlProcessing(XamlInflator.SourceGen)] | |
| public partial class Required : ContentPage | |
| { | |
| public RequiredRandomSelector Selector { get; set; } | |
| public RequiredPerson Person { get; set; } | |
| public Required() => InitializeComponent(); | |
| } | |
| public class RequiredRandomSelector : DataTemplateSelector | |
| { | |
| public /*required*/ Controls.DataTemplate Template1 { get; set; } | |
| public required Controls.DataTemplate Template2 { get; set; } | |
| protected override Controls.DataTemplate OnSelectTemplate(object item, BindableObject container) => new Random().Next(2) == 0 ? Template1 : Template2; | |
| } | |
| public class RequiredPerson | |
| { | |
| public required string Name { get; set; } | |
| public override string ToString() | |
| => Name; | |
| } | |
| """) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
support for simple values for required props and fields. as we do not build a depency tree, complex constructs (like referencing a staticresource) might not work as the info isn't available.
The general advice is to not use
requiredfields and props, or assign them in the most direct mannerrequiredworking #30491