Skip to content

Commit 3533b97

Browse files
Sergio0694Sergio0694
authored
Merge pull request #1008 from CommunityToolkit/dev/code-fixer-multiple-attributes
Support fixing with attribute lists with multiple attributes
2 parents df5ba86 + 8997b7e commit 3533b97

File tree

2 files changed

+327
-31
lines changed

2 files changed

+327
-31
lines changed

‎src/CommunityToolkit.Mvvm.CodeFixers/UsePartialPropertyForObservablePropertyCodeFixer.cs

+99-31
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,6 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
100100
if (root!.FindNode(diagnosticSpan).FirstAncestorOrSelf<FieldDeclarationSyntax>() is { Declaration.Variables: [{ Identifier.Text: string identifierName }] } fieldDeclaration &&
101101
identifierName == fieldName)
102102
{
103-
// We only support fields with up to one attribute per attribute list.
104-
// This is so we can easily check one attribute when updating targets.
105-
if (fieldDeclaration.AttributeLists.Any(static list => list.Attributes.Count > 1))
106-
{
107-
return;
108-
}
109-
110103
// Register the code fix to update the class declaration to inherit from ObservableObject instead
111104
context.RegisterCodeFix(
112105
CodeAction.Create(
@@ -194,33 +187,108 @@ private static async Task<Document> ConvertToPartialProperty(
194187
continue;
195188
}
196189

197-
// Make sure we can retrieve the symbol for the attribute type.
198-
// We are guaranteed to always find a single attribute in the list.
199-
if (!semanticModel.GetSymbolInfo(attributeListSyntax.Attributes[0], cancellationToken).TryGetAttributeTypeSymbol(out INamedTypeSymbol? attributeSymbol))
190+
if (attributeListSyntax.Attributes.Count == 1)
200191
{
201-
return document;
202-
}
203-
204-
// Case 3
205-
if (toolkitTypeSymbols.ContainsValue(attributeSymbol))
206-
{
207-
propertyAttributes[i] = attributeListSyntax.WithTarget(null);
208-
209-
continue;
192+
// Make sure we can retrieve the symbol for the attribute type
193+
if (!semanticModel.GetSymbolInfo(attributeListSyntax.Attributes[0], cancellationToken).TryGetAttributeTypeSymbol(out INamedTypeSymbol? attributeSymbol))
194+
{
195+
return document;
196+
}
197+
198+
// Case 3
199+
if (toolkitTypeSymbols.ContainsValue(attributeSymbol))
200+
{
201+
propertyAttributes[i] = attributeListSyntax.WithTarget(null);
202+
203+
continue;
204+
}
205+
206+
// Case 4
207+
if (annotationTypeSymbols.ContainsValue(attributeSymbol) || attributeSymbol.InheritsFromType(validationAttributeSymbol))
208+
{
209+
continue;
210+
}
211+
212+
// Case 5
213+
if (attributeListSyntax.Target is null)
214+
{
215+
propertyAttributes[i] = attributeListSyntax.WithTarget(AttributeTargetSpecifier(Token(SyntaxKind.FieldKeyword)));
216+
217+
continue;
218+
}
210219
}
211-
212-
// Case 4
213-
if (annotationTypeSymbols.ContainsValue(attributeSymbol) || attributeSymbol.InheritsFromType(validationAttributeSymbol))
214-
{
215-
continue;
216-
}
217-
218-
// Case 5
219-
if (attributeListSyntax.Target is null)
220+
else
220221
{
221-
propertyAttributes[i] = attributeListSyntax.WithTarget(AttributeTargetSpecifier(Token(SyntaxKind.FieldKeyword)));
222-
223-
continue;
222+
// If we have multiple attributes in the current list, we need additional logic here.
223+
// We could have any number of attributes here, so we split them into three buckets:
224+
// - MVVM Toolkit attributes: these should be moved over with no target
225+
// - Data annotation or validation attributes: these should be moved over with the same target
226+
// - Any other attributes: these should be moved over with the 'field' target
227+
List<AttributeSyntax> mvvmToolkitAttributes = [];
228+
List<AttributeSyntax> annotationOrValidationAttributes = [];
229+
List<AttributeSyntax> fieldAttributes = [];
230+
231+
foreach (AttributeSyntax attributeSyntax in attributeListSyntax.Attributes)
232+
{
233+
// Like for the single attribute case, make sure we can get the symbol for the attribute
234+
if (!semanticModel.GetSymbolInfo(attributeSyntax, cancellationToken).TryGetAttributeTypeSymbol(out INamedTypeSymbol? attributeSymbol))
235+
{
236+
return document;
237+
}
238+
239+
bool isAnnotationOrValidationAttribute = annotationTypeSymbols.ContainsValue(attributeSymbol) || attributeSymbol.InheritsFromType(validationAttributeSymbol);
240+
241+
// Split the attributes into the buckets. Note that we have a special rule for annotation and validation
242+
// attributes when no target is specified. In that case, we will merge them with the MVVM Toolkit items.
243+
// This allows us to try to keep the attributes in the same attribute list, rather than splitting them.
244+
if (toolkitTypeSymbols.ContainsValue(attributeSymbol) || (isAnnotationOrValidationAttribute && attributeListSyntax.Target is null))
245+
{
246+
mvvmToolkitAttributes.Add(attributeSyntax);
247+
}
248+
else if (isAnnotationOrValidationAttribute)
249+
{
250+
annotationOrValidationAttributes.Add(attributeSyntax);
251+
}
252+
else
253+
{
254+
fieldAttributes.Add(attributeSyntax);
255+
}
256+
}
257+
258+
// We need to start inserting the new lists right before the one we're currently
259+
// processing. We'll be removing it when we're done, the buckets will replace it.
260+
int insertionIndex = i;
261+
262+
// Helper to process and insert the new synthesized attribute lists into the target collection
263+
void InsertAttributeListIfNeeded(List<AttributeSyntax> attributes, AttributeTargetSpecifierSyntax? attributeTarget)
264+
{
265+
if (attributes is [])
266+
{
267+
return;
268+
}
269+
270+
AttributeListSyntax attributeList = AttributeList(SeparatedList(attributes)).WithTarget(attributeTarget);
271+
272+
// Only if this is the first non empty list we're adding, carry over the original trivia
273+
if (insertionIndex == i)
274+
{
275+
attributeList = attributeList.WithTriviaFrom(attributeListSyntax);
276+
}
277+
278+
// Finally, insert the new list into the final tree
279+
propertyAttributes.Insert(insertionIndex++, attributeList);
280+
}
281+
282+
InsertAttributeListIfNeeded(mvvmToolkitAttributes, attributeTarget: null);
283+
InsertAttributeListIfNeeded(annotationOrValidationAttributes, attributeTarget: attributeListSyntax.Target);
284+
InsertAttributeListIfNeeded(fieldAttributes, attributeTarget: AttributeTargetSpecifier(Token(SyntaxKind.FieldKeyword)));
285+
286+
// Remove the attribute list that we have just split into buckets
287+
propertyAttributes.RemoveAt(insertionIndex);
288+
289+
// Move the current loop iteration to the last inserted item.
290+
// We decrement by 1 because the new loop iteration will add 1.
291+
i = insertionIndex - 1;
224292
}
225293
}
226294

‎tests/CommunityToolkit.Mvvm.SourceGenerators.Roslyn4120.UnitTests/Test_UsePartialPropertyForObservablePropertyCodeFixer.cs

+228
Original file line numberDiff line numberDiff line change
@@ -767,4 +767,232 @@ public void M()
767767

768768
await test.RunAsync();
769769
}
770+
771+
[TestMethod]
772+
public async Task SimpleFields_WithMultipleAttributes_SingleProperty()
773+
{
774+
string original = """
775+
using System;
776+
using CommunityToolkit.Mvvm.ComponentModel;
777+
778+
public partial class Class1 : ObservableObject
779+
{
780+
[ObservableProperty, NotifyPropertyChangedFor("Age")] private string name = String.Empty;
781+
}
782+
""";
783+
784+
string @fixed = """
785+
using System;
786+
using CommunityToolkit.Mvvm.ComponentModel;
787+
788+
public partial class Class1 : ObservableObject
789+
{
790+
[ObservableProperty, NotifyPropertyChangedFor("Age")]
791+
public partial string Name { get; set; } = String.Empty;
792+
}
793+
""";
794+
795+
CSharpCodeFixTest test = new(LanguageVersion.Preview)
796+
{
797+
TestCode = original,
798+
FixedCode = @fixed,
799+
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
800+
};
801+
802+
test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly);
803+
test.ExpectedDiagnostics.AddRange(new[]
804+
{
805+
// /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)
806+
CSharpCodeFixVerifier.Diagnostic().WithSpan(6, 74, 6, 78).WithArguments("Class1", "name"),
807+
});
808+
809+
test.FixedState.ExpectedDiagnostics.AddRange(new[]
810+
{
811+
// /0/Test0.cs(7,27): error CS8050: Only auto-implemented properties, or properties that use the 'field' keyword, can have initializers.
812+
DiagnosticResult.CompilerError("CS8050").WithSpan(7, 27, 7, 31),
813+
814+
// /0/Test0.cs(7,27): error CS9248: Partial property 'Class1.Name' must have an implementation part.
815+
DiagnosticResult.CompilerError("CS9248").WithSpan(7, 27, 7, 31).WithArguments("Class1.Name"),
816+
});
817+
818+
await test.RunAsync();
819+
}
820+
821+
// See https://github.com/CommunityToolkit/dotnet/issues/1007
822+
[TestMethod]
823+
public async Task SimpleFields_WithMultipleAttributes_WithNoBlankLines()
824+
{
825+
string original = """
826+
using System;
827+
using CommunityToolkit.Mvvm.ComponentModel;
828+
829+
public partial class Class1 : ObservableObject
830+
{
831+
[ObservableProperty, NotifyPropertyChangedFor("Age")] private string name = String.Empty;
832+
[ObservableProperty] private int age;
833+
}
834+
""";
835+
836+
string @fixed = """
837+
using System;
838+
using CommunityToolkit.Mvvm.ComponentModel;
839+
840+
public partial class Class1 : ObservableObject
841+
{
842+
[ObservableProperty, NotifyPropertyChangedFor("Age")]
843+
public partial string Name { get; set; } = String.Empty;
844+
845+
[ObservableProperty]
846+
public partial int Age { get; set; }
847+
}
848+
""";
849+
850+
CSharpCodeFixTest test = new(LanguageVersion.Preview)
851+
{
852+
TestCode = original,
853+
FixedCode = @fixed,
854+
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
855+
};
856+
857+
test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly);
858+
test.ExpectedDiagnostics.AddRange(new[]
859+
{
860+
// /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)
861+
CSharpCodeFixVerifier.Diagnostic().WithSpan(6, 74, 6, 78).WithArguments("Class1", "name"),
862+
863+
// /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)
864+
CSharpCodeFixVerifier.Diagnostic().WithSpan(7, 38, 7, 41).WithArguments("Class1", "age"),
865+
});
866+
867+
test.FixedState.ExpectedDiagnostics.AddRange(new[]
868+
{
869+
// /0/Test0.cs(7,27): error CS8050: Only auto-implemented properties, or properties that use the 'field' keyword, can have initializers.
870+
DiagnosticResult.CompilerError("CS8050").WithSpan(7, 27, 7, 31),
871+
872+
// /0/Test0.cs(7,27): error CS9248: Partial property 'Class1.Name' must have an implementation part.
873+
DiagnosticResult.CompilerError("CS9248").WithSpan(7, 27, 7, 31).WithArguments("Class1.Name"),
874+
875+
// /0/Test0.cs(10,24): error CS9248: Partial property 'Class1.Age' must have an implementation part.
876+
DiagnosticResult.CompilerError("CS9248").WithSpan(10, 24, 10, 27).WithArguments("Class1.Age"),
877+
});
878+
879+
await test.RunAsync();
880+
}
881+
882+
[TestMethod]
883+
public async Task SimpleFields_WithMultipleAttributes_WithMixedBuckets_1()
884+
{
885+
string original = """
886+
using System;
887+
using System.ComponentModel.DataAnnotations;
888+
using CommunityToolkit.Mvvm.ComponentModel;
889+
890+
public partial class Class1 : ObservableObject
891+
{
892+
// Leading trivia
893+
[ObservableProperty, NotifyPropertyChangedFor("A"), Display]
894+
[NotifyPropertyChangedFor("B")]
895+
private string _name;
896+
}
897+
""";
898+
899+
string @fixed = """
900+
using System;
901+
using System.ComponentModel.DataAnnotations;
902+
using CommunityToolkit.Mvvm.ComponentModel;
903+
904+
public partial class Class1 : ObservableObject
905+
{
906+
// Leading trivia
907+
[ObservableProperty, NotifyPropertyChangedFor("A"), Display]
908+
[NotifyPropertyChangedFor("B")]
909+
public partial string Name { get; set; }
910+
}
911+
""";
912+
913+
CSharpCodeFixTest test = new(LanguageVersion.Preview)
914+
{
915+
TestCode = original,
916+
FixedCode = @fixed,
917+
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
918+
};
919+
920+
test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly);
921+
test.ExpectedDiagnostics.AddRange(new[]
922+
{
923+
// /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)
924+
CSharpCodeFixVerifier.Diagnostic().WithSpan(10, 20, 10, 25).WithArguments("Class1", "_name"),
925+
});
926+
927+
test.FixedState.ExpectedDiagnostics.AddRange(new[]
928+
{
929+
// /0/Test0.cs(10,27): error CS9248: Partial property 'Class1.Name' must have an implementation part.
930+
DiagnosticResult.CompilerError("CS9248").WithSpan(10, 27, 10, 31).WithArguments("Class1.Name"),
931+
});
932+
933+
await test.RunAsync();
934+
}
935+
936+
[TestMethod]
937+
public async Task SimpleFields_WithMultipleAttributes_WithMixedBuckets_2()
938+
{
939+
string original = """
940+
using System;
941+
using System.ComponentModel.DataAnnotations;
942+
using CommunityToolkit.Mvvm.ComponentModel;
943+
944+
public partial class Class1 : ObservableObject
945+
{
946+
// Leading trivia
947+
[NotifyPropertyChangedFor("B")]
948+
[ObservableProperty, NotifyPropertyChangedFor("A"), Display, Test]
949+
[NotifyPropertyChangedFor("C")]
950+
[property: UIHint("name"), Test]
951+
private string name;
952+
}
953+
954+
public class TestAttribute : Attribute;
955+
""";
956+
957+
string @fixed = """
958+
using System;
959+
using System.ComponentModel.DataAnnotations;
960+
using CommunityToolkit.Mvvm.ComponentModel;
961+
962+
public partial class Class1 : ObservableObject
963+
{
964+
// Leading trivia
965+
[NotifyPropertyChangedFor("B")]
966+
[ObservableProperty, NotifyPropertyChangedFor("A"), Display]
967+
[field: Test]
968+
[NotifyPropertyChangedFor("C")]
969+
[UIHint("name"), Test]
970+
public partial string Name { get; set; }
971+
}
972+
973+
public class TestAttribute : Attribute;
974+
""";
975+
976+
CSharpCodeFixTest test = new(LanguageVersion.Preview)
977+
{
978+
TestCode = original,
979+
FixedCode = @fixed,
980+
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
981+
};
982+
983+
test.TestState.AdditionalReferences.Add(typeof(ObservableObject).Assembly);
984+
test.ExpectedDiagnostics.AddRange(new[]
985+
{
986+
// /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)
987+
CSharpCodeFixVerifier.Diagnostic().WithSpan(12, 20, 12, 24).WithArguments("Class1", "name"),
988+
});
989+
990+
test.FixedState.ExpectedDiagnostics.AddRange(new[]
991+
{
992+
// /0/Test0.cs(13,27): error CS9248: Partial property 'Class1.Name' must have an implementation part.
993+
DiagnosticResult.CompilerError("CS9248").WithSpan(13, 27, 13, 31).WithArguments("Class1.Name"),
994+
});
995+
996+
await test.RunAsync();
997+
}
770998
}

0 commit comments

Comments
 (0)