From 92185e098a9234dce022929ebddb7e40550d0423 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 21 Jun 2023 00:55:26 +0200 Subject: [PATCH 1/2] Escape annotated fields using keyword identifiers --- .../ObservablePropertyGenerator.Execute.cs | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs b/src/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs index ec07e9f1..855dce1f 100644 --- a/src/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs +++ b/src/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs @@ -795,13 +795,33 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf // Get the property type syntax TypeSyntax propertyType = IdentifierName(propertyInfo.TypeNameWithNullabilityAnnotations); + string getterFieldIdentifierName; + ExpressionSyntax getterFieldExpression; + ExpressionSyntax setterFieldExpression; + // In case the backing field is exactly named "value", we need to add the "this." prefix to ensure that comparisons and assignments // with it in the generated setter body are executed correctly and without conflicts with the implicit value parameter. - ExpressionSyntax fieldExpression = propertyInfo.FieldName switch + if (propertyInfo.FieldName == "value") { - "value" => MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, ThisExpression(), IdentifierName("value")), - string name => IdentifierName(name) - }; + // We only need to add "this." when referencing the field in the setter (getter and XML docs are not ambiguous) + getterFieldIdentifierName = "value"; + getterFieldExpression = IdentifierName(getterFieldIdentifierName); + setterFieldExpression = MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, ThisExpression(), (IdentifierNameSyntax)getterFieldExpression); + } + else if (SyntaxFacts.GetKeywordKind(propertyInfo.FieldName) != SyntaxKind.None || + SyntaxFacts.GetContextualKeywordKind(propertyInfo.FieldName) != SyntaxKind.None) + { + // If the identifier for the field could potentially be a keyword, we must escape it. + // This usually happens if the annotated field was escaped as well (eg. "@event"). + // In this case, we must always escape the identifier, in all cases. + getterFieldIdentifierName = $"@{propertyInfo.FieldName}"; + getterFieldExpression = setterFieldExpression = IdentifierName(getterFieldIdentifierName); + } + else + { + getterFieldIdentifierName = propertyInfo.FieldName; + getterFieldExpression = setterFieldExpression = IdentifierName(getterFieldIdentifierName); + } if (propertyInfo.NotifyPropertyChangedRecipients || propertyInfo.IsOldPropertyValueDirectlyReferenced) { @@ -813,7 +833,7 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf VariableDeclaration(propertyType) .AddVariables( VariableDeclarator(Identifier("__oldValue")) - .WithInitializer(EqualsValueClause(fieldExpression))))); + .WithInitializer(EqualsValueClause(setterFieldExpression))))); } // Add the OnPropertyChanging() call first: @@ -863,7 +883,7 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf ExpressionStatement( AssignmentExpression( SyntaxKind.SimpleAssignmentExpression, - fieldExpression, + setterFieldExpression, IdentifierName("value")))); // If validation is requested, add a call to ValidateProperty: @@ -959,7 +979,7 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf IdentifierName("Default")), IdentifierName("Equals"))) .AddArgumentListArguments( - Argument(fieldExpression), + Argument(setterFieldExpression), Argument(IdentifierName("value")))), Block(setterStatements.AsEnumerable())); @@ -1009,13 +1029,13 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf .AddArgumentListArguments( AttributeArgument(LiteralExpression(SyntaxKind.StringLiteralExpression, Literal(typeof(ObservablePropertyGenerator).FullName))), AttributeArgument(LiteralExpression(SyntaxKind.StringLiteralExpression, Literal(typeof(ObservablePropertyGenerator).Assembly.GetName().Version.ToString())))))) - .WithOpenBracketToken(Token(TriviaList(Comment($"/// ")), SyntaxKind.OpenBracketToken, TriviaList())), + .WithOpenBracketToken(Token(TriviaList(Comment($"/// ")), SyntaxKind.OpenBracketToken, TriviaList())), AttributeList(SingletonSeparatedList(Attribute(IdentifierName("global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage"))))) .AddAttributeLists(forwardedAttributes.ToArray()) .AddModifiers(Token(SyntaxKind.PublicKeyword)) .AddAccessorListAccessors( AccessorDeclaration(SyntaxKind.GetAccessorDeclaration) - .WithExpressionBody(ArrowExpressionClause(IdentifierName(propertyInfo.FieldName))) + .WithExpressionBody(ArrowExpressionClause(getterFieldExpression)) .WithSemicolonToken(Token(SyntaxKind.SemicolonToken)), setAccessor); } From 84476ced1818252a0fa039765609a9db4cd08deb Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 21 Jun 2023 00:55:36 +0200 Subject: [PATCH 2/2] Add unit tests for new field escaping logic --- .../Test_SourceGeneratorsCodegen.cs | 307 ++++++++++++++++++ 1 file changed, 307 insertions(+) diff --git a/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsCodegen.cs b/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsCodegen.cs index bad92b09..236d6833 100644 --- a/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsCodegen.cs +++ b/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsCodegen.cs @@ -2060,6 +2060,313 @@ partial class MyViewModel VerifyGenerateSources(source, new[] { new RelayCommandGenerator() }, ("MyViewModel.Test.g.cs", result)); } + [TestMethod] + public void ObservableProperty_AnnotatedFieldHasValueIdentifier() + { + string source = """ + using CommunityToolkit.Mvvm.ComponentModel; + + namespace MyApp; + + partial class MyViewModel : ObservableObject + { + [ObservableProperty] + double value; + } + """; + + string result = """ + // + #pragma warning disable + #nullable enable + namespace MyApp + { + /// + partial class MyViewModel + { + /// + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] + public double Value + { + get => value; + set + { + if (!global::System.Collections.Generic.EqualityComparer.Default.Equals(this.value, value)) + { + OnValueChanging(value); + OnValueChanging(default, value); + OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Value); + this.value = value; + OnValueChanged(value); + OnValueChanged(default, value); + OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Value); + } + } + } + + /// Executes the logic for when is changing. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanging(double value); + /// Executes the logic for when is changing. + /// The previous property value that is being replaced. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanging(double oldValue, double newValue); + /// Executes the logic for when just changed. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanged(double value); + /// Executes the logic for when just changed. + /// The previous property value that was replaced. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanged(double oldValue, double newValue); + } + } + """; + + VerifyGenerateSources(source, new[] { new ObservablePropertyGenerator() }, ("MyApp.MyViewModel.g.cs", result)); + } + + [TestMethod] + public void ObservableProperty_AnnotatedFieldHasValueIdentifier_WithChangedMethods() + { + string source = """ + using CommunityToolkit.Mvvm.ComponentModel; + + namespace MyApp; + + partial class MyViewModel : ObservableObject + { + [ObservableProperty] + double value; + + partial void OnValueChanged(double oldValue, double NewValue) + { + } + } + """; + + string result = """ + // + #pragma warning disable + #nullable enable + namespace MyApp + { + /// + partial class MyViewModel + { + /// + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] + public double Value + { + get => value; + set + { + if (!global::System.Collections.Generic.EqualityComparer.Default.Equals(this.value, value)) + { + double __oldValue = this.value; + OnValueChanging(value); + OnValueChanging(__oldValue, value); + OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Value); + this.value = value; + OnValueChanged(value); + OnValueChanged(__oldValue, value); + OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Value); + } + } + } + + /// Executes the logic for when is changing. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanging(double value); + /// Executes the logic for when is changing. + /// The previous property value that is being replaced. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanging(double oldValue, double newValue); + /// Executes the logic for when just changed. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanged(double value); + /// Executes the logic for when just changed. + /// The previous property value that was replaced. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnValueChanged(double oldValue, double newValue); + } + } + """; + + VerifyGenerateSources(source, new[] { new ObservablePropertyGenerator() }, ("MyApp.MyViewModel.g.cs", result)); + } + + // See https://github.com/CommunityToolkit/dotnet/issues/710 + [TestMethod] + public void ObservableProperty_AnnotatedFieldWithEscapedIdentifier() + { + string source = """ + using CommunityToolkit.Mvvm.ComponentModel; + + namespace MyApp; + + partial class MyViewModel : ObservableObject + { + [ObservableProperty] + double @event; + } + """; + + string result = """ + // + #pragma warning disable + #nullable enable + namespace MyApp + { + /// + partial class MyViewModel + { + /// + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] + public double Event + { + get => @event; + set + { + if (!global::System.Collections.Generic.EqualityComparer.Default.Equals(@event, value)) + { + OnEventChanging(value); + OnEventChanging(default, value); + OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Event); + @event = value; + OnEventChanged(value); + OnEventChanged(default, value); + OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Event); + } + } + } + + /// Executes the logic for when is changing. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnEventChanging(double value); + /// Executes the logic for when is changing. + /// The previous property value that is being replaced. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnEventChanging(double oldValue, double newValue); + /// Executes the logic for when just changed. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnEventChanged(double value); + /// Executes the logic for when just changed. + /// The previous property value that was replaced. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnEventChanged(double oldValue, double newValue); + } + } + """; + + VerifyGenerateSources(source, new[] { new ObservablePropertyGenerator() }, ("MyApp.MyViewModel.g.cs", result)); + } + + [TestMethod] + public void ObservableProperty_AnnotatedFieldWithEscapedIdentifier_WithChangedMethods() + { + string source = """ + using CommunityToolkit.Mvvm.ComponentModel; + + namespace MyApp; + + partial class MyViewModel : ObservableObject + { + [ObservableProperty] + double @object; + + partial void OnObjectChanged(object oldValue, object NewValue) + { + } + } + """; + + string result = """ + // + #pragma warning disable + #nullable enable + namespace MyApp + { + /// + partial class MyViewModel + { + /// + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] + public double Object + { + get => @object; + set + { + if (!global::System.Collections.Generic.EqualityComparer.Default.Equals(@object, value)) + { + double __oldValue = @object; + OnObjectChanging(value); + OnObjectChanging(__oldValue, value); + OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Object); + @object = value; + OnObjectChanged(value); + OnObjectChanged(__oldValue, value); + OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Object); + } + } + } + + /// Executes the logic for when is changing. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnObjectChanging(double value); + /// Executes the logic for when is changing. + /// The previous property value that is being replaced. + /// The new property value being set. + /// This method is invoked right before the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnObjectChanging(double oldValue, double newValue); + /// Executes the logic for when just changed. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnObjectChanged(double value); + /// Executes the logic for when just changed. + /// The previous property value that was replaced. + /// The new property value that was set. + /// This method is invoked right after the value of is changed. + [global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", )] + partial void OnObjectChanged(double oldValue, double newValue); + } + } + """; + + VerifyGenerateSources(source, new[] { new ObservablePropertyGenerator() }, ("MyApp.MyViewModel.g.cs", result)); + } + /// /// Generates the requested sources ///