Skip to content

Commit b632429

Browse files
Sergio0694Sergio0694
committed
Add diagnostics for invalid [ObservableProperty] containing type
1 parent 914a7ac commit b632429

File tree

6 files changed

+85
-37
lines changed

6 files changed

+85
-37
lines changed

‎CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Unshipped.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,4 @@ MVVMTK0015 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error |
2323
MVVMTK0016 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error
2424
MVVMTK0017 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error
2525
MVVMTK0018 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error
26+
MVVMTK0019 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error

‎CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/INotifyPropertyChangedGenerator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public INotifyPropertyChangedGenerator()
3434
{
3535
static INotifyPropertyChangedInfo GetInfo(INamedTypeSymbol typeSymbol, AttributeData attributeData)
3636
{
37-
bool includeAdditionalHelperMethods = attributeData.GetNamedArgument<bool>("IncludeAdditionalHelperMethods", true);
37+
bool includeAdditionalHelperMethods = attributeData.GetNamedArgument("IncludeAdditionalHelperMethods", true);
3838

3939
return new(includeAdditionalHelperMethods);
4040
}

‎CommunityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,19 @@ internal static class Execute
3434
{
3535
ImmutableArray<Diagnostic>.Builder builder = ImmutableArray.CreateBuilder<Diagnostic>();
3636

37-
// Check whether the containing type implements INotifyPropertyChanging and whether it inherits from ObservableValidator
38-
bool isObservableObject = fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableObject");
39-
bool isObservableValidator = fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableValidator");
40-
bool isNotifyPropertyChanging = fieldSymbol.ContainingType.AllInterfaces.Any(static i => i.HasFullyQualifiedName("global::System.ComponentModel.INotifyPropertyChanging"));
41-
bool hasObservableObjectAttribute = fieldSymbol.ContainingType.GetAttributes().Any(static a => a.AttributeClass?.HasFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableObjectAttribute") == true);
37+
// Validate the target type
38+
if (!IsTargetTypeValid(fieldSymbol, out bool shouldInvokeOnPropertyChanging))
39+
{
40+
builder.Add(
41+
InvalidContainingTypeForObservablePropertyFieldError,
42+
fieldSymbol,
43+
fieldSymbol.ContainingType,
44+
fieldSymbol.Name);
45+
46+
diagnostics = builder.ToImmutable();
47+
48+
return null;
49+
}
4250

4351
// Get the property type and name
4452
string typeName = fieldSymbol.Type.GetFullyQualifiedName();
@@ -69,7 +77,7 @@ internal static class Execute
6977
ImmutableArray<AttributeInfo>.Builder validationAttributes = ImmutableArray.CreateBuilder<AttributeInfo>();
7078

7179
// Track the property changing event for the property, if the type supports it
72-
if (isObservableObject || isNotifyPropertyChanging || hasObservableObjectAttribute)
80+
if (shouldInvokeOnPropertyChanging)
7381
{
7482
propertyChangingNames.Add(propertyName);
7583
}
@@ -96,7 +104,7 @@ internal static class Execute
96104

97105
// Log the diagnostics if needed
98106
if (validationAttributes.Count > 0 &&
99-
!isObservableValidator)
107+
!fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableValidator"))
100108
{
101109
builder.Add(
102110
MissingObservableValidatorInheritanceError,
@@ -124,6 +132,30 @@ internal static class Execute
124132
validationAttributes.ToImmutable());
125133
}
126134

135+
/// <summary>
136+
/// Validates the containing type for a given field being annotated.
137+
/// </summary>
138+
/// <param name="fieldSymbol">The input <see cref="IFieldSymbol"/> instance to process.</param>
139+
/// <param name="shouldInvokeOnPropertyChanging">Whether or not property changing events should also be raised.</param>
140+
/// <returns>Whether or not the containing type for <paramref name="fieldSymbol"/> is valid.</returns>
141+
private static bool IsTargetTypeValid(
142+
IFieldSymbol fieldSymbol,
143+
out bool shouldInvokeOnPropertyChanging)
144+
{
145+
// The [ObservableProperty] attribute can only be used in types that are known to expose the necessary OnPropertyChanged and OnPropertyChanging methods.
146+
// That means that the containing type for the field needs to match one of the following conditions:
147+
// - It inherits from ObservableObject (in which case it also implements INotifyPropertyChanging).
148+
// - It has the [ObservableObject] attribute (on itself or any of its base types).
149+
// - It has the [INotifyPropertyChanged] attribute (on itself or any of its base types).
150+
bool isObservableObject = fieldSymbol.ContainingType.InheritsFromFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableObject");
151+
bool hasObservableObjectAttribute = fieldSymbol.ContainingType.HasOrInheritsAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.ObservableObjectAttribute");
152+
bool hasINotifyPropertyChangedAttribute = fieldSymbol.ContainingType.HasOrInheritsAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.ComponentModel.INotifyPropertyChangedAttribute");
153+
154+
shouldInvokeOnPropertyChanging = isObservableObject || hasObservableObjectAttribute;
155+
156+
return isObservableObject || hasObservableObjectAttribute || hasINotifyPropertyChangedAttribute;
157+
}
158+
127159
/// <summary>
128160
/// Tries to gather dependent properties from the given attribute.
129161
/// </summary>

‎CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ internal static class DiagnosticDescriptors
233233
category: typeof(ObservablePropertyGenerator).FullName,
234234
defaultSeverity: DiagnosticSeverity.Error,
235235
isEnabledByDefault: true,
236-
description: $"The name of fields annotated with [ObservableProperty] should use \"lowerCamel\", \"_lowerCamel\" or \"m_lowerCamel\" pattern to avoid collisions with the generated properties.",
236+
description: "The name of fields annotated with [ObservableProperty] should use \"lowerCamel\", \"_lowerCamel\" or \"m_lowerCamel\" pattern to avoid collisions with the generated properties.",
237237
helpLinkUri: "https://aka.ms/mvvmtoolkit");
238238

239239
/// <summary>
@@ -281,7 +281,7 @@ internal static class DiagnosticDescriptors
281281
category: typeof(INotifyPropertyChangedGenerator).FullName,
282282
defaultSeverity: DiagnosticSeverity.Error,
283283
isEnabledByDefault: true,
284-
description: $"Cannot apply [INotifyPropertyChanged] to a type that already has this attribute or [ObservableObject] applied to it (including base types).",
284+
description: "Cannot apply [INotifyPropertyChanged] to a type that already has this attribute or [ObservableObject] applied to it (including base types).",
285285
helpLinkUri: "https://aka.ms/mvvmtoolkit");
286286

287287
/// <summary>
@@ -297,6 +297,22 @@ internal static class DiagnosticDescriptors
297297
category: typeof(ObservableObjectGenerator).FullName,
298298
defaultSeverity: DiagnosticSeverity.Error,
299299
isEnabledByDefault: true,
300-
description: $"Cannot apply [ObservableObject] to a type that already has this attribute or [INotifyPropertyChanged] applied to it (including base types).",
300+
description: "Cannot apply [ObservableObject] to a type that already has this attribute or [INotifyPropertyChanged] applied to it (including base types).",
301+
helpLinkUri: "https://aka.ms/mvvmtoolkit");
302+
303+
/// <summary>
304+
/// Gets a <see cref="DiagnosticDescriptor"/> indicating when <c>[ObservableProperty]</c> is applied to a field in an invalid type.
305+
/// <para>
306+
/// Format: <c>"The field {0}.{1} cannot be used to generate an observable property, as its containing type doesn't inherit from ObservableObject, nor does it use [ObservableObject] or [INotifyPropertyChanged]"</c>.
307+
/// </para>
308+
/// </summary>
309+
public static readonly DiagnosticDescriptor InvalidContainingTypeForObservablePropertyFieldError = new DiagnosticDescriptor(
310+
id: "MVVMTK0019",
311+
title: $"Invalid containing type for [ObservableProperty] field",
312+
messageFormat: $"The field {{0}}.{{1}} cannot be used to generate an observable property, as its containing type doesn't inherit from ObservableObject, nor does it use [ObservableObject] or [INotifyPropertyChanged]",
313+
category: typeof(ObservablePropertyGenerator).FullName,
314+
defaultSeverity: DiagnosticSeverity.Error,
315+
isEnabledByDefault: true,
316+
description: "Fields annotated with [ObservableProperty] must be contained in a type that inherits from ObservableObject or that is annotated with [ObservableObject] or [INotifyPropertyChanged] (including base types).",
301317
helpLinkUri: "https://aka.ms/mvvmtoolkit");
302318
}

‎CommunityToolkit.Mvvm/ComponentModel/Attributes/ObservablePropertyAttribute.cs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,11 @@ namespace CommunityToolkit.Mvvm.ComponentModel;
1010

1111
/// <summary>
1212
/// An attribute that indicates that a given field should be wrapped by a generated observable property.
13-
/// In order to use this attribute, the containing type has to implement the <see cref="INotifyPropertyChanged"/> interface
14-
/// and expose a method with the same signature as <see cref="ObservableObject.OnPropertyChanged(string?)"/>. If the containing
15-
/// type also implements the <see cref="INotifyPropertyChanging"/> interface and exposes a method with the same signature as
16-
/// <see cref="ObservableObject.OnPropertyChanging(string?)"/>, then this method will be invoked as well by the property setter.
13+
/// In order to use this attribute, the containing type has to inherit from <see cref="ObservableObject"/>, or it
14+
/// must be using <see cref="ObservableObjectAttribute"/> or <see cref="INotifyPropertyChangedAttribute"/>.
15+
/// If the containing type also implements the <see cref="INotifyPropertyChanging"/> (that is, if it either inherits from
16+
/// <see cref="ObservableObject"/> or is using <see cref="ObservableObjectAttribute"/>), then the generated code will
17+
/// also invoke <see cref="ObservableObject.OnPropertyChanging(PropertyChangingEventArgs)"/> to signal that event.
1718
/// <para>
1819
/// This attribute can be used as follows:
1920
/// <code>

‎tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsDiagnostics.cs

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,6 @@ private async Task GreetUserAsync(User user)
620620
public void NameCollisionForGeneratedObservableProperty()
621621
{
622622
string source = @"
623-
using System.ComponentModel.DataAnnotations;
624623
using CommunityToolkit.Mvvm.ComponentModel;
625624
626625
namespace MyApp
@@ -639,7 +638,6 @@ public partial class SampleViewModel : ObservableObject
639638
public void AlsoNotifyChangeForInvalidTargetError_Null()
640639
{
641640
string source = @"
642-
using System.ComponentModel.DataAnnotations;
643641
using CommunityToolkit.Mvvm.ComponentModel;
644642
645643
namespace MyApp
@@ -659,7 +657,6 @@ public partial class SampleViewModel : ObservableObject
659657
public void AlsoNotifyChangeForInvalidTargetError_SamePropertyAsGeneratedOneFromSelf()
660658
{
661659
string source = @"
662-
using System.ComponentModel.DataAnnotations;
663660
using CommunityToolkit.Mvvm.ComponentModel;
664661
665662
namespace MyApp
@@ -679,7 +676,6 @@ public partial class SampleViewModel : ObservableObject
679676
public void AlsoNotifyChangeForInvalidTargetError_Missing()
680677
{
681678
string source = @"
682-
using System.ComponentModel.DataAnnotations;
683679
using CommunityToolkit.Mvvm.ComponentModel;
684680
685681
namespace MyApp
@@ -699,7 +695,6 @@ public partial class SampleViewModel : ObservableObject
699695
public void AlsoNotifyChangeForInvalidTargetError_InvalidType()
700696
{
701697
string source = @"
702-
using System.ComponentModel.DataAnnotations;
703698
using CommunityToolkit.Mvvm.ComponentModel;
704699
705700
namespace MyApp
@@ -723,7 +718,6 @@ public void Foo()
723718
public void AlsoNotifyCanExecuteForInvalidTargetError_Null()
724719
{
725720
string source = @"
726-
using System.ComponentModel.DataAnnotations;
727721
using CommunityToolkit.Mvvm.ComponentModel;
728722
729723
namespace MyApp
@@ -743,7 +737,6 @@ public partial class SampleViewModel : ObservableObject
743737
public void AlsoNotifyCanExecuteForInvalidTargetError_Missing()
744738
{
745739
string source = @"
746-
using System.ComponentModel.DataAnnotations;
747740
using CommunityToolkit.Mvvm.ComponentModel;
748741
749742
namespace MyApp
@@ -763,7 +756,6 @@ public partial class SampleViewModel : ObservableObject
763756
public void AlsoNotifyCanExecuteForInvalidTargetError_InvalidMemberType()
764757
{
765758
string source = @"
766-
using System.ComponentModel.DataAnnotations;
767759
using CommunityToolkit.Mvvm.ComponentModel;
768760
769761
namespace MyApp
@@ -787,7 +779,6 @@ public void Foo()
787779
public void AlsoNotifyCanExecuteForInvalidTargetError_InvalidPropertyType()
788780
{
789781
string source = @"
790-
using System.ComponentModel.DataAnnotations;
791782
using CommunityToolkit.Mvvm.ComponentModel;
792783
793784
namespace MyApp
@@ -809,7 +800,6 @@ public partial class SampleViewModel : ObservableObject
809800
public void AlsoNotifyCanExecuteForInvalidTargetError_InvalidCommandType()
810801
{
811802
string source = @"
812-
using System.ComponentModel.DataAnnotations;
813803
using CommunityToolkit.Mvvm.ComponentModel;
814804
using CommunityToolkit.Mvvm.Input;
815805
@@ -832,9 +822,7 @@ public partial class SampleViewModel : ObservableObject
832822
public void InvalidAttributeCombinationForINotifyPropertyChangedAttributeError_InheritingINotifyPropertyChangedAttribute()
833823
{
834824
string source = @"
835-
using System.ComponentModel.DataAnnotations;
836825
using CommunityToolkit.Mvvm.ComponentModel;
837-
using CommunityToolkit.Mvvm.Input;
838826
839827
namespace MyApp
840828
{
@@ -856,9 +844,7 @@ public partial class B : A
856844
public void InvalidAttributeCombinationForINotifyPropertyChangedAttributeError_InheritingObservableObjectAttribute()
857845
{
858846
string source = @"
859-
using System.ComponentModel.DataAnnotations;
860847
using CommunityToolkit.Mvvm.ComponentModel;
861-
using CommunityToolkit.Mvvm.Input;
862848
863849
namespace MyApp
864850
{
@@ -880,9 +866,7 @@ public partial class B : A
880866
public void InvalidAttributeCombinationForINotifyPropertyChangedAttributeError_WithAlsoObservableObjectAttribute()
881867
{
882868
string source = @"
883-
using System.ComponentModel.DataAnnotations;
884869
using CommunityToolkit.Mvvm.ComponentModel;
885-
using CommunityToolkit.Mvvm.Input;
886870
887871
namespace MyApp
888872
{
@@ -900,9 +884,7 @@ public partial class A
900884
public void InvalidAttributeCombinationForObservableObjectAttributeError_InheritingINotifyPropertyChangedAttribute()
901885
{
902886
string source = @"
903-
using System.ComponentModel.DataAnnotations;
904887
using CommunityToolkit.Mvvm.ComponentModel;
905-
using CommunityToolkit.Mvvm.Input;
906888
907889
namespace MyApp
908890
{
@@ -924,9 +906,7 @@ public partial class B : A
924906
public void InvalidAttributeCombinationForObservableObjectAttributeError_InheritingObservableObjectAttribute()
925907
{
926908
string source = @"
927-
using System.ComponentModel.DataAnnotations;
928909
using CommunityToolkit.Mvvm.ComponentModel;
929-
using CommunityToolkit.Mvvm.Input;
930910
931911
namespace MyApp
932912
{
@@ -948,9 +928,7 @@ public partial class B : A
948928
public void InvalidAttributeCombinationForObservableObjectAttributeError_WithAlsoINotifyPropertyChangedAttribute()
949929
{
950930
string source = @"
951-
using System.ComponentModel.DataAnnotations;
952931
using CommunityToolkit.Mvvm.ComponentModel;
953-
using CommunityToolkit.Mvvm.Input;
954932
955933
namespace MyApp
956934
{
@@ -964,6 +942,26 @@ public partial class A
964942
VerifyGeneratedDiagnostics<ObservableObjectGenerator>(source, "MVVMTK0018");
965943
}
966944

945+
[TestMethod]
946+
public void InvalidContainingTypeForObservablePropertyFieldError()
947+
{
948+
string source = @"
949+
using CommunityToolkit.Mvvm.ComponentModel;
950+
951+
namespace MyApp
952+
{
953+
public partial class MyViewModel : INotifyPropertyChanged
954+
{
955+
[ObservableProperty]
956+
public int number;
957+
958+
public event PropertyChangedEventHandler PropertyChanged;
959+
}
960+
}";
961+
962+
VerifyGeneratedDiagnostics<ObservablePropertyGenerator>(source, "MVVMTK0019");
963+
}
964+
967965
/// <summary>
968966
/// Verifies the output of a source generator.
969967
/// </summary>

0 commit comments

Comments
 (0)