Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when 'add await' analyzers binding expressions #75644

Merged
merged 2 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
/// </summary>
[ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = PredefinedCodeRefactoringProviderNames.AddAwait), Shared]
internal sealed partial class CSharpAddAwaitCodeRefactoringProvider : AbstractAddAwaitCodeRefactoringProvider<ExpressionSyntax>
[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<ExpressionSyntax>
{
[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;

Expand All @@ -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);
}
}

Expand Down
87 changes: 86 additions & 1 deletion src/Features/CSharpTest/AddAwait/AddAwaitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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<int> 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<int> 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<int> 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<int> this[int i] => null;

private async Task MyTestMethod1Async(TestClass c)
{
_ = c?[|[0]|];
}
}
""");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -51,15 +52,15 @@ 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);

var titleWithConfigureAwait = GetTitleWithConfigureAwait();
context.RegisterRefactoring(
CodeAction.Create(
titleWithConfigureAwait,
c => AddAwaitAsync(document, expression, withConfigureAwait: true, c),
cancellationToken => AddAwaitAsync(document, expression, withConfigureAwait: true, cancellationToken),
titleWithConfigureAwait),
expression.Span);
}
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ void GetPartsOfTupleExpression<TArgumentSyntax>(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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading