Skip to content

Commit d0a39c3

Browse files
Extract method cleanup (#76550)
2 parents 6af4373 + 2038e1a commit d0a39c3

File tree

99 files changed

+7393
-7471
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

99 files changed

+7393
-7471
lines changed

src/EditorFeatures/CSharpTest/ExtractMethod/ExtractMethodBase.cs

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
using Microsoft.CodeAnalysis.CSharp.ExtractMethod;
1515
using Microsoft.CodeAnalysis.CSharp.Syntax;
1616
using Microsoft.CodeAnalysis.ExtractMethod;
17+
using Microsoft.CodeAnalysis.Shared.Extensions;
1718
using Microsoft.CodeAnalysis.Test.Utilities;
1819
using Microsoft.CodeAnalysis.Text;
1920
using Roslyn.Test.Utilities;
@@ -23,7 +24,7 @@
2324
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.ExtractMethod;
2425

2526
[UseExportProvider]
26-
public class ExtractMethodBase
27+
public abstract class ExtractMethodBase
2728
{
2829
protected static async Task ExpectExtractMethodToFailAsync(
2930
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string codeWithMarker, string[] features = null)
@@ -128,19 +129,8 @@ protected static async Task<SyntaxNode> ExtractMethodAsync(
128129
CodeCleanupOptions = await document.GetCodeCleanupOptionsAsync(CancellationToken.None),
129130
};
130131

131-
var semanticDocument = await SemanticDocument.CreateAsync(document, CancellationToken.None);
132-
var validator = new CSharpSelectionValidator(semanticDocument, testDocument.SelectedSpans.Single(), localFunction);
133-
134-
var (selectedCode, status) = await validator.GetValidSelectionAsync(CancellationToken.None);
135-
if (!succeed && status.Failed)
136-
return null;
137-
138-
Assert.NotNull(selectedCode);
139-
140-
// extract method
141-
var extractor = new CSharpMethodExtractor(selectedCode, options, localFunction);
142-
var result = extractor.ExtractMethod(status, CancellationToken.None);
143-
Assert.NotNull(result);
132+
var result = await ExtractMethodService.ExtractMethodAsync(
133+
document, testDocument.SelectedSpans.Single(), localFunction, options, CancellationToken.None);
144134

145135
// If the test expects us to succeed, validate that we did. If it expects us to fail, ensure we either
146136
// failed or produced a message the user will have to confirm to continue.
@@ -174,7 +164,8 @@ protected static async Task TestSelectionAsync(
174164
Assert.NotNull(document);
175165

176166
var semanticDocument = await SemanticDocument.CreateAsync(document, CancellationToken.None);
177-
var validator = new CSharpSelectionValidator(semanticDocument, textSpanOverride ?? namedSpans["b"].Single(), localFunction: false);
167+
168+
var validator = new CSharpExtractMethodService.CSharpSelectionValidator(semanticDocument, textSpanOverride ?? namedSpans["b"].Single(), localFunction: false);
178169
var (result, status) = await validator.GetValidSelectionAsync(CancellationToken.None);
179170

180171
if (expectedFail)
@@ -203,7 +194,7 @@ protected static async Task IterateAllAsync(
203194

204195
foreach (var node in iterator)
205196
{
206-
var validator = new CSharpSelectionValidator(semanticDocument, node.Span, localFunction: false);
197+
var validator = new CSharpExtractMethodService.CSharpSelectionValidator(semanticDocument, node.Span, localFunction: false);
207198
var (_, status) = await validator.GetValidSelectionAsync(CancellationToken.None);
208199

209200
// check the obvious case

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1113,7 +1113,7 @@ public static void Main()
11131113
p1 = NewMethod(p2);
11141114
}
11151115
1116-
private static object NewMethod(object p2)
1116+
private static global::System.Object NewMethod(global::System.Object p2)
11171117
{
11181118
return p2;
11191119
}

src/EditorFeatures/VisualBasicTest/ExtractMethod/ExtractMethodTests.vb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.ExtractMethod
9595
Assert.NotNull(document)
9696

9797
Dim sdocument = Await SemanticDocument.CreateAsync(document, CancellationToken.None)
98-
Dim validator = New VisualBasicSelectionValidator(sdocument, snapshotSpan.Span.ToTextSpan())
98+
Dim validator = New VisualBasicExtractMethodService.VisualBasicSelectionValidator(sdocument, snapshotSpan.Span.ToTextSpan())
9999

100100
Dim tuple = Await validator.GetValidSelectionAsync(CancellationToken.None)
101101
Dim selectedCode = tuple.Item1
@@ -109,7 +109,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.ExtractMethod
109109
Await document.GetCodeGenerationOptionsAsync(CancellationToken.None),
110110
Await document.GetCodeCleanupOptionsAsync(CancellationToken.None))
111111

112-
Dim extractor = New VisualBasicMethodExtractor(selectedCode, extractGenerationOptions)
112+
Dim extractor = New VisualBasicExtractMethodService.VisualBasicMethodExtractor(selectedCode, extractGenerationOptions)
113113
Dim result = extractor.ExtractMethod(status, CancellationToken.None)
114114
Assert.NotNull(result)
115115

@@ -137,7 +137,7 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.ExtractMethod
137137
Assert.NotNull(document)
138138

139139
Dim sdocument = Await SemanticDocument.CreateAsync(document, CancellationToken.None)
140-
Dim validator = New VisualBasicSelectionValidator(sdocument, namedSpans("b").Single())
140+
Dim validator = New VisualBasicExtractMethodService.VisualBasicSelectionValidator(sdocument, namedSpans("b").Single())
141141
Dim tuple = Await validator.GetValidSelectionAsync(CancellationToken.None)
142142
Dim result = tuple.Item1
143143
Dim status = tuple.Item2
@@ -172,7 +172,7 @@ End Class</text>
172172

173173
For Each node In iterator
174174
Try
175-
Dim validator = New VisualBasicSelectionValidator(sdocument, node.Span)
175+
Dim validator = New VisualBasicExtractMethodService.VisualBasicSelectionValidator(sdocument, node.Span)
176176
Dim tuple = Await validator.GetValidSelectionAsync(CancellationToken.None)
177177
Dim result = tuple.Item1
178178
Dim status = tuple.Item2

src/Features/CSharp/Portable/CSharpFeaturesResources.resx

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,6 @@
151151
<data name="Remove_this_qualification" xml:space="preserve">
152152
<value>Remove 'this' qualification</value>
153153
</data>
154-
<data name="Cannot_determine_valid_range_of_statements_to_extract" xml:space="preserve">
155-
<value>Cannot determine valid range of statements to extract</value>
156-
</data>
157154
<data name="Selection_does_not_contain_a_valid_node" xml:space="preserve">
158155
<value>Selection does not contain a valid node</value>
159156
</data>
@@ -181,9 +178,6 @@
181178
<data name="The_selected_code_is_inside_an_unsafe_context" xml:space="preserve">
182179
<value>The selected code is inside an unsafe context.</value>
183180
</data>
184-
<data name="No_valid_statement_range_to_extract" xml:space="preserve">
185-
<value>No valid statement range to extract</value>
186-
</data>
187181
<data name="deprecated" xml:space="preserve">
188182
<value>deprecated</value>
189183
</data>

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@ namespace Microsoft.CodeAnalysis.CSharp.ExtractMethod;
1515
[ExportLanguageService(typeof(IExtractMethodService), LanguageNames.CSharp)]
1616
[method: ImportingConstructor]
1717
[method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
18-
internal sealed class CSharpExtractMethodService() : AbstractExtractMethodService<
19-
CSharpSelectionValidator,
20-
CSharpMethodExtractor,
21-
CSharpSelectionResult,
18+
internal sealed partial class CSharpExtractMethodService() : AbstractExtractMethodService<
19+
CSharpExtractMethodService.CSharpSelectionValidator,
20+
CSharpExtractMethodService.CSharpMethodExtractor,
21+
CSharpExtractMethodService.CSharpSelectionResult,
22+
StatementSyntax,
2223
StatementSyntax,
2324
ExpressionSyntax>
2425
{

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

Lines changed: 60 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -3,86 +3,88 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System.Collections.Generic;
6+
using System.Collections.Immutable;
67
using System.Linq;
78
using System.Threading;
89
using Microsoft.CodeAnalysis.CSharp.Syntax;
910
using Microsoft.CodeAnalysis.ExtractMethod;
1011

1112
namespace Microsoft.CodeAnalysis.CSharp.ExtractMethod;
1213

13-
internal sealed partial class CSharpMethodExtractor
14+
internal sealed partial class CSharpExtractMethodService
1415
{
15-
private sealed class CSharpAnalyzer(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken) : Analyzer(selectionResult, localFunction, cancellationToken)
16+
internal sealed partial class CSharpMethodExtractor
1617
{
17-
private static readonly HashSet<int> s_nonNoisySyntaxKindSet = [(int)SyntaxKind.WhitespaceTrivia, (int)SyntaxKind.EndOfLineTrivia];
18-
19-
public static AnalyzerResult Analyze(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken)
18+
private sealed class CSharpAnalyzer(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken) : Analyzer(selectionResult, localFunction, cancellationToken)
2019
{
21-
var analyzer = new CSharpAnalyzer(selectionResult, localFunction, cancellationToken);
22-
return analyzer.Analyze();
23-
}
20+
public static AnalyzerResult Analyze(CSharpSelectionResult selectionResult, bool localFunction, CancellationToken cancellationToken)
21+
{
22+
var analyzer = new CSharpAnalyzer(selectionResult, localFunction, cancellationToken);
23+
return analyzer.Analyze();
24+
}
2425

25-
protected override bool TreatOutAsRef
26-
=> false;
26+
protected override bool TreatOutAsRef
27+
=> false;
2728

28-
protected override bool IsInPrimaryConstructorBaseType()
29-
=> this.SelectionResult.GetContainingScopeOf<PrimaryConstructorBaseTypeSyntax>() != null;
29+
protected override bool IsInPrimaryConstructorBaseType()
30+
=> this.SelectionResult.GetContainingScopeOf<PrimaryConstructorBaseTypeSyntax>() != null;
3031

31-
protected override VariableInfo CreateFromSymbol(
32-
ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared)
33-
{
34-
return CreateFromSymbolCommon<LocalDeclarationStatementSyntax>(symbol, type, style, s_nonNoisySyntaxKindSet);
35-
}
32+
protected override VariableInfo CreateFromSymbol(
33+
ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared)
34+
{
35+
return CreateFromSymbolCommon(symbol, type, style);
36+
}
3637

37-
protected override ITypeSymbol? GetRangeVariableType(SemanticModel model, IRangeVariableSymbol symbol)
38-
{
39-
var info = model.GetSpeculativeTypeInfo(SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression);
40-
if (info.Type is IErrorTypeSymbol)
41-
return null;
38+
protected override ITypeSymbol? GetRangeVariableType(IRangeVariableSymbol symbol)
39+
{
40+
var info = this.SemanticModel.GetSpeculativeTypeInfo(SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression);
41+
if (info.Type is IErrorTypeSymbol)
42+
return null;
4243

43-
return info.Type == null || info.Type.SpecialType == SpecialType.System_Object
44-
? info.Type
45-
: info.ConvertedType;
46-
}
44+
return info.Type == null || info.Type.SpecialType == SpecialType.System_Object
45+
? info.Type
46+
: info.ConvertedType;
47+
}
4748

48-
protected override bool ContainsReturnStatementInSelectedCode(IEnumerable<SyntaxNode> jumpOutOfRegionStatements)
49-
=> jumpOutOfRegionStatements.Where(n => n is ReturnStatementSyntax).Any();
49+
protected override bool ContainsReturnStatementInSelectedCode(ImmutableArray<SyntaxNode> exitPoints)
50+
=> exitPoints.Any(n => n is ReturnStatementSyntax);
5051

51-
protected override bool ReadOnlyFieldAllowed()
52-
{
53-
var scope = SelectionResult.GetContainingScopeOf<ConstructorDeclarationSyntax>();
54-
return scope == null;
55-
}
52+
protected override bool ReadOnlyFieldAllowed()
53+
{
54+
var scope = SelectionResult.GetContainingScopeOf<ConstructorDeclarationSyntax>();
55+
return scope == null;
56+
}
5657

57-
protected override bool IsReadOutside(ISymbol symbol, HashSet<ISymbol> readOutsideMap)
58-
{
59-
if (!base.IsReadOutside(symbol, readOutsideMap))
60-
return false;
58+
protected override bool IsReadOutside(ISymbol symbol, HashSet<ISymbol> readOutsideMap)
59+
{
60+
if (!base.IsReadOutside(symbol, readOutsideMap))
61+
return false;
6162

62-
// Special case `using var v = ...` where the selection grabs the last statement that follows the local
63-
// declaration. The compiler here considers the local variable 'read outside' since it makes it to the
64-
// implicit 'dispose' call that comes after the last statement. However, as that implicit dispose would
65-
// move if we move the `using var v` entirely into the new method, then it's still safe to move as there's
66-
// no actual "explicit user read" that happens in the outer caller at all.
67-
if (!this.SelectionResult.SelectionInExpression &&
68-
symbol is ILocalSymbol { IsUsing: true, DeclaringSyntaxReferences: [var reference] } &&
69-
reference.GetSyntax(this.CancellationToken) is VariableDeclaratorSyntax
70-
{
71-
Parent: VariableDeclarationSyntax
63+
// Special case `using var v = ...` where the selection grabs the last statement that follows the local
64+
// declaration. The compiler here considers the local variable 'read outside' since it makes it to the
65+
// implicit 'dispose' call that comes after the last statement. However, as that implicit dispose would
66+
// move if we move the `using var v` entirely into the new method, then it's still safe to move as there's
67+
// no actual "explicit user read" that happens in the outer caller at all.
68+
if (!this.SelectionResult.IsExtractMethodOnExpression &&
69+
symbol is ILocalSymbol { IsUsing: true, DeclaringSyntaxReferences: [var reference] } &&
70+
reference.GetSyntax(this.CancellationToken) is VariableDeclaratorSyntax
7271
{
73-
Parent: LocalDeclarationStatementSyntax
72+
Parent: VariableDeclarationSyntax
7473
{
75-
Parent: BlockSyntax { Statements: [.., var lastBlockStatement] },
76-
},
77-
}
78-
})
79-
{
80-
var lastStatement = this.SelectionResult.GetLastStatement();
81-
if (lastStatement == lastBlockStatement)
82-
return false;
83-
}
74+
Parent: LocalDeclarationStatementSyntax
75+
{
76+
Parent: BlockSyntax { Statements: [.., var lastBlockStatement] },
77+
},
78+
}
79+
})
80+
{
81+
var lastStatement = this.SelectionResult.GetLastStatement();
82+
if (lastStatement == lastBlockStatement)
83+
return false;
84+
}
8485

85-
return true;
86+
return true;
87+
}
8688
}
8789
}
8890
}

0 commit comments

Comments
 (0)