diff --git a/src/CommunityToolkit.Mvvm.CodeFixers/UsePartialPropertyForObservablePropertyCodeFixer.cs b/src/CommunityToolkit.Mvvm.CodeFixers/UsePartialPropertyForObservablePropertyCodeFixer.cs index 168f43b5b..a8067ace3 100644 --- a/src/CommunityToolkit.Mvvm.CodeFixers/UsePartialPropertyForObservablePropertyCodeFixer.cs +++ b/src/CommunityToolkit.Mvvm.CodeFixers/UsePartialPropertyForObservablePropertyCodeFixer.cs @@ -100,13 +100,6 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (root!.FindNode(diagnosticSpan).FirstAncestorOrSelf() is { Declaration.Variables: [{ Identifier.Text: string identifierName }] } fieldDeclaration && identifierName == fieldName) { - // We only support fields with up to one attribute per attribute list. - // This is so we can easily check one attribute when updating targets. - if (fieldDeclaration.AttributeLists.Any(static list => list.Attributes.Count > 1)) - { - return; - } - // Register the code fix to update the class declaration to inherit from ObservableObject instead context.RegisterCodeFix( CodeAction.Create( @@ -194,33 +187,108 @@ private static async Task ConvertToPartialProperty( continue; } - // Make sure we can retrieve the symbol for the attribute type. - // We are guaranteed to always find a single attribute in the list. - if (!semanticModel.GetSymbolInfo(attributeListSyntax.Attributes[0], cancellationToken).TryGetAttributeTypeSymbol(out INamedTypeSymbol? attributeSymbol)) + if (attributeListSyntax.Attributes.Count == 1) { - return document; - } - - // Case 3 - if (toolkitTypeSymbols.ContainsValue(attributeSymbol)) - { - propertyAttributes[i] = attributeListSyntax.WithTarget(null); - - continue; + // Make sure we can retrieve the symbol for the attribute type + if (!semanticModel.GetSymbolInfo(attributeListSyntax.Attributes[0], cancellationToken).TryGetAttributeTypeSymbol(out INamedTypeSymbol? attributeSymbol)) + { + return document; + } + + // Case 3 + if (toolkitTypeSymbols.ContainsValue(attributeSymbol)) + { + propertyAttributes[i] = attributeListSyntax.WithTarget(null); + + continue; + } + + // Case 4 + if (annotationTypeSymbols.ContainsValue(attributeSymbol) || attributeSymbol.InheritsFromType(validationAttributeSymbol)) + { + continue; + } + + // Case 5 + if (attributeListSyntax.Target is null) + { + propertyAttributes[i] = attributeListSyntax.WithTarget(AttributeTargetSpecifier(Token(SyntaxKind.FieldKeyword))); + + continue; + } } - - // Case 4 - if (annotationTypeSymbols.ContainsValue(attributeSymbol) || attributeSymbol.InheritsFromType(validationAttributeSymbol)) - { - continue; - } - - // Case 5 - if (attributeListSyntax.Target is null) + else { - propertyAttributes[i] = attributeListSyntax.WithTarget(AttributeTargetSpecifier(Token(SyntaxKind.FieldKeyword))); - - continue; + // If we have multiple attributes in the current list, we need additional logic here. + // We could have any number of attributes here, so we split them into three buckets: + // - MVVM Toolkit attributes: these should be moved over with no target + // - Data annotation or validation attributes: these should be moved over with the same target + // - Any other attributes: these should be moved over with the 'field' target + List mvvmToolkitAttributes = []; + List annotationOrValidationAttributes = []; + List fieldAttributes = []; + + foreach (AttributeSyntax attributeSyntax in attributeListSyntax.Attributes) + { + // Like for the single attribute case, make sure we can get the symbol for the attribute + if (!semanticModel.GetSymbolInfo(attributeSyntax, cancellationToken).TryGetAttributeTypeSymbol(out INamedTypeSymbol? attributeSymbol)) + { + return document; + } + + bool isAnnotationOrValidationAttribute = annotationTypeSymbols.ContainsValue(attributeSymbol) || attributeSymbol.InheritsFromType(validationAttributeSymbol); + + // Split the attributes into the buckets. Note that we have a special rule for annotation and validation + // attributes when no target is specified. In that case, we will merge them with the MVVM Toolkit items. + // This allows us to try to keep the attributes in the same attribute list, rather than splitting them. + if (toolkitTypeSymbols.ContainsValue(attributeSymbol) || (isAnnotationOrValidationAttribute && attributeListSyntax.Target is null)) + { + mvvmToolkitAttributes.Add(attributeSyntax); + } + else if (isAnnotationOrValidationAttribute) + { + annotationOrValidationAttributes.Add(attributeSyntax); + } + else + { + fieldAttributes.Add(attributeSyntax); + } + } + + // We need to start inserting the new lists right before the one we're currently + // processing. We'll be removing it when we're done, the buckets will replace it. + int insertionIndex = i; + + // Helper to process and insert the new synthesized attribute lists into the target collection + void InsertAttributeListIfNeeded(List attributes, AttributeTargetSpecifierSyntax? attributeTarget) + { + if (attributes is []) + { + return; + } + + AttributeListSyntax attributeList = AttributeList(SeparatedList(attributes)).WithTarget(attributeTarget); + + // Only if this is the first non empty list we're adding, carry over the original trivia + if (insertionIndex == i) + { + attributeList = attributeList.WithTriviaFrom(attributeListSyntax); + } + + // Finally, insert the new list into the final tree + propertyAttributes.Insert(insertionIndex++, attributeList); + } + + InsertAttributeListIfNeeded(mvvmToolkitAttributes, attributeTarget: null); + InsertAttributeListIfNeeded(annotationOrValidationAttributes, attributeTarget: attributeListSyntax.Target); + InsertAttributeListIfNeeded(fieldAttributes, attributeTarget: AttributeTargetSpecifier(Token(SyntaxKind.FieldKeyword))); + + // Remove the attribute list that we have just split into buckets + propertyAttributes.RemoveAt(insertionIndex); + + // Move the current loop iteration to the last inserted item. + // We decrement by 1 because the new loop iteration will add 1. + i = insertionIndex - 1; } } diff --git a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_UsePartialPropertyForObservablePropertyCodeFixer.cs b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_UsePartialPropertyForObservablePropertyCodeFixer.cs index 0ccc7032f..f72ca8261 100644 --- a/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_UsePartialPropertyForObservablePropertyCodeFixer.cs +++ b/tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_UsePartialPropertyForObservablePropertyCodeFixer.cs @@ -767,4 +767,232 @@ public void M() await test.RunAsync(); } + + [TestMethod] + public async Task SimpleFields_WithMultipleAttributes_SingleProperty() + { + string original = """ + using System; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + [ObservableProperty, NotifyPropertyChangedFor("Age")] private string name = String.Empty; + } + """; + + string @fixed = """ + using System; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + [ObservableProperty, NotifyPropertyChangedFor("Age")] + public partial string Name { get; set; } = String.Empty; + } + """; + + CSharpCodeFixTest test = new(LanguageVersion.Preview) + { + TestCode = original, + FixedCode = @fixed, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + + test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly); + test.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(6,74): info MVVMTK0042: The field Class1.name using [ObservableProperty] can be converted to a partial property instead, which is recommended (doing so improves the developer experience and allows other generators and analyzers to correctly see the generated property as well) + CSharpCodeFixVerifier.Diagnostic().WithSpan(6, 74, 6, 78).WithArguments("Class1", "name"), + }); + + test.FixedState.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(7,27): error CS8050: Only auto-implemented properties, or properties that use the 'field' keyword, can have initializers. + DiagnosticResult.CompilerError("CS8050").WithSpan(7, 27, 7, 31), + + // /0/Test0.cs(7,27): error CS9248: Partial property 'Class1.Name' must have an implementation part. + DiagnosticResult.CompilerError("CS9248").WithSpan(7, 27, 7, 31).WithArguments("Class1.Name"), + }); + + await test.RunAsync(); + } + + // See https://github.com/CommunityToolkit/dotnet/issues/1007 + [TestMethod] + public async Task SimpleFields_WithMultipleAttributes_WithNoBlankLines() + { + string original = """ + using System; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + [ObservableProperty, NotifyPropertyChangedFor("Age")] private string name = String.Empty; + [ObservableProperty] private int age; + } + """; + + string @fixed = """ + using System; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + [ObservableProperty, NotifyPropertyChangedFor("Age")] + public partial string Name { get; set; } = String.Empty; + + [ObservableProperty] + public partial int Age { get; set; } + } + """; + + CSharpCodeFixTest test = new(LanguageVersion.Preview) + { + TestCode = original, + FixedCode = @fixed, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + + test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly); + test.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(6,74): info MVVMTK0042: The field Class1.name using [ObservableProperty] can be converted to a partial property instead, which is recommended (doing so improves the developer experience and allows other generators and analyzers to correctly see the generated property as well) + CSharpCodeFixVerifier.Diagnostic().WithSpan(6, 74, 6, 78).WithArguments("Class1", "name"), + + // /0/Test0.cs(7,38): info MVVMTK0042: The field Class1.age using [ObservableProperty] can be converted to a partial property instead, which is recommended (doing so improves the developer experience and allows other generators and analyzers to correctly see the generated property as well) + CSharpCodeFixVerifier.Diagnostic().WithSpan(7, 38, 7, 41).WithArguments("Class1", "age"), + }); + + test.FixedState.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(7,27): error CS8050: Only auto-implemented properties, or properties that use the 'field' keyword, can have initializers. + DiagnosticResult.CompilerError("CS8050").WithSpan(7, 27, 7, 31), + + // /0/Test0.cs(7,27): error CS9248: Partial property 'Class1.Name' must have an implementation part. + DiagnosticResult.CompilerError("CS9248").WithSpan(7, 27, 7, 31).WithArguments("Class1.Name"), + + // /0/Test0.cs(10,24): error CS9248: Partial property 'Class1.Age' must have an implementation part. + DiagnosticResult.CompilerError("CS9248").WithSpan(10, 24, 10, 27).WithArguments("Class1.Age"), + }); + + await test.RunAsync(); + } + + [TestMethod] + public async Task SimpleFields_WithMultipleAttributes_WithMixedBuckets_1() + { + string original = """ + using System; + using System.ComponentModel.DataAnnotations; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + // Leading trivia + [ObservableProperty, NotifyPropertyChangedFor("A"), Display] + [NotifyPropertyChangedFor("B")] + private string _name; + } + """; + + string @fixed = """ + using System; + using System.ComponentModel.DataAnnotations; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + // Leading trivia + [ObservableProperty, NotifyPropertyChangedFor("A"), Display] + [NotifyPropertyChangedFor("B")] + public partial string Name { get; set; } + } + """; + + CSharpCodeFixTest test = new(LanguageVersion.Preview) + { + TestCode = original, + FixedCode = @fixed, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + + test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly); + test.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(10,20): info MVVMTK0042: The field Class1._name using [ObservableProperty] can be converted to a partial property instead, which is recommended (doing so improves the developer experience and allows other generators and analyzers to correctly see the generated property as well) + CSharpCodeFixVerifier.Diagnostic().WithSpan(10, 20, 10, 25).WithArguments("Class1", "_name"), + }); + + test.FixedState.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(10,27): error CS9248: Partial property 'Class1.Name' must have an implementation part. + DiagnosticResult.CompilerError("CS9248").WithSpan(10, 27, 10, 31).WithArguments("Class1.Name"), + }); + + await test.RunAsync(); + } + + [TestMethod] + public async Task SimpleFields_WithMultipleAttributes_WithMixedBuckets_2() + { + string original = """ + using System; + using System.ComponentModel.DataAnnotations; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + // Leading trivia + [NotifyPropertyChangedFor("B")] + [ObservableProperty, NotifyPropertyChangedFor("A"), Display, Test] + [NotifyPropertyChangedFor("C")] + [property: UIHint("name"), Test] + private string name; + } + + public class TestAttribute : Attribute; + """; + + string @fixed = """ + using System; + using System.ComponentModel.DataAnnotations; + using CommunityToolkit.Mvvm.ComponentModel; + + public partial class Class1 : ObservableObject + { + // Leading trivia + [NotifyPropertyChangedFor("B")] + [ObservableProperty, NotifyPropertyChangedFor("A"), Display] + [field: Test] + [NotifyPropertyChangedFor("C")] + [UIHint("name"), Test] + public partial string Name { get; set; } + } + + public class TestAttribute : Attribute; + """; + + CSharpCodeFixTest test = new(LanguageVersion.Preview) + { + TestCode = original, + FixedCode = @fixed, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }; + + test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly); + test.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(12,20): info MVVMTK0042: The field Class1.name using [ObservableProperty] can be converted to a partial property instead, which is recommended (doing so improves the developer experience and allows other generators and analyzers to correctly see the generated property as well) + CSharpCodeFixVerifier.Diagnostic().WithSpan(12, 20, 12, 24).WithArguments("Class1", "name"), + }); + + test.FixedState.ExpectedDiagnostics.AddRange(new[] + { + // /0/Test0.cs(13,27): error CS9248: Partial property 'Class1.Name' must have an implementation part. + DiagnosticResult.CompilerError("CS9248").WithSpan(13, 27, 13, 31).WithArguments("Class1.Name"), + }); + + await test.RunAsync(); + } }