From 6193b2230ad6c5068e1345e6aa002cbef89a9541 Mon Sep 17 00:00:00 2001 From: Brad Wilson Date: Wed, 2 Aug 2023 13:50:46 -0700 Subject: [PATCH] xunit/xunit#2628: Add "Don't use ConfigureAwait" analyzer and fixer (xUnit1030) --- .../X1000/DoNotUseConfigureAwaitFixer.cs | 50 ++++++++++++ .../X1000/DoNotUseConfigureAwaitTests.cs | 78 ++++++++++++++++++ .../X1000/DoNotUseConfigureAwaitFixerTests.cs | 68 ++++++++++++++++ src/xunit.analyzers/Utility/Descriptors.cs | 9 ++- .../X1000/DoNotUseConfigureAwait.cs | 81 +++++++++++++++++++ 5 files changed, 285 insertions(+), 1 deletion(-) create mode 100644 src/xunit.analyzers.fixes/X1000/DoNotUseConfigureAwaitFixer.cs create mode 100644 src/xunit.analyzers.tests/Analyzers/X1000/DoNotUseConfigureAwaitTests.cs create mode 100644 src/xunit.analyzers.tests/Fixes/X1000/DoNotUseConfigureAwaitFixerTests.cs create mode 100644 src/xunit.analyzers/X1000/DoNotUseConfigureAwait.cs diff --git a/src/xunit.analyzers.fixes/X1000/DoNotUseConfigureAwaitFixer.cs b/src/xunit.analyzers.fixes/X1000/DoNotUseConfigureAwaitFixer.cs new file mode 100644 index 00000000..3aa956f4 --- /dev/null +++ b/src/xunit.analyzers.fixes/X1000/DoNotUseConfigureAwaitFixer.cs @@ -0,0 +1,50 @@ +using System.Composition; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Editing; + +namespace Xunit.Analyzers.Fixes; + +[ExportCodeFixProvider(LanguageNames.CSharp), Shared] +public class DoNotUseConfigureAwaitFixer : BatchedCodeFixProvider +{ + public const string Key_RemoveConfigureAwait = "xUnit1030_RemoveConfigureAwait"; + + public DoNotUseConfigureAwaitFixer() : + base(Descriptors.X1030_DoNotUseConfigureAwait.Id) + { } + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) + return; + + // The syntax node (the invocation) will include "(any preceding trivia)(any preceding code).ConfigureAwait(args)" despite + // the context.Span only covering "ConfigureAwait(args)". So we need to replace the whole invocation + // with an invocation that does not include the ConfigureAwait call. + var syntaxNode = root.FindNode(context.Span); + var syntaxText = syntaxNode.ToFullString(); + + // Remove the context span (plus the preceding .) + var newSyntaxText = syntaxText.Substring(0, context.Span.Start - syntaxNode.FullSpan.Start - 1); + var newSyntaxNode = SyntaxFactory.ParseExpression(newSyntaxText); + + context.RegisterCodeFix( + CodeAction.Create( + "Remove ConfigureAwait call", + async ct => + { + var editor = await DocumentEditor.CreateAsync(context.Document, ct).ConfigureAwait(false); + editor.ReplaceNode(syntaxNode, newSyntaxNode); + return editor.GetChangedDocument(); + }, + Key_RemoveConfigureAwait + ), + context.Diagnostics + ); + } +} diff --git a/src/xunit.analyzers.tests/Analyzers/X1000/DoNotUseConfigureAwaitTests.cs b/src/xunit.analyzers.tests/Analyzers/X1000/DoNotUseConfigureAwaitTests.cs new file mode 100644 index 00000000..33d30d17 --- /dev/null +++ b/src/xunit.analyzers.tests/Analyzers/X1000/DoNotUseConfigureAwaitTests.cs @@ -0,0 +1,78 @@ +using Xunit; +using Verify = CSharpVerifier; + +public class DoNotUseConfigureAwaitTests +{ + [Fact] + public async void SuccessCase() + { + var source = @" +using System.Threading.Tasks; +using Xunit; + +public class TestClass { + [Fact] + public async Task TestMethod() { + await Task.Delay(1); + } +}"; + + await Verify.VerifyAnalyzer(source); + } + + [Theory] + [InlineData("true")] + [InlineData("false")] + [InlineData("1 == 2")] + public async void FailureCase_Async(string argumentValue) + { + var source = @$" +using System.Threading.Tasks; +using Xunit; + +public class TestClass {{ + [Fact] + public async Task TestMethod() {{ + await Task.Delay(1).[|ConfigureAwait({argumentValue})|]; + }} +}}"; + + await Verify.VerifyAnalyzer(source); + } + + [Theory] + [InlineData("true")] + [InlineData("false")] + [InlineData("1 == 2")] + public async void FailureCase_NonAsync(string argumentValue) + { + var source = @$" +using System.Threading.Tasks; +using Xunit; + +public class TestClass {{ + [Fact] + public void TestMethod() {{ + Task.Delay(1).[|ConfigureAwait({argumentValue})|].GetAwaiter().GetResult(); + }} +}}"; + + await Verify.VerifyAnalyzer(source); + } + + [Fact] + public async void IgnoredCase() + { + var source = @" +using System.Threading.Tasks; +using Xunit; + +public class NonTestClass { + public async Task NonTestMethod() { + await Task.Delay(1).ConfigureAwait(false); + } +}"; + + await Verify.VerifyAnalyzer(source); + } +} diff --git a/src/xunit.analyzers.tests/Fixes/X1000/DoNotUseConfigureAwaitFixerTests.cs b/src/xunit.analyzers.tests/Fixes/X1000/DoNotUseConfigureAwaitFixerTests.cs new file mode 100644 index 00000000..67ac6aa4 --- /dev/null +++ b/src/xunit.analyzers.tests/Fixes/X1000/DoNotUseConfigureAwaitFixerTests.cs @@ -0,0 +1,68 @@ +using Xunit; +using Xunit.Analyzers.Fixes; +using Verify = CSharpVerifier; + +public class DoNotUseConfigureAwaitFixerTests +{ + [Theory] + [InlineData("true")] + [InlineData("false")] + [InlineData("1 == 2")] + public async void RemovesConfigureAwait_Async(string argumentValue) + { + var before = @$" +using System.Threading.Tasks; +using Xunit; + +public class TestClass {{ + [Fact] + public async Task TestMethod() {{ + await Task.Delay(1).[|ConfigureAwait({argumentValue})|]; + }} +}}"; + + var after = @" +using System.Threading.Tasks; +using Xunit; + +public class TestClass { + [Fact] + public async Task TestMethod() { + await Task.Delay(1); + } +}"; + + await Verify.VerifyCodeFix(before, after, DoNotUseConfigureAwaitFixer.Key_RemoveConfigureAwait); + } + + [Theory] + [InlineData("true")] + [InlineData("false")] + [InlineData("1 == 2")] + public async void RemovesConfigureAwait_NonAsync(string argumentValue) + { + var before = @$" +using System.Threading.Tasks; +using Xunit; + +public class TestClass {{ + [Fact] + public void TestMethod() {{ + Task.Delay(1).[|ConfigureAwait({argumentValue})|].GetAwaiter().GetResult(); + }} +}}"; + + var after = @" +using System.Threading.Tasks; +using Xunit; + +public class TestClass { + [Fact] + public void TestMethod() { + Task.Delay(1).GetAwaiter().GetResult(); + } +}"; + + await Verify.VerifyCodeFix(before, after, DoNotUseConfigureAwaitFixer.Key_RemoveConfigureAwait); + } +} diff --git a/src/xunit.analyzers/Utility/Descriptors.cs b/src/xunit.analyzers/Utility/Descriptors.cs index cc7b0725..84cbae4b 100644 --- a/src/xunit.analyzers/Utility/Descriptors.cs +++ b/src/xunit.analyzers/Utility/Descriptors.cs @@ -305,7 +305,14 @@ static DiagnosticDescriptor Rule( "Local functions cannot be test functions. Remove '{0}'." ); - // Placeholder for rule X1030 + public static DiagnosticDescriptor X1030_DoNotUseConfigureAwait { get; } = + Rule( + "xUnit1030", + "Do not call ConfigureAwait in test method", + Usage, + Warning, + "Test methods should not call ConfigureAwait(), as it may bypass parallelization limits." + ); // Placeholder for rule X1031 diff --git a/src/xunit.analyzers/X1000/DoNotUseConfigureAwait.cs b/src/xunit.analyzers/X1000/DoNotUseConfigureAwait.cs new file mode 100644 index 00000000..b66dbff0 --- /dev/null +++ b/src/xunit.analyzers/X1000/DoNotUseConfigureAwait.cs @@ -0,0 +1,81 @@ +using System.Linq; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using Microsoft.CodeAnalysis.Text; + +namespace Xunit.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class DoNotUseConfigureAwait : XunitDiagnosticAnalyzer +{ + public DoNotUseConfigureAwait() : + base(Descriptors.X1030_DoNotUseConfigureAwait) + { } + + public override void AnalyzeCompilation( + CompilationStartAnalysisContext context, + XunitContext xunitContext) + { + var taskType = TypeSymbolFactory.Task(context.Compilation); + + if (xunitContext.Core.FactAttributeType is null || xunitContext.Core.TheoryAttributeType is null) + return; + + context.RegisterOperationAction(context => + { + if (context.Operation is not IInvocationOperation invocation) + return; + + var methodSymbol = invocation.TargetMethod; + if (methodSymbol.MethodKind != MethodKind.Ordinary || !SymbolEqualityComparer.Default.Equals(methodSymbol.ContainingType, taskType) || methodSymbol.Name != nameof(Task.ConfigureAwait)) + return; + + var semanticModel = invocation.SemanticModel; + if (semanticModel is null) + return; + + // Only trigger when you're inside a test method + for (var parent = invocation.Parent; parent != null; parent = parent.Parent) + { + if (parent is not IMethodBodyOperation methodBodyOperation) + continue; + if (methodBodyOperation.Syntax is not MethodDeclarationSyntax methodSyntax) + continue; + + var inTestMethod = methodSyntax.AttributeLists.SelectMany(list => list.Attributes).Any(attr => + { + var typeInfo = semanticModel.GetTypeInfo(attr); + if (typeInfo.Type is null) + return false; + + return + SymbolEqualityComparer.Default.Equals(typeInfo.Type, xunitContext.Core.FactAttributeType) || + SymbolEqualityComparer.Default.Equals(typeInfo.Type, xunitContext.Core.TheoryAttributeType); + }); + + if (!inTestMethod) + return; + + // invocation should be two nodes: "(some other code).ConfigureAwait" and the arguments (like "(false)") + var invocationChildren = invocation.Syntax.ChildNodes().ToList(); + if (invocationChildren.Count != 2) + return; + + // First child node should be split into three pieces: "(some other code)", ".", and "ConfigureAwait" + var methodCallChildren = invocationChildren[0].ChildNodesAndTokens().ToList(); + if (methodCallChildren.Count != 3) + return; + + // Construct a location that covers "ConfigureAwait(arguments)" + var length = methodCallChildren[2].Span.Length + invocationChildren[1].Span.Length; + var textSpan = new TextSpan(methodCallChildren[2].SpanStart, length); + var location = Location.Create(invocation.Syntax.SyntaxTree, textSpan); + + context.ReportDiagnostic(Diagnostic.Create(Descriptors.X1030_DoNotUseConfigureAwait, location)); + } + }, OperationKind.Invocation); + } +}