PR feedback (moved things around, renamed few things).

src/Features/CSharp/Portable/CodeRefactorings/PullMemberUp/CSharpPullMemberUpCodeRefactoringProvider.cs

@@ -31,8 +31,15 @@ public CSharpPullMemberUpCodeRefactoringProvider() : base(null)
         protected override async Task<SyntaxNode> GetSelectedNodeAsync(Document document, TextSpan span, CancellationToken cancellationToken)
         {
             var refactoringHelperService = document.GetLanguageService<IRefactoringHelpersService>();
-            return await refactoringHelperService.TryGetSelectedNodeAsync(
-                document, span, n => n is MemberDeclarationSyntax || n is VariableDeclaratorSyntax, cancellationToken).ConfigureAwait(false);
+
+            var memberDecl = await refactoringHelperService.TryGetSelectedNodeAsync<MemberDeclarationSyntax>(document, span, cancellationToken).ConfigureAwait(false);
+            if (memberDecl != default)
+            {
+                return memberDecl;
+            }
+
+            var varDecl = await refactoringHelperService.TryGetSelectedNodeAsync<VariableDeclaratorSyntax>(document, span, cancellationToken).ConfigureAwait(false);
+            return varDecl;
         }
     }
 }

src/Features/CSharp/Portable/ConvertLinq/CSharpConvertLinqQueryToForEachProvider.cs

@@ -43,10 +43,10 @@ public CSharpConvertLinqQueryToForEachProvider()
         /// <summary>
         /// Finds a node for the span and checks that it is either a QueryExpressionSyntax or a QueryExpressionSyntax argument within ArgumentSyntax.
         /// </summary>
-        protected override async Task<QueryExpressionSyntax> FindNodeToRefactorAsync(Document document, TextSpan selection, CancellationToken cancellationToken)
+        protected override Task<QueryExpressionSyntax> FindNodeToRefactorAsync(Document document, TextSpan selection, CancellationToken cancellationToken)
         {
             var refactoringHelperService = document.GetLanguageService<IRefactoringHelpersService>();
-            return await refactoringHelperService.TryGetSelectedNodeAsync<QueryExpressionSyntax>(document, selection, cancellationToken).ConfigureAwait(false);
+            return refactoringHelperService.TryGetSelectedNodeAsync<QueryExpressionSyntax>(document, selection, cancellationToken);
         }
 
         private sealed class Converter

src/Features/Core/Portable/CodeRefactoringHelpers.cs

@@ -135,13 +135,13 @@ private static async Task<TextSpan> GetExpandedTextSpan(Document document, TextS
         }
 
         /// <summary>
-        /// Strips leading and trailing whitespace from <paramref name="span"/>.
+        /// Trims leading and trailing whitespace from <paramref name="span"/>.
         /// </summary>
         /// <remarks>
         /// Returns unchanged <paramref name="span"/> in case <see cref="TextSpan.IsEmpty"/>.
         /// Returns empty Span with original <see cref="TextSpan.Start"/> in case it contains only whitespace.
         /// </remarks>
-        public static async Task<TextSpan> GetStrippedTextSpan(Document document, TextSpan span, CancellationToken cancellationToken)
+        public static async Task<TextSpan> GetTrimmedTextSpan(Document document, TextSpan span, CancellationToken cancellationToken)
         {
             if (span.IsEmpty)
             {

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<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, CancellationToken cancellationToken)
+        /// <summary>
+        /// <para>
+        /// Returns a Node for refactoring given specified selection that passes <paramref name="predicate"/> 
+        /// or null if no such instance exists.
+        /// </para>
+        /// <para>
+        /// A node instance is return if:
+        /// - Selection is zero-width and inside/touching a Token with direct parent passing <paramref name="predicate"/>.
+        /// - Selection is zero-width and touching a Token whose ancestor Node passing <paramref name="predicate"/> ends/starts precisely on current selection.
+        /// - Token whose direct parent passing <paramref name="predicate"/> is selected.
+        /// - Whole node passing <paramref name="predicate"/> is selected.
+        /// </para>
+        /// <para>
+        /// Attempts extracting (and testing with <paramref name="predicate"/> a Node for each Node it consideres (see above).
+        /// By default extracts initializer expressions from declarations and assignments.
+        /// </para>
+        /// <para>
+        /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selection"/>.
+        /// The trimmed version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
+        /// of tokens gracefully.
+        /// </para>
+        /// </summary>
+        protected Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, CancellationToken cancellationToken)
         {
             return TryGetSelectedNodeAsync(document, selection, predicate, DefaultNodeExtractor, cancellationToken);
         }
 
-        public async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, Func<SyntaxNode, ISyntaxFactsService, SyntaxNode> extractNode, CancellationToken cancellationToken)
+        /// <summary>
+        /// <para>
+        /// Returns a Node for refactoring given specified selection that passes <paramref name="predicate"/> 
+        /// or null if no such instance exists.
+        /// </para>
+        /// <para>
+        /// A node instance is return if:
+        /// - Selection is zero-width and inside/touching a Token with direct parent passing <paramref name="predicate"/>.
+        /// - Selection is zero-width and touching a Token whose ancestor Node passing <paramref name="predicate"/> ends/starts precisely on current selection.
+        /// - Token whose direct parent passing <paramref name="predicate"/> is selected.
+        /// - Whole node passing <paramref name="predicate"/> is selected.
+        /// </para>
+        /// <para>
+        /// The <paramref name="extractNode"/> enables testing with <paramref name="predicate"/> and potentially returning Nodes 
+        /// that are under those that might be selected / considered (as described above). It is a <see cref="Func{SyntaxNode, SyntaxNode}"/> that
+        /// should always return either given Node or a Node somewhere below it that should be tested with <paramref name="predicate"/> and
+        /// potentially returned instead of current Node. 
+        /// </para>
+        /// <para>
+        /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selection"/>.
+        /// The trimmed version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
+        /// of tokens gracefully.
+        /// </para>
+        /// </summary>
+        protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, Func<SyntaxNode, ISyntaxFactsService, SyntaxNode> extractNode, CancellationToken cancellationToken)
         {
             var syntaxFacts = document.GetLanguageService<ISyntaxFactsService>();
             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<SyntaxNode> 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<SyntaxNode> 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<Synta
             }
         }
 
-        public virtual SyntaxNode DefaultNodeExtractor(SyntaxNode node, ISyntaxFactsService syntaxFacts)
+        /// <summary>
+        /// <para>
+        /// Extractor function for <see cref="TryGetSelectedNodeAsync(Document, TextSpan, Predicate{SyntaxNode}, Func{SyntaxNode, ISyntaxFactsService, SyntaxNode}, CancellationToken)"/> methods 
+        /// that retrieves expressions from  declarations and assignments. Otherwise returns unchanged <paramref name="node"/>.
+        /// </para>
+        /// <para>
+        /// 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.
+        /// </para>
+        /// </summary>
+        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;

src/Features/Core/Portable/CodeRefactorings/IRefactoringHelpersService.cs

 // 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
     {
-        /// <summary>
-        /// <para>
-        /// Returns a Node for refactoring given specified selection that passes <paramref name="predicate"/> 
-        /// or null if no such instance exists.
-        /// </para>
-        /// <para>
-        /// A node instance is return if:
-        /// - Selection is zero-width and inside/touching a Token with direct parent passing <paramref name="predicate"/>.
-        /// - Selection is zero-width and touching a Token whose ancestor Node passing <paramref name="predicate"/> ends/starts precisely on current selection.
-        /// - Token whose direct parent passing <paramref name="predicate"/> is selected.
-        /// - Whole node passing <paramref name="predicate"/> is selected.
-        /// </para>
-        /// <para>
-        /// The <paramref name="extractNode"/> enables testing with <paramref name="predicate"/> and potentially returning Nodes 
-        /// that are under those that might be selected / considered (as described above). It is a <see cref="Func{SyntaxNode, SyntaxNode}"/> that
-        /// should always return either given Node or a Node somewhere below it that should be tested with <paramref name="predicate"/> and
-        /// potentially returned instead of current Node. 
-        /// E.g. <see cref="DefaultNodeExtractor(SyntaxNode, ISyntaxFactsService)"/>
-        /// allows returning right side Expresion node even if whole AssignmentNode is selected.
-        /// </para>
-        /// <para>
-        /// Note: this function strips all whitespace from both the beginning and the end of given <paramref name="selection"/>.
-        /// The stripped version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
-        /// of tokens gracefully.
-        /// </para>
-        /// </summary>
-        Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, Func<SyntaxNode, ISyntaxFactsService, SyntaxNode> extractNode, CancellationToken cancellationToken);
-
         /// <summary>
         /// <para>
         /// Returns an instance of <typeparamref name="TSyntaxNode"/> 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.
         /// </para>
         /// <para>
-        /// Note: this function strips all whitespace from both the beginning and the end of given <paramref name="selection"/>.
-        /// The stripped version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
+        /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selection"/>.
+        /// The trimmed version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
         /// of tokens gracefully.
         /// </para>
         /// </summary>
         Task<TSyntaxNode> TryGetSelectedNodeAsync<TSyntaxNode>(Document document, TextSpan selection, CancellationToken cancellationToken) where TSyntaxNode : SyntaxNode;
-
-        /// <summary>
-        /// <para>
-        /// Returns a Node for refactoring given specified selection that passes <paramref name="predicate"/> 
-        /// or null if no such instance exists.
-        /// </para>
-        /// <para>
-        /// A node instance is return if:
-        /// - Selection is zero-width and inside/touching a Token with direct parent passing <paramref name="predicate"/>.
-        /// - Selection is zero-width and touching a Token whose ancestor Node passing <paramref name="predicate"/> ends/starts precisely on current selection.
-        /// - Token whose direct parent passing <paramref name="predicate"/> is selected.
-        /// - Whole node passing <paramref name="predicate"/> is selected.
-        /// </para>
-        /// <para>
-        /// Attempts extracting (and testing with <paramref name="predicate"/> a Node for each Node it consideres (see above).
-        /// By default extracts initializer expressions from declarations and assignments.
-        /// </para>
-        /// <para>
-        /// Note: this function strips all whitespace from both the beginning and the end of given <paramref name="selection"/>.
-        /// The stripped version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
-        /// of tokens gracefully.
-        /// </para>
-        /// </summary>
-        Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, CancellationToken cancellationToken);
-
-        /// <summary>
-        /// Extractor function for <see cref="TryGetSelectedNodeAsync(Document, TextSpan, Predicate{SyntaxNode}, Func{SyntaxNode, ISyntaxFactsService, SyntaxNode}, CancellationToken)"/> methods that retrieves expressions from 
-        /// declarations and assignments. Otherwise returns unchanged <paramref name="node"/>.
-        /// </summary>
-        SyntaxNode DefaultNodeExtractor(SyntaxNode node, ISyntaxFactsService syntaxFacts);
     }
 }