-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[X] Add warning for duplicate property assignments in XAML #32654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a warning diagnostic (XC0067/MAUIX2006) to detect when XAML properties are assigned multiple times, addressing issue #3059. The implementation is generalized to check all property assignments, not just ContentProperty.
Key changes:
- Added duplicate property detection in both Build.Tasks and SourceGen XAML processing
- Introduced new warning diagnostic MAUIX2006 with resource strings for error messages
- Added comprehensive unit tests covering duplicate property scenarios and proper NoWarn suppression
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/tests/Xaml.UnitTests/Issues/Issue3059b.xaml.cs | New test class verifying duplicate property assignment behavior (Text, Placeholder) - missing XamlInflator constructor |
| src/Controls/tests/Xaml.UnitTests/Issues/Issue3059b.xaml | Test XAML with duplicate property assignments for validation |
| src/Controls/tests/Xaml.UnitTests/Issues/Issue3059.xaml.cs | New test class verifying ContentProperty duplicate behavior - missing XamlInflator constructor |
| src/Controls/tests/Xaml.UnitTests/Issues/Issue3059.xaml | Test XAML with multiple children in Border to test ContentProperty duplication |
| src/Controls/tests/Xaml.UnitTests/Issues/Gh5095.rt.xaml.cs | Updated diagnostic count assertion (1→2) to account for new duplicate property warning |
| src/Controls/tests/Xaml.UnitTests/Controls.Xaml.UnitTests.csproj | Added NoWarn suppressions for test files and globally suppressed CS1030 |
| src/Controls/tests/SourceGen.UnitTests/InitializeComponent/MultipleChildrenWarningTests.cs | Comprehensive unit tests for duplicate property warning in SourceGen |
| src/Controls/src/SourceGen/xlf/MauiGResources.Designer.cs | Added resource string accessors for new diagnostic messages |
| src/Controls/src/SourceGen/Visitors/SetPropertiesVisitor.cs | Implemented duplicate property tracking and warning emission logic |
| src/Controls/src/SourceGen/MauiGResources.resx | Added resource strings for warning title and message |
| src/Controls/src/SourceGen/InitializeComponentCodeWriter.cs | Minor formatting adjustment to raw string literal |
| src/Controls/src/SourceGen/Descriptors.cs | Added MAUIX2006 diagnostic descriptor for duplicate properties |
| src/Controls/src/SourceGen/AnalyzerReleases.Unshipped.md | Documented new MAUIX2006 warning in analyzer releases |
| src/Controls/src/Build.Tasks/SetPropertiesVisitor.cs | Implemented duplicate property tracking for Build.Tasks (with formatting fix) |
| src/Controls/src/Build.Tasks/ErrorMessages.resx | Added error message resource for Build.Tasks |
| src/Controls/src/Build.Tasks/BuildException.cs | Added XC0067 exception code for duplicate property warnings |
Files not reviewed (1)
- src/Controls/src/SourceGen/xlf/MauiGResources.Designer.cs: Language not supported
| public static BuildExceptionCode NamescopeDuplicate = new BuildExceptionCode("XC", 0064, nameof(NamescopeDuplicate), ""); | ||
| public static BuildExceptionCode ContentPropertyAttributeMissing = new BuildExceptionCode("XC", 0065, nameof(ContentPropertyAttributeMissing), ""); | ||
| public static BuildExceptionCode InvalidXaml = new BuildExceptionCode("XC", 0066, nameof(InvalidXaml), ""); | ||
| public static BuildExceptionCode MultipleChildrenInContentProperty = new BuildExceptionCode("XC", 0067, nameof(MultipleChildrenInContentProperty), ""); //warning |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception code constant name MultipleChildrenInContentProperty is misleading since this diagnostic now detects duplicate assignments for any property, not just ContentProperty. Consider renaming to something more accurate like DuplicatePropertyAssignment or PropertySetMultipleTimes to better reflect its general-purpose nature.
| public static BuildExceptionCode MultipleChildrenInContentProperty = new BuildExceptionCode("XC", 0067, nameof(MultipleChildrenInContentProperty), ""); //warning | |
| public static BuildExceptionCode DuplicatePropertyAssignment = new BuildExceptionCode("XC", 0067, nameof(DuplicatePropertyAssignment), ""); //warning |
| <data name="MultipleChildrenInContentProperty" xml:space="preserve"> | ||
| <value>Property '{0}' is being set multiple times. Only the last value will be used.</value> | ||
| <comment>0 is property name (e.g. "Border.Content" or "Label.Text")</comment> |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message resource key MultipleChildrenInContentProperty is misleading since this diagnostic now detects duplicate assignments for any property, not just ContentProperty. Consider renaming to something more accurate like DuplicatePropertyAssignment or PropertySetMultipleTimes to better reflect its general-purpose nature.
| <data name="MultipleChildrenInContentProperty" xml:space="preserve"> | |
| <value>Property '{0}' is being set multiple times. Only the last value will be used.</value> | |
| <comment>0 is property name (e.g. "Border.Content" or "Label.Text")</comment> | |
| <data name="DuplicatePropertyAssignment" xml:space="preserve"> | |
| <value>Property '{0}' is being set multiple times. Only the last value will be used.</value> | |
| <comment>0 is the property name (e.g. "Border.Content" or "Label.Text")</comment> |
| <NoWarn>$(NoWarn);0672;0219;0414;CS0436;CS0618;CS1030</NoWarn> | ||
| <WarningsNotAsErrors>$(WarningsNotAsErrors);XC0618;XC0022;XC0023;XC0025;XC0045;XC0067;MAUID1000;MAUIX2005</WarningsNotAsErrors> |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CS1030 is "#warning directive" which means files are generating #warning directives. Globally suppressing this warning may hide legitimate warnings. Consider investigating why CS1030 warnings are occurring and fixing the root cause rather than suppressing them globally.
If the warnings are coming from generated code or intentional #warning directives in test files, document this in a comment explaining why CS1030 is suppressed.
| <NoWarn>$(NoWarn);0672;0219;0414;CS0436;CS0618;CS1030</NoWarn> | |
| <WarningsNotAsErrors>$(WarningsNotAsErrors);XC0618;XC0022;XC0023;XC0025;XC0045;XC0067;MAUID1000;MAUIX2005</WarningsNotAsErrors> | |
| <!-- Suppress CS1030 ("#warning directive") because this test project may intentionally include #warning directives |
| MAUIX2003 | XamlInflation | Error | Descriptors | ||
| MAUIX2004 | XamlInflation | Error | Descriptors | ||
| MAUIX2005 | XamlInflation | Warning | Descriptors | ||
| MAUIX2006 | XamlInflation | Warning | PropertySetMultipleTimes |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description "PropertySetMultipleTimes" doesn't match the misleading internal identifier name MultipleChildrenInContentProperty. While the description is accurate for the feature, it should ideally align with the descriptor name used in code. Consider updating the descriptor name in code to match this more accurate description.
| MAUIX2006 | XamlInflation | Warning | PropertySetMultipleTimes | |
| MAUIX2006 | XamlInflation | Warning | MultipleChildrenInContentProperty |
| var name = new XmlName(node.NamespaceURI, contentProperty); | ||
| if (skips.Contains(name)) | ||
| return; | ||
| if (parentNode is ElementNode node1 && node1.SkipProperties.Contains(propertyName)) |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 182 checks node1.SkipProperties.Contains(propertyName) but propertyName is still the old value from the outer scope, not the newly assigned name variable from line 179. This should be checking name instead of propertyName.
Change to:
if (parentNode is ElementNode node1 && node1.SkipProperties.Contains(name))
return;| if (parentNode is ElementNode node1 && node1.SkipProperties.Contains(propertyName)) | |
| if (parentNode is ElementNode node1 && node1.SkipProperties.Contains(name)) |
| var name = new XmlName(node.NamespaceURI, contentProperty); | ||
| if (skips.Contains(name)) | ||
| return; | ||
| if (parentNode is ElementNode node2 && node2.SkipProperties.Contains(propertyName)) |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 175 checks node2.SkipProperties.Contains(propertyName) but propertyName is still the old value from the outer scope, not the newly assigned name variable from line 172. This should be checking name instead of propertyName.
Change to:
if (parentNode is ElementNode node2 && node2.SkipProperties.Contains(name))
return;| if (parentNode is ElementNode node2 && node2.SkipProperties.Contains(propertyName)) | |
| if (parentNode is ElementNode node2 && node2.SkipProperties.Contains(name)) |
| { | ||
| InitializeComponent(); | ||
| } | ||
|
|
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test method expects Issue3059b to have a constructor that accepts XamlInflator, but the class only defines a parameterless constructor. The test will fail to compile because line 34 calls new Issue3059b(inflator) which doesn't exist.
Add a constructor overload:
public Issue3059b(XamlInflator inflator) : this(shouldInitialize: true, inflator)
{
}Note: This pattern is used in other test files (e.g., Bz40906.xaml.cs) where a similar XamlInflator parameter is passed to tests.
| public Issue3059b(XamlInflator inflator) : this() | |
| { | |
| } |
| <data name="MultipleChildrenInContentPropertyTitle" xml:space="preserve"> | ||
| <value>Property set multiple times</value> | ||
| </data> | ||
| <data name="MultipleChildrenInContentPropertyMessage" xml:space="preserve"> |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resource key MultipleChildrenInContentPropertyTitle is misleading since this diagnostic now detects duplicate assignments for any property, not just ContentProperty. Consider renaming to something more accurate like DuplicatePropertyAssignmentTitle or PropertySetMultipleTimesTitle to better reflect its general-purpose nature.
| <data name="MultipleChildrenInContentPropertyTitle" xml:space="preserve"> | |
| <value>Property set multiple times</value> | |
| </data> | |
| <data name="MultipleChildrenInContentPropertyMessage" xml:space="preserve"> | |
| <data name="DuplicatePropertyAssignmentTitle" xml:space="preserve"> | |
| <value>Property set multiple times</value> | |
| </data> | |
| <data name="DuplicatePropertyAssignmentMessage" xml:space="preserve"> |
f73e09a to
418703a
Compare
- Renamed MultipleChildrenInContentProperty to DuplicatePropertyAssignment across all files - Updated diagnostic IDs: XC0067 (Build.Tasks) and MAUIX2006 (SourceGen) - Renamed Issue3059 to Maui3059.rt (runtime XAML - no sourcegen/xamlC) - Added tests to verify MockSourceGenerator and MockCompiler report warnings correctly - Removed NoWarn suppression to allow warning verification in tests Addresses review comments on PR #32654
ca16410 to
95d81ee
Compare
Implements XC0067/MAUIX2006 warning to detect when XAML properties are set multiple times (e.g., multiple children in single-child content properties like Border.Content). This initial implementation detects duplicates for both explicit attribute assignments and implicit content properties.
Remove checks for explicit attribute duplicates (ValueNode) as the XAML parser already rejects these at parse time. Focus the warning on implicit content property assignments which the parser doesn't catch.
Avoid emitting warnings for properties that resolve to System.Object (unresolved generics) as we can't reliably determine if they're collections.
Rename from MultipleChildren to DuplicatePropertyAssignment to better reflect what the diagnostic actually detects. Update test names and comments accordingly.
Separate test files for runtime (.rt.xaml) and source generation: - Maui3059.xaml: Tests SourceGen warnings (MAUIX2006) - Maui3059.rt.xaml: Tests XamlC warnings (XC0067)
The duplicate property warning was incorrectly checking if the parent type was a collection, instead of checking if the property being set is a collection. This caused false positives for properties like: - VisualStateGroup.States (IList<VisualState>) - Style.Setters (IList<Setter>) - TabbedPage.Children (IList<Page>) Fixed by resolving the actual property type and checking if that type is a collection (implements IEnumerable and has Add method). Added test case to verify VisualStateGroup.States doesn't trigger warnings.
95d81ee to
1889047
Compare
The .rt.xaml file uses [XamlCompilation(Skip)] which means it only supports Runtime inflator. The test was using [Values] XamlInflator which parameterized the test to run with XamlC and SourceGen as well, causing failures with 'no code for XamlC/SourceGen generated' error. Fixed by removing the [Values] parameter and explicitly using XamlInflator.Runtime.
The CheckForDuplicateProperty was only called for ElementNode content properties, but not for ValueNode content properties (e.g., text content like '8' in XAML). This caused the Gh5095 test to only receive 1 diagnostic instead of 2. Now the duplicate check is also performed for ValueNode implicit content properties, allowing the warning MAUIX2006 to be emitted when both ElementNode and ValueNode children are set for a single-value content property.
The previous changes for XC0067 warning accidentally removed the HasExplicitConversion logic from PR #32780 which was needed for collection-to-collection assignment with explicit cast. This restores: - HasExplicitConversion method for detecting when explicit cast is needed - Call to HasExplicitConversion in CanSetValue and CanSet methods - Explicit cast generation in SetValue method Fixes the regression where Picker.ItemsSource with x:Static was using Add() instead of SetValue() with proper cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- src/Controls/src/SourceGen/xlf/MauiGResources.Designer.cs: Language not supported
| var noWarn = Context.ProjectItem.NoWarn; | ||
| bool shouldSuppress = !string.IsNullOrEmpty(noWarn) && | ||
| noWarn.Split(new[] { ',', ';', ' ' }) | ||
| .Select(code => code.Trim()) |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NoWarn parsing logic includes space (' ') as a delimiter, which could cause issues when combined with other delimiters. For example, NoWarn="67; 2006" would produce empty strings after splitting. The .Trim() call helps, but empty strings could still match warnings if not filtered. Consider adding .Where(code => !string.IsNullOrWhiteSpace(code)) before the .Any() check to filter out empty entries.
| .Select(code => code.Trim()) | |
| .Select(code => code.Trim()) | |
| .Where(code => !string.IsNullOrWhiteSpace(code)) |
| namespace Microsoft.Maui.Controls.Xaml.UnitTests; | ||
|
|
||
| // Note: This is a .rt.xaml file (runtime) which skips source generation and XamlC compilation | ||
| // This file tests that MockCompiler (XamlC) emits the XC0067 warning |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This comment has a typo: "XamlC" should be "XamlC compilation" or "XamlC Build.Tasks" for clarity. The term "XamlC" alone might be confusing as it doesn't immediately convey that it refers to the Build.Tasks compilation path.
| // This file tests that MockCompiler (XamlC) emits the XC0067 warning | |
| // This file tests that MockCompiler (XamlC compilation) emits the XC0067 warning |
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <ContentPage | ||
| xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| x:Class="Microsoft.Maui.Controls.Xaml.UnitTests.Issue3059"> | ||
| <Border> | ||
| <Label Text="First" /> | ||
| <Label Text="Second" /> | ||
| </Border> | ||
| </ContentPage> |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The test file naming is inconsistent. Both Issue3059.xaml and Maui3059.xaml exist, testing the same issue (#3059). The repository convention typically uses the prefix "Maui" for MAUI-specific issues (e.g., Maui3059) rather than "Issue". Consider standardizing on one naming pattern. Since this PR references issue #3059 which is a .NET MAUI issue, Maui3059 would be more consistent with the repository's current naming conventions.
|
|
||
| <!-- Suppress XC0067/MAUIX2006 (property set multiple times) for test files that intentionally test this scenario --> | ||
| <MauiXaml Update="Issues\Bz40906.xaml" NoWarn="67;2006" /> | ||
| <MauiXaml Update="Issues\Issue3059b.xaml" NoWarn="67;2006" /> |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file Issue3059b.xaml referenced in this NoWarn directive does not exist in the repository. This appears to be a typo or reference to a file that was never created. Consider removing this line or verifying if a different file name was intended.
| <MauiXaml Update="Issues\Issue3059b.xaml" NoWarn="67;2006" /> |
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!
Fixes #3059
Description of Change
Adds XC0067/MAUIX2006 warning diagnostic to detect when XAML implicit content properties are set multiple times (e.g.,
<Border><Label/><Label/></Border>).This implementation specifically targets the issue described in #3059: detecting multiple children in single-child content properties.
Key Changes
<Label Text="A" Text="B" />(XAML parser already rejects these at parse time)Style.Setters,VisualStateGroup.States) - no warnings when adding multiple items<Border><Label/><Label/></Border>→ warns aboutBorder.Content<Border><Border.Content><Label/><Label/></Border.Content></Border>→ warns aboutBorder.ContentImplementation Details
Visit(ElementNode)for implicit content propertiesAdd()method, so multiple children are expected and don't trigger warningsTesting
Issues Fixed
Fixes #3059