From 922b01620e930a77592436781c2d2e48c7a392ff Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sun, 29 Dec 2024 00:34:46 +0100 Subject: [PATCH] Don't warn on instance fields and properties Also fix some other misc bugs, eg. around trivia --- .../UseFieldDeclarationCodeFixer.cs | 18 +---------- .../UseFieldDeclarationCorrectlyCodeFixer.cs | 5 ++- .../Analyzers/UseFieldDeclarationAnalyzer.cs | 12 +++++++ .../UseFieldDeclarationCorrectlyAnalyzer.cs | 8 ++++- .../Test_Analyzers.cs | 5 ++- .../Test_UseFieldDeclarationCodeFixer.cs | 32 ++++--------------- ...t_UseFieldDeclarationCorrectlyCodeFixer.cs | 16 ++++------ 7 files changed, 38 insertions(+), 58 deletions(-) diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCodeFixer.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCodeFixer.cs index e75e8ed0..80b5ffc1 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCodeFixer.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCodeFixer.cs @@ -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) @@ -84,14 +82,6 @@ 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) { @@ -99,12 +89,6 @@ private static bool IsCodeFixSupportedForPropertyDeclaration(PropertyDeclaration } } - // 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) { @@ -112,7 +96,7 @@ private static bool IsCodeFixSupportedForPropertyDeclaration(PropertyDeclaration } // 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; } diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCorrectlyCodeFixer.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCorrectlyCodeFixer.cs index 4ad19f20..5e750ab4 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCorrectlyCodeFixer.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.CodeFixers/UseFieldDeclarationCorrectlyCodeFixer.cs @@ -68,6 +68,9 @@ private static async Task 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); @@ -82,7 +85,7 @@ private static async Task FixDependencyPropertyFieldDeclaration(Docume node = ((FieldDeclarationSyntax)node).WithDeclaration(variableDeclaration.WithType(typeDeclaration)); } - return node; + return node.WithTriviaFrom(originalNode); }); // Create the new document with the single change diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationAnalyzer.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationAnalyzer.cs index 845ad0b7..0e095652 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationAnalyzer.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationAnalyzer.cs @@ -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)) { diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationCorrectlyAnalyzer.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationCorrectlyAnalyzer.cs index 1acf9b2d..2b6394af 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationCorrectlyAnalyzer.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.SourceGenerators/Diagnostics/Analyzers/UseFieldDeclarationCorrectlyAnalyzer.cs @@ -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)) { @@ -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 diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_Analyzers.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_Analyzers.cs index 9ccbd559..58996f02 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_Analyzers.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_Analyzers.cs @@ -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?")] @@ -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; @@ -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; } } """; diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCodeFixer.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCodeFixer.cs index 3cf59ab0..10aaa50b 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCodeFixer.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCodeFixer.cs @@ -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; @@ -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; @@ -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(); diff --git a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCorrectlyCodeFixer.cs b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCorrectlyCodeFixer.cs index afd3cb36..d31d3ad7 100644 --- a/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCorrectlyCodeFixer.cs +++ b/components/DependencyPropertyGenerator/CommunityToolkit.DependencyPropertyGenerator.Tests/Test_UseFieldDeclarationCorrectlyCodeFixer.cs @@ -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?")] @@ -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?")] @@ -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; } """; @@ -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; } """; @@ -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),