From df36f3a1777fb8c95a429f21468cc50310b4bc3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Hou=C5=A1ka?= Date: Thu, 27 Jun 2019 11:35:29 -0700 Subject: [PATCH] PR feedback (moved things around, renamed few things). --- ...harpPullMemberUpCodeRefactoringProvider.cs | 11 ++- ...CSharpConvertLinqQueryToForEachProvider.cs | 4 +- .../Core/Portable/CodeRefactoringHelpers.cs | 4 +- .../AbstractRefactoringHelpersService.cs | 81 +++++++++++++++---- .../IRefactoringHelpersService.cs | 64 +-------------- 5 files changed, 82 insertions(+), 82 deletions(-) diff --git a/src/Features/CSharp/Portable/CodeRefactorings/PullMemberUp/CSharpPullMemberUpCodeRefactoringProvider.cs b/src/Features/CSharp/Portable/CodeRefactorings/PullMemberUp/CSharpPullMemberUpCodeRefactoringProvider.cs index 08100a3b20f..1ffc05914bd 100644 --- a/src/Features/CSharp/Portable/CodeRefactorings/PullMemberUp/CSharpPullMemberUpCodeRefactoringProvider.cs +++ b/src/Features/CSharp/Portable/CodeRefactorings/PullMemberUp/CSharpPullMemberUpCodeRefactoringProvider.cs @@ -31,8 +31,15 @@ public CSharpPullMemberUpCodeRefactoringProvider() : base(null) protected override async Task GetSelectedNodeAsync(Document document, TextSpan span, CancellationToken cancellationToken) { var refactoringHelperService = document.GetLanguageService(); - return await refactoringHelperService.TryGetSelectedNodeAsync( - document, span, n => n is MemberDeclarationSyntax || n is VariableDeclaratorSyntax, cancellationToken).ConfigureAwait(false); + + var memberDecl = await refactoringHelperService.TryGetSelectedNodeAsync(document, span, cancellationToken).ConfigureAwait(false); + if (memberDecl != default) + { + return memberDecl; + } + + var varDecl = await refactoringHelperService.TryGetSelectedNodeAsync(document, span, cancellationToken).ConfigureAwait(false); + return varDecl; } } } diff --git a/src/Features/CSharp/Portable/ConvertLinq/CSharpConvertLinqQueryToForEachProvider.cs b/src/Features/CSharp/Portable/ConvertLinq/CSharpConvertLinqQueryToForEachProvider.cs index 120d6ea9835..237c05a744b 100644 --- a/src/Features/CSharp/Portable/ConvertLinq/CSharpConvertLinqQueryToForEachProvider.cs +++ b/src/Features/CSharp/Portable/ConvertLinq/CSharpConvertLinqQueryToForEachProvider.cs @@ -43,10 +43,10 @@ public CSharpConvertLinqQueryToForEachProvider() /// /// Finds a node for the span and checks that it is either a QueryExpressionSyntax or a QueryExpressionSyntax argument within ArgumentSyntax. /// - protected override async Task FindNodeToRefactorAsync(Document document, TextSpan selection, CancellationToken cancellationToken) + protected override Task FindNodeToRefactorAsync(Document document, TextSpan selection, CancellationToken cancellationToken) { var refactoringHelperService = document.GetLanguageService(); - return await refactoringHelperService.TryGetSelectedNodeAsync(document, selection, cancellationToken).ConfigureAwait(false); + return refactoringHelperService.TryGetSelectedNodeAsync(document, selection, cancellationToken); } private sealed class Converter diff --git a/src/Features/Core/Portable/CodeRefactoringHelpers.cs b/src/Features/Core/Portable/CodeRefactoringHelpers.cs index 2a8136f29c6..d30b19edbdf 100644 --- a/src/Features/Core/Portable/CodeRefactoringHelpers.cs +++ b/src/Features/Core/Portable/CodeRefactoringHelpers.cs @@ -135,13 +135,13 @@ private static async Task GetExpandedTextSpan(Document document, TextS } /// - /// Strips leading and trailing whitespace from . + /// Trims leading and trailing whitespace from . /// /// /// Returns unchanged in case . /// Returns empty Span with original in case it contains only whitespace. /// - public static async Task GetStrippedTextSpan(Document document, TextSpan span, CancellationToken cancellationToken) + public static async Task GetTrimmedTextSpan(Document document, TextSpan span, CancellationToken cancellationToken) { if (span.IsEmpty) { diff --git a/src/Features/Core/Portable/CodeRefactorings/AbstractRefactoringHelpersService.cs b/src/Features/Core/Portable/CodeRefactorings/AbstractRefactoringHelpersService.cs index f22a4f65a47..70f648ca70e 100644 --- a/src/Features/Core/Portable/CodeRefactorings/AbstractRefactoringHelpersService.cs +++ b/src/Features/Core/Portable/CodeRefactorings/AbstractRefactoringHelpersService.cs @@ -17,22 +17,68 @@ internal abstract class AbstractRefactoringHelpersService : IRefactoringHelpersS return await TryGetSelectedNodeAsync(document, selection, n => n is TSyntaxNode, cancellationToken).ConfigureAwait(false) as TSyntaxNode; } - public Task TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate predicate, CancellationToken cancellationToken) + /// + /// + /// Returns a Node for refactoring given specified selection that passes + /// or null if no such instance exists. + /// + /// + /// A node instance is return if: + /// - Selection is zero-width and inside/touching a Token with direct parent passing . + /// - Selection is zero-width and touching a Token whose ancestor Node passing ends/starts precisely on current selection. + /// - Token whose direct parent passing is selected. + /// - Whole node passing is selected. + /// + /// + /// Attempts extracting (and testing with a Node for each Node it consideres (see above). + /// By default extracts initializer expressions from declarations and assignments. + /// + /// + /// Note: this function trims all whitespace from both the beginning and the end of given . + /// The trimmed version is then used to determine relevant . It also handles incomplete selections + /// of tokens gracefully. + /// + /// + protected Task TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate predicate, CancellationToken cancellationToken) { return TryGetSelectedNodeAsync(document, selection, predicate, DefaultNodeExtractor, cancellationToken); } - public async Task TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate predicate, Func extractNode, CancellationToken cancellationToken) + /// + /// + /// Returns a Node for refactoring given specified selection that passes + /// or null if no such instance exists. + /// + /// + /// A node instance is return if: + /// - Selection is zero-width and inside/touching a Token with direct parent passing . + /// - Selection is zero-width and touching a Token whose ancestor Node passing ends/starts precisely on current selection. + /// - Token whose direct parent passing is selected. + /// - Whole node passing is selected. + /// + /// + /// The enables testing with and potentially returning Nodes + /// that are under those that might be selected / considered (as described above). It is a that + /// should always return either given Node or a Node somewhere below it that should be tested with and + /// potentially returned instead of current Node. + /// + /// + /// Note: this function trims all whitespace from both the beginning and the end of given . + /// The trimmed version is then used to determine relevant . It also handles incomplete selections + /// of tokens gracefully. + /// + /// + protected async Task TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate predicate, Func extractNode, CancellationToken cancellationToken) { var syntaxFacts = document.GetLanguageService(); var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); - var selectionStripped = await CodeRefactoringHelpers.GetStrippedTextSpan(document, selection, cancellationToken).ConfigureAwait(false); + var selectionTrimmed = await CodeRefactoringHelpers.GetTrimmedTextSpan(document, selection, cancellationToken).ConfigureAwait(false); // Handle selections // - the smallest node whose span inlcudes whole selection // - extraction from such node is attempted, result tested via predicate // - travels upwards through same-sized nodes, extracting and testing predicate - var node = root.FindNode(selectionStripped, getInnermostNodeForTie: true); + var node = root.FindNode(selectionTrimmed, getInnermostNodeForTie: true); SyntaxNode prevNode; do { @@ -55,10 +101,10 @@ public async Task TryGetSelectedNodeAsync(Document document, TextSpa } // get Token for current selection (empty) location - var tokenOnSelection = root.FindToken(selectionStripped.Start); + var tokenOnSelection = root.FindToken(selectionTrimmed.Start); // Token to right or containing current selection - var tokenToRightOrIn = tokenOnSelection.Span.Contains(selectionStripped.Start) + var tokenToRightOrIn = tokenOnSelection.Span.Contains(selectionTrimmed.Start) ? tokenOnSelection : default; @@ -80,17 +126,17 @@ public async Task TryGetSelectedNodeAsync(Document document, TextSpa } // if selection inside tokenToRightOrIn -> no Token can be to Left (tokenToRightOrIn is left from selection) - if (tokenToRightOrIn != default && tokenToRightOrIn.Span.Start != selectionStripped.Start) + if (tokenToRightOrIn != default && tokenToRightOrIn.Span.Start != selectionTrimmed.Start) { return default; } // Token to left - var tokenPreSelection = (tokenOnSelection.Span.End == selectionStripped.Start) + var tokenPreSelection = (tokenOnSelection.Span.End == selectionTrimmed.Start) ? tokenOnSelection : tokenOnSelection.GetPreviousToken(); - var tokenToLeft = (tokenPreSelection.Span.End == selectionStripped.Start) + var tokenToLeft = (tokenPreSelection.Span.End == selectionTrimmed.Start) ? tokenPreSelection : default; @@ -136,7 +182,17 @@ static SyntaxNode TryGetAcceptedNodeOrExtracted(SyntaxNode node, Predicate + /// + /// Extractor function for methods + /// that retrieves expressions from declarations and assignments. Otherwise returns unchanged . + /// + /// + /// The rationale is that when user selects e.g. entire local delaration statement [|var a = b;|] it is reasonable + /// to provide refactoring for `b` node. Similarly for other types of refactorings. + /// + /// + protected virtual SyntaxNode DefaultNodeExtractor(SyntaxNode node, ISyntaxFactsService syntaxFacts) { // var a = b; // -> b @@ -164,10 +220,7 @@ public virtual SyntaxNode DefaultNodeExtractor(SyntaxNode node, ISyntaxFactsServ if (syntaxFacts.IsSimpleAssignmentStatement(node)) { syntaxFacts.GetPartsOfAssignmentExpressionOrStatement(node, out _, out _, out var rightSide); - if (rightSide != default) - { - return rightSide; - } + return rightSide; } return node; diff --git a/src/Features/Core/Portable/CodeRefactorings/IRefactoringHelpersService.cs b/src/Features/Core/Portable/CodeRefactorings/IRefactoringHelpersService.cs index 64b0e9d6a17..0aa23fdc70e 100644 --- a/src/Features/Core/Portable/CodeRefactorings/IRefactoringHelpersService.cs +++ b/src/Features/Core/Portable/CodeRefactorings/IRefactoringHelpersService.cs @@ -1,44 +1,14 @@ // 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.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Host; -using Microsoft.CodeAnalysis.LanguageServices; using Microsoft.CodeAnalysis.Text; namespace Microsoft.CodeAnalysis.CodeRefactorings { internal interface IRefactoringHelpersService : ILanguageService { - /// - /// - /// Returns a Node for refactoring given specified selection that passes - /// or null if no such instance exists. - /// - /// - /// A node instance is return if: - /// - Selection is zero-width and inside/touching a Token with direct parent passing . - /// - Selection is zero-width and touching a Token whose ancestor Node passing ends/starts precisely on current selection. - /// - Token whose direct parent passing is selected. - /// - Whole node passing is selected. - /// - /// - /// The enables testing with and potentially returning Nodes - /// that are under those that might be selected / considered (as described above). It is a that - /// should always return either given Node or a Node somewhere below it that should be tested with and - /// potentially returned instead of current Node. - /// E.g. - /// allows returning right side Expresion node even if whole AssignmentNode is selected. - /// - /// - /// Note: this function strips all whitespace from both the beginning and the end of given . - /// The stripped version is then used to determine relevant . It also handles incomplete selections - /// of tokens gracefully. - /// - /// - Task TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate predicate, Func extractNode, CancellationToken cancellationToken); - /// /// /// Returns an instance of for refactoring given specified selection in document or null @@ -56,41 +26,11 @@ internal interface IRefactoringHelpersService : ILanguageService /// By default extracts initializer expressions from declarations and assignments. /// /// - /// Note: this function strips all whitespace from both the beginning and the end of given . - /// The stripped version is then used to determine relevant . It also handles incomplete selections + /// Note: this function trims all whitespace from both the beginning and the end of given . + /// The trimmed version is then used to determine relevant . It also handles incomplete selections /// of tokens gracefully. /// /// Task TryGetSelectedNodeAsync(Document document, TextSpan selection, CancellationToken cancellationToken) where TSyntaxNode : SyntaxNode; - - /// - /// - /// Returns a Node for refactoring given specified selection that passes - /// or null if no such instance exists. - /// - /// - /// A node instance is return if: - /// - Selection is zero-width and inside/touching a Token with direct parent passing . - /// - Selection is zero-width and touching a Token whose ancestor Node passing ends/starts precisely on current selection. - /// - Token whose direct parent passing is selected. - /// - Whole node passing is selected. - /// - /// - /// Attempts extracting (and testing with a Node for each Node it consideres (see above). - /// By default extracts initializer expressions from declarations and assignments. - /// - /// - /// Note: this function strips all whitespace from both the beginning and the end of given . - /// The stripped version is then used to determine relevant . It also handles incomplete selections - /// of tokens gracefully. - /// - /// - Task TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate predicate, CancellationToken cancellationToken); - - /// - /// Extractor function for methods that retrieves expressions from - /// declarations and assignments. Otherwise returns unchanged . - /// - SyntaxNode DefaultNodeExtractor(SyntaxNode node, ISyntaxFactsService syntaxFacts); } } -- GitLab