From 05a61726a1afb60716cf67b4f383bbf46fca82f1 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 21 Apr 2026 21:12:29 -0700 Subject: [PATCH] Special-case 'string' in optimized DP setter codegen XamlBindingHelper.SetPropertyFromString does not handle 'null' or empty strings correctly, so for 'string' typed dependency properties, fall back to SetValue in those cases and only call the optimized helper for non-empty strings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../DependencyPropertyGenerator.Execute.cs | 65 +++- .../Test_DependencyPropertyGenerator.cs | 294 +++++++++++++++--- 2 files changed, 300 insertions(+), 59 deletions(-) diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/DependencyPropertyGenerator.Execute.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/DependencyPropertyGenerator.Execute.cs index 14da98575..bf5bdafcc 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/DependencyPropertyGenerator.Execute.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/DependencyPropertyGenerator.Execute.cs @@ -764,9 +764,29 @@ static string GetExpressionWithTrailingSpace(Accessibility accessibility) field = value; """, isMultiline: true); - // If an optimized 'XamlBindingHelper' method is available, use it directly - if (propertyInfo.XamlBindingHelperSetMethodName is string setMethodName) + // If the property is of type 'string', we need a special path. That is because 'XamlBindingHelper.SetPropertyFromString' + // doesn't work correctly for 'null' or empty strings, so we need to fall back to 'SetValue' in those cases. + if (propertyInfo.TypeName == "string") { + writer.Write($$""" + + if (value is null || value.Length == 0) + { + SetValue({{propertyInfo.PropertyName}}Property, value); + } + else + { + global::{{WellKnownTypeNames.XamlBindingHelper(propertyInfo.UseWindowsUIXaml)}}.SetPropertyFromString(this, {{propertyInfo.PropertyName}}Property, value); + } + + On{{propertyInfo.PropertyName}}Changed(value); + On{{propertyInfo.PropertyName}}Changed(__oldValue, value); + } + """, isMultiline: true); + } + else if (propertyInfo.XamlBindingHelperSetMethodName is string setMethodName) + { + // If an optimized 'XamlBindingHelper' method is available, use it directly writer.Write($$""" global::{{WellKnownTypeNames.XamlBindingHelper(propertyInfo.UseWindowsUIXaml)}}.{{setMethodName}}(this, {{propertyInfo.PropertyName}}Property, value); @@ -846,15 +866,42 @@ static string GetExpressionWithTrailingSpace(Accessibility accessibility) return __unboxedValue; } - {{GetExpressionWithTrailingSpace(propertyInfo.SetterAccessibility)}}set - { - On{{propertyInfo.PropertyName}}Set(ref value); + """, isMultiline: true); - global::{{WellKnownTypeNames.XamlBindingHelper(propertyInfo.UseWindowsUIXaml)}}.{{setMethodName}}(this, {{propertyInfo.PropertyName}}Property, value); + // For 'string' properties, we need a specialized path (see comment in the local caching branch above) + if (propertyInfo.TypeName == "string") + { + writer.WriteLine($$""" + {{GetExpressionWithTrailingSpace(propertyInfo.SetterAccessibility)}}set + { + On{{propertyInfo.PropertyName}}Set(ref value); - On{{propertyInfo.PropertyName}}Changed(value); - } - """, isMultiline: true); + if (value is null || value.Length == 0) + { + SetValue({{propertyInfo.PropertyName}}Property, value); + } + else + { + global::{{WellKnownTypeNames.XamlBindingHelper(propertyInfo.UseWindowsUIXaml)}}.SetPropertyFromString(this, {{propertyInfo.PropertyName}}Property, value); + } + + On{{propertyInfo.PropertyName}}Changed(value); + } + """, isMultiline: true); + } + else + { + writer.WriteLine($$""" + {{GetExpressionWithTrailingSpace(propertyInfo.SetterAccessibility)}}set + { + On{{propertyInfo.PropertyName}}Set(ref value); + + global::{{WellKnownTypeNames.XamlBindingHelper(propertyInfo.UseWindowsUIXaml)}}.{{setMethodName}}(this, {{propertyInfo.PropertyName}}Property, value); + + On{{propertyInfo.PropertyName}}Changed(value); + } + """, isMultiline: true); + } } else { diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_DependencyPropertyGenerator.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_DependencyPropertyGenerator.cs index de5209666..c56ca9688 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_DependencyPropertyGenerator.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_DependencyPropertyGenerator.cs @@ -1017,7 +1017,14 @@ public partial string? Name { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -1637,7 +1644,14 @@ public partial string? Name field = value; - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); OnNameChanged(__oldValue, value); @@ -1758,7 +1772,14 @@ public partial string? Name { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -1870,7 +1891,14 @@ public required partial string Name { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -1987,7 +2015,14 @@ partial class MyControl { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -2099,7 +2134,14 @@ public virtual partial string Name { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -2218,7 +2260,14 @@ partial class MyControl { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -2330,7 +2379,14 @@ private set { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -2455,7 +2511,14 @@ public partial string? FirstName { OnFirstNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(FirstNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + } OnFirstNameChanged(value); } @@ -2483,7 +2546,14 @@ public partial string? LastName { OnLastNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(LastNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + } OnLastNameChanged(value); } @@ -2650,7 +2720,14 @@ public partial string? FirstName { OnFirstNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(FirstNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + } OnFirstNameChanged(value); } @@ -2678,7 +2755,14 @@ public partial string? LastName { OnLastNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(LastNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + } OnLastNameChanged(value); } @@ -2895,7 +2979,14 @@ public partial string? FirstName { OnFirstNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(FirstNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + } OnFirstNameChanged(value); } @@ -2923,7 +3014,14 @@ public partial string? LastName { OnLastNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(LastNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + } OnLastNameChanged(value); } @@ -3156,7 +3254,14 @@ public partial string? FirstName { OnFirstNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(FirstNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + } OnFirstNameChanged(value); } @@ -3184,7 +3289,14 @@ public partial string? LastName { OnLastNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(LastNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + } OnLastNameChanged(value); } @@ -3431,7 +3543,14 @@ public partial string? FirstName { OnFirstNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(FirstNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, FirstNameProperty, value); + } OnFirstNameChanged(value); } @@ -3459,7 +3578,14 @@ public partial string? LastName { OnLastNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(LastNameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, LastNameProperty, value); + } OnLastNameChanged(value); } @@ -3932,7 +4058,14 @@ public partial string? Name { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -4048,18 +4181,32 @@ public void SingleProperty_MultipleTypes_WithNoCaching_DefaultValueIsOptimized( string? typeDefinition = "", string? setMethodName = null) { - // Compute the setter body and partial method block based on whether the optimization is used - string setterBody = setMethodName is not null - ? $""" - global::Windows.UI.Xaml.Markup.XamlBindingHelper.{setMethodName}(this, NameProperty, value) - """ - : """ - object? __boxedValue = value; - - OnNameSet(ref __boxedValue); - - SetValue(NameProperty, __boxedValue) - """; + // Compute the setter body and partial method block based on whether the optimization is used. + // The 'string' type needs a special path, since 'XamlBindingHelper.SetPropertyFromString' doesn't + // handle 'null' or empty strings correctly, so we need to fall back to 'SetValue' in those cases. + string setterBody = setMethodName switch + { + "SetPropertyFromString" => """ + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } + """, + not null => $""" + global::Windows.UI.Xaml.Markup.XamlBindingHelper.{setMethodName}(this, NameProperty, value); + """, + _ => """ + object? __boxedValue = value; + + OnNameSet(ref __boxedValue); + + SetValue(NameProperty, __boxedValue); + """ + }; string source = $$""" using System; @@ -4124,7 +4271,7 @@ public partial {{propertyType}} Name { OnNameSet(ref value); - {{setterBody}}; + {{setterBody}} OnNameChanged(value); } @@ -4266,7 +4413,14 @@ public partial string? Name { OnNameSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NameProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NameProperty, value); + } OnNameChanged(value); } @@ -4454,18 +4608,30 @@ public void SingleProperty_WithCustomMetadataType_WithNoCaching( string propertyMetadata, string? setMethodName = null) { - // Compute the setter body and partial method block based on whether the optimization is used - string setterBody = setMethodName is not null - ? $""" - global::Windows.UI.Xaml.Markup.XamlBindingHelper.{setMethodName}(this, IsSelectedProperty, value) - """ - : """ - object? __boxedValue = value; - - OnIsSelectedSet(ref __boxedValue); - - SetValue(IsSelectedProperty, __boxedValue) - """; + // Compute the setter body (see 'SingleProperty_MultipleTypes_WithNoCaching_DefaultValueIsOptimized' for context on the 'string' case) + string setterBody = setMethodName switch + { + "SetPropertyFromString" => """ + if (value is null || value.Length == 0) + { + SetValue(IsSelectedProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, IsSelectedProperty, value); + } + """, + not null => $""" + global::Windows.UI.Xaml.Markup.XamlBindingHelper.{setMethodName}(this, IsSelectedProperty, value); + """, + _ => """ + object? __boxedValue = value; + + OnIsSelectedSet(ref __boxedValue); + + SetValue(IsSelectedProperty, __boxedValue); + """ + }; string source = $$""" using CommunityToolkit.WinUI; @@ -4524,7 +4690,7 @@ public partial {{generatedDeclaredType}} IsSelected { OnIsSelectedSet(ref value); - {{setterBody}}; + {{setterBody}} OnIsSelectedChanged(value); } @@ -4788,7 +4954,14 @@ public partial string? Number { OnNumberSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NumberProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + } OnNumberChanged(value); } @@ -4911,7 +5084,14 @@ public partial string? Number { OnNumberSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NumberProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + } OnNumberChanged(value); } @@ -5034,7 +5214,14 @@ public partial string? Number { OnNumberSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NumberProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + } OnNumberChanged(value); } @@ -5167,7 +5354,14 @@ public partial string? Number { OnNumberSet(ref value); - global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + if (value is null || value.Length == 0) + { + SetValue(NumberProperty, value); + } + else + { + global::Windows.UI.Xaml.Markup.XamlBindingHelper.SetPropertyFromString(this, NumberProperty, value); + } OnNumberChanged(value); }