Skip to content

Commit

Permalink
Don't warn on instance fields and properties
Browse files Browse the repository at this point in the history 
Also fix some other misc bugs, eg. around trivia
  • Loading branch information
@Sergio0694
Sergio0694 committed Dec 28, 2024
1 parent 0d01d5c commit 922b016
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 58 deletions.
18 changes: 1 addition & 17 deletions ...r/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCodeFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ private static bool IsCodeFixSupportedForPropertyDeclaration(PropertyDeclaration
return false;
}

bool isStatic = false;

foreach (SyntaxToken modifier in propertyDeclaration.Modifiers)
{
// Accessibility modifiers are allowed (the property will however become public)
Expand All @@ -84,35 +82,21 @@ private static bool IsCodeFixSupportedForPropertyDeclaration(PropertyDeclaration
continue;
}

// Track whether the property is static
if (modifier.IsKind(SyntaxKind.StaticKeyword))
{
isStatic = true;

continue;
}

// If the property is abstract or an override, or other weird things (which shouldn't really happen), we don't support it
if (modifier.Kind() is SyntaxKind.AbstractKeyword or SyntaxKind.OverrideKeyword or SyntaxKind.PartialKeyword or SyntaxKind.ExternKeyword)
{
return false;
}
}

// We don't support fixing instance properties automatically, as that might break code
if (!isStatic)
{
return false;
}

// Properties with an expression body are supported and will be converted to field initializers
if (propertyDeclaration.ExpressionBody is not null)
{
return true;
}

// The property must have at least an accessor
if (propertyDeclaration.AccessorList is not { Accessors: { Count: > 0 } } accessorList)
if (propertyDeclaration.AccessorList is not { Accessors.Count: > 0 } accessorList)
{
return false;
}
Expand Down
5 changes: 4 additions & 1 deletion ...tyToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCorrectlyCodeFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ private static async Task<Document> FixDependencyPropertyFieldDeclaration(Docume
// We use the lambda overload mostly for convenient, so we can easily get a generator to use
syntaxEditor.ReplaceNode(fieldDeclaration, (node, generator) =>
{
// Keep the original node to get the trivia back from it
SyntaxNode originalNode = node;

// Update the field to ensure it's declared as 'public static readonly'
node = generator.WithAccessibility(node, Accessibility.Public);
node = generator.WithModifiers(node, DeclarationModifiers.Static | DeclarationModifiers.ReadOnly);
Expand All @@ -82,7 +85,7 @@ private static async Task<Document> FixDependencyPropertyFieldDeclaration(Docume
node = ((FieldDeclarationSyntax)node).WithDeclaration(variableDeclaration.WithType(typeDeclaration));
}

return node;
return node.WithTriviaFrom(originalNode);
});

// Create the new document with the single change
Expand Down
12 changes: 12 additions & 0 deletions ...cyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,18 @@ public override void Initialize(AnalysisContext context)
{
IPropertySymbol propertySymbol = (IPropertySymbol)context.Symbol;

// Ignore instance properties (same as in the other analyzer), and interface members
if (propertySymbol is { IsStatic: false } or { ContainingType.TypeKind: TypeKind.Interface })
{
return;
}

// Also ignore properties that are write-only (there can't really be normal dependency properties)
if (propertySymbol.IsWriteOnly)
{
return;
}

// We only care about properties which are of type 'DependencyProperty'
if (!SymbolEqualityComparer.Default.Equals(propertySymbol.Type, dependencyPropertySymbol))
{
Expand Down
8 changes: 7 additions & 1 deletion ...yGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationCorrectlyAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ public override void Initialize(AnalysisContext context)
{
IFieldSymbol fieldSymbol = (IFieldSymbol)context.Symbol;

// Ignore instance fields, to reduce false positives. There might be edge cases
// where property instances are used as state, and that's technically not invalid.
if (!fieldSymbol.IsStatic)
{
return;
}

// We only care about fields which are of type 'DependencyProperty'
if (!SymbolEqualityComparer.Default.Equals(fieldSymbol.Type, dependencyPropertySymbol))
{
Expand All @@ -51,7 +58,6 @@ public override void Initialize(AnalysisContext context)
// Fields should always be public, static, readonly, and with nothing else on them
if (fieldSymbol is
{ DeclaredAccessibility: not Accessibility.Public } or
{ IsStatic: false } or
{ IsRequired: true } or
{ IsReadOnly: false } or
{ IsVolatile: true } or
Expand Down
5 changes: 2 additions & 3 deletions ...ncyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_Analyzers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1850,7 +1850,6 @@ public class MyObject : DependencyObject

[TestMethod]
[DataRow("private static readonly DependencyProperty")]
[DataRow("public readonly DependencyProperty")]
[DataRow("public static DependencyProperty")]
[DataRow("public static volatile DependencyProperty")]
[DataRow("public static readonly DependencyProperty?")]
Expand Down Expand Up @@ -1964,7 +1963,7 @@ public abstract class MyBase : DependencyObject
}

[TestMethod]
public async Task UseFieldDeclarationAnalyzer_WithNoSetter_DoesNotWarn()
public async Task UseFieldDeclarationAnalyzer_WithNoGetter_DoesNotWarn()
{
const string source = """
using Windows.UI.Xaml;
Expand All @@ -1991,7 +1990,7 @@ public class MyObject : DependencyObject
{
public static DependencyProperty {|WCTDP0021:Test1Property|} => DependencyProperty.Register("Test1", typeof(string), typeof(MyObject), null);
public static DependencyProperty {|WCTDP0021:Test2Property|} { get; } = DependencyProperty.Register("Test2", typeof(string), typeof(MyObject), null);
public DependencyProperty {|WCTDP0021:Test3Property|} { get; set; }
public static DependencyProperty {|WCTDP0021:Test3Property|} { get; set; }
}
""";

Expand Down
32 changes: 6 additions & 26 deletions ...r/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCodeFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,17 +137,10 @@ public class MyObject : DependencyObject
typeof(MyObject),
null);
public DependencyProperty [|Test4Property|] => DependencyProperty.Register(
"Test4",
typeof(string),
typeof(MyObject),
null);
public static DependencyProperty [|Test5Property|] { get; }
public virtual DependencyProperty [|Test6Property|] { get; }
public static DependencyProperty [|Test4Property|] { get; }
[Test]
public static DependencyProperty [|Test7Property|] { get; }
public static DependencyProperty [|Test5Property|] { get; }
}
public class TestAttribute : Attribute;
Expand Down Expand Up @@ -179,17 +172,10 @@ public class MyObject : DependencyObject
typeof(MyObject),
null);
public DependencyProperty Test4Property => DependencyProperty.Register(
"Test4",
typeof(string),
typeof(MyObject),
null);
public static readonly DependencyProperty Test5Property;
public virtual DependencyProperty Test6Property { get; }
public static readonly DependencyProperty Test4Property;
[Test]
public static DependencyProperty [|Test7Property|] { get; }
public static DependencyProperty Test5Property { get; }
}
public class TestAttribute : Attribute;
Expand All @@ -203,14 +189,8 @@ public class TestAttribute : Attribute;

test.FixedState.ExpectedDiagnostics.AddRange(
[
// /0/Test0.cs(26,31): warning WCTDP0021: The property 'MyApp.MyObject.Test4Property' is a dependency property, which is not the correct declaration type (all dependency properties should be declared as fields, unless implementing interface members or in authored WinRT component types)
CSharpCodeFixVerifier.Diagnostic().WithSpan(26, 31, 26, 44).WithArguments("MyApp.MyObject.Test4Property"),

// /0/Test0.cs(33,39): warning WCTDP0021: The property 'MyApp.MyObject.Test6Property' is a dependency property, which is not the correct declaration type (all dependency properties should be declared as fields, unless implementing interface members or in authored WinRT component types)
CSharpCodeFixVerifier.Diagnostic().WithSpan(33, 39, 33, 52).WithArguments("MyApp.MyObject.Test6Property"),

// /0/Test0.cs(36,38): warning WCTDP0021: The property 'MyApp.MyObject.Test7Property' is a dependency property, which is not the correct declaration type (all dependency properties should be declared as fields, unless implementing interface members or in authored WinRT component types)
CSharpCodeFixVerifier.Diagnostic().WithSpan(36, 38, 36, 51).WithArguments("MyApp.MyObject.Test7Property")
// /0/Test0.cs(29,38): warning WCTDP0021: The property 'MyApp.MyObject.Test5Property' is a dependency property, which is not the correct declaration type (all dependency properties should be declared as fields, unless implementing interface members or in authored WinRT component types)
CSharpCodeFixVerifier.Diagnostic().WithSpan(29, 38, 29, 51).WithArguments("MyApp.MyObject.Test5Property")
]);

await test.RunAsync();
Expand Down
16 changes: 6 additions & 10 deletions ...tyToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCorrectlyCodeFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ public class Test_UseFieldDeclarationCorrectlyCodeFixer
{
[TestMethod]
[DataRow("private static readonly DependencyProperty")]
[DataRow("public readonly DependencyProperty")]
[DataRow("public static DependencyProperty")]
[DataRow("public static volatile DependencyProperty")]
[DataRow("public static readonly DependencyProperty?")]
Expand Down Expand Up @@ -59,7 +58,6 @@ public class MyObject : DependencyObject

[TestMethod]
[DataRow("private static readonly DependencyProperty")]
[DataRow("public readonly DependencyProperty")]
[DataRow("public static DependencyProperty")]
[DataRow("public static volatile DependencyProperty")]
[DataRow("public static readonly DependencyProperty?")]
Expand Down Expand Up @@ -121,13 +119,12 @@ namespace MyApp;
public class MyObject : DependencyObject
{
private static readonly DependencyProperty [|Test1Property|];
public readonly DependencyProperty [|Test2Property|];
public static DependencyProperty [|Test3Property|];
public static readonly DependencyProperty Test4Property;
public static volatile DependencyProperty [|Test5Property|];
public static readonly DependencyProperty? [|Test6Property|];
public static DependencyProperty [|Test2Property|];
public static readonly DependencyProperty Test3Property;
public static volatile DependencyProperty [|Test4Property|];
public static readonly DependencyProperty? [|Test5Property|];
public static readonly DependencyProperty Test6Property;
public static readonly DependencyProperty Test7Property;
public static readonly DependencyProperty Test8Property;
}
""";

Expand All @@ -147,7 +144,6 @@ public class MyObject : DependencyObject
public static readonly DependencyProperty Test5Property;
public static readonly DependencyProperty Test6Property;
public static readonly DependencyProperty Test7Property;
public static readonly DependencyProperty Test8Property;
}
""";

Expand Down Expand Up @@ -178,7 +174,7 @@ public class MyObject : DependencyObject
typeof(MyObject),
null);
public readonly DependencyProperty [|Test2Property|] = DependencyProperty.Register(
internal static volatile DependencyProperty [|Test2Property|] = DependencyProperty.Register(
"Test2",
typeof(string),
typeof(MyObject),
Expand Down

0 comments on commit 922b016

Please sign in to comment.