From 0e507ce9e61683d09dcfd9914b08052ec13f97b5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 12 Aug 2018 19:26:43 -0700 Subject: [PATCH] Update 'use block body' to be better understand async methods. --- ...seExpressionBodyForMethodsAnalyzerTests.cs | 156 ++++++++++++++++++ .../UseExpressionBodyForAccessorsHelper.cs | 2 +- .../UseExpressionBodyForConstructorsHelper.cs | 2 +- ...ressionBodyForConversionOperatorsHelper.cs | 2 +- .../UseExpressionBodyForIndexersHelper.cs | 6 +- .../UseExpressionBodyForMethodsHelper.cs | 15 +- .../UseExpressionBodyForOperatorsHelper.cs | 2 +- .../UseExpressionBodyForPropertiesHelper.cs | 7 +- .../Helpers/UseExpressionBodyHelper.cs | 2 +- .../Helpers/UseExpressionBodyHelper`1.cs | 21 ++- .../UseExpressionBodyCodeFixProvider.cs | 7 +- ...seExpressionBodyCodeRefactoringProvider.cs | 7 +- 12 files changed, 201 insertions(+), 28 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/UseExpressionBody/Analyzer/UseExpressionBodyForMethodsAnalyzerTests.cs b/src/EditorFeatures/CSharpTest/UseExpressionBody/Analyzer/UseExpressionBodyForMethodsAnalyzerTests.cs index 887ee8cd348..5aab76a3785 100644 --- a/src/EditorFeatures/CSharpTest/UseExpressionBody/Analyzer/UseExpressionBodyForMethodsAnalyzerTests.cs +++ b/src/EditorFeatures/CSharpTest/UseExpressionBody/Analyzer/UseExpressionBodyForMethodsAnalyzerTests.cs @@ -504,5 +504,161 @@ void M(int i) int M(bool b) => 0; }", options: UseExpressionBody, parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp6)); } + + [WorkItem(25202, "https://github.com/dotnet/roslyn/issues/25202")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)] + public async Task TestUseBlockBodyAsync1() + { + await TestInRegularAndScriptAsync( +@"using System.Threading.Tasks; + +class C +{ + async Task Goo() [|=>|] await Bar(); + + Task Bar() { } +}", +@"using System.Threading.Tasks; + +class C +{ + async Task Goo() + { + await Bar(); + } + + Task Bar() { } +}", options: UseBlockBody); + } + + [WorkItem(25202, "https://github.com/dotnet/roslyn/issues/25202")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)] + public async Task TestUseBlockBodyAsync2() + { + await TestInRegularAndScriptAsync( +@"using System.Threading.Tasks; + +class C +{ + async void Goo() [|=>|] await Bar(); + + Task Bar() { } +}", +@"using System.Threading.Tasks; + +class C +{ + async void Goo() + { + await Bar(); + } + + Task Bar() { } +}", options: UseBlockBody); + } + + [WorkItem(25202, "https://github.com/dotnet/roslyn/issues/25202")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)] + public async Task TestUseBlockBodyAsync3() + { + await TestInRegularAndScriptAsync( +@"using System.Threading.Tasks; + +class C +{ + async void Goo() [|=>|] await Bar(); + + Task Bar() { } +}", +@"using System.Threading.Tasks; + +class C +{ + async void Goo() + { + await Bar(); + } + + Task Bar() { } +}", options: UseBlockBody); + } + + [WorkItem(25202, "https://github.com/dotnet/roslyn/issues/25202")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)] + public async Task TestUseBlockBodyAsync4() + { + await TestInRegularAndScriptAsync( +@"using System.Threading.Tasks; + +class C +{ + async ValueTask Goo() [|=>|] await Bar(); + + Task Bar() { } +}", +@"using System.Threading.Tasks; + +class C +{ + async ValueTask Goo() + { + await Bar(); + } + + Task Bar() { } +}", options: UseBlockBody); + } + + [WorkItem(25202, "https://github.com/dotnet/roslyn/issues/25202")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)] + public async Task TestUseBlockBodyAsync5() + { + await TestInRegularAndScriptAsync( +@"using System.Threading.Tasks; + +class C +{ + async Task Goo() [|=>|] await Bar(); + + Task Bar() { } +}", +@"using System.Threading.Tasks; + +class C +{ + async Task Goo() + { + return await Bar(); + } + + Task Bar() { } +}", options: UseBlockBody); + } + + [WorkItem(25202, "https://github.com/dotnet/roslyn/issues/25202")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExpressionBody)] + public async Task TestUseBlockBodyAsync6() + { + await TestInRegularAndScriptAsync( +@"using System.Threading.Tasks; + +class C +{ + Task Goo() [|=>|] Bar(); + + Task Bar() { } +}", +@"using System.Threading.Tasks; + +class C +{ + Task Goo() + { + return Bar(); + } + + Task Bar() { } +}", options: UseBlockBody); + } } } diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForAccessorsHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForAccessorsHelper.cs index 4472c49e12f..be62a760164 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForAccessorsHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForAccessorsHelper.cs @@ -39,7 +39,7 @@ protected override AccessorDeclarationSyntax WithExpressionBody(AccessorDeclarat protected override AccessorDeclarationSyntax WithBody(AccessorDeclarationSyntax declaration, BlockSyntax body) => declaration.WithBody(body); - protected override bool CreateReturnStatementForExpression(AccessorDeclarationSyntax declaration) + protected override bool CreateReturnStatementForExpression(SemanticModel semanticModel, AccessorDeclarationSyntax declaration) => declaration.IsKind(SyntaxKind.GetAccessorDeclaration); } } diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConstructorsHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConstructorsHelper.cs index e5434e411c6..9265d75dbf8 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConstructorsHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConstructorsHelper.cs @@ -39,6 +39,6 @@ protected override ConstructorDeclarationSyntax WithExpressionBody(ConstructorDe protected override ConstructorDeclarationSyntax WithBody(ConstructorDeclarationSyntax declaration, BlockSyntax body) => declaration.WithBody(body); - protected override bool CreateReturnStatementForExpression(ConstructorDeclarationSyntax declaration) => false; + protected override bool CreateReturnStatementForExpression(SemanticModel semanticModel, ConstructorDeclarationSyntax declaration) => false; } } diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConversionOperatorsHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConversionOperatorsHelper.cs index e612fc85aea..cd3f85bfc2e 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConversionOperatorsHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForConversionOperatorsHelper.cs @@ -39,7 +39,7 @@ protected override ConversionOperatorDeclarationSyntax WithExpressionBody(Conver protected override ConversionOperatorDeclarationSyntax WithBody(ConversionOperatorDeclarationSyntax declaration, BlockSyntax body) => declaration.WithBody(body); - protected override bool CreateReturnStatementForExpression(ConversionOperatorDeclarationSyntax declaration) + protected override bool CreateReturnStatementForExpression(SemanticModel semanticModel, ConversionOperatorDeclarationSyntax declaration) => true; } } diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForIndexersHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForIndexersHelper.cs index 6fb4ae9b8c0..5b36389f480 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForIndexersHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForIndexersHelper.cs @@ -54,12 +54,12 @@ protected override IndexerDeclarationSyntax WithBody(IndexerDeclarationSyntax de } protected override IndexerDeclarationSyntax WithGenerateBody( - IndexerDeclarationSyntax declaration, OptionSet options, ParseOptions parseOptions) + SemanticModel semanticModel, IndexerDeclarationSyntax declaration, OptionSet options, ParseOptions parseOptions) { - return WithAccessorList(declaration, options, parseOptions); + return WithAccessorList(semanticModel, declaration, options, parseOptions); } - protected override bool CreateReturnStatementForExpression(IndexerDeclarationSyntax declaration) => true; + protected override bool CreateReturnStatementForExpression(SemanticModel semanticModel, IndexerDeclarationSyntax declaration) => true; protected override bool TryConvertToExpressionBody( IndexerDeclarationSyntax declaration, ParseOptions options, diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForMethodsHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForMethodsHelper.cs index acdcb6846e7..9c7e4917a2c 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForMethodsHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForMethodsHelper.cs @@ -40,7 +40,18 @@ protected override MethodDeclarationSyntax WithExpressionBody(MethodDeclarationS protected override MethodDeclarationSyntax WithBody(MethodDeclarationSyntax declaration, BlockSyntax body) => declaration.WithBody(body); - protected override bool CreateReturnStatementForExpression(MethodDeclarationSyntax declaration) - => !declaration.ReturnType.IsVoid(); + protected override bool CreateReturnStatementForExpression( + SemanticModel semanticModel, MethodDeclarationSyntax declaration) + { + if (declaration.Modifiers.Any(SyntaxKind.AsyncKeyword)) + { + // if it's 'async TaskLike' (where TaskLike is non-generic) we do *not* want to + // create a return statement. This is just the 'async' version of a 'void' method. + var method = semanticModel.GetDeclaredSymbol(declaration); + return method.ReturnType is INamedTypeSymbol namedType && namedType.Arity != 0; + } + + return !declaration.ReturnType.IsVoid(); + } } } diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForOperatorsHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForOperatorsHelper.cs index 0b50e8087f2..c3cd5c0bc58 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForOperatorsHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForOperatorsHelper.cs @@ -39,7 +39,7 @@ protected override OperatorDeclarationSyntax WithExpressionBody(OperatorDeclarat protected override OperatorDeclarationSyntax WithBody(OperatorDeclarationSyntax declaration, BlockSyntax body) => declaration.WithBody(body); - protected override bool CreateReturnStatementForExpression(OperatorDeclarationSyntax declaration) + protected override bool CreateReturnStatementForExpression(SemanticModel semanticModel, OperatorDeclarationSyntax declaration) => true; } } diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForPropertiesHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForPropertiesHelper.cs index 54316fcb11f..483d61b80e6 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForPropertiesHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyForPropertiesHelper.cs @@ -54,12 +54,13 @@ protected override PropertyDeclarationSyntax WithBody(PropertyDeclarationSyntax } protected override PropertyDeclarationSyntax WithGenerateBody( - PropertyDeclarationSyntax declaration, OptionSet options, ParseOptions parseOptions) + SemanticModel semanticModel, PropertyDeclarationSyntax declaration, + OptionSet options, ParseOptions parseOptions) { - return WithAccessorList(declaration, options, parseOptions); + return WithAccessorList(semanticModel, declaration, options, parseOptions); } - protected override bool CreateReturnStatementForExpression(PropertyDeclarationSyntax declaration) => true; + protected override bool CreateReturnStatementForExpression(SemanticModel semanticModel, PropertyDeclarationSyntax declaration) => true; protected override bool TryConvertToExpressionBody( PropertyDeclarationSyntax declaration, ParseOptions options, diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper.cs index 18d76805d8c..37c63ace3f3 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper.cs @@ -21,7 +21,7 @@ internal abstract class UseExpressionBodyHelper public abstract bool CanOfferUseExpressionBody(OptionSet optionSet, SyntaxNode declaration, bool forAnalyzer); public abstract (bool canOffer, bool fixesError) CanOfferUseBlockBody(OptionSet optionSet, SyntaxNode declaration, bool forAnalyzer); - public abstract SyntaxNode Update(SyntaxNode declaration, OptionSet options, ParseOptions parseOptions, bool useExpressionBody); + public abstract SyntaxNode Update(SemanticModel semanticModel, SyntaxNode declaration, OptionSet options, ParseOptions parseOptions, bool useExpressionBody); public abstract Location GetDiagnosticLocation(SyntaxNode declaration); diff --git a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper`1.cs b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper`1.cs index 539470c3ead..bb6e5612ac0 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper`1.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/Helpers/UseExpressionBodyHelper`1.cs @@ -69,8 +69,8 @@ public override bool CanOfferUseExpressionBody(OptionSet optionSet, SyntaxNode d public override (bool canOffer, bool fixesError) CanOfferUseBlockBody(OptionSet optionSet, SyntaxNode declaration, bool forAnalyzer) => CanOfferUseBlockBody(optionSet, (TDeclaration)declaration, forAnalyzer); - public override SyntaxNode Update(SyntaxNode declaration, OptionSet options, ParseOptions parseOptions, bool useExpressionBody) - => Update((TDeclaration)declaration, options, parseOptions, useExpressionBody); + public sealed override SyntaxNode Update(SemanticModel semanticModel, SyntaxNode declaration, OptionSet options, ParseOptions parseOptions, bool useExpressionBody) + => Update(semanticModel, (TDeclaration)declaration, options, parseOptions, useExpressionBody); public override Location GetDiagnosticLocation(SyntaxNode declaration) => GetDiagnosticLocation((TDeclaration)declaration); @@ -207,7 +207,7 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration) } public TDeclaration Update( - TDeclaration declaration, OptionSet options, + SemanticModel semanticModel, TDeclaration declaration, OptionSet options, ParseOptions parseOptions, bool useExpressionBody) { if (useExpressionBody) @@ -231,7 +231,7 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration) { return WithSemicolonToken( WithExpressionBody( - WithGenerateBody(declaration, options, parseOptions), + WithGenerateBody(semanticModel, declaration, options, parseOptions), expressionBody: null), default); } @@ -241,7 +241,7 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration) protected abstract ArrowExpressionClauseSyntax GetExpressionBody(TDeclaration declaration); - protected abstract bool CreateReturnStatementForExpression(TDeclaration declaration); + protected abstract bool CreateReturnStatementForExpression(SemanticModel semanticModel, TDeclaration declaration); protected abstract SyntaxToken GetSemicolonToken(TDeclaration declaration); @@ -250,14 +250,14 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration) protected abstract TDeclaration WithBody(TDeclaration declaration, BlockSyntax body); protected virtual TDeclaration WithGenerateBody( - TDeclaration declaration, OptionSet options, ParseOptions parseOptions) + SemanticModel semanticModel, TDeclaration declaration, OptionSet options, ParseOptions parseOptions) { var expressionBody = GetExpressionBody(declaration); var semicolonToken = GetSemicolonToken(declaration); if (expressionBody.TryConvertToBlock( GetSemicolonToken(declaration), - CreateReturnStatementForExpression(declaration), + CreateReturnStatementForExpression(semanticModel, declaration), out var block)) { return WithBody(declaration, block); @@ -267,7 +267,7 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration) } protected TDeclaration WithAccessorList( - TDeclaration declaration, OptionSet options, ParseOptions parseOptions) + SemanticModel semanticModel, TDeclaration declaration, OptionSet options, ParseOptions parseOptions) { var expressionBody = GetExpressionBody(declaration); var semicolonToken = GetSemicolonToken(declaration); @@ -280,7 +280,10 @@ protected virtual Location GetDiagnosticLocation(TDeclaration declaration) // an expression bodied accessor they'll just have to convert that to a block as well // and that means two steps to take instead of one. - expressionBody.TryConvertToBlock(GetSemicolonToken(declaration), CreateReturnStatementForExpression(declaration), out var block); + expressionBody.TryConvertToBlock( + GetSemicolonToken(declaration), + CreateReturnStatementForExpression(semanticModel, declaration), + out var block); var accessor = SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration); accessor = block != null diff --git a/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeFixProvider.cs b/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeFixProvider.cs index 309fd7337cb..69722d125e1 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeFixProvider.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeFixProvider.cs @@ -52,13 +52,14 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) Document document, ImmutableArray diagnostics, SyntaxEditor editor, CancellationToken cancellationToken) { + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); var options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false); var accessorLists = new HashSet(); foreach (var diagnostic in diagnostics) { cancellationToken.ThrowIfCancellationRequested(); - AddEdits(editor, diagnostic, options, accessorLists, cancellationToken); + AddEdits(semanticModel, editor, diagnostic, options, accessorLists, cancellationToken); } // Ensure that if we changed any accessors that the accessor lists they're contained @@ -71,7 +72,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) } private void AddEdits( - SyntaxEditor editor, Diagnostic diagnostic, + SemanticModel semanticModel, SyntaxEditor editor, Diagnostic diagnostic, OptionSet options, HashSet accessorLists, CancellationToken cancellationToken) { @@ -81,7 +82,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) var useExpressionBody = diagnostic.Properties.ContainsKey(nameof(UseExpressionBody)); var parseOptions = declaration.SyntaxTree.Options; - var updatedDeclaration = helper.Update(declaration, options, parseOptions, useExpressionBody) + var updatedDeclaration = helper.Update(semanticModel, declaration, options, parseOptions, useExpressionBody) .WithAdditionalAnnotations(Formatter.Annotation); editor.ReplaceNode(declaration, updatedDeclaration); diff --git a/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeRefactoringProvider.cs b/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeRefactoringProvider.cs index 4a7bbcda5ca..198b5d2a102 100644 --- a/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeRefactoringProvider.cs +++ b/src/Features/CSharp/Portable/UseExpressionBody/UseExpressionBodyCodeRefactoringProvider.cs @@ -116,13 +116,14 @@ private SyntaxNode GetDeclaration(SyntaxNode node, UseExpressionBodyHelper helpe return null; } - private Task UpdateDocumentAsync( + private async Task UpdateDocumentAsync( Document document, SyntaxNode root, SyntaxNode declaration, OptionSet options, UseExpressionBodyHelper helper, bool useExpressionBody, CancellationToken cancellationToken) { var parseOptions = root.SyntaxTree.Options; - var updatedDeclaration = helper.Update(declaration, options, parseOptions, useExpressionBody); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + var updatedDeclaration = helper.Update(semanticModel, declaration, options, parseOptions, useExpressionBody); var parent = declaration is AccessorDeclarationSyntax ? declaration.Parent @@ -131,7 +132,7 @@ private SyntaxNode GetDeclaration(SyntaxNode node, UseExpressionBodyHelper helpe .WithAdditionalAnnotations(Formatter.Annotation); var newRoot = root.ReplaceNode(parent, updatedParent); - return Task.FromResult(document.WithSyntaxRoot(newRoot)); + return document.WithSyntaxRoot(newRoot); } private class MyCodeAction : CodeAction.DocumentChangeAction -- GitLab