From 991c7caf5d06f32a10ccfd13e2819535980e70b3 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 26 Oct 2024 10:15:26 -0700 Subject: [PATCH 1/2] Fix crash when 'add await' analyzers binding expressions --- .../CSharpAddAwaitCodeRefactoringProvider.cs | 23 ++--- .../CSharpTest/AddAwait/AddAwaitTests.cs | 87 ++++++++++++++++++- ...AbstractAddAwaitCodeRefactoringProvider.cs | 19 +++- .../Services/SyntaxFacts/CSharpSyntaxFacts.cs | 3 + .../Core/Services/SyntaxFacts/ISyntaxFacts.cs | 1 + .../SyntaxFacts/VisualBasicSyntaxFacts.vb | 5 ++ 6 files changed, 121 insertions(+), 17 deletions(-) diff --git a/src/Features/CSharp/Portable/CodeRefactorings/AddAwait/CSharpAddAwaitCodeRefactoringProvider.cs b/src/Features/CSharp/Portable/CodeRefactorings/AddAwait/CSharpAddAwaitCodeRefactoringProvider.cs index 3c82c06a3e052..1473b5b62db1c 100644 --- a/src/Features/CSharp/Portable/CodeRefactorings/AddAwait/CSharpAddAwaitCodeRefactoringProvider.cs +++ b/src/Features/CSharp/Portable/CodeRefactorings/AddAwait/CSharpAddAwaitCodeRefactoringProvider.cs @@ -16,14 +16,11 @@ namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.AddAwait; /// This refactoring complements the AddAwait fixer. It allows adding `await` and `await ... .ConfigureAwait(false)` even there is no compiler error to trigger the fixer. /// [ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = PredefinedCodeRefactoringProviderNames.AddAwait), Shared] -internal sealed partial class CSharpAddAwaitCodeRefactoringProvider : AbstractAddAwaitCodeRefactoringProvider +[method: ImportingConstructor] +[method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] +internal sealed partial class CSharpAddAwaitCodeRefactoringProvider() + : AbstractAddAwaitCodeRefactoringProvider { - [ImportingConstructor] - [SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] - public CSharpAddAwaitCodeRefactoringProvider() - { - } - protected override string GetTitle() => CSharpFeaturesResources.Add_await; @@ -34,14 +31,12 @@ protected override bool IsInAsyncContext(SyntaxNode node) { foreach (var current in node.Ancestors()) { - switch (current.Kind()) + switch (current) { - case SyntaxKind.ParenthesizedLambdaExpression: - case SyntaxKind.SimpleLambdaExpression: - case SyntaxKind.AnonymousMethodExpression: - return ((AnonymousFunctionExpressionSyntax)current).AsyncKeyword != default; - case SyntaxKind.MethodDeclaration: - return ((MethodDeclarationSyntax)current).Modifiers.Any(SyntaxKind.AsyncKeyword); + case AnonymousFunctionExpressionSyntax anonymousFunction: + return anonymousFunction.AsyncKeyword != default; + case MethodDeclarationSyntax methodDeclaration: + return methodDeclaration.Modifiers.Any(SyntaxKind.AsyncKeyword); } } diff --git a/src/Features/CSharpTest/AddAwait/AddAwaitTests.cs b/src/Features/CSharpTest/AddAwait/AddAwaitTests.cs index 0e17ac0ebb2a4..f19dc9aa12f56 100644 --- a/src/Features/CSharpTest/AddAwait/AddAwaitTests.cs +++ b/src/Features/CSharpTest/AddAwait/AddAwaitTests.cs @@ -13,7 +13,7 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.CodeRefactorings.AddAwa [Trait(Traits.Feature, Traits.Features.AddAwait)] [Trait(Traits.Feature, Traits.Features.CodeActionsAddAwait)] -public class AddAwaitTests : AbstractCSharpCodeActionTest_NoEditor +public sealed class AddAwaitTests : AbstractCSharpCodeActionTest_NoEditor { protected override CodeRefactoringProvider CreateCodeRefactoringProvider(TestWorkspace workspace, TestParameters parameters) => new CSharpAddAwaitCodeRefactoringProvider(); @@ -1610,4 +1610,89 @@ class Program } """); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/66726")] + public async Task NotOnBindingExpression1() + { + await TestMissingInRegularAndScriptAsync(""" + using System.Threading.Tasks; + + class TestClass + { + private async Task MyTestMethod1Async(TestClass c) + { + _ = c?.[|MyIntMethodAsync()|]; + } + + private Task MyIntMethodAsync() + { + return Task.FromResult(result: 1); + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/66726")] + public async Task NotOnBindingExpression2() + { + await TestMissingInRegularAndScriptAsync(""" + using System.Threading.Tasks; + + class TestClass + { + private TestClass C; + + private async Task MyTestMethod1Async(TestClass c) + { + _ = c?.C.[|MyIntMethodAsync()|]; + } + + private Task MyIntMethodAsync() + { + return Task.FromResult(result: 1); + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/66726")] + public async Task NotOnBindingExpression3() + { + await TestMissingInRegularAndScriptAsync(""" + using System.Threading.Tasks; + + class TestClass + { + private TestClass this[int i] => null; + + private async Task MyTestMethod1Async(TestClass c) + { + _ = c?[0].[|MyIntMethodAsync()|]; + } + + private Task MyIntMethodAsync() + { + return Task.FromResult(result: 1); + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/66726")] + public async Task NotOnBindingExpression4() + { + await TestMissingInRegularAndScriptAsync(""" + using System.Threading.Tasks; + + class TestClass + { + private Task this[int i] => null; + + private async Task MyTestMethod1Async(TestClass c) + { + _ = c?[|[0]|]; + } + } + """); + } } diff --git a/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs b/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs index 4b24940be8e0b..ac42c9d68d8d6 100644 --- a/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs +++ b/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.CodeActions; @@ -51,7 +52,7 @@ public sealed override async Task ComputeRefactoringsAsync(CodeRefactoringContex context.RegisterRefactoring( CodeAction.Create( title, - c => AddAwaitAsync(document, expression, withConfigureAwait: false, c), + cancellationToken => AddAwaitAsync(document, expression, withConfigureAwait: false, cancellationToken), title), expression.Span); @@ -59,7 +60,7 @@ public sealed override async Task ComputeRefactoringsAsync(CodeRefactoringContex context.RegisterRefactoring( CodeAction.Create( titleWithConfigureAwait, - c => AddAwaitAsync(document, expression, withConfigureAwait: true, c), + cancellationToken => AddAwaitAsync(document, expression, withConfigureAwait: true, cancellationToken), titleWithConfigureAwait), expression.Span); } @@ -80,6 +81,20 @@ private static bool IsValidAwaitableExpression( if (syntaxFacts.IsExpressionOfAwaitExpression(node)) return false; + for (var current = node; current != null;) + { + if (syntaxFacts.IsMemberBindingExpression(current) || + syntaxFacts.IsElementBindingExpression(current)) + { + // Can't add 'await' to the `.X` in `a?.X`. Nor would we want to. Those could return null, which + // `await` would blow up on. Note: this could be reconsidered if we end up adding `await?` support to + // the language. + return false; + } + + current = current.ChildNodesAndTokens().FirstOrDefault().AsNode() as TExpressionSyntax; + } + // if we're on an actual type symbol itself (like literally `Task`) we don't want to offer to add await. // we only want to add for actual expressions whose type is awaitable, not on the awaitable type itself. var symbol = model.GetSymbolInfo(node, cancellationToken).GetAnySymbol(); diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs index cebd1153d3b37..74652e0c0418f 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs @@ -535,6 +535,9 @@ public bool IsBindableToken(SyntaxToken token) public bool IsPostfixUnaryExpression([NotNullWhen(true)] SyntaxNode? node) => node is PostfixUnaryExpressionSyntax; + public bool IsElementBindingExpression([NotNullWhen(true)] SyntaxNode? node) + => node is ElementBindingExpressionSyntax; + public bool IsMemberBindingExpression([NotNullWhen(true)] SyntaxNode? node) => node is MemberBindingExpressionSyntax; diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SyntaxFacts/ISyntaxFacts.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SyntaxFacts/ISyntaxFacts.cs index 6bd812470edbe..cd199f1d8fabd 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SyntaxFacts/ISyntaxFacts.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SyntaxFacts/ISyntaxFacts.cs @@ -287,6 +287,7 @@ void GetPartsOfTupleExpression(SyntaxNode node, SyntaxNode GetExpressionOfAttributeArgument(SyntaxNode node); SyntaxNode GetExpressionOfInterpolation(SyntaxNode node); + bool IsElementBindingExpression([NotNullWhen(true)] SyntaxNode? node); bool IsMemberBindingExpression([NotNullWhen(true)] SyntaxNode? node); bool IsPostfixUnaryExpression([NotNullWhen(true)] SyntaxNode? node); diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb index bc31c6403f9a3..bf8f406ff26f6 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb @@ -1569,6 +1569,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.LanguageService Return False End Function + Public Function IsElementBindingExpression(node As SyntaxNode) As Boolean Implements ISyntaxFacts.IsElementBindingExpression + ' Does not exist in VB. VB represents an element binding as a InvocationExpression with null target. + Return False + End Function + Public Function IsMemberBindingExpression(node As SyntaxNode) As Boolean Implements ISyntaxFacts.IsMemberBindingExpression ' Does not exist in VB. VB represents a member binding as a MemberAccessExpression with null target. Return False From 04a2591885f3a4624569655b97f5255738aa1770 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 26 Oct 2024 17:30:01 -0700 Subject: [PATCH 2/2] lint --- .../AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs b/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs index ac42c9d68d8d6..a724e7f5a0dfb 100644 --- a/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs +++ b/src/Features/Core/Portable/CodeRefactorings/AddAwait/AbstractAddAwaitCodeRefactoringProvider.cs @@ -83,7 +83,7 @@ private static bool IsValidAwaitableExpression( for (var current = node; current != null;) { - if (syntaxFacts.IsMemberBindingExpression(current) || + if (syntaxFacts.IsMemberBindingExpression(current) || syntaxFacts.IsElementBindingExpression(current)) { // Can't add 'await' to the `.X` in `a?.X`. Nor would we want to. Those could return null, which