From fdc390a4aa5def13d99ae95329d027a198f99cd4 Mon Sep 17 00:00:00 2001 From: Vianney de Bellabre Date: Tue, 16 Apr 2024 18:32:24 +0200 Subject: [PATCH] EC84 avoid async void method (#29) --- ...3_ReplaceEnumToStringWithNameOfAnalyzer.cs | 3 +- .../EC84_AvoidAsyncVoidMethodsAnalyzer.cs | 39 ++++++++ src/EcoCode.Analyzers/Rule.cs | 1 + ...EC81_SpecifyStructLayoutCodeFixProvider.cs | 10 +- ...ariableCanBeMadeConstantCodeFixProvider.cs | 35 ++++--- ...ceEnumToStringWithNameOfCodeFixProvider.cs | 9 +- ...84_AvoidAsyncVoidMethodsCodeFixProvider.cs | 56 ++++++++++++ src/EcoCode.CodeFixes/GlobalUsings.cs | 6 ++ .../Extensions/SymbolExtensions.cs | 17 +++- .../Extensions/SyntaxNodeExtensions.cs | 34 ++++++- src/EcoCode.Shared/GlobalUsings.cs | 1 - .../EC84_AvoidAsyncVoidMethodsUnitTests.cs | 91 +++++++++++++++++++ .../source.extension.vsixmanifest | 2 +- 13 files changed, 261 insertions(+), 43 deletions(-) create mode 100644 src/EcoCode.Analyzers/Analyzers/EC84_AvoidAsyncVoidMethodsAnalyzer.cs create mode 100644 src/EcoCode.CodeFixes/CodeFixes/EC84_AvoidAsyncVoidMethodsCodeFixProvider.cs create mode 100644 src/EcoCode.Tests/Tests/EC84_AvoidAsyncVoidMethodsUnitTests.cs diff --git a/src/EcoCode.Analyzers/Analyzers/EC83_ReplaceEnumToStringWithNameOfAnalyzer.cs b/src/EcoCode.Analyzers/Analyzers/EC83_ReplaceEnumToStringWithNameOfAnalyzer.cs index 168b7c16..96365579 100644 --- a/src/EcoCode.Analyzers/Analyzers/EC83_ReplaceEnumToStringWithNameOfAnalyzer.cs +++ b/src/EcoCode.Analyzers/Analyzers/EC83_ReplaceEnumToStringWithNameOfAnalyzer.cs @@ -1,5 +1,4 @@ using Microsoft.CodeAnalysis.Operations; -using System.Linq; namespace EcoCode.Analyzers; @@ -34,7 +33,7 @@ public override void Initialize(AnalysisContext context) } private static IFieldSymbol GetStringEmptySymbol(Compilation compilation) => - (IFieldSymbol)compilation.GetTypeByMetadataName("System.String")!.GetMembers("Empty").First(); + (IFieldSymbol)compilation.GetTypeByMetadataName("System.String")!.GetMembers("Empty")[0]; private static bool UsesConstantFormat(object? format) => format is null || format is string str && (str.Length == 0 || str is "g" or "G" or "f" or "F"); diff --git a/src/EcoCode.Analyzers/Analyzers/EC84_AvoidAsyncVoidMethodsAnalyzer.cs b/src/EcoCode.Analyzers/Analyzers/EC84_AvoidAsyncVoidMethodsAnalyzer.cs new file mode 100644 index 00000000..ef887a48 --- /dev/null +++ b/src/EcoCode.Analyzers/Analyzers/EC84_AvoidAsyncVoidMethodsAnalyzer.cs @@ -0,0 +1,39 @@ +namespace EcoCode.Analyzers; + +/// Analyzer for avoid async void methods. +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class AvoidAsyncVoidMethodsAnalyzer : DiagnosticAnalyzer +{ + /// The diagnostic descriptor. + public static DiagnosticDescriptor Descriptor { get; } = new( + Rule.Ids.EC84_AvoidAsyncVoidMethods, + title: "Avoid async void methods", + messageFormat: "Avoid async void methods", + Rule.Categories.Design, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: null, + helpLinkUri: Rule.GetHelpUri(Rule.Ids.EC84_AvoidAsyncVoidMethods)); + + /// + public override ImmutableArray SupportedDiagnostics => [Descriptor]; + + /// + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(static context => AnalyzeMethod(context), SyntaxKind.MethodDeclaration); + } + + private static void AnalyzeMethod(SyntaxNodeAnalysisContext context) + { + var methodDeclaration = (MethodDeclarationSyntax)context.Node; + if (methodDeclaration.Modifiers.Any(SyntaxKind.AsyncKeyword) && + methodDeclaration.ReturnType is PredefinedTypeSyntax returnType && + returnType.Keyword.IsKind(SyntaxKind.VoidKeyword)) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptor, methodDeclaration.Identifier.GetLocation())); + } + } +} diff --git a/src/EcoCode.Analyzers/Rule.cs b/src/EcoCode.Analyzers/Rule.cs index 67043ad1..6dd3b16b 100644 --- a/src/EcoCode.Analyzers/Rule.cs +++ b/src/EcoCode.Analyzers/Rule.cs @@ -20,5 +20,6 @@ public static class Ids public const string EC81_UseStructLayout = "EC81"; public const string EC82_VariableCanBeMadeConstant = "EC82"; public const string EC83_ReplaceEnumToStringWithNameOf = "EC83"; + public const string EC84_AvoidAsyncVoidMethods = "EC84"; } } diff --git a/src/EcoCode.CodeFixes/CodeFixes/EC81_SpecifyStructLayoutCodeFixProvider.cs b/src/EcoCode.CodeFixes/CodeFixes/EC81_SpecifyStructLayoutCodeFixProvider.cs index 529c12d0..466eb7b7 100644 --- a/src/EcoCode.CodeFixes/CodeFixes/EC81_SpecifyStructLayoutCodeFixProvider.cs +++ b/src/EcoCode.CodeFixes/CodeFixes/EC81_SpecifyStructLayoutCodeFixProvider.cs @@ -1,10 +1,4 @@ -using Microsoft.CodeAnalysis.CodeActions; -using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.Editing; -using Microsoft.CodeAnalysis.Simplification; -using System.Threading; - -namespace EcoCode.CodeFixes; +namespace EcoCode.CodeFixes; /// The code fix provider for use struct layout. [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(SpecifyStructLayoutCodeFixProvider)), Shared] @@ -19,6 +13,8 @@ public sealed class SpecifyStructLayoutCodeFixProvider : CodeFixProvider /// public override async Task RegisterCodeFixesAsync(CodeFixContext context) { + if (context.Diagnostics.Length == 0) return; + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); var node = root?.FindNode(context.Span, getInnermostNodeForTie: true); if (node is not TypeDeclarationSyntax nodeToFix) return; diff --git a/src/EcoCode.CodeFixes/CodeFixes/EC82_VariableCanBeMadeConstantCodeFixProvider.cs b/src/EcoCode.CodeFixes/CodeFixes/EC82_VariableCanBeMadeConstantCodeFixProvider.cs index a8696e6b..d6d9a34f 100644 --- a/src/EcoCode.CodeFixes/CodeFixes/EC82_VariableCanBeMadeConstantCodeFixProvider.cs +++ b/src/EcoCode.CodeFixes/CodeFixes/EC82_VariableCanBeMadeConstantCodeFixProvider.cs @@ -1,9 +1,4 @@ -using Microsoft.CodeAnalysis.CodeActions; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.Formatting; -using Microsoft.CodeAnalysis.Simplification; -using System.Threading; +using Microsoft.CodeAnalysis.Formatting; namespace EcoCode.CodeFixes; @@ -26,20 +21,22 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) var root = await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); if (root is null) return; - var diagnostic = context.Diagnostics[0]; - var parent = root.FindToken(diagnostic.Location.SourceSpan.Start).Parent; - if (parent is null) return; - - foreach (var node in parent.AncestorsAndSelf()) + foreach (var diagnostic in context.Diagnostics) { - if (node is not LocalDeclarationStatementSyntax declaration) continue; - context.RegisterCodeFix( - CodeAction.Create( - title: "Make variable constant", - createChangedDocument: token => RefactorAsync(document, declaration, token), - equivalenceKey: "Make variable constant"), - diagnostic); - break; + var parent = root.FindToken(diagnostic.Location.SourceSpan.Start).Parent; + if (parent is null) return; + + foreach (var node in parent.AncestorsAndSelf()) + { + if (node is not LocalDeclarationStatementSyntax declaration) continue; + context.RegisterCodeFix( + CodeAction.Create( + title: "Make variable constant", + createChangedDocument: token => RefactorAsync(document, declaration, token), + equivalenceKey: "Make variable constant"), + diagnostic); + break; + } } } diff --git a/src/EcoCode.CodeFixes/CodeFixes/EC83_ReplaceEnumToStringWithNameOfCodeFixProvider.cs b/src/EcoCode.CodeFixes/CodeFixes/EC83_ReplaceEnumToStringWithNameOfCodeFixProvider.cs index ed01d3d1..a695ee08 100644 --- a/src/EcoCode.CodeFixes/CodeFixes/EC83_ReplaceEnumToStringWithNameOfCodeFixProvider.cs +++ b/src/EcoCode.CodeFixes/CodeFixes/EC83_ReplaceEnumToStringWithNameOfCodeFixProvider.cs @@ -1,9 +1,4 @@ -using Microsoft.CodeAnalysis.CodeActions; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.Editing; -using Microsoft.CodeAnalysis.Operations; -using System.Threading; +using Microsoft.CodeAnalysis.Operations; namespace EcoCode.CodeFixes; @@ -32,7 +27,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) title: "Use nameof", createChangedDocument: token => RefactorAsync(document, nodeToFix, token), equivalenceKey: "Use nameof"), - context.Diagnostics[0]); + context.Diagnostics); } private static async Task RefactorAsync(Document document, SyntaxNode node, CancellationToken token) diff --git a/src/EcoCode.CodeFixes/CodeFixes/EC84_AvoidAsyncVoidMethodsCodeFixProvider.cs b/src/EcoCode.CodeFixes/CodeFixes/EC84_AvoidAsyncVoidMethodsCodeFixProvider.cs new file mode 100644 index 00000000..9ee54747 --- /dev/null +++ b/src/EcoCode.CodeFixes/CodeFixes/EC84_AvoidAsyncVoidMethodsCodeFixProvider.cs @@ -0,0 +1,56 @@ +namespace EcoCode.CodeFixes; + +/// The code fix provider for avoid async void methods. +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AvoidAsyncVoidMethodsCodeFixProvider)), Shared] +public sealed class AvoidAsyncVoidMethodsCodeFixProvider : CodeFixProvider +{ + /// + public override ImmutableArray FixableDiagnosticIds => [AvoidAsyncVoidMethodsAnalyzer.Descriptor.Id]; + + /// + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + /// + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + if (context.Diagnostics.Length == 0) return; + + var document = context.Document; + var root = await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) return; + + foreach (var diagnostic in context.Diagnostics) + { + var parent = root.FindToken(diagnostic.Location.SourceSpan.Start).Parent; + if (parent is null) continue; + + foreach (var node in parent.AncestorsAndSelf()) + { + if (node is not MethodDeclarationSyntax declaration) continue; + context.RegisterCodeFix( + CodeAction.Create( + title: "Convert to async Task", + createChangedDocument: token => RefactorAsync(document, declaration, token), + equivalenceKey: "Convert to async Task"), + diagnostic); + break; + } + } + } + + private static async Task RefactorAsync(Document document, MethodDeclarationSyntax methodDecl, CancellationToken token) + { + // Note : it may seem a good idea to add the System.Thread.Tasks using directive if it isn't present yet + // However it isn't properly doable because : + // - It could be added as a global using in a different file, but Roslyn doesn't give easy access to those + // - The user could have enabled the ImplicitUsings option, which makes the using directives both global and invisible to the analyzer + // So as a result, we simply don't handle it + + var root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false); + return root is null ? document : document.WithSyntaxRoot( + root.ReplaceNode(methodDecl, methodDecl.WithReturnType( + SyntaxFactory.IdentifierName("Task") // Change the return type of the method to Task + .WithLeadingTrivia(methodDecl.ReturnType.GetLeadingTrivia()) + .WithTrailingTrivia(methodDecl.ReturnType.GetTrailingTrivia())))); + } +} diff --git a/src/EcoCode.CodeFixes/GlobalUsings.cs b/src/EcoCode.CodeFixes/GlobalUsings.cs index 6bf54f4b..19a890f1 100644 --- a/src/EcoCode.CodeFixes/GlobalUsings.cs +++ b/src/EcoCode.CodeFixes/GlobalUsings.cs @@ -1,8 +1,14 @@ global using EcoCode.Analyzers; global using EcoCode.Shared; global using Microsoft.CodeAnalysis; +global using Microsoft.CodeAnalysis.CodeActions; global using Microsoft.CodeAnalysis.CodeFixes; +global using Microsoft.CodeAnalysis.CSharp; +global using Microsoft.CodeAnalysis.CSharp.Syntax; +global using Microsoft.CodeAnalysis.Editing; +global using Microsoft.CodeAnalysis.Simplification; global using System.Collections.Immutable; global using System.Composition; global using System.Runtime.InteropServices; +global using System.Threading; global using System.Threading.Tasks; diff --git a/src/EcoCode.Shared/Extensions/SymbolExtensions.cs b/src/EcoCode.Shared/Extensions/SymbolExtensions.cs index 87c257b1..1d017023 100644 --- a/src/EcoCode.Shared/Extensions/SymbolExtensions.cs +++ b/src/EcoCode.Shared/Extensions/SymbolExtensions.cs @@ -58,9 +58,10 @@ public static bool IsDeclaredOutsideLoop(this ISymbol symbol, SyntaxNode loopNod switch (symbol) { case ILocalSymbol localSymbol: - if (localSymbol.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference syntaxRef) + var localRefs = localSymbol.DeclaringSyntaxReferences; + if (localRefs.Length != 0) { - for (var node = syntaxRef.GetSyntax(); node is not null; node = node.Parent) + for (var node = localRefs[0].GetSyntax(); node is not null; node = node.Parent) { if (node is BlockSyntax or MethodDeclarationSyntax or CompilationUnitSyntax) return node; @@ -69,13 +70,19 @@ public static bool IsDeclaredOutsideLoop(this ISymbol symbol, SyntaxNode loopNod break; case IFieldSymbol fieldSymbol: - return fieldSymbol.ContainingType.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); + var fieldRefs = fieldSymbol.ContainingType.DeclaringSyntaxReferences; + if (fieldRefs.Length != 0) return fieldRefs[0].GetSyntax(); + break; case IPropertySymbol propertySymbol: - return propertySymbol.ContainingType.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); + var propRefs = propertySymbol.ContainingType.DeclaringSyntaxReferences; + if (propRefs.Length != 0) return propRefs[0].GetSyntax(); + break; case IParameterSymbol parameterSymbol: - return parameterSymbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); + var paramRefs = parameterSymbol.DeclaringSyntaxReferences; + if (paramRefs.Length != 0) return paramRefs[0].GetSyntax(); + break; } return null; } diff --git a/src/EcoCode.Shared/Extensions/SyntaxNodeExtensions.cs b/src/EcoCode.Shared/Extensions/SyntaxNodeExtensions.cs index e481764a..03d30eb1 100644 --- a/src/EcoCode.Shared/Extensions/SyntaxNodeExtensions.cs +++ b/src/EcoCode.Shared/Extensions/SyntaxNodeExtensions.cs @@ -1,4 +1,6 @@ -namespace EcoCode.Shared; +using Microsoft.CodeAnalysis.CSharp; + +namespace EcoCode.Shared; /// Extensions methods for . public static class SyntaxNodeExtensions @@ -37,4 +39,34 @@ public static SyntaxList GetLoopStatements(this SyntaxNode node _ => default, }; } + + /// Returns whether a node has a given using directive, use with a document root. + /// The node. + /// The namespace of the using directive. + /// True if the node has the given using, false otherwise. + public static bool HasUsingDirective(this SyntaxNode node, string @namespace) + { + foreach (var descendant in node.DescendantNodes()) + { + if ((descendant as UsingDirectiveSyntax)?.Name?.ToString() == @namespace) + return true; + } + return false; + } + + /// Adds a using directive to the given node, use with a document root. + /// The node. + /// The namespace of the using directive. + /// The updated node with the using directive. + public static SyntaxNode AddUsingDirective(this SyntaxNode node, string @namespace) + { + var usingDirective = SyntaxFactory.UsingDirective(SyntaxFactory.ParseName(@namespace)).NormalizeWhitespace(); + + foreach (var descendant in node.DescendantNodes()) + { + if (descendant is NamespaceDeclarationSyntax namespaceDeclaration) + return node.ReplaceNode(namespaceDeclaration, namespaceDeclaration.AddUsings(usingDirective)); + } + return ((CompilationUnitSyntax)node).AddUsings(usingDirective); // Add the using directive at the top of the file + } } diff --git a/src/EcoCode.Shared/GlobalUsings.cs b/src/EcoCode.Shared/GlobalUsings.cs index 2e7d51b1..06f414ac 100644 --- a/src/EcoCode.Shared/GlobalUsings.cs +++ b/src/EcoCode.Shared/GlobalUsings.cs @@ -1,3 +1,2 @@ global using Microsoft.CodeAnalysis; global using Microsoft.CodeAnalysis.CSharp.Syntax; -global using System.Linq; diff --git a/src/EcoCode.Tests/Tests/EC84_AvoidAsyncVoidMethodsUnitTests.cs b/src/EcoCode.Tests/Tests/EC84_AvoidAsyncVoidMethodsUnitTests.cs new file mode 100644 index 00000000..60a898bc --- /dev/null +++ b/src/EcoCode.Tests/Tests/EC84_AvoidAsyncVoidMethodsUnitTests.cs @@ -0,0 +1,91 @@ +namespace EcoCode.Tests; + +[TestClass] +public class AvoidAsyncVoidMethodsUnitTests +{ + private static readonly VerifyDlg VerifyAsync = CodeFixVerifier.VerifyAsync< + AvoidAsyncVoidMethodsAnalyzer, + AvoidAsyncVoidMethodsCodeFixProvider>; + + [TestMethod] + public async Task EmptyCodeAsync() => await VerifyAsync("").ConfigureAwait(false); + + [TestMethod] + public async Task AvoidAsyncVoidMethodAsync() => await VerifyAsync(""" + using System; + using System.Threading.Tasks; + public static class Program + { + public static async void [|Main|]() + { + await Task.Delay(1000).ConfigureAwait(false); + Console.WriteLine(); + } + } + """, + fixedSource: """ + using System; + using System.Threading.Tasks; + public static class Program + { + public static async Task Main() + { + await Task.Delay(1000).ConfigureAwait(false); + Console.WriteLine(); + } + } + """).ConfigureAwait(false); + + [TestMethod] + public async Task AvoidAsyncVoidMethodWithMissingUsingAsync() => await VerifyAsync(""" + using System; + using System.Net.Http; + public static class Program + { + public static async void [|Main|]() + { + using var httpClient = new HttpClient(); + _ = await httpClient.GetAsync(new Uri("URL")).ConfigureAwait(false); + } + } + """, + fixedSource: """ + using System; + using System.Net.Http; + public static class Program + { + public static async {|CS0246:Task|} {|CS0161:Main|}() + { + using var httpClient = new HttpClient(); + _ = await httpClient.GetAsync(new Uri("URL")).ConfigureAwait(false); + } + } + """).ConfigureAwait(false); + + [TestMethod] + public async Task AsyncTaskMethodIsOkAsync() => await VerifyAsync(""" + using System; + using System.Threading.Tasks; + public static class Program + { + public static async Task Main() + { + await Task.Delay(1000).ConfigureAwait(false); + Console.WriteLine(); + } + } + """).ConfigureAwait(false); + + [TestMethod] + public async Task AsyncGenericTaskMethodIsOkAsync() => await VerifyAsync(""" + using System.Threading.Tasks; + public static class Program + { + public static async Task Main() + { + await Task.Delay(1000).ConfigureAwait(false); + return 1; + } + } + """).ConfigureAwait(false); +} diff --git a/src/EcoCode.Vsix/source.extension.vsixmanifest b/src/EcoCode.Vsix/source.extension.vsixmanifest index 230576d6..231880a4 100644 --- a/src/EcoCode.Vsix/source.extension.vsixmanifest +++ b/src/EcoCode.Vsix/source.extension.vsixmanifest @@ -1,7 +1,7 @@ - + EcoCode EcoCode extension for the .NET Compiler Platform ("Roslyn"). https://github.com/green-code-initiative/ecoCode-csharp