From 8702af112626fd78d4dc938872b53ad0a794a3cf Mon Sep 17 00:00:00 2001 From: Allison Chou Date: Thu, 21 Nov 2019 18:43:57 -0800 Subject: [PATCH] Refactoring + added more tests --- .../ExtractLocalFunctionTests.cs | 37 ++++++++-- .../ExtractMethod/ExtractMethodTests.cs | 33 +++++++++ .../CSharpMethodExtractor.TriviaResult.cs | 70 +++++++++---------- ...actExtractMethodCodeRefactoringProvider.cs | 22 +++--- .../PredefinedCodeRefactoringProviderNames.cs | 1 - .../VisualBasicMethodExtractor.vb | 1 + .../CSharpSyntaxFactsService.cs | 3 +- .../SyntaxFactsService/ISyntaxFactsService.cs | 2 +- .../VisualBasicSyntaxFactsService.vb | 2 +- 9 files changed, 112 insertions(+), 59 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractLocalFunctionTests.cs b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractLocalFunctionTests.cs index 258a5e4c914..4106576f66f 100644 --- a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractLocalFunctionTests.cs +++ b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractLocalFunctionTests.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Threading.Tasks; -using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeRefactorings; using Microsoft.CodeAnalysis.CodeRefactorings.ExtractMethod; using Microsoft.CodeAnalysis.CodeStyle; @@ -1539,7 +1538,7 @@ static void NewMethod() }", CodeActionIndex); } - [Fact(Skip = "https://github.com/dotnet/roslyn/issues/39946")] + [Fact(Skip = "https://github.com/dotnet/roslyn/issues/39946"), Trait(Traits.Feature, Traits.Features.CodeActionsExtractLocalFunction)] public async Task ExtractLocalFunctionCall_3() { await TestInRegularAndScriptAsync(@" @@ -1573,7 +1572,7 @@ static void NewMethod() }", CodeActionIndex); } - [Fact(Skip = "https://github.com/dotnet/roslyn/issues/39946")] + [Fact(Skip = "https://github.com/dotnet/roslyn/issues/39946"), Trait(Traits.Feature, Traits.Features.CodeActionsExtractLocalFunction)] public async Task ExtractFunctionUnderLocalFunctionCall() { await TestInRegularAndScriptAsync(@" @@ -3234,7 +3233,7 @@ bool NewMethod(bool b) return b != true; } } -}", CodeActionIndex, options: Option(CSharpCodeStyleOptions.PreferStaticLocalFunction, CodeStyleOptions.TrueWithSilentEnforcement), parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp6)); +}", CodeActionIndex, options: Option(CSharpCodeStyleOptions.PreferStaticLocalFunction, CodeStyleOptions.TrueWithSilentEnforcement), parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7)); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractLocalFunction)] @@ -3291,6 +3290,36 @@ static bool NewMethod(bool b) }", CodeActionIndex, options: Option(CSharpCodeStyleOptions.PreferStaticLocalFunction, CodeStyleOptions.TrueWithSilentEnforcement), parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.Latest)); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractLocalFunction)] + public async Task TestPartialSelection_CSharp5NotApplicable() + { + var code = @" +class Program +{ + static void Main(string[] args) + { + bool b = true; + System.Console.WriteLine([|b != true|] ? b = true : b = false); + } +}"; + await TestExactActionSetOfferedAsync(code, new[] { FeaturesResources.Extract_method }, new TestParameters(parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp6))); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractLocalFunction)] + public async Task TestPartialSelection_CSharp6NotApplicable() + { + var code = @" +class Program +{ + static void Main(string[] args) + { + bool b = true; + System.Console.WriteLine([|b != true|] ? b = true : b = false); + } +}"; + await TestExactActionSetOfferedAsync(code, new[] { FeaturesResources.Extract_method }, new TestParameters(parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp5))); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractLocalFunction)] public async Task TestMissingInLocalFunctionDeclaration_ExpressionBody() { diff --git a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs index 596dac74dba..4530acde336 100644 --- a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs +++ b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs @@ -3153,5 +3153,38 @@ private static bool NewMethod(bool b) } }", options: Option(CSharpCodeStyleOptions.PreferStaticLocalFunction, CodeStyleOptions.FalseWithSuggestionEnforcement)); } + + [Fact(Skip = "https://github.com/dotnet/roslyn/issues/39946"), Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod)] + public async Task ExtractLocalFunctionCallAndDeclaration() + { + await TestInRegularAndScriptAsync(@" +class C +{ + public static void Main() + { + static void LocalParent() + { + [|void Local() { } + Local();|] + } + } +}", @" +class C +{ + public static void Main() + { + static void LocalParent() + { + {|Rename:NewMethod|}(); + } + } + + private static void NewMethod() + { + void Local() { } + Local(); + } +}"); + } } } diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.TriviaResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.TriviaResult.cs index 332abcc3061..43c2cc8a207 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.TriviaResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.TriviaResult.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Linq; +using System.Reflection.Metadata; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; @@ -32,39 +33,24 @@ private CSharpTriviaResult(SemanticDocument document, ITriviaSavedResult result) protected override AnnotationResolver GetAnnotationResolver(SyntaxNode callsite, SyntaxNode method) { - if (callsite == null) + var isMethodOrLocalFunction = method is MethodDeclarationSyntax || method is LocalFunctionStatementSyntax; + if (callsite == null || !isMethodOrLocalFunction) { return null; } - if (method is MethodDeclarationSyntax methodDefinition) - { - return (node, location, annotation) => AnnotationResolver(node, location, annotation, callsite, methodDefinition.Body, methodDefinition.ExpressionBody, methodDefinition.SemicolonToken); - } - else if (method is LocalFunctionStatementSyntax localMethodDefinition) - { - return (node, location, annotation) => AnnotationResolver(node, location, annotation, callsite, localMethodDefinition.Body, localMethodDefinition.ExpressionBody, localMethodDefinition.SemicolonToken); - } - else - { - return null; - } + return (node, location, annotation) => AnnotationResolver(node, location, annotation, callsite, method); } protected override TriviaResolver GetTriviaResolver(SyntaxNode method) { - if (method is MethodDeclarationSyntax methodDefinition) - { - return (location, tokenPair, triviaMap) => TriviaResolver(location, tokenPair, triviaMap, methodDefinition.Body, methodDefinition.ExpressionBody, methodDefinition.SemicolonToken); - } - else if (method is LocalFunctionStatementSyntax localMethodDefinition) - { - return (location, tokenPair, triviaMap) => TriviaResolver(location, tokenPair, triviaMap, localMethodDefinition.Body, localMethodDefinition.ExpressionBody, localMethodDefinition.SemicolonToken); - } - else + var isMethodOrLocalFunction = method is MethodDeclarationSyntax || method is LocalFunctionStatementSyntax; + if (!isMethodOrLocalFunction) { return null; } + + return (location, tokenPair, triviaMap) => TriviaResolver(location, tokenPair, triviaMap, method); } private SyntaxToken AnnotationResolver( @@ -72,9 +58,7 @@ protected override TriviaResolver GetTriviaResolver(SyntaxNode method) TriviaLocation location, SyntaxAnnotation annotation, SyntaxNode callsite, - BlockSyntax methodBody, - ArrowExpressionClauseSyntax expressionBody, - SyntaxToken semicolonToken) + SyntaxNode method) { var token = node.GetAnnotatedNodesAndTokens(annotation).FirstOrDefault().AsToken(); if (token.RawKind != 0) @@ -82,15 +66,16 @@ protected override TriviaResolver GetTriviaResolver(SyntaxNode method) return token; } + var (body, expressionBody, semicolonToken) = GetResolverElements(method); return location switch { TriviaLocation.BeforeBeginningOfSpan => callsite.GetFirstToken(includeZeroWidth: true).GetPreviousToken(includeZeroWidth: true), TriviaLocation.AfterEndOfSpan => callsite.GetLastToken(includeZeroWidth: true).GetNextToken(includeZeroWidth: true), - TriviaLocation.AfterBeginningOfSpan => methodBody != null - ? methodBody.OpenBraceToken.GetNextToken(includeZeroWidth: true) + TriviaLocation.AfterBeginningOfSpan => body != null + ? body.OpenBraceToken.GetNextToken(includeZeroWidth: true) : expressionBody.ArrowToken.GetNextToken(includeZeroWidth: true), - TriviaLocation.BeforeEndOfSpan => methodBody != null - ? methodBody.CloseBraceToken.GetPreviousToken(includeZeroWidth: true) + TriviaLocation.BeforeEndOfSpan => body != null + ? body.CloseBraceToken.GetPreviousToken(includeZeroWidth: true) : semicolonToken, _ => Contract.FailWithReturn("can't happen"), }; @@ -100,19 +85,19 @@ protected override TriviaResolver GetTriviaResolver(SyntaxNode method) TriviaLocation location, PreviousNextTokenPair tokenPair, Dictionary triviaMap, - BlockSyntax methodBody, - ArrowExpressionClauseSyntax methodExpressionBody, - SyntaxToken methodSemicolonToken) + SyntaxNode method) { // Resolve trivia at the edge of the selection. simple case is easy to deal with, but complex cases where // elastic trivia and user trivia are mixed (hybrid case) and we want to preserve some part of user coding style // but not others can be dealt with here. + var (body, expressionBody, semicolonToken) = GetResolverElements(method); + // method has no statement in them. so basically two trivia list now pointing to same thing. "{" and "}" - if (methodBody != null) + if (body != null) { - if (tokenPair.PreviousToken == methodBody.OpenBraceToken && - tokenPair.NextToken == methodBody.CloseBraceToken) + if (tokenPair.PreviousToken == body.OpenBraceToken && + tokenPair.NextToken == body.CloseBraceToken) { return (location == TriviaLocation.AfterBeginningOfSpan) ? SpecializedCollections.SingletonEnumerable(SyntaxFactory.ElasticMarker) @@ -121,8 +106,8 @@ protected override TriviaResolver GetTriviaResolver(SyntaxNode method) } else { - if (tokenPair.PreviousToken == methodExpressionBody.ArrowToken && - tokenPair.NextToken.GetPreviousToken() == methodSemicolonToken) + if (tokenPair.PreviousToken == expressionBody.ArrowToken && + tokenPair.NextToken.GetPreviousToken() == semicolonToken) { return (location == TriviaLocation.AfterBeginningOfSpan) ? SpecializedCollections.SingletonEnumerable(SyntaxFactory.ElasticMarker) @@ -148,6 +133,17 @@ protected override TriviaResolver GetTriviaResolver(SyntaxNode method) }; } + private (BlockSyntax body, ArrowExpressionClauseSyntax expressionBody, SyntaxToken semicolonToken) GetResolverElements(SyntaxNode method) + { + if (method is MethodDeclarationSyntax methodDeclaration) + { + return (methodDeclaration.Body, methodDeclaration.ExpressionBody, methodDeclaration.SemicolonToken); + } + + var localFunctionDeclaration = (LocalFunctionStatementSyntax)method; + return (localFunctionDeclaration.Body, localFunctionDeclaration.ExpressionBody, localFunctionDeclaration.SemicolonToken); + } + private IEnumerable FilterBeforeBeginningOfSpan(PreviousNextTokenPair tokenPair, IEnumerable list) { var allList = FilterTriviaList(tokenPair.PreviousToken.TrailingTrivia.Concat(list).Concat(AppendLeadingTrivia(tokenPair))); diff --git a/src/Features/Core/Portable/CodeRefactorings/ExtractMethod/AbstractExtractMethodCodeRefactoringProvider.cs b/src/Features/Core/Portable/CodeRefactorings/ExtractMethod/AbstractExtractMethodCodeRefactoringProvider.cs index 3b2f8becb57..840b48f786a 100644 --- a/src/Features/Core/Portable/CodeRefactorings/ExtractMethod/AbstractExtractMethodCodeRefactoringProvider.cs +++ b/src/Features/Core/Portable/CodeRefactorings/ExtractMethod/AbstractExtractMethodCodeRefactoringProvider.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; using System.Threading; @@ -9,6 +8,7 @@ using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.ExtractMethod; using Microsoft.CodeAnalysis.LanguageServices; +using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.Text; using Roslyn.Utilities; @@ -62,23 +62,17 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte } } - protected virtual async Task> GetCodeActionAsync( + private async Task> GetCodeActionAsync( Document document, TextSpan textSpan, CancellationToken cancellationToken) { - var actions = ImmutableArray.CreateBuilder(); + var actions = ArrayBuilder.GetInstance(); var methodAction = await ExtractMethod(document, textSpan, cancellationToken).ConfigureAwait(false); - if (methodAction != default) - { - actions.Add(methodAction); - } + actions.AddIfNotNull(methodAction); var localFunctionAction = await ExtractLocalFunction(document, textSpan, cancellationToken).ConfigureAwait(false); - if (localFunctionAction != default) - { - actions.Add(localFunctionAction); - } + actions.AddIfNotNull(localFunctionAction); return actions.ToImmutable(); } @@ -111,7 +105,7 @@ private async Task ExtractLocalFunction(Document document, TextSpan { var syntaxTree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false); var syntaxFacts = document.GetLanguageService(); - if (!syntaxFacts.SupportsLocalFunctionDeclaration()) + if (!syntaxFacts.SupportsLocalFunctionDeclaration(syntaxTree.Options)) { return default; } @@ -132,7 +126,7 @@ private async Task ExtractLocalFunction(Document document, TextSpan return default; } - protected async Task AddRenameAnnotationAsync(Document document, SyntaxToken invocationNameToken, CancellationToken cancellationToken) + private async Task AddRenameAnnotationAsync(Document document, SyntaxToken invocationNameToken, CancellationToken cancellationToken) { var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); @@ -143,7 +137,7 @@ protected async Task AddRenameAnnotationAsync(Document document, Synta return document.WithSyntaxRoot(finalRoot); } - protected class MyCodeAction : CodeAction.DocumentChangeAction + private class MyCodeAction : CodeAction.DocumentChangeAction { public MyCodeAction(string title, Func> createChangedDocument) : base(title, createChangedDocument) diff --git a/src/Features/Core/Portable/CodeRefactorings/PredefinedCodeRefactoringProviderNames.cs b/src/Features/Core/Portable/CodeRefactorings/PredefinedCodeRefactoringProviderNames.cs index bee90c030d6..1d06ba94c0b 100644 --- a/src/Features/Core/Portable/CodeRefactorings/PredefinedCodeRefactoringProviderNames.cs +++ b/src/Features/Core/Portable/CodeRefactorings/PredefinedCodeRefactoringProviderNames.cs @@ -14,7 +14,6 @@ internal static class PredefinedCodeRefactoringProviderNames public const string ConvertTupleToStruct = "Convert Tuple to Struct Code Action Provider"; public const string EncapsulateField = "Encapsulate Field"; public const string ExtractInterface = "Extract Interface Code Action Provider"; - public const string ExtractLocalFunction = "Extract Local Function Code Action Provider"; public const string ExtractMethod = "Extract Method Code Action Provider"; public const string GenerateConstructorFromMembers = "Generate Constructor From Members Code Action Provider"; public const string GenerateDefaultConstructors = "Generate Default Constructors Code Action Provider"; diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb index 094e2872da8..cfcf4fabf9b 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb @@ -155,6 +155,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod methodName As SyntaxToken, methodDefinition As SyntaxNode, cancellationToken As CancellationToken) As Task(Of (document As Document, methodName As SyntaxToken, methodDefinition As SyntaxNode)) + ' VB doesn't need to do any correction, so we just return the values untouched Return Task.FromResult((document, methodName, methodDefinition)) End Function End Class diff --git a/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs b/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs index 4cc79754faf..db282d88066 100644 --- a/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs +++ b/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs @@ -50,7 +50,8 @@ public bool SupportsIndexingInitializer(ParseOptions options) public bool SupportsThrowExpression(ParseOptions options) => ((CSharpParseOptions)options).LanguageVersion >= LanguageVersion.CSharp7; - public bool SupportsLocalFunctionDeclaration() => true; + public bool SupportsLocalFunctionDeclaration(ParseOptions options) + => ((CSharpParseOptions)options).LanguageVersion >= LanguageVersion.CSharp7; public SyntaxToken ParseToken(string text) => SyntaxFactory.ParseToken(text); diff --git a/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs b/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs index 5d6eb4c10b7..a7f83e22969 100644 --- a/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs +++ b/src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs @@ -22,7 +22,7 @@ internal interface ISyntaxFactsService : ILanguageService bool SupportsIndexingInitializer(ParseOptions options); bool SupportsThrowExpression(ParseOptions options); - bool SupportsLocalFunctionDeclaration(); + bool SupportsLocalFunctionDeclaration(ParseOptions options); SyntaxToken ParseToken(string text); diff --git a/src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb b/src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb index 1b762b2a3e6..5cf9b16b622 100644 --- a/src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb +++ b/src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb @@ -63,7 +63,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic Return False End Function - Public Function SupportsLocalFunctionDeclaration() As Boolean Implements ISyntaxFactsService.SupportsLocalFunctionDeclaration + Public Function SupportsLocalFunctionDeclaration(options As ParseOptions) As Boolean Implements ISyntaxFactsService.SupportsLocalFunctionDeclaration Return False End Function -- GitLab