Skip to content

Support fixing with attribute lists with multiple attributes #1008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Sergio0694 merged 2 commits into from
Nov 27, 2024
Merged

Support fixing with attribute lists with multiple attributes #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e75139
Update code fixer to support multiple attributes
Sergio0694 Nov 27, 2024
8997b7e
Add unit tests for multiple attributes
Sergio0694 Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 99 additions & 31 deletions src/CommunityToolkit.Mvvm.CodeFixers/UsePartialPropertyForObservablePropertyCodeFixer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,6 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
if (root!.FindNode(diagnosticSpan).FirstAncestorOrSelf<FieldDeclarationSyntax>() 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(
Expand Down Expand Up @@ -194,33 +187,108 @@ private static async Task<Document> 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<AttributeSyntax> mvvmToolkitAttributes = [];
List<AttributeSyntax> annotationOrValidationAttributes = [];
List<AttributeSyntax> 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<AttributeSyntax> 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;
}
}

Expand Down
228 changes: 228 additions & 0 deletions ...eGenerators.Roslyn4120.UnitTests/Test_UsePartialPropertyForObservablePropertyCodeFixer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}