From e202a554e7c91739c8505afd2a2332647c5d7d36 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 24 May 2022 10:28:20 +0200 Subject: [PATCH 1/5] Forward all DataAnnotations attribute for [ObservableProperty] --- .../ObservablePropertyGenerator.Execute.cs | 15 +++++- .../Test_ObservablePropertyAttribute.cs | 51 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs index f8fb4a70..7f933d77 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs @@ -118,8 +118,19 @@ internal static class Execute continue; } - // Track the current validation attribute, if applicable - if (attributeData.AttributeClass?.InheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.ValidationAttribute") == true) + // Track the current attribute for forwarding if applicable. The following attributes are special cased: + // - Validation attributes (System.ComponentModel.DataAnnotations.ValidationAttribute) + // - Display attributes (System.ComponentModel.DataAnnotations.DisplayAttribute) + // - UI hint attributes(System.ComponentModel.DataAnnotations.UIHintAttribute) + // - Scaffold column attributes (System.ComponentModel.DataAnnotations.ScaffoldColumnAttribute) + // - Editable attributes (System.ComponentModel.DataAnnotations.EditableAttribute) + // - Key attributes (System.ComponentModel.DataAnnotations.KeyAttribute) + if (attributeData.AttributeClass?.InheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.ValidationAttribute") == true || + attributeData.AttributeClass?.HasOrInheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.UIHintAttribute") == true || + attributeData.AttributeClass?.HasOrInheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.ScaffoldColumnAttribute") == true || + attributeData.AttributeClass?.HasFullyQualifiedName("global::System.ComponentModel.DataAnnotations.DisplayAttribute") == true || + attributeData.AttributeClass?.HasFullyQualifiedName("global::System.ComponentModel.DataAnnotations.EditableAttribute") == true || + attributeData.AttributeClass?.HasFullyQualifiedName("global::System.ComponentModel.DataAnnotations.KeyAttribute") == true) { validationAttributes.Add(AttributeInfo.From(attributeData)); } diff --git a/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs b/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs index cb110b25..e75d9f59 100644 --- a/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs +++ b/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs @@ -658,6 +658,46 @@ public void Test_ObservableProperty_InheritedModelWithCommandUsingInheritedObser Assert.IsTrue(model.SaveCommand.CanExecute(null)); } + [TestMethod] + public void Test_ObservableProperty_ForwardsSpecialCasesDataAnnotationAttributes() + { + PropertyInfo propertyInfo = typeof(ModelWithAdditionalDataAnnotationAttributes).GetProperty(nameof(ModelWithAdditionalDataAnnotationAttributes.Name))!; + + DisplayAttribute? displayAttribute = (DisplayAttribute?)propertyInfo.GetCustomAttribute(typeof(DisplayAttribute)); + + Assert.IsNotNull(displayAttribute); + Assert.AreEqual(displayAttribute!.Name, "MyProperty"); + Assert.AreEqual(displayAttribute.ResourceType, typeof(List)); + Assert.AreEqual(displayAttribute.Prompt, "Foo bar baz"); + + KeyAttribute? keyAttribute = (KeyAttribute?)propertyInfo.GetCustomAttribute(typeof(KeyAttribute)); + + Assert.IsNotNull(keyAttribute); + + EditableAttribute? editableAttribute = (EditableAttribute?)propertyInfo.GetCustomAttribute(typeof(EditableAttribute)); + + Assert.IsNotNull(keyAttribute); + Assert.IsTrue(editableAttribute!.AllowEdit); + + UIHintAttribute? uiHintAttribute = (UIHintAttribute?)propertyInfo.GetCustomAttribute(typeof(UIHintAttribute)); + + Assert.IsNotNull(uiHintAttribute); + Assert.AreEqual(uiHintAttribute!.UIHint, "MyControl"); + Assert.AreEqual(uiHintAttribute.PresentationLayer, "WPF"); + Assert.AreEqual(uiHintAttribute.ControlParameters.Count, 3); + Assert.IsTrue(uiHintAttribute.ControlParameters.ContainsKey("Foo")); + Assert.IsTrue(uiHintAttribute.ControlParameters.ContainsKey("Bar")); + Assert.IsTrue(uiHintAttribute.ControlParameters.ContainsKey("Baz")); + Assert.AreEqual(uiHintAttribute.ControlParameters["Foo"], 42); + Assert.AreEqual(uiHintAttribute.ControlParameters["Bar"], 3.14); + Assert.AreEqual(uiHintAttribute.ControlParameters["Baz"], "Hello"); + + ScaffoldColumnAttribute? scaffoldColumnAttribute = (ScaffoldColumnAttribute?)propertyInfo.GetCustomAttribute(typeof(ScaffoldColumnAttribute)); + + Assert.IsNotNull(scaffoldColumnAttribute); + Assert.IsTrue(scaffoldColumnAttribute!.Scaffold); + } + public abstract partial class BaseViewModel : ObservableObject { public string? Content { get; set; } @@ -1066,4 +1106,15 @@ public override void Save() { } } + + public partial class ModelWithAdditionalDataAnnotationAttributes : ObservableValidator + { + [ObservableProperty] + [Display(Name = "MyProperty", ResourceType = typeof(List), Prompt = "Foo bar baz")] + [Key] + [Editable(true)] + [UIHint("MyControl", "WPF", new object[] { "Foo", 42, "Bar", 3.14, "Baz", "Hello" })] + [ScaffoldColumn(true)] + private string? name; + } } From 488a7628f3ae7230fd59e658cd5a668bf4ab8a43 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 24 May 2022 10:46:06 +0200 Subject: [PATCH 2/5] Add AlsoValidatePropertyAttribute type --- .../AlsoValidatePropertyAttribute.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 CommunityToolkit.Mvvm/ComponentModel/Attributes/AlsoValidatePropertyAttribute.cs diff --git a/CommunityToolkit.Mvvm/ComponentModel/Attributes/AlsoValidatePropertyAttribute.cs b/CommunityToolkit.Mvvm/ComponentModel/Attributes/AlsoValidatePropertyAttribute.cs new file mode 100644 index 00000000..c8be76a4 --- /dev/null +++ b/CommunityToolkit.Mvvm/ComponentModel/Attributes/AlsoValidatePropertyAttribute.cs @@ -0,0 +1,44 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; + +namespace CommunityToolkit.Mvvm.ComponentModel; + +/// +/// An attribute that can be used to support in generated properties, when applied to +/// fields contained in a type that is inheriting from and using any validation attributes. +/// When this attribute is used, the generated property setter will also call . +/// This allows generated properties to opt-in into validation behavior without having to fallback into a full explicit observable property. +/// +/// This attribute can be used as follows: +/// +/// partial class MyViewModel : ObservableValidator +/// { +/// [ObservableProperty] +/// [AlsoValidateProperty] +/// [Required] +/// [MinLength(2)] +/// private string username; +/// } +/// +/// +/// And with this, code analogous to this will be generated: +/// +/// partial class MyViewModel +/// { +/// [Required] +/// [MinLength(2)] +/// public string Username +/// { +/// get => username; +/// set => SetProperty(ref username, value, validate: true); +/// } +/// } +/// +/// +[AttributeUsage(AttributeTargets.Field, AllowMultiple = false, Inherited = false)] +public sealed class AlsoValidatePropertyAttribute : Attribute +{ +} From 171b89b0f0833d937c9144b344cb809e32e3a398 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 24 May 2022 10:46:19 +0200 Subject: [PATCH 3/5] Enable AlsoValidatePropertyAttribute generation --- .../AnalyzerReleases.Shipped.md | 1 + .../ComponentModel/Models/PropertyInfo.cs | 12 ++- .../ObservablePropertyGenerator.Execute.cs | 97 ++++++++++++++----- .../ObservablePropertyGenerator.cs | 5 +- .../Diagnostics/DiagnosticDescriptors.cs | 26 ++++- 5 files changed, 108 insertions(+), 33 deletions(-) diff --git a/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md b/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md index 8f3725fc..77414dba 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md +++ b/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md @@ -31,3 +31,4 @@ MVVMTK0021 | CommunityToolkit.Mvvm.SourceGenerators.ObservableRecipientGenerator MVVMTK0022 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error MVVMTK0023 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error MVVMTK0024 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error +MVVMTK0025 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error diff --git a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/Models/PropertyInfo.cs b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/Models/PropertyInfo.cs index 88851ff5..cd1ecde1 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/Models/PropertyInfo.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/Models/PropertyInfo.cs @@ -21,7 +21,8 @@ namespace CommunityToolkit.Mvvm.SourceGenerators.ComponentModel.Models; /// The sequence of property changed properties to notify. /// The sequence of commands to notify. /// Whether or not the generated property also broadcasts changes. -/// The sequence of validation attributes for the generated property. +/// Whether or not the generated property also validates its value. +/// The sequence of forwarded attributes for the generated property. internal sealed record PropertyInfo( string TypeNameWithNullabilityAnnotations, string FieldName, @@ -30,7 +31,8 @@ internal sealed record PropertyInfo( ImmutableArray PropertyChangedNames, ImmutableArray NotifiedCommandNames, bool AlsoBroadcastChange, - ImmutableArray ValidationAttributes) + bool AlsoValidateProperty, + ImmutableArray ForwardedAttributes) { /// /// An implementation for . @@ -47,7 +49,8 @@ protected override void AddToHashCode(ref HashCode hashCode, PropertyInfo obj) hashCode.AddRange(obj.PropertyChangedNames); hashCode.AddRange(obj.NotifiedCommandNames); hashCode.Add(obj.AlsoBroadcastChange); - hashCode.AddRange(obj.ValidationAttributes, AttributeInfo.Comparer.Default); + hashCode.Add(obj.AlsoValidateProperty); + hashCode.AddRange(obj.ForwardedAttributes, AttributeInfo.Comparer.Default); } /// @@ -61,7 +64,8 @@ protected override bool AreEqual(PropertyInfo x, PropertyInfo y) x.PropertyChangedNames.SequenceEqual(y.PropertyChangedNames) && x.NotifiedCommandNames.SequenceEqual(y.NotifiedCommandNames) && x.AlsoBroadcastChange == y.AlsoBroadcastChange && - x.ValidationAttributes.SequenceEqual(y.ValidationAttributes, AttributeInfo.Comparer.Default); + x.AlsoValidateProperty == y.AlsoValidateProperty && + x.ForwardedAttributes.SequenceEqual(y.ForwardedAttributes, AttributeInfo.Comparer.Default); } } } diff --git a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs index 7f933d77..901911c5 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs @@ -88,8 +88,10 @@ internal static class Execute ImmutableArray.Builder propertyChangedNames = ImmutableArray.CreateBuilder(); ImmutableArray.Builder propertyChangingNames = ImmutableArray.CreateBuilder(); ImmutableArray.Builder notifiedCommandNames = ImmutableArray.CreateBuilder(); - ImmutableArray.Builder validationAttributes = ImmutableArray.CreateBuilder(); + ImmutableArray.Builder forwardedAttributes = ImmutableArray.CreateBuilder(); bool alsoBroadcastChange = false; + bool alsoValidateProperty = false; + bool hasAnyValidationAttributes = false; // Track the property changing event for the property, if the type supports it if (shouldInvokeOnPropertyChanging) @@ -118,39 +120,48 @@ internal static class Execute continue; } - // Track the current attribute for forwarding if applicable. The following attributes are special cased: - // - Validation attributes (System.ComponentModel.DataAnnotations.ValidationAttribute) + // Check whether the property should also be validated + if (TryGetIsValidatingProperty(fieldSymbol, attributeData, builder, out bool isValidationTargetValid)) + { + alsoValidateProperty = isValidationTargetValid; + + continue; + } + + // Track the current attribute for forwarding if it is a validation attribute + if (attributeData.AttributeClass?.InheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.ValidationAttribute") == true) + { + hasAnyValidationAttributes = true; + + forwardedAttributes.Add(AttributeInfo.From(attributeData)); + } + + // Also track the current attribute for forwarding if it is of any of the following types: // - Display attributes (System.ComponentModel.DataAnnotations.DisplayAttribute) // - UI hint attributes(System.ComponentModel.DataAnnotations.UIHintAttribute) // - Scaffold column attributes (System.ComponentModel.DataAnnotations.ScaffoldColumnAttribute) // - Editable attributes (System.ComponentModel.DataAnnotations.EditableAttribute) // - Key attributes (System.ComponentModel.DataAnnotations.KeyAttribute) - if (attributeData.AttributeClass?.InheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.ValidationAttribute") == true || - attributeData.AttributeClass?.HasOrInheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.UIHintAttribute") == true || + if (attributeData.AttributeClass?.HasOrInheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.UIHintAttribute") == true || attributeData.AttributeClass?.HasOrInheritsFromFullyQualifiedName("global::System.ComponentModel.DataAnnotations.ScaffoldColumnAttribute") == true || attributeData.AttributeClass?.HasFullyQualifiedName("global::System.ComponentModel.DataAnnotations.DisplayAttribute") == true || attributeData.AttributeClass?.HasFullyQualifiedName("global::System.ComponentModel.DataAnnotations.EditableAttribute") == true || attributeData.AttributeClass?.HasFullyQualifiedName("global::System.ComponentModel.DataAnnotations.KeyAttribute") == true) { - validationAttributes.Add(AttributeInfo.From(attributeData)); + forwardedAttributes.Add(AttributeInfo.From(attributeData)); } } // Log the diagnostics if needed - if (validationAttributes.Count > 0 && + if (hasAnyValidationAttributes && !fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableValidator")) { builder.Add( - MissingObservableValidatorInheritanceError, + MissingObservableValidatorInheritanceForValidationAttributeError, fieldSymbol, fieldSymbol.ContainingType, fieldSymbol.Name, - validationAttributes.Count); - - // Remove all validation attributes so that the generated code doesn't cause a build error about the - // "ValidateProperty" method not existing (as the type doesn't inherit from ObservableValidator). The - // compilation will still fail due to this diagnostics, but with just this easier to understand error. - validationAttributes.Clear(); + forwardedAttributes.Count); } diagnostics = builder.ToImmutable(); @@ -163,7 +174,8 @@ internal static class Execute propertyChangedNames.ToImmutable(), notifiedCommandNames.ToImmutable(), alsoBroadcastChange, - validationAttributes.ToImmutable()); + alsoValidateProperty, + forwardedAttributes.ToImmutable()); } /// @@ -423,6 +435,47 @@ private static bool TryGetIsBroadcastingChanges( return false; } + /// + /// Checks whether a given generated property should also validate its value. + /// + /// The input instance to process. + /// The instance for . + /// The current collection of gathered diagnostics. + /// Whether or not the the property is in a valid target that can validate values. + /// Whether or not the generated property for used [AlsoValidateProperty]. + private static bool TryGetIsValidatingProperty( + IFieldSymbol fieldSymbol, + AttributeData attributeData, + ImmutableArray.Builder diagnostics, + out bool isValidationTargetValid) + { + if (attributeData.AttributeClass?.HasFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.AlsoValidatePropertyAttribute") == true) + { + // If the containing type is valid, track it + if (fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableValidator")) + { + isValidationTargetValid = true; + + return true; + } + + // Otherwise just emit the diagnostic and then ignore the attribute + diagnostics.Add( + MissingObservableValidatorInheritanceForAlsoValidatePropertyError, + fieldSymbol, + fieldSymbol.ContainingType, + fieldSymbol.Name); + + isValidationTargetValid = false; + + return true; + } + + isValidationTargetValid = false; + + return false; + } + /// /// Gets a instance with the cached args for property changing notifications. /// @@ -516,10 +569,10 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf fieldExpression, IdentifierName("value")))); - // If there are validation attributes, add a call to ValidateProperty: + // If validation is requested, add a call to ValidateProperty: // // ValidateProperty(value, ); - if (propertyInfo.ValidationAttributes.Length > 0) + if (propertyInfo.AlsoValidateProperty) { setterStatements.Add( ExpressionStatement( @@ -605,9 +658,9 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf Argument(IdentifierName("value")))), Block(setterStatements)); - // Prepare the validation attributes, if any - ImmutableArray validationAttributes = - propertyInfo.ValidationAttributes + // Prepare the forwarded attributes, if any + ImmutableArray forwardedAttributes = + propertyInfo.ForwardedAttributes .Select(static a => AttributeList(SingletonSeparatedList(a.GetSyntax()))) .ToImmutableArray(); @@ -616,7 +669,7 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf // /// // [global::System.CodeDom.Compiler.GeneratedCode("...", "...")] // [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] - // + // // public // { // get => ; @@ -635,7 +688,7 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf AttributeArgument(LiteralExpression(SyntaxKind.StringLiteralExpression, Literal(typeof(ObservablePropertyGenerator).Assembly.GetName().Version.ToString())))))) .WithOpenBracketToken(Token(TriviaList(Comment($"/// ")), SyntaxKind.OpenBracketToken, TriviaList())), AttributeList(SingletonSeparatedList(Attribute(IdentifierName("global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage"))))) - .AddAttributeLists(validationAttributes.ToArray()) + .AddAttributeLists(forwardedAttributes.ToArray()) .AddModifiers(Token(SyntaxKind.PublicKeyword)) .AddAccessorListAccessors( AccessorDeclaration(SyntaxKind.GetAccessorDeclaration) diff --git a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.cs b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.cs index 2fa8bbe2..bcb37522 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.cs @@ -38,13 +38,14 @@ public void Initialize(IncrementalGeneratorInitializationContext context) fieldSymbols .Where(static item => item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute")); - // Get diagnostics for fields using [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor] and [AlsoBroadcastChange], but not [ObservableProperty] + // Get diagnostics for fields using [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor], [AlsoBroadcastChange] and [AlsoValidateProperty], but not [ObservableProperty] IncrementalValuesProvider fieldSymbolsWithOrphanedDependentAttributeWithErrors = fieldSymbols .Where(static item => (item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.AlsoNotifyChangeForAttribute") || item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.AlsoNotifyCanExecuteForAttribute") || - item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.AlsoBroadcastChangeAttribute")) && + item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.AlsoBroadcastChangeAttribute") || + item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.AlsoValidatePropertyAttribute")) && !item.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservablePropertyAttribute")) .Select(static (item, _) => Execute.GetDiagnosticForFieldWithOrphanedDependentAttributes(item)); diff --git a/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs b/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs index 613bd751..0ca65de9 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs @@ -101,14 +101,14 @@ internal static class DiagnosticDescriptors /// Format: "The field {0}.{1} cannot be used to generate an observable property, as it has {2} validation attribute(s) but is declared in a type that doesn't inherit from ObservableValidator". /// /// - public static readonly DiagnosticDescriptor MissingObservableValidatorInheritanceError = new DiagnosticDescriptor( + public static readonly DiagnosticDescriptor MissingObservableValidatorInheritanceForValidationAttributeError = new DiagnosticDescriptor( id: "MVVMTK0006", title: "Missing ObservableValidator inheritance", messageFormat: "The field {0}.{1} cannot be used to generate an observable property, as it has {2} validation attribute(s) but is declared in a type that doesn't inherit from ObservableValidator", category: typeof(ObservablePropertyGenerator).FullName, defaultSeverity: DiagnosticSeverity.Error, isEnabledByDefault: true, - description: $"Cannot apply [ObservableProperty] to fields with validation attributes if they are declared in a type that doesn't inherit from ObservableValidator.", + description: "Cannot apply [ObservableProperty] to fields with validation attributes if they are declared in a type that doesn't inherit from ObservableValidator.", helpLinkUri: "https://aka.ms/mvvmtoolkit"); /// @@ -319,17 +319,17 @@ internal static class DiagnosticDescriptors /// /// Gets a indicating when [ObservableProperty] is applied to a field in an invalid type. /// - /// Format: "The field {0}.{1} needs to be annotated with [ObservableProperty] in order to enable using [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor] and [AlsoBroadcastChange]". + /// Format: "The field {0}.{1} needs to be annotated with [ObservableProperty] in order to enable using [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor], [AlsoBroadcastChange] and [AlsoValidateProperty]". /// /// public static readonly DiagnosticDescriptor FieldWithOrphanedDependentObservablePropertyAttributesError = new DiagnosticDescriptor( id: "MVVMTK0020", title: "Invalid use of attributes dependent on [ObservableProperty]", - messageFormat: "The field {0}.{1} needs to be annotated with [ObservableProperty] in order to enable using [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor] and [AlsoBroadcastChange]", + messageFormat: "The field {0}.{1} needs to be annotated with [ObservableProperty] in order to enable using [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor], [AlsoBroadcastChange] and [AlsoValidateProperty]", category: typeof(ObservablePropertyGenerator).FullName, defaultSeverity: DiagnosticSeverity.Error, isEnabledByDefault: true, - description: "Fields not annotated with [ObservableProperty] cannot use [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor] and [AlsoBroadcastChange].", + description: "Fields not annotated with [ObservableProperty] cannot use [AlsoNotifyChangeFor], [AlsoNotifyCanExecuteFor], [AlsoBroadcastChange] and [AlsoValidateProperty].", helpLinkUri: "https://aka.ms/mvvmtoolkit"); /// @@ -395,4 +395,20 @@ internal static class DiagnosticDescriptors isEnabledByDefault: true, description: "The fields annotated with [ObservableProperty] cannot result in a property name or have a type that would cause conflicts with other generated members.", helpLinkUri: "https://aka.ms/mvvmtoolkit"); + + /// + /// Gets a indicating when the target type doesn't inherit from the ObservableValidator class. + /// + /// Format: "The field {0}.{1} cannot be annotated with [AlsoValidateProperty], as it is declared in a type that doesn't inherit from ObservableValidator". + /// + /// + public static readonly DiagnosticDescriptor MissingObservableValidatorInheritanceForAlsoValidatePropertyError = new DiagnosticDescriptor( + id: "MVVMTK0025", + title: "Missing ObservableValidator inheritance", + messageFormat: "The field {0}.{1} cannot be annotated with [AlsoValidateProperty], as it is declared in a type that doesn't inherit from ObservableValidator", + category: typeof(ObservablePropertyGenerator).FullName, + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "Cannot apply [AlsoValidateProperty] to field that are declared in a type that doesn't inherit from ObservableValidator.", + helpLinkUri: "https://aka.ms/mvvmtoolkit"); } From 31918450efd9354adad1977848080636a58f064d Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 24 May 2022 10:57:01 +0200 Subject: [PATCH 4/5] Improve diagnostics, add unit tests --- .../AnalyzerReleases.Shipped.md | 1 + .../ObservablePropertyGenerator.Execute.cs | 12 ++++- .../Diagnostics/DiagnosticDescriptors.cs | 18 +++++++- .../Test_SourceGeneratorsDiagnostics.cs | 46 ++++++++++++++++++- 4 files changed, 73 insertions(+), 4 deletions(-) diff --git a/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md b/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md index 77414dba..138e40ba 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md +++ b/CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Shipped.md @@ -32,3 +32,4 @@ MVVMTK0022 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator MVVMTK0023 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error MVVMTK0024 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error MVVMTK0025 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error +MVVMTK0026 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error diff --git a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs index 901911c5..f18cdb93 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs @@ -152,7 +152,7 @@ internal static class Execute } } - // Log the diagnostics if needed + // Log the diagnostic for missing ObservableValidator, if needed if (hasAnyValidationAttributes && !fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableValidator")) { @@ -164,6 +164,16 @@ internal static class Execute forwardedAttributes.Count); } + // Log the diagnostic for missing validation attributes, if any + if (alsoValidateProperty && !hasAnyValidationAttributes) + { + builder.Add( + MissingValidationAttributesForAlsoValidatePropertyError, + fieldSymbol, + fieldSymbol.ContainingType, + fieldSymbol.Name); + } + diagnostics = builder.ToImmutable(); return new( diff --git a/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs b/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs index 0ca65de9..74ae6039 100644 --- a/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs +++ b/CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs @@ -409,6 +409,22 @@ internal static class DiagnosticDescriptors category: typeof(ObservablePropertyGenerator).FullName, defaultSeverity: DiagnosticSeverity.Error, isEnabledByDefault: true, - description: "Cannot apply [AlsoValidateProperty] to field that are declared in a type that doesn't inherit from ObservableValidator.", + description: "Cannot apply [AlsoValidateProperty] to fields that are declared in a type that doesn't inherit from ObservableValidator.", + helpLinkUri: "https://aka.ms/mvvmtoolkit"); + + /// + /// Gets a indicating when the target field uses [AlsoValidateProperty] but has no validation attributes. + /// + /// Format: "The field {0}.{1} cannot be annotated with [AlsoValidateProperty], as it doesn't have any validation attributes to use during validation". + /// + /// + public static readonly DiagnosticDescriptor MissingValidationAttributesForAlsoValidatePropertyError = new DiagnosticDescriptor( + id: "MVVMTK0026", + title: "Missing validation attributes", + messageFormat: "The field {0}.{1} cannot be annotated with [AlsoValidateProperty], as it doesn't have any validation attributes to use during validation", + category: typeof(ObservablePropertyGenerator).FullName, + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "Cannot apply [AlsoValidateProperty] to fields that don't have any validation attributes to use during validation.", helpLinkUri: "https://aka.ms/mvvmtoolkit"); } diff --git a/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsDiagnostics.cs b/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsDiagnostics.cs index 14d087d1..a9b80238 100644 --- a/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsDiagnostics.cs +++ b/tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsDiagnostics.cs @@ -192,7 +192,7 @@ public partial class SampleViewModel } [TestMethod] - public void MissingObservableValidatorInheritanceError() + public void MissingObservableValidatorInheritanceForValidationAttributeError() { string source = @" using System.ComponentModel.DataAnnotations; @@ -1188,6 +1188,48 @@ public partial class MyViewModel : ObservableObject VerifyGeneratedDiagnostics(source, "MVVMTK0024"); } + [TestMethod] + public void MissingObservableValidatorInheritanceForAlsoValidatePropertyError() + { + string source = @" + using System.ComponentModel.DataAnnotations; + using CommunityToolkit.Mvvm.ComponentModel; + + namespace MyApp + { + [INotifyPropertyChanged] + public partial class SampleViewModel + { + [ObservableProperty] + [Required] + [AlsoValidateProperty] + private string name; + } + }"; + + VerifyGeneratedDiagnostics(source, "MVVMTK0006", "MVVMTK0025"); + } + + [TestMethod] + public void MissingValidationAttributesForAlsoValidatePropertyError() + { + string source = @" + using System.ComponentModel.DataAnnotations; + using CommunityToolkit.Mvvm.ComponentModel; + + namespace MyApp + { + public partial class SampleViewModel : ObservableValidator + { + [ObservableProperty] + [AlsoValidateProperty] + private string name; + } + }"; + + VerifyGeneratedDiagnostics(source, "MVVMTK0026"); + } + /// /// Verifies the output of a source generator. /// @@ -1232,7 +1274,7 @@ from assembly in AppDomain.CurrentDomain.GetAssemblies() HashSet resultingIds = diagnostics.Select(diagnostic => diagnostic.Id).ToHashSet(); - Assert.IsTrue(resultingIds.SetEquals(diagnosticsIds)); + CollectionAssert.AreEquivalent(diagnosticsIds, resultingIds.ToArray()); GC.KeepAlive(observableObjectType); GC.KeepAlive(validationAttributeType); From 0b15992b6644c3b6df060fcd63a4132c0810a18c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 24 May 2022 12:10:11 +0200 Subject: [PATCH 5/5] Add unit test for [AlsoValidateProperty] --- .../Test_ObservablePropertyAttribute.cs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs b/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs index e75d9f59..3b067457 100644 --- a/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs +++ b/tests/CommunityToolkit.Mvvm.UnitTests/Test_ObservablePropertyAttribute.cs @@ -224,13 +224,46 @@ public void Test_ObservablePropertyWithValueNamedField_WithValidationAttributes( model.PropertyChanged += (s, e) => propertyNames.Add(e.PropertyName); + bool errorsChanged = false; + + model.ErrorsChanged += (s, e) => errorsChanged = true; + model.Value = "Hello world"; Assert.AreEqual(model.Value, "Hello world"); + // The [AlsoValidateProperty] attribute wasn't used, so the property shouldn't be validated + Assert.IsFalse(errorsChanged); + CollectionAssert.AreEqual(new[] { nameof(model.Value) }, propertyNames); } + [TestMethod] + public void Test_ObservablePropertyWithValueNamedField_WithValidationAttributesAndValidation() + { + ModelWithValuePropertyWithAutomaticValidation model = new(); + + List propertyNames = new(); + + model.PropertyChanged += (s, e) => propertyNames.Add(e.PropertyName); + + List errors = new(); + + model.ErrorsChanged += (s, e) => errors.Add(e); + + model.Value = "Bo"; + + Assert.IsTrue(model.HasErrors); + Assert.AreEqual(errors.Count, 1); + Assert.AreEqual(errors[0].PropertyName, nameof(ModelWithValuePropertyWithAutomaticValidation.Value)); + + model.Value = "Hello world"; + + Assert.IsFalse(model.HasErrors); + Assert.AreEqual(errors.Count, 2); + Assert.AreEqual(errors[1].PropertyName, nameof(ModelWithValuePropertyWithAutomaticValidation.Value)); + } + // See https://github.com/CommunityToolkit/WindowsCommunityToolkit/issues/4184 [TestMethod] public void Test_GeneratedPropertiesWithValidationAttributesOverFields() @@ -893,6 +926,15 @@ public partial class ModelWithValuePropertyWithValidation : ObservableValidator private string? value; } + public partial class ModelWithValuePropertyWithAutomaticValidation : ObservableValidator + { + [ObservableProperty] + [Required] + [MinLength(5)] + [AlsoValidateProperty] + private string? value; + } + public partial class ViewModelWithValidatableGeneratedProperties : ObservableValidator { [Required]