Extract method cleanup

src/EditorFeatures/CSharpTest/ExtractMethod/ExtractMethodBase.cs

@@ -14,6 +14,7 @@
 using Microsoft.CodeAnalysis.CSharp.ExtractMethod;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.ExtractMethod;
+using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Test.Utilities;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Test.Utilities;
@@ -23,7 +24,7 @@
 namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.ExtractMethod;
 
 [UseExportProvider]
-public class ExtractMethodBase
+public abstract class ExtractMethodBase
 {
     protected static async Task ExpectExtractMethodToFailAsync(
         [StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string codeWithMarker, string[] features = null)
@@ -128,19 +129,8 @@ protected static async Task<SyntaxNode> ExtractMethodAsync(
             CodeCleanupOptions = await document.GetCodeCleanupOptionsAsync(CancellationToken.None),
         };
 
-        var semanticDocument = await SemanticDocument.CreateAsync(document, CancellationToken.None);
-        var validator = new CSharpSelectionValidator(semanticDocument, testDocument.SelectedSpans.Single(), localFunction);
-
-        var (selectedCode, status) = await validator.GetValidSelectionAsync(CancellationToken.None);
-        if (!succeed && status.Failed)
-            return null;
-
-        Assert.NotNull(selectedCode);
-
-        // extract method
-        var extractor = new CSharpMethodExtractor(selectedCode, options, localFunction);
-        var result = extractor.ExtractMethod(status, CancellationToken.None);
-        Assert.NotNull(result);
+        var result = await ExtractMethodService.ExtractMethodAsync(
+            document, testDocument.SelectedSpans.Single(), localFunction, options, CancellationToken.None);
 
         // If the test expects us to succeed, validate that we did.  If it expects us to fail, ensure we either
         // failed or produced a message the user will have to confirm to continue. 
@@ -174,7 +164,8 @@ protected static async Task TestSelectionAsync(
         Assert.NotNull(document);
 
         var semanticDocument = await SemanticDocument.CreateAsync(document, CancellationToken.None);
-        var validator = new CSharpSelectionValidator(semanticDocument, textSpanOverride ?? namedSpans["b"].Single(), localFunction: false);
+
+        var validator = new CSharpExtractMethodService.CSharpSelectionValidator(semanticDocument, textSpanOverride ?? namedSpans["b"].Single(), localFunction: false);
         var (result, status) = await validator.GetValidSelectionAsync(CancellationToken.None);
 
         if (expectedFail)
@@ -203,7 +194,7 @@ protected static async Task IterateAllAsync(
 
         foreach (var node in iterator)
         {
-            var validator = new CSharpSelectionValidator(semanticDocument, node.Span, localFunction: false);
+            var validator = new CSharpExtractMethodService.CSharpSelectionValidator(semanticDocument, node.Span, localFunction: false);
             var (_, status) = await validator.GetValidSelectionAsync(CancellationToken.None);
 
             // check the obvious case

src/EditorFeatures/CSharpTest/ExtractMethod/ExtractMethodTests.LanguageInteraction.cs

@@ -1113,7 +1113,7 @@ public static void Main()
                         p1 = NewMethod(p2);
                     }
 
-                    private static object NewMethod(object p2)
+                    private static global::System.Object NewMethod(global::System.Object p2)
                     {
                         return p2;
                     }

src/EditorFeatures/VisualBasicTest/ExtractMethod/ExtractMethodTests.vb

@@ -95,7 +95,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.ExtractMethod
             Assert.NotNull(document)
 
             Dim sdocument = Await SemanticDocument.CreateAsync(document, CancellationToken.None)
-            Dim validator = New VisualBasicSelectionValidator(sdocument, snapshotSpan.Span.ToTextSpan())
+            Dim validator = New VisualBasicExtractMethodService.VisualBasicSelectionValidator(sdocument, snapshotSpan.Span.ToTextSpan())
 
             Dim tuple = Await validator.GetValidSelectionAsync(CancellationToken.None)
             Dim selectedCode = tuple.Item1
@@ -109,7 +109,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.ExtractMethod
                 Await document.GetCodeGenerationOptionsAsync(CancellationToken.None),
                 Await document.GetCodeCleanupOptionsAsync(CancellationToken.None))
 
-            Dim extractor = New VisualBasicMethodExtractor(selectedCode, extractGenerationOptions)
+            Dim extractor = New VisualBasicExtractMethodService.VisualBasicMethodExtractor(selectedCode, extractGenerationOptions)
             Dim result = extractor.ExtractMethod(status, CancellationToken.None)
             Assert.NotNull(result)
 
@@ -137,7 +137,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.ExtractMethod
                 Assert.NotNull(document)
 
                 Dim sdocument = Await SemanticDocument.CreateAsync(document, CancellationToken.None)
-                Dim validator = New VisualBasicSelectionValidator(sdocument, namedSpans("b").Single())
+                Dim validator = New VisualBasicExtractMethodService.VisualBasicSelectionValidator(sdocument, namedSpans("b").Single())
                 Dim tuple = Await validator.GetValidSelectionAsync(CancellationToken.None)
                 Dim result = tuple.Item1
                 Dim status = tuple.Item2
@@ -172,7 +172,7 @@ End Class</text>
 
                 For Each node In iterator
                     Try
-                        Dim validator = New VisualBasicSelectionValidator(sdocument, node.Span)
+                        Dim validator = New VisualBasicExtractMethodService.VisualBasicSelectionValidator(sdocument, node.Span)
                         Dim tuple = Await validator.GetValidSelectionAsync(CancellationToken.None)
                         Dim result = tuple.Item1
                         Dim status = tuple.Item2

src/Features/CSharp/Portable/CSharpFeaturesResources.resx

@@ -151,9 +151,6 @@
   <data name="Remove_this_qualification" xml:space="preserve">
     <value>Remove 'this' qualification</value>
   </data>
-  <data name="Cannot_determine_valid_range_of_statements_to_extract" xml:space="preserve">
-    <value>Cannot determine valid range of statements to extract</value>
-  </data>
   <data name="Selection_does_not_contain_a_valid_node" xml:space="preserve">
     <value>Selection does not contain a valid node</value>
   </data>
@@ -181,9 +178,6 @@
   <data name="The_selected_code_is_inside_an_unsafe_context" xml:space="preserve">
     <value>The selected code is inside an unsafe context.</value>
   </data>
-  <data name="No_valid_statement_range_to_extract" xml:space="preserve">
-    <value>No valid statement range to extract</value>
-  </data>
   <data name="deprecated" xml:space="preserve">
     <value>deprecated</value>
   </data>

src/Features/CSharp/Portable/ExtractMethod/CSharpExtractMethodService.cs

@@ -15,10 +15,11 @@ namespace Microsoft.CodeAnalysis.CSharp.ExtractMethod;
 [ExportLanguageService(typeof(IExtractMethodService), LanguageNames.CSharp)]
 [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 class CSharpExtractMethodService() : AbstractExtractMethodService<
-    CSharpSelectionValidator,
-    CSharpMethodExtractor,
-    CSharpSelectionResult,
+internal sealed partial class CSharpExtractMethodService() : AbstractExtractMethodService<
+    CSharpExtractMethodService.CSharpSelectionValidator,
+    CSharpExtractMethodService.CSharpMethodExtractor,
+    CSharpExtractMethodService.CSharpSelectionResult,
+    StatementSyntax,
     StatementSyntax,
     ExpressionSyntax>
 {

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

@@ -3,86 +3,88 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Collections.Generic;
+using System.Collections.Immutable;
 using System.Linq;
 using System.Threading;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.ExtractMethod;
 
 namespace Microsoft.CodeAnalysis.CSharp.ExtractMethod;
 
-internal sealed partial class CSharpMethodExtractor
+internal sealed partial class CSharpExtractMethodService
 {
-    private sealed class CSharpAnalyzer(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken) : Analyzer(selectionResult, localFunction, cancellationToken)
+    internal sealed partial class CSharpMethodExtractor
     {
-        private static readonly HashSet<int> s_nonNoisySyntaxKindSet = [(int)SyntaxKind.WhitespaceTrivia, (int)SyntaxKind.EndOfLineTrivia];
-
-        public static AnalyzerResult Analyze(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken)
+        private sealed class CSharpAnalyzer(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken) : Analyzer(selectionResult, localFunction, cancellationToken)
         {
-            var analyzer = new CSharpAnalyzer(selectionResult, localFunction, cancellationToken);
-            return analyzer.Analyze();
-        }
+            public static AnalyzerResult Analyze(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken)
+            {
+                var analyzer = new CSharpAnalyzer(selectionResult, localFunction, cancellationToken);
+                return analyzer.Analyze();
+            }
 
-        protected override bool TreatOutAsRef
-            => false;
+            protected override bool TreatOutAsRef
+                => false;
 
-        protected override bool IsInPrimaryConstructorBaseType()
-            => this.SelectionResult.GetContainingScopeOf<PrimaryConstructorBaseTypeSyntax>() != null;
+            protected override bool IsInPrimaryConstructorBaseType()
+                => this.SelectionResult.GetContainingScopeOf<PrimaryConstructorBaseTypeSyntax>() != null;
 
-        protected override VariableInfo CreateFromSymbol(
-            ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared)
-        {
-            return CreateFromSymbolCommon<LocalDeclarationStatementSyntax>(symbol, type, style, s_nonNoisySyntaxKindSet);
-        }
+            protected override VariableInfo CreateFromSymbol(
+                ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared)
+            {
+                return CreateFromSymbolCommon(symbol, type, style);
+            }
 
-        protected override ITypeSymbol? GetRangeVariableType(SemanticModel model, IRangeVariableSymbol symbol)
-        {
-            var info = model.GetSpeculativeTypeInfo(SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression);
-            if (info.Type is IErrorTypeSymbol)
-                return null;
+            protected override ITypeSymbol? GetRangeVariableType(IRangeVariableSymbol symbol)
+            {
+                var info = this.SemanticModel.GetSpeculativeTypeInfo(SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression);
+                if (info.Type is IErrorTypeSymbol)
+                    return null;
 
-            return info.Type == null || info.Type.SpecialType == SpecialType.System_Object
-                ? info.Type
-                : info.ConvertedType;
-        }
+                return info.Type == null || info.Type.SpecialType == SpecialType.System_Object
+                    ? info.Type
+                    : info.ConvertedType;
+            }
 
-        protected override bool ContainsReturnStatementInSelectedCode(IEnumerable<SyntaxNode> jumpOutOfRegionStatements)
-            => jumpOutOfRegionStatements.Where(n => n is ReturnStatementSyntax).Any();
+            protected override bool ContainsReturnStatementInSelectedCode(ImmutableArray<SyntaxNode> exitPoints)
+                => exitPoints.Any(n => n is ReturnStatementSyntax);
 
-        protected override bool ReadOnlyFieldAllowed()
-        {
-            var scope = SelectionResult.GetContainingScopeOf<ConstructorDeclarationSyntax>();
-            return scope == null;
-        }
+            protected override bool ReadOnlyFieldAllowed()
+            {
+                var scope = SelectionResult.GetContainingScopeOf<ConstructorDeclarationSyntax>();
+                return scope == null;
+            }
 
-        protected override bool IsReadOutside(ISymbol symbol, HashSet<ISymbol> readOutsideMap)
-        {
-            if (!base.IsReadOutside(symbol, readOutsideMap))
-                return false;
+            protected override bool IsReadOutside(ISymbol symbol, HashSet<ISymbol> readOutsideMap)
+            {
+                if (!base.IsReadOutside(symbol, readOutsideMap))
+                    return false;
 
-            // Special case `using var v = ...` where the selection grabs the last statement that follows the local
-            // declaration.  The compiler here considers the local variable 'read outside' since it makes it to the
-            // implicit 'dispose' call that comes after the last statement.  However, as that implicit dispose would
-            // move if we move the `using var v` entirely into the new method, then it's still safe to move as there's
-            // no actual "explicit user read" that happens in the outer caller at all.
-            if (!this.SelectionResult.SelectionInExpression &&
-                symbol is ILocalSymbol { IsUsing: true, DeclaringSyntaxReferences: [var reference] } &&
-                reference.GetSyntax(this.CancellationToken) is VariableDeclaratorSyntax
-                {
-                    Parent: VariableDeclarationSyntax
+                // Special case `using var v = ...` where the selection grabs the last statement that follows the local
+                // declaration.  The compiler here considers the local variable 'read outside' since it makes it to the
+                // implicit 'dispose' call that comes after the last statement.  However, as that implicit dispose would
+                // move if we move the `using var v` entirely into the new method, then it's still safe to move as there's
+                // no actual "explicit user read" that happens in the outer caller at all.
+                if (!this.SelectionResult.IsExtractMethodOnExpression &&
+                    symbol is ILocalSymbol { IsUsing: true, DeclaringSyntaxReferences: [var reference] } &&
+                    reference.GetSyntax(this.CancellationToken) is VariableDeclaratorSyntax
                     {
-                        Parent: LocalDeclarationStatementSyntax
+                        Parent: VariableDeclarationSyntax
                         {
-                            Parent: BlockSyntax { Statements: [.., var lastBlockStatement] },
-                        },
-                    }
-                })
-            {
-                var lastStatement = this.SelectionResult.GetLastStatement();
-                if (lastStatement == lastBlockStatement)
-                    return false;
-            }
+                            Parent: LocalDeclarationStatementSyntax
+                            {
+                                Parent: BlockSyntax { Statements: [.., var lastBlockStatement] },
+                            },
+                        }
+                    })
+                {
+                    var lastStatement = this.SelectionResult.GetLastStatement();
+                    if (lastStatement == lastBlockStatement)
+                        return false;
+                }
 
-            return true;
+                return true;
+            }
         }
     }
 }