From 5fb756d22dec225b88353ff0c06ee4f308937eda Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 09:01:49 -0800 Subject: [PATCH 01/32] Simplify --- .../CSharpMethodExtractor.CSharpCodeGenerator.cs | 2 +- .../CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs | 4 ++-- src/Features/Core/Portable/ExtractMethod/SelectionResult.cs | 2 +- .../Portable/ExtractMethod/VisualBasicSelectionResult.vb | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs index de18e60d3d44b..3b9797df2ef25 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs @@ -159,7 +159,7 @@ private static bool IsExpressionBodiedMember(SyntaxNode node) => node is MemberDeclarationSyntax member && member.GetExpressionBody() != null; private static bool IsExpressionBodiedAccessor(SyntaxNode node) - => node is AccessorDeclarationSyntax accessor && accessor.ExpressionBody != null; + => node is AccessorDeclarationSyntax { ExpressionBody: not null }; private SimpleNameSyntax CreateMethodNameForInvocation() { diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs index 35fe95a317bb9..71952c45448d2 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs @@ -189,8 +189,8 @@ public SyntaxNode GetInnermostStatementContainer() return last.Parent.Parent; } - public override bool ContainsNonReturnExitPointsStatements(ImmutableArray jumpsOutOfRegion) - => jumpsOutOfRegion.Any(n => n is not ReturnStatementSyntax); + public override bool ContainsNonReturnExitPointsStatements(ImmutableArray exitPoints) + => exitPoints.Any(n => n is not ReturnStatementSyntax); public override ImmutableArray GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray jumpsOutOfRegion) { diff --git a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs index d47649dc60e7d..ee256160c1538 100644 --- a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs +++ b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs @@ -60,7 +60,7 @@ internal abstract class SelectionResult( public abstract ImmutableArray GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray jumpsOutOfRegion); public abstract bool IsFinalSpanSemanticallyValidSpan(ImmutableArray returnStatements, CancellationToken cancellationToken); - public abstract bool ContainsNonReturnExitPointsStatements(ImmutableArray jumpsOutOfRegion); + public abstract bool ContainsNonReturnExitPointsStatements(ImmutableArray exitPoints); protected abstract OperationStatus ValidateLanguageSpecificRules(CancellationToken cancellationToken); diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb index b5fae769b4f13..5e89f83576233 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb @@ -291,11 +291,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod Throw ExceptionUtilities.Unreachable End Function - Public Overrides Function ContainsNonReturnExitPointsStatements(jumpsOutOfRegion As ImmutableArray(Of SyntaxNode)) As Boolean + Public Overrides Function ContainsNonReturnExitPointsStatements(exitPoints As ImmutableArray(Of SyntaxNode)) As Boolean Dim returnStatement = False Dim exitStatement = False - For Each statement In jumpsOutOfRegion + For Each statement In exitPoints If TypeOf statement Is ReturnStatementSyntax Then returnStatement = True ElseIf TypeOf statement Is ExitStatementSyntax Then From 969f3c59d35475ae2263028737333b4af57ab6d0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 09:08:20 -0800 Subject: [PATCH 02/32] Simplify --- .../ExtractMethod/CSharpSelectionResult.cs | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs index 71952c45448d2..496064aadb9f2 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs @@ -226,22 +226,7 @@ public override bool IsFinalSpanSemanticallyValidSpan( if (body.CloseBraceToken != GetLastTokenInSelection().GetNextToken(includeZeroWidth: true)) return false; - // alright, for these constructs, it must be okay to be extracted - switch (container.Kind()) - { - case SyntaxKind.AnonymousMethodExpression: - case SyntaxKind.SimpleLambdaExpression: - case SyntaxKind.ParenthesizedLambdaExpression: - return true; - } - - // now, only method is okay to be extracted out - if (body.Parent is not MethodDeclarationSyntax method) - return false; - - // make sure this method doesn't have return type. - return method.ReturnType is PredefinedTypeSyntax p && - p.Keyword.Kind() == SyntaxKind.VoidKeyword; + return true; } } } From 5802cbdfb5ee8d9dc83d2d3227ef452f704799e7 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 09:10:40 -0800 Subject: [PATCH 03/32] Simplify --- .../CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs index 496064aadb9f2..9cb7d51f00a40 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs @@ -192,14 +192,14 @@ public SyntaxNode GetInnermostStatementContainer() public override bool ContainsNonReturnExitPointsStatements(ImmutableArray exitPoints) => exitPoints.Any(n => n is not ReturnStatementSyntax); - public override ImmutableArray GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray jumpsOutOfRegion) + public override ImmutableArray GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray exitPoints) { - var container = commonRoot.GetAncestorsOrThis().Where(a => a.IsReturnableConstruct()).FirstOrDefault(); + var container = commonRoot.AncestorsAndSelf().FirstOrDefault(a => a.IsReturnableConstruct()); if (container == null) return []; // now filter return statements to only include the one under outmost container - return jumpsOutOfRegion + return exitPoints .OfType() .Select(returnStatement => (returnStatement, container: returnStatement.GetAncestors().Where(a => a.IsReturnableConstruct()).FirstOrDefault())) .Where(p => p.container == container) From 40eb46d6e2110171245ed98b3fc43045883069c8 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 09:17:38 -0800 Subject: [PATCH 04/32] Simplify --- .../ExtractMethod/CSharpSelectionResult.cs | 14 +------------- .../ExtractMethod/VisualBasicSelectionResult.vb | 14 +++----------- 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs index 9cb7d51f00a40..09a4681639865 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs @@ -193,19 +193,7 @@ public override bool ContainsNonReturnExitPointsStatements(ImmutableArray exitPoints.Any(n => n is not ReturnStatementSyntax); public override ImmutableArray GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray exitPoints) - { - var container = commonRoot.AncestorsAndSelf().FirstOrDefault(a => a.IsReturnableConstruct()); - if (container == null) - return []; - - // now filter return statements to only include the one under outmost container - return exitPoints - .OfType() - .Select(returnStatement => (returnStatement, container: returnStatement.GetAncestors().Where(a => a.IsReturnableConstruct()).FirstOrDefault())) - .Where(p => p.container == container) - .SelectAsArray(p => p.returnStatement) - .CastArray(); - } + => exitPoints.OfType().ToImmutableArray().CastArray(); public override bool IsFinalSpanSemanticallyValidSpan( ImmutableArray returnStatements, CancellationToken cancellationToken) diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb index 5e89f83576233..6c03477f2499e 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb @@ -312,19 +312,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod Return False End Function - Public Overrides Function GetOuterReturnStatements(commonRoot As SyntaxNode, jumpsOutOfRegionStatements As ImmutableArray(Of SyntaxNode)) As ImmutableArray(Of ExecutableStatementSyntax) - Dim container = commonRoot.GetAncestorsOrThis(Of SyntaxNode)().Where(Function(a) a.IsReturnableConstruct()).FirstOrDefault() - If container Is Nothing Then - Return ImmutableArray(Of ExecutableStatementSyntax).Empty - End If - - ' now filter return statements to only include the one under outmost container - Return jumpsOutOfRegionStatements. + Public Overrides Function GetOuterReturnStatements(commonRoot As SyntaxNode, exitPoints As ImmutableArray(Of SyntaxNode)) As ImmutableArray(Of ExecutableStatementSyntax) + Return exitPoints. OfType(Of ExecutableStatementSyntax). Where(Function(n) TypeOf n Is ReturnStatementSyntax OrElse TypeOf n Is ExitStatementSyntax). - Select(Function(returnStatement) (returnStatement, container:=returnStatement.GetAncestors(Of SyntaxNode)().Where(Function(a) a.IsReturnableConstruct()).FirstOrDefault())). - Where(Function(p) p.container Is container). - SelectAsArray(Function(p) p.returnStatement) + ToImmutableArray() End Function Public Overrides Function IsFinalSpanSemanticallyValidSpan( From db220c85a20652e39b4c6f9217f2f57814274569 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 09:48:35 -0800 Subject: [PATCH 05/32] Share code --- .../CSharpMethodExtractor.Analyzer.cs | 6 --- .../ExtractMethod/MethodExtractor.Analyzer.cs | 51 ++++++++++--------- .../VisualBasicMethodExtractor.Analyzer.vb | 14 ----- 3 files changed, 28 insertions(+), 43 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs index 73b6d6f8285ec..d9ffbbe6ddd4b 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs @@ -24,12 +24,6 @@ protected override bool TreatOutAsRef protected override bool IsInPrimaryConstructorBaseType() => this.SelectionResult.GetContainingScopeOf() != null; - protected override VariableInfo CreateFromSymbol( - ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared) - { - return CreateFromSymbolCommon(symbol, type, style); - } - protected override ITypeSymbol? GetRangeVariableType(IRangeVariableSymbol symbol) { var info = this.SemanticModel.GetSpeculativeTypeInfo(SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 06c474bb679e9..be596bad86499 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -52,11 +52,6 @@ protected Analyzer(SelectionResult selectionResult, bool localFunction, Cancella /// protected abstract bool ContainsReturnStatementInSelectedCode(ImmutableArray exitPoints); - /// - /// create VariableInfo type - /// - protected abstract VariableInfo CreateFromSymbol(ISymbol symbol, ITypeSymbol type, VariableStyle variableStyle, bool variableDeclared); - protected virtual bool IsReadOutside(ISymbol symbol, HashSet readOutsideMap) => readOutsideMap.Contains(symbol); @@ -525,10 +520,20 @@ private void GenerateVariableInfoMap( continue; } + // An assignment to a VB 'function value'. (e.g. `MethodName = value`) needs to be treated as a + // return value from the inner function which the outer function then still assigns to its function + // value. + if (symbol is ILocalSymbol { IsFunctionValue: true } && + variableStyle.ParameterStyle.DeclarationBehavior != DeclarationBehavior.None) + { + Contract.ThrowIfFalse(variableStyle.ParameterStyle.DeclarationBehavior == DeclarationBehavior.MoveIn || variableStyle.ParameterStyle.DeclarationBehavior == DeclarationBehavior.SplitIn); + variableStyle = AlwaysReturn(variableStyle); + } + AddVariableToMap( variableInfoMap, symbol, - CreateFromSymbol(symbol, type, variableStyle, variableDeclared)); + CreateFromSymbol(symbol, type, variableStyle)); } return; @@ -572,6 +577,23 @@ PooledDisposer> GetPooledSymbolSet(ImmutableArray new VariableInfo( + new LocalVariableSymbol(local, type), + style, + useAsReturnValue: false), + IParameterSymbol parameter => new VariableInfo(new ParameterVariableSymbol(parameter, type), style, useAsReturnValue: false), + IRangeVariableSymbol rangeVariable => new VariableInfo(new QueryVariableSymbol(rangeVariable, type), style, useAsReturnValue: false), + _ => throw ExceptionUtilities.UnexpectedValue(symbol) + }; + } } private static void AddVariableToMap(IDictionary variableInfoMap, ISymbol localOrParameter, VariableInfo variableInfo) @@ -933,23 +955,6 @@ private OperationStatus CheckReadOnlyFields(Dictionary new VariableInfo( - new LocalVariableSymbol(local, type), - style, - useAsReturnValue: false), - IParameterSymbol parameter => new VariableInfo(new ParameterVariableSymbol(parameter, type), style, useAsReturnValue: false), - IRangeVariableSymbol rangeVariable => new VariableInfo(new QueryVariableSymbol(rangeVariable, type), style, useAsReturnValue: false), - _ => throw ExceptionUtilities.UnexpectedValue(symbol) - }; - } } } } diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.Analyzer.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.Analyzer.vb index 40c6bff0bd00a..11a5153167538 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.Analyzer.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.Analyzer.vb @@ -5,7 +5,6 @@ Imports System.Collections.Immutable Imports System.Threading Imports Microsoft.CodeAnalysis -Imports Microsoft.CodeAnalysis.ExtractMethod Imports Microsoft.CodeAnalysis.VisualBasic.Symbols Imports Microsoft.CodeAnalysis.VisualBasic.Syntax @@ -25,19 +24,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod Return False End Function - Protected Overrides Function CreateFromSymbol( - symbol As ISymbol, - type As ITypeSymbol, - style As VariableStyle, - requiresDeclarationExpressionRewrite As Boolean) As VariableInfo - If symbol.IsFunctionValue() AndAlso style.ParameterStyle.DeclarationBehavior <> DeclarationBehavior.None Then - Contract.ThrowIfFalse(style.ParameterStyle.DeclarationBehavior = DeclarationBehavior.MoveIn OrElse style.ParameterStyle.DeclarationBehavior = DeclarationBehavior.SplitIn) - style = AlwaysReturn(style) - End If - - Return CreateFromSymbolCommon(symbol, type, style) - End Function - Protected Overrides Function GetRangeVariableType(symbol As IRangeVariableSymbol) As ITypeSymbol Dim info = Me.SemanticModel.GetSpeculativeTypeInfo(Me.SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression) If info.Type.IsErrorType() Then From be145e5e3a739ec440b66fd0af70f8b29f4b6da6 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 09:57:42 -0800 Subject: [PATCH 06/32] Simplify --- .../ExtractMethod/MethodExtractor.Analyzer.cs | 65 +++++++------------ .../Extensions/INamedTypeSymbolExtensions.cs | 4 +- 2 files changed, 26 insertions(+), 43 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index be596bad86499..2686585e6feb7 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -844,80 +844,63 @@ private ImmutableArray GetMethodTypeParametersInConstraint private static void AppendTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints(SortedDictionary sortedMap) { - var visited = new HashSet(); - var candidates = SpecializedCollections.EmptyEnumerable(); + using var _1 = PooledHashSet.GetInstance(out var visited); + using var _2 = PooledHashSet.GetInstance(out var candidates); // collect all type parameter appears in constraint foreach (var typeParameter in sortedMap.Values) { var constraintTypes = typeParameter.ConstraintTypes; if (constraintTypes.IsDefaultOrEmpty) - { continue; - } foreach (var type in constraintTypes) - { - candidates = candidates.Concat(AppendTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints(type, visited)); - } + AddTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints(type, visited, candidates); } // pick up only valid type parameter and add them to the map foreach (var typeParameter in candidates) - { AddTypeParameterToMap(typeParameter, sortedMap); - } } - private static IEnumerable AppendTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints( - ITypeSymbol type, HashSet visited) + private static void AddTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints( + ITypeSymbol type, HashSet visited, HashSet typeParameters) { - if (visited.Contains(type)) - return []; - - visited.Add(type); + if (!visited.Add(type)) + return; if (type.OriginalDefinition.Equals(type)) - return []; + return; if (type is not INamedTypeSymbol constructedType) - return []; + return; - var parameters = constructedType.GetAllTypeParameters().ToList(); - var arguments = constructedType.GetAllTypeArguments().ToList(); + var parameters = constructedType.GetAllTypeParameters(); + var arguments = constructedType.GetAllTypeArguments(); - Contract.ThrowIfFalse(parameters.Count == arguments.Count); + Contract.ThrowIfFalse(parameters.Length == arguments.Length); - var typeParameters = new List(); - for (var i = 0; i < parameters.Count; i++) + for (var i = 0; i < parameters.Length; i++) { - var parameter = parameters[i]; - if (arguments[i] is ITypeParameterSymbol argument) { - // no constraint, nothing to do - if (!parameter.HasConstructorConstraint && - !parameter.HasReferenceTypeConstraint && - !parameter.HasValueTypeConstraint && - !parameter.AllowsRefLikeType && - parameter.ConstraintTypes.IsDefaultOrEmpty) + if (parameters[i] is not + { + HasConstructorConstraint: false, + HasReferenceTypeConstraint: false, + HasValueTypeConstraint: false, + AllowsRefLikeType: false, + ConstraintTypes.IsDefaultOrEmpty: true + }) { - continue; + typeParameters.Add(argument); } - - typeParameters.Add(argument); - continue; } - - if (arguments[i] is not INamedTypeSymbol candidate) + else if (arguments[i] is INamedTypeSymbol candidate) { - continue; + AddTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints(candidate, visited, typeParameters); } - - typeParameters.AddRange(AppendTypeParametersInConstraintsUsedByConstructedTypeWithItsOwnConstraints(candidate, visited)); } - - return typeParameters; } private static ImmutableArray GetMethodTypeParametersInDeclaration(ITypeSymbol returnType, SortedDictionary sortedMap) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs index 5b38e22d06a58..28a8cdf916c9e 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/INamedTypeSymbolExtensions.cs @@ -33,10 +33,10 @@ public static ImmutableArray GetAllTypeParameters(this INa return stack.SelectManyAsArray(n => n.TypeParameters); } - public static IEnumerable GetAllTypeArguments(this INamedTypeSymbol? symbol) + public static ImmutableArray GetAllTypeArguments(this INamedTypeSymbol? symbol) { var stack = GetContainmentStack(symbol); - return stack.SelectMany(n => n.TypeArguments); + return stack.SelectManyAsArray(n => n.TypeArguments); } private static Stack GetContainmentStack(INamedTypeSymbol? symbol) From f1a16f3618a1fc70e59d108e1bf6b6ce16c8b507 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:22:02 -0800 Subject: [PATCH 07/32] Simplify --- .../CSharpSelectionResult.ExpressionResult.cs | 2 +- .../CSharpSelectionResult.StatementResult.cs | 2 +- .../Portable/ExtractMethod/MethodExtractor.Analyzer.cs | 5 ++--- .../Core/Portable/ExtractMethod/SelectionResult.cs | 10 ++++++++-- .../ExtractMethod/VisualBasicSelectionResult.vb | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs index 55478f127b12b..ee5ede34f3666 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs @@ -45,7 +45,7 @@ public override SyntaxNode GetContainingScope() return CSharpSyntaxFacts.Instance.GetRootStandaloneExpression(scope); } - public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken) + protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfoWorker(CancellationToken cancellationToken) { if (GetContainingScope() is not ExpressionSyntax node) { diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs index ade8613022445..cd1baf6221042 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs @@ -55,7 +55,7 @@ AnonymousFunctionExpressionSyntax or CompilationUnitSyntax); } - public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken) + protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfoWorker(CancellationToken cancellationToken) { Contract.ThrowIfTrue(IsExtractMethodOnExpression); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 2686585e6feb7..b98df499d01f1 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -215,14 +215,11 @@ private ITypeSymbol UnwrapTaskIfNeeded(ITypeSymbol returnType) Dictionary variableInfoMap, bool isInExpressionOrHasReturnStatement) { - var model = this.SemanticDocument.SemanticModel; - var compilation = model.Compilation; if (isInExpressionOrHasReturnStatement) { // check whether current selection contains return statement var parameters = GetMethodParameters(variableInfoMap); var (returnType, returnsByRef) = SelectionResult.GetReturnTypeInfo(this.CancellationToken); - returnType ??= compilation.GetSpecialType(SpecialType.System_Object); return (parameters, returnType, returnsByRef, []); } @@ -239,6 +236,8 @@ private ITypeSymbol UnwrapTaskIfNeeded(ITypeSymbol returnType) ITypeSymbol GetReturnType(ImmutableArray variablesToUseAsReturnValue) { + var compilation = this.SemanticDocument.SemanticModel.Compilation; + if (variablesToUseAsReturnValue.IsEmpty) return compilation.GetSpecialType(SpecialType.System_Void); diff --git a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs index ee256160c1538..510fec18939f4 100644 --- a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs +++ b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs @@ -56,7 +56,7 @@ internal abstract class SelectionResult( public abstract SyntaxNode GetContainingScope(); public abstract SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken); - public abstract (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken); + protected abstract (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfoWorker(CancellationToken cancellationToken); public abstract ImmutableArray GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray jumpsOutOfRegion); public abstract bool IsFinalSpanSemanticallyValidSpan(ImmutableArray returnStatements, CancellationToken cancellationToken); @@ -64,7 +64,13 @@ internal abstract class SelectionResult( protected abstract OperationStatus ValidateLanguageSpecificRules(CancellationToken cancellationToken); - public ITypeSymbol? GetReturnType(CancellationToken cancellationToken) + public (ITypeSymbol returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken) + { + var (returnType, returnsByRef) = GetReturnTypeInfoWorker(cancellationToken); + return (returnType ?? this.SemanticDocument.SemanticModel.Compilation.GetSpecialType(SpecialType.System_Object), returnsByRef); + } + + public ITypeSymbol GetReturnType(CancellationToken cancellationToken) => GetReturnTypeInfo(cancellationToken).returnType; public bool IsExtractMethodOnExpression => this.SelectionType == SelectionType.Expression; diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb index 6c03477f2499e..74d3b2ea2fc64 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb @@ -130,7 +130,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod End If End Function - Public Overrides Function GetReturnTypeInfo(cancellationToken As CancellationToken) As (returnType As ITypeSymbol, returnsByRef As Boolean) + Protected Overrides Function GetReturnTypeInfoWorker(cancellationToken As CancellationToken) As (returnType As ITypeSymbol, returnsByRef As Boolean) ' Todo: consider supporting byref return types in VB Dim returnType = GetReturnTypeWorker() Return (returnType, returnsByRef:=False) From e3c94b78e2db6aedd9280ad92e48f358e2295eab Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:31:49 -0800 Subject: [PATCH 08/32] Move specific code to subclass --- .../CSharpSelectionResult.ExpressionResult.cs | 30 ++++++++++++----- .../CSharpSelectionResult.StatementResult.cs | 24 +++++++++++--- .../ExtractMethod/CSharpSelectionResult.cs | 33 ------------------- 3 files changed, 41 insertions(+), 46 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs index ee5ede34f3666..f7f13fd408ec5 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs @@ -130,19 +130,31 @@ private static (ITypeSymbol? typeSymbol, bool returnsByRef) GetRegularExpression return !info.Type.IsObjectType() ? info.GetTypeWithAnnotatedNullability() : info.GetConvertedTypeWithAnnotatedNullability(); } } - } - private static bool IsCoClassImplicitConversion(TypeInfo info, Conversion conversion, INamedTypeSymbol? coclassSymbol) - { - if (!conversion.IsImplicit || - info.ConvertedType == null || - info.ConvertedType.TypeKind != TypeKind.Interface) + private static bool IsCoClassImplicitConversion(TypeInfo info, Conversion conversion, INamedTypeSymbol? coclassSymbol) { - return false; + if (!conversion.IsImplicit || + info.ConvertedType == null || + info.ConvertedType.TypeKind != TypeKind.Interface) + { + return false; + } + + // let's see whether this interface has coclass attribute + return info.ConvertedType.HasAttribute(coclassSymbol); } - // let's see whether this interface has coclass attribute - return info.ConvertedType.HasAttribute(coclassSymbol); + public override SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken) + { + var container = this.GetInnermostStatementContainer(); + + Contract.ThrowIfNull(container); + Contract.ThrowIfFalse( + container.IsStatementContainerNode() || + container is BaseListSyntax or TypeDeclarationSyntax or ConstructorDeclarationSyntax or CompilationUnitSyntax); + + return container; + } } } } diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs index cd1baf6221042..ab373de64c22a 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs @@ -32,11 +32,9 @@ public override bool ContainingScopeHasAsyncKeyword() return node switch { - AccessorDeclarationSyntax _ => false, MethodDeclarationSyntax method => method.Modifiers.Any(SyntaxKind.AsyncKeyword), - ParenthesizedLambdaExpressionSyntax lambda => lambda.AsyncKeyword.Kind() == SyntaxKind.AsyncKeyword, - SimpleLambdaExpressionSyntax lambda => lambda.AsyncKeyword.Kind() == SyntaxKind.AsyncKeyword, - AnonymousMethodExpressionSyntax anonymous => anonymous.AsyncKeyword.Kind() == SyntaxKind.AsyncKeyword, + LocalFunctionStatementSyntax localFunction => localFunction.Modifiers.Any(SyntaxKind.AsyncKeyword), + AnonymousFunctionExpressionSyntax anonymousFunction => anonymousFunction.AsyncKeyword != default, _ => false, }; } @@ -89,6 +87,24 @@ protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInf return default; } } + + public override SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken) + { + if (this.IsExtractMethodOnSingleStatement) + { + var firstStatement = this.GetFirstStatement(); + return firstStatement.GetRequiredParent(); + } + + if (this.IsExtractMethodOnMultipleStatements) + { + var firstStatement = this.GetFirstStatementUnderContainer(); + var container = firstStatement.GetRequiredParent(); + return container is GlobalStatementSyntax ? container.GetRequiredParent() : container; + } + + throw ExceptionUtilities.Unreachable(); + } } } } diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs index 09a4681639865..a53b8c2e73221 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs @@ -92,39 +92,6 @@ public static bool IsUnderAnonymousOrLocalMethod(SyntaxToken token, SyntaxToken return false; } - public override SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken) - { - if (this.IsExtractMethodOnExpression) - { - var container = this.GetInnermostStatementContainer(); - - Contract.ThrowIfNull(container); - Contract.ThrowIfFalse( - container.IsStatementContainerNode() || - container is BaseListSyntax or TypeDeclarationSyntax or ConstructorDeclarationSyntax or CompilationUnitSyntax); - - return container; - } - - if (this.IsExtractMethodOnSingleStatement) - { - var firstStatement = this.GetFirstStatement(); - return firstStatement.Parent; - } - - if (this.IsExtractMethodOnMultipleStatements) - { - var firstStatement = this.GetFirstStatementUnderContainer(); - var container = firstStatement.Parent; - if (container is GlobalStatementSyntax) - return container.Parent; - - return container; - } - - throw ExceptionUtilities.Unreachable(); - } - public override StatementSyntax GetFirstStatementUnderContainer() { Contract.ThrowIfTrue(IsExtractMethodOnExpression); From 281c3eb55f14bd1b9e71fde7ec543cdad2681ef5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:33:36 -0800 Subject: [PATCH 09/32] Remove unused prop --- .../CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs | 3 --- src/Features/Core/Portable/ExtractMethod/SelectionResult.cs | 1 - .../Portable/ExtractMethod/VisualBasicSelectionResult.vb | 2 -- 3 files changed, 6 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs index a53b8c2e73221..e53cb050c304a 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs @@ -52,9 +52,6 @@ public static async Task CreateAsync( : new StatementResult(newDocument, selectionType, finalSpan); } - protected override ISyntaxFacts SyntaxFacts - => CSharpSyntaxFacts.Instance; - protected override OperationStatus ValidateLanguageSpecificRules(CancellationToken cancellationToken) { // Nothing language specific for C#. diff --git a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs index 510fec18939f4..04cf65466d1e4 100644 --- a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs +++ b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs @@ -45,7 +45,6 @@ internal abstract class SelectionResult( /// private ControlFlowAnalysis? _statementControlFlowAnalysis; - protected abstract ISyntaxFacts SyntaxFacts { get; } protected abstract bool UnderAnonymousOrLocalMethod(SyntaxToken token, SyntaxToken firstToken, SyntaxToken lastToken); public abstract TExecutableStatementSyntax GetFirstStatementUnderContainer(); diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb index 74d3b2ea2fc64..519669e55f160 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb @@ -43,8 +43,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod MyBase.New(document, selectionType, finalSpan) End Sub - Protected Overrides ReadOnly Property SyntaxFacts As ISyntaxFacts = VisualBasicSyntaxFacts.Instance - Protected Overrides Function UnderAnonymousOrLocalMethod(token As SyntaxToken, firstToken As SyntaxToken, lastToken As SyntaxToken) As Boolean Dim current = token.Parent From 04c78ec2df2cf6ea57b075aaebc33229a8b0b93f Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:37:00 -0800 Subject: [PATCH 10/32] Simplify generics --- .../CSharpMethodExtractor.CSharpCodeGenerator.cs | 2 +- .../Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs | 5 +---- ...actor.VisualBasicCodeGenerator.ExpressionCodeGenerator.vb | 1 - .../VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs index 3b9797df2ef25..419e2902ecd0f 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs @@ -37,7 +37,7 @@ internal sealed partial class CSharpExtractMethodService { internal sealed partial class CSharpMethodExtractor { - private abstract partial class CSharpCodeGenerator : CodeGenerator + private abstract partial class CSharpCodeGenerator : CodeGenerator { private readonly SyntaxToken _methodName; diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs index e0fdac40eaf26..7f5f5a2589e34 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs @@ -35,10 +35,7 @@ public abstract OperationStatus> GetNewMethodStatemen SyntaxNode insertionPointNode, CancellationToken cancellationToken); } -#pragma warning disable CS0693 // Intentionally hiding the outer TStatementSyntax - protected abstract partial class CodeGenerator : CodeGenerator -#pragma warning restore CS0693 - where TStatementSyntax : SyntaxNode + protected abstract partial class CodeGenerator : CodeGenerator where TNodeUnderContainer : SyntaxNode where TCodeGenerationOptions : CodeGenerationOptions { diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.ExpressionCodeGenerator.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.ExpressionCodeGenerator.vb index d81f883d3ca66..1d70eda65011d 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.ExpressionCodeGenerator.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.ExpressionCodeGenerator.vb @@ -6,7 +6,6 @@ Imports System.Collections.Immutable Imports System.Threading Imports Microsoft.CodeAnalysis Imports Microsoft.CodeAnalysis.ExtractMethod -Imports Microsoft.CodeAnalysis.VisualBasic.CodeGeneration Imports Microsoft.CodeAnalysis.VisualBasic.Syntax Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb index 9b96344da5032..a875da7a553dc 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb @@ -19,7 +19,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod Partial Friend NotInheritable Class VisualBasicExtractMethodService Partial Friend Class VisualBasicMethodExtractor Partial Private MustInherit Class VisualBasicCodeGenerator - Inherits CodeGenerator(Of StatementSyntax, StatementSyntax, VisualBasicCodeGenerationOptions) + Inherits CodeGenerator(Of StatementSyntax, VisualBasicCodeGenerationOptions) Private ReadOnly _methodName As SyntaxToken From f5cc86a08b1028b99258a903bbf09e94c8508655 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:45:09 -0800 Subject: [PATCH 11/32] Simplify annnotations --- ...harpMethodExtractor.CSharpCodeGenerator.cs | 4 +- .../ExtractMethod/CSharpMethodExtractor.cs | 2 +- .../MethodExtractor.CodeGenerator.cs | 27 +++---- .../MethodExtractor.GeneratedCode.cs | 72 +++++++++---------- .../MethodExtractor.TriviaResult.cs | 7 +- .../Portable/ExtractMethod/MethodExtractor.cs | 6 +- ...ethodExtractor.VisualBasicCodeGenerator.vb | 8 +-- .../VisualBasicMethodExtractor.vb | 2 +- 8 files changed, 58 insertions(+), 70 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs index 419e2902ecd0f..a341e48101ebb 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs @@ -712,7 +712,7 @@ [.. variableInfos.Select(v => Argument(v.ReturnBehavior == ReturnBehavior.Initia } } - protected override async Task CreateGeneratedCodeAsync( + protected override async Task PerformFinalTriviaFixupAsync( SemanticDocument newDocument, CancellationToken cancellationToken) { // in hybrid code cases such as extract method, formatter will have some difficulties on where it breaks lines in two. @@ -731,7 +731,7 @@ protected override async Task CreateGeneratedCodeAsync( newDocument = await newDocument.WithSyntaxRootAsync( root.ReplaceNode(methodDefinition, newMethodDefinition), cancellationToken).ConfigureAwait(false); - return await base.CreateGeneratedCodeAsync(newDocument, cancellationToken).ConfigureAwait(false); + return newDocument; } private static MethodDeclarationSyntax TweakNewLinesInMethod(MethodDeclarationSyntax method) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs index d72fb316d2e7a..2a847849479d5 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs @@ -170,7 +170,7 @@ await semanticDocument.WithSyntaxRootAsync(result.Root, cancellationToken).Confi result); } - protected override Task GenerateCodeAsync(InsertionPoint insertionPoint, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken) + protected override Task GenerateCodeAsync(InsertionPoint insertionPoint, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken) { var codeGenerator = CSharpCodeGenerator.Create(selectionResult, analyzeResult, options, this.LocalFunction); return codeGenerator.GenerateAsync(insertionPoint, cancellationToken); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs index 7f5f5a2589e34..c6f2c10b363c1 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs @@ -26,6 +26,10 @@ internal abstract partial class AbstractExtractMethodService< { internal abstract partial class MethodExtractor { + public static readonly SyntaxAnnotation MethodNameAnnotation = new(); + public static readonly SyntaxAnnotation MethodDefinitionAnnotation = new(); + public static readonly SyntaxAnnotation CallSiteAnnotation = new(); + protected abstract class CodeGenerator { /// @@ -41,10 +45,6 @@ protected abstract partial class CodeGenerator SelectionResult.SemanticDocument; @@ -99,9 +95,11 @@ protected CodeGenerator( protected abstract Task UpdateMethodAfterGenerationAsync( SemanticDocument originalDocument, IMethodSymbol methodSymbolResult, CancellationToken cancellationToken); + protected abstract Task PerformFinalTriviaFixupAsync( + SemanticDocument newDocument, CancellationToken cancellationToken); #endregion - public async Task GenerateAsync(InsertionPoint insertionPoint, CancellationToken cancellationToken) + public async Task GenerateAsync(InsertionPoint insertionPoint, CancellationToken cancellationToken) { var newMethodDefinition = GenerateMethodDefinition(insertionPoint.GetContext(), cancellationToken); var callSiteDocument = await InsertMethodAndUpdateCallSiteAsync(insertionPoint, newMethodDefinition, cancellationToken).ConfigureAwait(false); @@ -117,7 +115,7 @@ public async Task GenerateAsync(InsertionPoint insertionPoint, Ca // happen in the generator. var finalDocument = await UpdateMethodAfterGenerationAsync(callSiteDocument, newMethodDefinition, cancellationToken).ConfigureAwait(false); - return await CreateGeneratedCodeAsync(finalDocument, cancellationToken).ConfigureAwait(false); + return await PerformFinalTriviaFixupAsync(finalDocument, cancellationToken).ConfigureAwait(false); } private async Task InsertMethodAndUpdateCallSiteAsync( @@ -197,15 +195,6 @@ private SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken canc return callSiteContainer ?? this.SelectionResult.GetOutermostCallSiteContainerToProcess(cancellationToken); } - protected virtual Task CreateGeneratedCodeAsync(SemanticDocument newDocument, CancellationToken cancellationToken) - { - return Task.FromResult(new GeneratedCode( - newDocument, - MethodNameAnnotation, - CallSiteAnnotation, - MethodDefinitionAnnotation)); - } - protected VariableInfo GetOutermostVariableToMoveIntoMethodDefinition() { return this.AnalyzerResult.GetOutermostVariableToMoveIntoMethodDefinition(); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs index 57690465d36a1..1a54228829842 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs @@ -1,42 +1,42 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. +//// Licensed to the .NET Foundation under one or more agreements. +//// The .NET Foundation licenses this file to you under the MIT license. +//// See the LICENSE file in the project root for more information. -using Roslyn.Utilities; +//using Roslyn.Utilities; -namespace Microsoft.CodeAnalysis.ExtractMethod; +//namespace Microsoft.CodeAnalysis.ExtractMethod; -internal abstract partial class AbstractExtractMethodService< - TStatementSyntax, - TExecutableStatementSyntax, - TExpressionSyntax> -{ - internal abstract partial class MethodExtractor - { - internal sealed class GeneratedCode - { - public GeneratedCode( - SemanticDocument document, - SyntaxAnnotation methodNameAnnotation, - SyntaxAnnotation callSiteAnnotation, - SyntaxAnnotation methodDefinitionAnnotation) - { - Contract.ThrowIfNull(document); - Contract.ThrowIfNull(methodNameAnnotation); - Contract.ThrowIfNull(callSiteAnnotation); - Contract.ThrowIfNull(methodDefinitionAnnotation); +//internal abstract partial class AbstractExtractMethodService< +// TStatementSyntax, +// TExecutableStatementSyntax, +// TExpressionSyntax> +//{ +// internal abstract partial class MethodExtractor +// { +// internal sealed class GeneratedCode +// { +// public GeneratedCode( +// SemanticDocument document, +// SyntaxAnnotation methodNameAnnotation, +// SyntaxAnnotation callSiteAnnotation, +// SyntaxAnnotation methodDefinitionAnnotation) +// { +// Contract.ThrowIfNull(document); +// Contract.ThrowIfNull(methodNameAnnotation); +// Contract.ThrowIfNull(callSiteAnnotation); +// Contract.ThrowIfNull(methodDefinitionAnnotation); - SemanticDocument = document; - MethodNameAnnotation = methodNameAnnotation; - CallSiteAnnotation = callSiteAnnotation; - MethodDefinitionAnnotation = methodDefinitionAnnotation; - } +// SemanticDocument = document; +// MethodNameAnnotation = methodNameAnnotation; +// CallSiteAnnotation = callSiteAnnotation; +// MethodDefinitionAnnotation = methodDefinitionAnnotation; +// } - public SemanticDocument SemanticDocument { get; } +// public SemanticDocument SemanticDocument { get; } - public SyntaxAnnotation MethodNameAnnotation { get; } - public SyntaxAnnotation CallSiteAnnotation { get; } - public SyntaxAnnotation MethodDefinitionAnnotation { get; } - } - } -} +// public SyntaxAnnotation MethodNameAnnotation { get; } +// public SyntaxAnnotation CallSiteAnnotation { get; } +// public SyntaxAnnotation MethodDefinitionAnnotation { get; } +// } +// } +//} diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.TriviaResult.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.TriviaResult.cs index ee224489416b3..ceb31b3f78372 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.TriviaResult.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.TriviaResult.cs @@ -32,13 +32,12 @@ protected abstract class TriviaResult(SemanticDocument document, ITriviaSavedRes public SemanticDocument SemanticDocument { get; } = document; - public async Task ApplyAsync(GeneratedCode generatedCode, CancellationToken cancellationToken) + public async Task ApplyAsync(SemanticDocument document, CancellationToken cancellationToken) { - var document = generatedCode.SemanticDocument; var root = document.Root; - var callsiteAnnotation = generatedCode.CallSiteAnnotation; - var methodDefinitionAnnotation = generatedCode.MethodDefinitionAnnotation; + var callsiteAnnotation = CallSiteAnnotation; + var methodDefinitionAnnotation = MethodDefinitionAnnotation; var callsite = root.GetAnnotatedNodesAndTokens(callsiteAnnotation).SingleOrDefault().AsNode(); var method = root.GetAnnotatedNodesAndTokens(methodDefinitionAnnotation).SingleOrDefault().AsNode(); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs index c5a16bdc7f752..fcc6fe9ac63e9 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs @@ -39,7 +39,7 @@ internal abstract partial class MethodExtractor( protected abstract Task PreserveTriviaAsync(SyntaxNode root, CancellationToken cancellationToken); protected abstract CodeGenerator CreateCodeGenerator(AnalyzerResult analyzerResult); - protected abstract Task GenerateCodeAsync( + protected abstract Task GenerateCodeAsync( InsertionPoint insertionPoint, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken); protected abstract SyntaxToken? GetInvocationNameToken(IEnumerable tokens); @@ -94,12 +94,12 @@ public ExtractMethodResult ExtractMethod(OperationStatus initialStatus, Cancella cancellationToken.ThrowIfCancellationRequested(); var newRoot = afterTriviaRestored.Root; - var invocationNameToken = GetInvocationNameToken(newRoot.GetAnnotatedTokens(generatedCode.MethodNameAnnotation)); + var invocationNameToken = GetInvocationNameToken(newRoot.GetAnnotatedTokens(MethodNameAnnotation)); // Do some final patchups of whitespace when inserting a local function. if (LocalFunction) { - var methodDefinition = newRoot.GetAnnotatedNodesAndTokens(generatedCode.MethodDefinitionAnnotation).FirstOrDefault().AsNode(); + var methodDefinition = newRoot.GetAnnotatedNodesAndTokens(MethodDefinitionAnnotation).FirstOrDefault().AsNode(); (documentWithoutFinalFormatting, invocationNameToken) = await InsertNewLineBeforeLocalFunctionIfNecessaryAsync( documentWithoutFinalFormatting, invocationNameToken, methodDefinition, cancellationToken).ConfigureAwait(false); } diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb index a875da7a553dc..47813725c4268 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb @@ -75,7 +75,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod parameters:=CreateMethodParameters(), statements:=statements.CastArray(Of SyntaxNode)) - Return Me.MethodDefinitionAnnotation.AddAnnotationToSymbol( + Return MethodDefinitionAnnotation.AddAnnotationToSymbol( Formatter.Annotation.AddAnnotationToSymbol(methodSymbol)) End Function @@ -442,12 +442,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod Return SyntaxFactory.LocalDeclarationStatement(modifiers, declarators) End Function - Protected Overrides Async Function CreateGeneratedCodeAsync(newDocument As SemanticDocument, cancellationToken As CancellationToken) As Task(Of GeneratedCode) + Protected Overrides Async Function PerformFinalTriviaFixupAsync(newDocument As SemanticDocument, cancellationToken As CancellationToken) As Task(Of SemanticDocument) ' in hybrid code cases such as extract method, formatter will have some difficulties on where it breaks lines in two. ' here, we explicitly insert newline at the end of auto generated method decl's begin statement so that anchor knows how to find out ' indentation of inserted statements (from users code) with user code style preserved Dim root = newDocument.Root - Dim methodDefinition = root.GetAnnotatedNodes(Of MethodBlockBaseSyntax)(Me.MethodDefinitionAnnotation).First() + Dim methodDefinition = root.GetAnnotatedNodes(Of MethodBlockBaseSyntax)(MethodDefinitionAnnotation).First() Dim lastTokenOfBeginStatement = methodDefinition.BlockStatement.GetLastToken(includeZeroWidth:=True) Dim newMethodDefinition = methodDefinition.ReplaceToken( @@ -457,7 +457,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod newDocument = Await newDocument.WithSyntaxRootAsync(root.ReplaceNode(methodDefinition, newMethodDefinition), cancellationToken).ConfigureAwait(False) - Return Await MyBase.CreateGeneratedCodeAsync(newDocument, cancellationToken).ConfigureAwait(False) + Return newDocument End Function End Class End Class diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb index e963b61c90648..937c5c9d61fbd 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb @@ -57,7 +57,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod result) End Function - Protected Overrides Async Function GenerateCodeAsync(insertionPoint As InsertionPoint, selectionResult As SelectionResult, analyzeResult As AnalyzerResult, options As ExtractMethodGenerationOptions, cancellationToken As CancellationToken) As Task(Of GeneratedCode) + Protected Overrides Async Function GenerateCodeAsync(insertionPoint As InsertionPoint, selectionResult As SelectionResult, analyzeResult As AnalyzerResult, options As ExtractMethodGenerationOptions, cancellationToken As CancellationToken) As Task(Of SemanticDocument) Dim generator = VisualBasicCodeGenerator.Create(selectionResult, analyzeResult, options) Return Await generator.GenerateAsync(insertionPoint, cancellationToken).ConfigureAwait(False) End Function From 0fabbe38135005aba69051112c86aa2c70f2eb6e Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:45:56 -0800 Subject: [PATCH 12/32] delete file --- .../MethodExtractor.GeneratedCode.cs | 42 ------------------- 1 file changed, 42 deletions(-) delete mode 100644 src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs deleted file mode 100644 index 1a54228829842..0000000000000 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.GeneratedCode.cs +++ /dev/null @@ -1,42 +0,0 @@ -//// Licensed to the .NET Foundation under one or more agreements. -//// The .NET Foundation licenses this file to you under the MIT license. -//// See the LICENSE file in the project root for more information. - -//using Roslyn.Utilities; - -//namespace Microsoft.CodeAnalysis.ExtractMethod; - -//internal abstract partial class AbstractExtractMethodService< -// TStatementSyntax, -// TExecutableStatementSyntax, -// TExpressionSyntax> -//{ -// internal abstract partial class MethodExtractor -// { -// internal sealed class GeneratedCode -// { -// public GeneratedCode( -// SemanticDocument document, -// SyntaxAnnotation methodNameAnnotation, -// SyntaxAnnotation callSiteAnnotation, -// SyntaxAnnotation methodDefinitionAnnotation) -// { -// Contract.ThrowIfNull(document); -// Contract.ThrowIfNull(methodNameAnnotation); -// Contract.ThrowIfNull(callSiteAnnotation); -// Contract.ThrowIfNull(methodDefinitionAnnotation); - -// SemanticDocument = document; -// MethodNameAnnotation = methodNameAnnotation; -// CallSiteAnnotation = callSiteAnnotation; -// MethodDefinitionAnnotation = methodDefinitionAnnotation; -// } - -// public SemanticDocument SemanticDocument { get; } - -// public SyntaxAnnotation MethodNameAnnotation { get; } -// public SyntaxAnnotation CallSiteAnnotation { get; } -// public SyntaxAnnotation MethodDefinitionAnnotation { get; } -// } -// } -//} From ea10e13bcf9d7414db77f8803db695f83b31e125 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 10:51:39 -0800 Subject: [PATCH 13/32] Avoid allocs --- ...lDisplayService.AbstractSymbolDescriptionBuilder.cs | 10 +++++----- .../ITypeSymbolExtensions.SubstituteTypesVisitor.cs | 4 ++-- .../Compiler/Core/Extensions/ITypeSymbolExtensions.cs | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Features/Core/Portable/LanguageServices/SymbolDisplayService/AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder.cs b/src/Features/Core/Portable/LanguageServices/SymbolDisplayService/AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder.cs index 0d409a29b664b..6e19bbea189a5 100644 --- a/src/Features/Core/Portable/LanguageServices/SymbolDisplayService/AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder.cs +++ b/src/Features/Core/Portable/LanguageServices/SymbolDisplayService/AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder.cs @@ -488,7 +488,7 @@ private void AddDescriptionForNamedType(INamedTypeSymbol symbol) !symbol.IsAnonymousDelegateType()) { var allTypeParameters = symbol.GetAllTypeParameters(); - var allTypeArguments = symbol.GetAllTypeArguments().ToList(); + var allTypeArguments = symbol.GetAllTypeArguments(); AddTypeParameterMapPart(allTypeParameters, allTypeArguments); } @@ -503,10 +503,10 @@ private void AddDescriptionForNamedType(INamedTypeSymbol symbol) private static bool TypeArgumentsAndParametersAreSame(INamedTypeSymbol symbol) { - var typeArguments = symbol.GetAllTypeArguments().ToList(); - var typeParameters = symbol.GetAllTypeParameters().ToList(); + var typeArguments = symbol.GetAllTypeArguments(); + var typeParameters = symbol.GetAllTypeParameters(); - for (var i = 0; i < typeArguments.Count; i++) + for (var i = 0; i < typeArguments.Length; i++) { var typeArgument = typeArguments[i]; var typeParameter = typeParameters[i]; @@ -727,7 +727,7 @@ private static int GetOverloadCount(ImmutableArray symbolGroup) protected void AddTypeParameterMapPart( ImmutableArray typeParameters, - List typeArguments) + ImmutableArray typeArguments) { using var _ = ArrayBuilder.GetInstance(out var parts); diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.SubstituteTypesVisitor.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.SubstituteTypesVisitor.cs index c7d3e9df85806..8d06aabb6cc66 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.SubstituteTypesVisitor.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.SubstituteTypesVisitor.cs @@ -68,8 +68,8 @@ public override ITypeSymbol VisitNamedType(INamedTypeSymbol symbol) } // If we don't even have any type arguments, then there's nothing to do. - var allTypeArguments = symbol.GetAllTypeArguments().ToList(); - if (allTypeArguments.Count == 0) + var allTypeArguments = symbol.GetAllTypeArguments(); + if (allTypeArguments.Length == 0) { return symbol; } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs index 508bf101037e5..dfa471e44ec1e 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs @@ -588,8 +588,8 @@ private static IEnumerable SelectAccessibleMembers(this IEnumerable Date: Fri, 3 Jan 2025 11:07:35 -0800 Subject: [PATCH 14/32] Inline method --- .../ExtractMethod/MethodExtractor.Analyzer.cs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index b98df499d01f1..8856520ab7029 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -122,12 +122,9 @@ public AnalyzerResult Analyze() // check whether end of selection is reachable var endOfSelectionReachable = this.SelectionResult.IsEndOfSelectionReachable(); - var isInExpressionOrHasReturnStatement = IsInExpressionOrHasReturnStatement(); - // check whether the selection contains "&" over a symbol exist var unsafeAddressTakenUsed = dataFlowAnalysisData.UnsafeAddressTaken.Intersect(variableInfoMap.Keys).Any(); - var (parameters, returnType, returnsByRef, variablesToUseAsReturnValue) = - GetSignatureInformation(variableInfoMap, isInExpressionOrHasReturnStatement); + var (parameters, returnType, returnsByRef, variablesToUseAsReturnValue) = GetSignatureInformation(variableInfoMap); (returnType, var awaitTaskReturn) = AdjustReturnType(returnType); @@ -211,11 +208,9 @@ private ITypeSymbol UnwrapTaskIfNeeded(ITypeSymbol returnType) } private (ImmutableArray parameters, ITypeSymbol returnType, bool returnsByRef, ImmutableArray variablesToUseAsReturnValue) - GetSignatureInformation( - Dictionary variableInfoMap, - bool isInExpressionOrHasReturnStatement) + GetSignatureInformation(Dictionary variableInfoMap) { - if (isInExpressionOrHasReturnStatement) + if (this.IsInExpressionOrHasReturnStatement()) { // check whether current selection contains return statement var parameters = GetMethodParameters(variableInfoMap); From 4fc88429c1f6328d102147f1a6a4a8f231451171 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 11:29:29 -0800 Subject: [PATCH 15/32] Simplify code --- ...harpMethodExtractor.CSharpCodeGenerator.cs | 5 +- .../ExtractMethod/MethodExtractor.Analyzer.cs | 4 +- .../MethodExtractor.CodeGenerator.cs | 4 +- .../MethodExtractor.VariableInfo.cs | 9 +- .../MethodExtractor.VariableSymbol.cs | 140 ++++++------------ .../Portable/ExtractMethod/MethodExtractor.cs | 3 +- ...ethodExtractor.VisualBasicCodeGenerator.vb | 3 +- 7 files changed, 57 insertions(+), 111 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs index a341e48101ebb..3442fbe40a6da 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs @@ -662,8 +662,7 @@ protected override StatementSyntax CreateDeclarationStatement( if (variableInfos is [var singleVariable]) { - var type = singleVariable.GetVariableType(); - var typeNode = type.GenerateTypeSyntax(); + var typeNode = singleVariable.SymbolType.GenerateTypeSyntax(); var originalIdentifierToken = singleVariable.GetOriginalIdentifierToken(cancellationToken); @@ -704,7 +703,7 @@ protected override StatementSyntax CreateDeclarationStatement( { left = TupleExpression( [.. variableInfos.Select(v => Argument(v.ReturnBehavior == ReturnBehavior.Initialization - ? DeclarationExpression(v.GetVariableType().GenerateTypeSyntax(), SingleVariableDesignation(v.Name.ToIdentifierToken())) + ? DeclarationExpression(v.SymbolType.GenerateTypeSyntax(), SingleVariableDesignation(v.Name.ToIdentifierToken())) : v.Name.ToIdentifierName()))]); } diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 8856520ab7029..8337fd60eec1c 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -237,10 +237,10 @@ ITypeSymbol GetReturnType(ImmutableArray variablesToUseAsReturnVal return compilation.GetSpecialType(SpecialType.System_Void); if (variablesToUseAsReturnValue is [var info]) - return info.GetVariableType(); + return info.SymbolType; return compilation.CreateTupleTypeSymbol( - variablesToUseAsReturnValue.SelectAsArray(v => v.GetVariableType()), + variablesToUseAsReturnValue.SelectAsArray(v => v.SymbolType), variablesToUseAsReturnValue.SelectAsArray(v => v.Name)!); } } diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs index c6f2c10b363c1..f52a53b8e0611 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs @@ -333,13 +333,11 @@ protected ImmutableArray CreateMethodParameters() if (!isLocalFunction || !parameter.CanBeCapturedByLocalFunction) { var refKind = GetRefKind(parameter.ParameterModifier); - var type = parameter.GetVariableType(); - parameters.Add(CodeGenerationSymbolFactory.CreateParameterSymbol( attributes: [], refKind: refKind, isParams: false, - type: type, + type: parameter.SymbolType, name: parameter.Name)); } } diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs index 1cbc2b0cbdace..f5497e61ba208 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs @@ -9,6 +9,7 @@ using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.PooledObjects; +using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.ExtractMethod; @@ -105,12 +106,12 @@ public void AddIdentifierTokenAnnotationPair( public bool CanBeCapturedByLocalFunction => _variableSymbol.CanBeCapturedByLocalFunction; - public bool OriginalTypeHadAnonymousTypeOrDelegate => _variableSymbol.OriginalTypeHadAnonymousTypeOrDelegate; + public bool OriginalTypeHadAnonymousTypeOrDelegate => _variableSymbol.SymbolType.ContainsAnonymousType(); - public ITypeSymbol OriginalType => _variableSymbol.OriginalType; + public ITypeSymbol SymbolType => _variableSymbol.SymbolType; - public ITypeSymbol GetVariableType() - => _variableSymbol.OriginalType; + //public ITypeSymbol GetVariableType() + // => _variableSymbol.OriginalType; public SyntaxToken GetIdentifierTokenAtDeclaration(SemanticDocument document) => document.GetTokenWithAnnotation(_variableSymbol.IdentifierTokenAnnotation); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs index 59029e0743715..61d978b908ec6 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs @@ -27,43 +27,38 @@ internal abstract partial class MethodExtractor protected abstract class VariableSymbol { /// - /// return true if original type had anonymous type or delegate somewhere in the type + /// The underlying symbol this points at. /// - public bool OriginalTypeHadAnonymousTypeOrDelegate { get; } + public ISymbol Symbol { get; } /// - /// get the original type with anonymous type removed + /// Get the type of to use when generating code. May contain anonymous types in it. + /// Note: this is not necessarily the type symbol that can be directly accessed off of + /// itself. For example, it may have had nullability information changes applied to it. /// - public ITypeSymbol OriginalType { get; } + public ITypeSymbol SymbolType { get; } private readonly bool _isCancellationToken; - protected VariableSymbol(ITypeSymbol type) + protected VariableSymbol(ISymbol symbol, ITypeSymbol symbolType) { - OriginalTypeHadAnonymousTypeOrDelegate = type.ContainsAnonymousType(); - OriginalType = type; - _isCancellationToken = IsCancellationToken(OriginalType); + Symbol = symbol; + SymbolType = symbolType; + _isCancellationToken = IsCancellationToken(SymbolType); static bool IsCancellationToken(ITypeSymbol originalType) { return originalType is { Name: nameof(CancellationToken), - ContainingNamespace: - { - Name: nameof(System.Threading), - ContainingNamespace: - { - Name: nameof(System), - ContainingNamespace.IsGlobalNamespace: true, - } - } + ContainingNamespace.Name: nameof(System.Threading), + ContainingNamespace.ContainingNamespace.Name: nameof(System), + ContainingNamespace.ContainingNamespace.ContainingNamespace.IsGlobalNamespace: true, }; } } public abstract int DisplayOrder { get; } - public abstract string Name { get; } public abstract bool CanBeCapturedByLocalFunction { get; } public abstract SyntaxAnnotation IdentifierTokenAnnotation { get; } @@ -96,9 +91,15 @@ public static int Compare(VariableSymbol left, VariableSymbol right) return left.DisplayOrder - right.DisplayOrder; } + + public string Name => this.Symbol.ToDisplayString( + new SymbolDisplayFormat( + parameterOptions: SymbolDisplayParameterOptions.IncludeName, + miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); } - protected abstract class NotMovableVariableSymbol(ITypeSymbol type) : VariableSymbol(type) + protected abstract class NotMovableVariableSymbol( + ISymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbol, symbolType) { public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) => default; @@ -112,16 +113,10 @@ public override void AddIdentifierTokenAnnotationPair( } } - protected sealed class ParameterVariableSymbol : NotMovableVariableSymbol, IComparable + protected sealed class ParameterVariableSymbol(IParameterSymbol symbol, ITypeSymbol symbolType) + : NotMovableVariableSymbol(symbol, symbolType), IComparable { - private readonly IParameterSymbol _parameterSymbol; - - public ParameterVariableSymbol(IParameterSymbol parameterSymbol, ITypeSymbol type) - : base(type) - { - Contract.ThrowIfNull(parameterSymbol); - _parameterSymbol = parameterSymbol; - } + private IParameterSymbol ParameterSymbol => (IParameterSymbol)Symbol; public override int DisplayOrder => 0; @@ -137,14 +132,14 @@ public int CompareTo(ParameterVariableSymbol other) return 0; } - var compare = CompareMethodParameters((IMethodSymbol)_parameterSymbol.ContainingSymbol, (IMethodSymbol)other._parameterSymbol.ContainingSymbol); + var compare = CompareMethodParameters((IMethodSymbol)this.ParameterSymbol.ContainingSymbol, (IMethodSymbol)other.ParameterSymbol.ContainingSymbol); if (compare != 0) { return compare; } - Contract.ThrowIfFalse(_parameterSymbol.Ordinal != other._parameterSymbol.Ordinal); - return (_parameterSymbol.Ordinal > other._parameterSymbol.Ordinal) ? 1 : -1; + Contract.ThrowIfFalse(ParameterSymbol.Ordinal != other.ParameterSymbol.Ordinal); + return (ParameterSymbol.Ordinal > other.ParameterSymbol.Ordinal) ? 1 : -1; } private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol right) @@ -175,32 +170,15 @@ private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol rig return leftLocation.SourceSpan.Start - rightLocation.SourceSpan.Start; } - public override string Name - { - get - { - return _parameterSymbol.ToDisplayString( - new SymbolDisplayFormat( - parameterOptions: SymbolDisplayParameterOptions.IncludeName, - miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); - } - } - public override bool CanBeCapturedByLocalFunction => true; } - protected sealed class LocalVariableSymbol : VariableSymbol, IComparable + protected sealed class LocalVariableSymbol(ILocalSymbol localSymbol, ITypeSymbol symbolType) + : VariableSymbol(localSymbol, symbolType), IComparable { private readonly SyntaxAnnotation _annotation = new(); - private readonly ILocalSymbol _localSymbol; - - public LocalVariableSymbol(ILocalSymbol localSymbol, ITypeSymbol type) - : base(type) - { - Contract.ThrowIfNull(localSymbol); - _localSymbol = localSymbol; - } + private ILocalSymbol LocalSymbol => (ILocalSymbol)Symbol; public override int DisplayOrder => 1; @@ -216,34 +194,24 @@ public int CompareTo(LocalVariableSymbol other) return 0; } - Contract.ThrowIfFalse(_localSymbol.Locations.Length == 1); - Contract.ThrowIfFalse(other._localSymbol.Locations.Length == 1); - Contract.ThrowIfFalse(_localSymbol.Locations[0].IsInSource); - Contract.ThrowIfFalse(other._localSymbol.Locations[0].IsInSource); - Contract.ThrowIfFalse(_localSymbol.Locations[0].SourceTree == other._localSymbol.Locations[0].SourceTree); - Contract.ThrowIfFalse(_localSymbol.Locations[0].SourceSpan.Start != other._localSymbol.Locations[0].SourceSpan.Start); - - return _localSymbol.Locations[0].SourceSpan.Start - other._localSymbol.Locations[0].SourceSpan.Start; - } + Contract.ThrowIfFalse(LocalSymbol.Locations.Length == 1); + Contract.ThrowIfFalse(other.LocalSymbol.Locations.Length == 1); + Contract.ThrowIfFalse(LocalSymbol.Locations[0].IsInSource); + Contract.ThrowIfFalse(other.LocalSymbol.Locations[0].IsInSource); + Contract.ThrowIfFalse(LocalSymbol.Locations[0].SourceTree == other.LocalSymbol.Locations[0].SourceTree); + Contract.ThrowIfFalse(LocalSymbol.Locations[0].SourceSpan.Start != other.LocalSymbol.Locations[0].SourceSpan.Start); - public override string Name - { - get - { - return _localSymbol.ToDisplayString( - new SymbolDisplayFormat( - miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); - } + return LocalSymbol.Locations[0].SourceSpan.Start - other.LocalSymbol.Locations[0].SourceSpan.Start; } public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) { - Contract.ThrowIfFalse(_localSymbol.Locations.Length == 1); - Contract.ThrowIfFalse(_localSymbol.Locations[0].IsInSource); - Contract.ThrowIfNull(_localSymbol.Locations[0].SourceTree); + Contract.ThrowIfFalse(LocalSymbol.Locations.Length == 1); + Contract.ThrowIfFalse(LocalSymbol.Locations[0].IsInSource); + Contract.ThrowIfNull(LocalSymbol.Locations[0].SourceTree); - var tree = _localSymbol.Locations[0].SourceTree; - var span = _localSymbol.Locations[0].SourceSpan; + var tree = LocalSymbol.Locations[0].SourceTree; + var span = LocalSymbol.Locations[0].SourceSpan; var token = tree.GetRoot(cancellationToken).FindToken(span.Start); Contract.ThrowIfFalse(token.Span.Equals(span)); @@ -262,17 +230,9 @@ public override void AddIdentifierTokenAnnotationPair( } } - protected sealed class QueryVariableSymbol : NotMovableVariableSymbol, IComparable + protected sealed class QueryVariableSymbol(IRangeVariableSymbol symbol, ITypeSymbol symbolType) + : NotMovableVariableSymbol(symbol, symbolType), IComparable { - private readonly IRangeVariableSymbol _symbol; - - public QueryVariableSymbol(IRangeVariableSymbol symbol, ITypeSymbol type) - : base(type) - { - Contract.ThrowIfNull(symbol); - _symbol = symbol; - } - public override int DisplayOrder => 2; protected override int CompareTo(VariableSymbol right) @@ -287,8 +247,8 @@ public int CompareTo(QueryVariableSymbol other) return 0; } - var locationLeft = _symbol.Locations.First(); - var locationRight = other._symbol.Locations.First(); + var locationLeft = this.Symbol.Locations.First(); + var locationRight = other.Symbol.Locations.First(); Contract.ThrowIfFalse(locationLeft.IsInSource); Contract.ThrowIfFalse(locationRight.IsInSource); @@ -298,16 +258,6 @@ public int CompareTo(QueryVariableSymbol other) return locationLeft.SourceSpan.Start - locationRight.SourceSpan.Start; } - public override string Name - { - get - { - return _symbol.ToDisplayString( - new SymbolDisplayFormat( - miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); - } - } - public override bool CanBeCapturedByLocalFunction => false; } } diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs index fcc6fe9ac63e9..290fa6595e71b 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs @@ -208,8 +208,7 @@ private OperationStatus CheckVariableTypes( foreach (var variable in analyzeResult.Variables) { - var originalType = variable.GetVariableType(); - status = status.With(CheckType(semanticModel, originalType)); + status = status.With(CheckType(semanticModel, variable.SymbolType)); if (status.Failed) return status; } diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb index 47813725c4268..b5087f356061d 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb @@ -431,8 +431,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod Dim initializer = If(initialValue, If(shouldInitializeWithNothing, SyntaxFactory.NothingLiteralExpression(SyntaxFactory.Token(SyntaxKind.NothingKeyword)), Nothing)) - Dim variableType = variable.GetVariableType() - Dim typeNode = variableType.GenerateTypeSyntax() + Dim typeNode = variable.SymbolType.GenerateTypeSyntax() Dim names = SyntaxFactory.SingletonSeparatedList(SyntaxFactory.ModifiedIdentifier(SyntaxFactory.Identifier(variable.Name))) Dim modifiers = SyntaxFactory.TokenList(SyntaxFactory.Token(SyntaxKind.DimKeyword)) From 2ae47776508d3a0d13cf9ddf73eed8bd2f7c5a9f Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 11:36:53 -0800 Subject: [PATCH 16/32] Simplify code --- .../MethodExtractor.VariableInfo.cs | 3 - .../MethodExtractor.VariableSymbol.cs | 78 ++++++++++--------- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs index f5497e61ba208..5045dee544886 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs @@ -110,9 +110,6 @@ public bool CanBeCapturedByLocalFunction public ITypeSymbol SymbolType => _variableSymbol.SymbolType; - //public ITypeSymbol GetVariableType() - // => _variableSymbol.OriginalType; - public SyntaxToken GetIdentifierTokenAtDeclaration(SemanticDocument document) => document.GetTokenWithAnnotation(_variableSymbol.IdentifierTokenAnnotation); diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs index 61d978b908ec6..599bd5d43502d 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs @@ -21,28 +21,19 @@ internal abstract partial class AbstractExtractMethodService< { internal abstract partial class MethodExtractor { - /// - /// temporary symbol until we have a symbol that can hold onto both local and parameter symbol - /// protected abstract class VariableSymbol { /// - /// The underlying symbol this points at. - /// - public ISymbol Symbol { get; } - - /// - /// Get the type of to use when generating code. May contain anonymous types in it. - /// Note: this is not necessarily the type symbol that can be directly accessed off of - /// itself. For example, it may have had nullability information changes applied to it. + /// Get the type of to use when generating code. May contain anonymous types in it. + /// Note: this is not necessarily the type symbol that can be directly accessed off of itself. For example, it may have had nullability information changes applied to it. /// public ITypeSymbol SymbolType { get; } private readonly bool _isCancellationToken; - protected VariableSymbol(ISymbol symbol, ITypeSymbol symbolType) + protected VariableSymbol(ITypeSymbol symbolType) { - Symbol = symbol; SymbolType = symbolType; _isCancellationToken = IsCancellationToken(SymbolType); @@ -58,6 +49,11 @@ static bool IsCancellationToken(ITypeSymbol originalType) } } + /// + /// The underlying symbol this points at. + /// + protected abstract ISymbol GetSymbol(); + public abstract int DisplayOrder { get; } public abstract bool CanBeCapturedByLocalFunction { get; } @@ -92,14 +88,24 @@ public static int Compare(VariableSymbol left, VariableSymbol right) return left.DisplayOrder - right.DisplayOrder; } - public string Name => this.Symbol.ToDisplayString( + public string Name => this.GetSymbol().ToDisplayString( new SymbolDisplayFormat( parameterOptions: SymbolDisplayParameterOptions.IncludeName, miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); } - protected abstract class NotMovableVariableSymbol( - ISymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbol, symbolType) + protected abstract class VariableSymbol(TSymbol symbol, ITypeSymbol symbolType) + : VariableSymbol(symbolType) + where TSymbol : ISymbol + { + protected TSymbol Symbol { get; } = symbol; + + protected override ISymbol GetSymbol() => this.Symbol; + } + + protected abstract class NotMovableVariableSymbol( + TSymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbol, symbolType) + where TSymbol : ISymbol { public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) => default; @@ -114,10 +120,8 @@ public override void AddIdentifierTokenAnnotationPair( } protected sealed class ParameterVariableSymbol(IParameterSymbol symbol, ITypeSymbol symbolType) - : NotMovableVariableSymbol(symbol, symbolType), IComparable + : NotMovableVariableSymbol(symbol, symbolType), IComparable { - private IParameterSymbol ParameterSymbol => (IParameterSymbol)Symbol; - public override int DisplayOrder => 0; protected override int CompareTo(VariableSymbol right) @@ -132,14 +136,14 @@ public int CompareTo(ParameterVariableSymbol other) return 0; } - var compare = CompareMethodParameters((IMethodSymbol)this.ParameterSymbol.ContainingSymbol, (IMethodSymbol)other.ParameterSymbol.ContainingSymbol); + var compare = CompareMethodParameters((IMethodSymbol)this.Symbol.ContainingSymbol, (IMethodSymbol)other.Symbol.ContainingSymbol); if (compare != 0) { return compare; } - Contract.ThrowIfFalse(ParameterSymbol.Ordinal != other.ParameterSymbol.Ordinal); - return (ParameterSymbol.Ordinal > other.ParameterSymbol.Ordinal) ? 1 : -1; + Contract.ThrowIfFalse(Symbol.Ordinal != other.Symbol.Ordinal); + return (Symbol.Ordinal > other.Symbol.Ordinal) ? 1 : -1; } private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol right) @@ -174,12 +178,10 @@ private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol rig } protected sealed class LocalVariableSymbol(ILocalSymbol localSymbol, ITypeSymbol symbolType) - : VariableSymbol(localSymbol, symbolType), IComparable + : VariableSymbol(localSymbol, symbolType), IComparable { private readonly SyntaxAnnotation _annotation = new(); - private ILocalSymbol LocalSymbol => (ILocalSymbol)Symbol; - public override int DisplayOrder => 1; protected override int CompareTo(VariableSymbol right) @@ -194,24 +196,24 @@ public int CompareTo(LocalVariableSymbol other) return 0; } - Contract.ThrowIfFalse(LocalSymbol.Locations.Length == 1); - Contract.ThrowIfFalse(other.LocalSymbol.Locations.Length == 1); - Contract.ThrowIfFalse(LocalSymbol.Locations[0].IsInSource); - Contract.ThrowIfFalse(other.LocalSymbol.Locations[0].IsInSource); - Contract.ThrowIfFalse(LocalSymbol.Locations[0].SourceTree == other.LocalSymbol.Locations[0].SourceTree); - Contract.ThrowIfFalse(LocalSymbol.Locations[0].SourceSpan.Start != other.LocalSymbol.Locations[0].SourceSpan.Start); + Contract.ThrowIfFalse(Symbol.Locations.Length == 1); + Contract.ThrowIfFalse(other.Symbol.Locations.Length == 1); + Contract.ThrowIfFalse(Symbol.Locations[0].IsInSource); + Contract.ThrowIfFalse(other.Symbol.Locations[0].IsInSource); + Contract.ThrowIfFalse(Symbol.Locations[0].SourceTree == other.Symbol.Locations[0].SourceTree); + Contract.ThrowIfFalse(Symbol.Locations[0].SourceSpan.Start != other.Symbol.Locations[0].SourceSpan.Start); - return LocalSymbol.Locations[0].SourceSpan.Start - other.LocalSymbol.Locations[0].SourceSpan.Start; + return Symbol.Locations[0].SourceSpan.Start - other.Symbol.Locations[0].SourceSpan.Start; } public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) { - Contract.ThrowIfFalse(LocalSymbol.Locations.Length == 1); - Contract.ThrowIfFalse(LocalSymbol.Locations[0].IsInSource); - Contract.ThrowIfNull(LocalSymbol.Locations[0].SourceTree); + Contract.ThrowIfFalse(Symbol.Locations.Length == 1); + Contract.ThrowIfFalse(Symbol.Locations[0].IsInSource); + Contract.ThrowIfNull(Symbol.Locations[0].SourceTree); - var tree = LocalSymbol.Locations[0].SourceTree; - var span = LocalSymbol.Locations[0].SourceSpan; + var tree = Symbol.Locations[0].SourceTree; + var span = Symbol.Locations[0].SourceSpan; var token = tree.GetRoot(cancellationToken).FindToken(span.Start); Contract.ThrowIfFalse(token.Span.Equals(span)); @@ -231,7 +233,7 @@ public override void AddIdentifierTokenAnnotationPair( } protected sealed class QueryVariableSymbol(IRangeVariableSymbol symbol, ITypeSymbol symbolType) - : NotMovableVariableSymbol(symbol, symbolType), IComparable + : NotMovableVariableSymbol(symbol, symbolType), IComparable { public override int DisplayOrder => 2; From c5b653eb2a898e2347172d069bd4262fc58faff5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 11:42:06 -0800 Subject: [PATCH 17/32] Simplify code --- .../MethodExtractor.VariableSymbol.cs | 74 +++++++------------ 1 file changed, 27 insertions(+), 47 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs index 599bd5d43502d..0080f49f44000 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs @@ -80,12 +80,11 @@ public static int Compare(VariableSymbol left, VariableSymbol right) return -1; } - if (left.DisplayOrder == right.DisplayOrder) - { - return left.CompareTo(right); - } + var orderDiff = left.DisplayOrder - right.DisplayOrder; + if (orderDiff != 0) + return orderDiff; - return left.DisplayOrder - right.DisplayOrder; + return left.CompareTo(right); } public string Name => this.GetSymbol().ToDisplayString( @@ -94,17 +93,30 @@ public static int Compare(VariableSymbol left, VariableSymbol right) miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); } - protected abstract class VariableSymbol(TSymbol symbol, ITypeSymbol symbolType) + protected abstract class VariableSymbol(TSymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbolType) + where TVariableSymbol : VariableSymbol where TSymbol : ISymbol { protected TSymbol Symbol { get; } = symbol; protected override ISymbol GetSymbol() => this.Symbol; + + protected sealed override int CompareTo(VariableSymbol right) + { + Contract.ThrowIfTrue(this.DisplayOrder != right.DisplayOrder); + if (this == right) + return 0; + + return this.CompareTo((TVariableSymbol)right); + } + + protected abstract int CompareTo(TVariableSymbol right); } - protected abstract class NotMovableVariableSymbol( - TSymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbol, symbolType) + protected abstract class NotMovableVariableSymbol( + TSymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbol, symbolType) + where TVariableSymbol : VariableSymbol where TSymbol : ISymbol { public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) @@ -120,30 +132,18 @@ public override void AddIdentifierTokenAnnotationPair( } protected sealed class ParameterVariableSymbol(IParameterSymbol symbol, ITypeSymbol symbolType) - : NotMovableVariableSymbol(symbol, symbolType), IComparable + : NotMovableVariableSymbol(symbol, symbolType) { public override int DisplayOrder => 0; - protected override int CompareTo(VariableSymbol right) - => CompareTo((ParameterVariableSymbol)right); - - public int CompareTo(ParameterVariableSymbol other) + protected override int CompareTo(ParameterVariableSymbol other) { - Contract.ThrowIfNull(other); - - if (this == other) - { - return 0; - } - var compare = CompareMethodParameters((IMethodSymbol)this.Symbol.ContainingSymbol, (IMethodSymbol)other.Symbol.ContainingSymbol); if (compare != 0) - { return compare; - } Contract.ThrowIfFalse(Symbol.Ordinal != other.Symbol.Ordinal); - return (Symbol.Ordinal > other.Symbol.Ordinal) ? 1 : -1; + return Symbol.Ordinal > other.Symbol.Ordinal ? 1 : -1; } private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol right) @@ -178,24 +178,14 @@ private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol rig } protected sealed class LocalVariableSymbol(ILocalSymbol localSymbol, ITypeSymbol symbolType) - : VariableSymbol(localSymbol, symbolType), IComparable + : VariableSymbol(localSymbol, symbolType) { private readonly SyntaxAnnotation _annotation = new(); public override int DisplayOrder => 1; - protected override int CompareTo(VariableSymbol right) - => CompareTo((LocalVariableSymbol)right); - - public int CompareTo(LocalVariableSymbol other) + protected override int CompareTo(LocalVariableSymbol other) { - Contract.ThrowIfNull(other); - - if (this == other) - { - return 0; - } - Contract.ThrowIfFalse(Symbol.Locations.Length == 1); Contract.ThrowIfFalse(other.Symbol.Locations.Length == 1); Contract.ThrowIfFalse(Symbol.Locations[0].IsInSource); @@ -233,22 +223,12 @@ public override void AddIdentifierTokenAnnotationPair( } protected sealed class QueryVariableSymbol(IRangeVariableSymbol symbol, ITypeSymbol symbolType) - : NotMovableVariableSymbol(symbol, symbolType), IComparable + : NotMovableVariableSymbol(symbol, symbolType) { public override int DisplayOrder => 2; - protected override int CompareTo(VariableSymbol right) - => CompareTo((QueryVariableSymbol)right); - - public int CompareTo(QueryVariableSymbol other) + protected override int CompareTo(QueryVariableSymbol other) { - Contract.ThrowIfNull(other); - - if (this == other) - { - return 0; - } - var locationLeft = this.Symbol.Locations.First(); var locationRight = other.Symbol.Locations.First(); From 3a2c0603e3b843115f4db740c06c1362bb9d592b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:00:56 -0800 Subject: [PATCH 18/32] Simplify code --- .../MethodExtractor.VariableInfo.cs | 1 - .../MethodExtractor.VariableSymbol.cs | 163 ++++++------------ 2 files changed, 53 insertions(+), 111 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs index 5045dee544886..40fa837aa0d86 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs @@ -5,7 +5,6 @@ #nullable disable using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.PooledObjects; diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs index 0080f49f44000..904b861b9e8cb 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs @@ -2,14 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -#nullable disable - -using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Shared.Extensions; +using Microsoft.CodeAnalysis.SymbolMapping; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.ExtractMethod; @@ -30,11 +27,13 @@ protected abstract class VariableSymbol /// public ITypeSymbol SymbolType { get; } + private readonly int _displayOrder; private readonly bool _isCancellationToken; - protected VariableSymbol(ITypeSymbol symbolType) + protected VariableSymbol(ITypeSymbol symbolType, int displayOrder) { SymbolType = symbolType; + _displayOrder = displayOrder; _isCancellationToken = IsCancellationToken(SymbolType); static bool IsCancellationToken(ITypeSymbol originalType) @@ -54,7 +53,6 @@ static bool IsCancellationToken(ITypeSymbol originalType) /// protected abstract ISymbol GetSymbol(); - public abstract int DisplayOrder { get; } public abstract bool CanBeCapturedByLocalFunction { get; } public abstract SyntaxAnnotation IdentifierTokenAnnotation { get; } @@ -67,24 +65,19 @@ public abstract void AddIdentifierTokenAnnotationPair( public static int Compare(VariableSymbol left, VariableSymbol right) { - // CancellationTokens always go at the end of method signature. - var leftIsCancellationToken = left._isCancellationToken; - var rightIsCancellationToken = right._isCancellationToken; + if (left == right) + return 0; - if (leftIsCancellationToken && !rightIsCancellationToken) - { - return 1; - } - else if (!leftIsCancellationToken && rightIsCancellationToken) + // CancellationTokens always go at the end of method signature. + return (left._isCancellationToken, right._isCancellationToken) switch { - return -1; - } - - var orderDiff = left.DisplayOrder - right.DisplayOrder; - if (orderDiff != 0) - return orderDiff; - - return left.CompareTo(right); + (true, false) => 1, + (false, true) => -1, + // Then order by the general class of the variable (parameter, local, range-var). + _ when (left._displayOrder != right._displayOrder) => left._displayOrder - right._displayOrder, + // Finally, compare within the general class of the variable. + _ => left.CompareTo(right), + }; } public string Name => this.GetSymbol().ToDisplayString( @@ -93,8 +86,9 @@ public static int Compare(VariableSymbol left, VariableSymbol right) miscellaneousOptions: SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers)); } - protected abstract class VariableSymbol(TSymbol symbol, ITypeSymbol symbolType) - : VariableSymbol(symbolType) + protected abstract class VariableSymbol( + TSymbol symbol, ITypeSymbol symbolType, int displayOrder) + : VariableSymbol(symbolType, displayOrder) where TVariableSymbol : VariableSymbol where TSymbol : ISymbol { @@ -103,28 +97,37 @@ protected abstract class VariableSymbol(TSymbol symbol protected override ISymbol GetSymbol() => this.Symbol; protected sealed override int CompareTo(VariableSymbol right) + => this.CompareTo((TVariableSymbol)right); + + protected abstract int CompareTo(TVariableSymbol right); + + protected static int DefaultCompareTo(ISymbol left, ISymbol right) { - Contract.ThrowIfTrue(this.DisplayOrder != right.DisplayOrder); - if (this == right) - return 0; + var locationLeft = left.Locations.First(); + var locationRight = right.Locations.First(); - return this.CompareTo((TVariableSymbol)right); - } + Contract.ThrowIfFalse(locationLeft.IsInSource); + Contract.ThrowIfFalse(locationRight.IsInSource); + Contract.ThrowIfFalse(locationLeft.SourceTree == locationRight.SourceTree); + Contract.ThrowIfFalse(locationLeft.SourceSpan.Start != locationRight.SourceSpan.Start); - protected abstract int CompareTo(TVariableSymbol right); + return locationLeft.SourceSpan.Start - locationRight.SourceSpan.Start; + } } protected abstract class NotMovableVariableSymbol( - TSymbol symbol, ITypeSymbol symbolType) : VariableSymbol(symbol, symbolType) + TSymbol symbol, ITypeSymbol symbolType, int displayOrder) + : VariableSymbol(symbol, symbolType, displayOrder) where TVariableSymbol : VariableSymbol where TSymbol : ISymbol { - public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) + public sealed override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) => default; - public override SyntaxAnnotation IdentifierTokenAnnotation => throw ExceptionUtilities.Unreachable(); + public sealed override SyntaxAnnotation IdentifierTokenAnnotation + => throw ExceptionUtilities.Unreachable(); - public override void AddIdentifierTokenAnnotationPair( + public sealed override void AddIdentifierTokenAnnotationPair( MultiDictionary annotations, CancellationToken cancellationToken) { // do nothing for parameter @@ -132,89 +135,40 @@ public override void AddIdentifierTokenAnnotationPair( } protected sealed class ParameterVariableSymbol(IParameterSymbol symbol, ITypeSymbol symbolType) - : NotMovableVariableSymbol(symbol, symbolType) + : NotMovableVariableSymbol( + symbol, symbolType, displayOrder: 0) { - public override int DisplayOrder => 0; + public override bool CanBeCapturedByLocalFunction => true; protected override int CompareTo(ParameterVariableSymbol other) { - var compare = CompareMethodParameters((IMethodSymbol)this.Symbol.ContainingSymbol, (IMethodSymbol)other.Symbol.ContainingSymbol); + // these methods can be either regular one, anonymous function, local function and etc but all must + // belong to same outer regular method. so, it should have location pointing to same tree + var compare = DefaultCompareTo((IMethodSymbol)this.Symbol.ContainingSymbol, (IMethodSymbol)other.Symbol.ContainingSymbol); if (compare != 0) return compare; Contract.ThrowIfFalse(Symbol.Ordinal != other.Symbol.Ordinal); - return Symbol.Ordinal > other.Symbol.Ordinal ? 1 : -1; - } - - private static int CompareMethodParameters(IMethodSymbol left, IMethodSymbol right) - { - if (left == null && right == null) - { - // not method parameters - return 0; - } - - if (left.Equals(right)) - { - // parameter of same method - return 0; - } - - // these methods can be either regular one, anonymous function, local function and etc - // but all must belong to same outer regular method. - // so, it should have location pointing to same tree - var leftLocations = left.Locations; - var rightLocations = right.Locations; - - var commonTree = leftLocations.Select(l => l.SourceTree).Intersect(rightLocations.Select(l => l.SourceTree)).WhereNotNull().First(); - - var leftLocation = leftLocations.First(l => l.SourceTree == commonTree); - var rightLocation = rightLocations.First(l => l.SourceTree == commonTree); - - return leftLocation.SourceSpan.Start - rightLocation.SourceSpan.Start; + return Symbol.Ordinal - other.Symbol.Ordinal; } - - public override bool CanBeCapturedByLocalFunction => true; } protected sealed class LocalVariableSymbol(ILocalSymbol localSymbol, ITypeSymbol symbolType) - : VariableSymbol(localSymbol, symbolType) + : VariableSymbol( + localSymbol, symbolType, displayOrder: 1) { private readonly SyntaxAnnotation _annotation = new(); - public override int DisplayOrder => 1; + public override bool CanBeCapturedByLocalFunction => true; protected override int CompareTo(LocalVariableSymbol other) - { - Contract.ThrowIfFalse(Symbol.Locations.Length == 1); - Contract.ThrowIfFalse(other.Symbol.Locations.Length == 1); - Contract.ThrowIfFalse(Symbol.Locations[0].IsInSource); - Contract.ThrowIfFalse(other.Symbol.Locations[0].IsInSource); - Contract.ThrowIfFalse(Symbol.Locations[0].SourceTree == other.Symbol.Locations[0].SourceTree); - Contract.ThrowIfFalse(Symbol.Locations[0].SourceSpan.Start != other.Symbol.Locations[0].SourceSpan.Start); - - return Symbol.Locations[0].SourceSpan.Start - other.Symbol.Locations[0].SourceSpan.Start; - } + => DefaultCompareTo(this.Symbol, other.Symbol); public override SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) - { - Contract.ThrowIfFalse(Symbol.Locations.Length == 1); - Contract.ThrowIfFalse(Symbol.Locations[0].IsInSource); - Contract.ThrowIfNull(Symbol.Locations[0].SourceTree); - - var tree = Symbol.Locations[0].SourceTree; - var span = Symbol.Locations[0].SourceSpan; - - var token = tree.GetRoot(cancellationToken).FindToken(span.Start); - Contract.ThrowIfFalse(token.Span.Equals(span)); - - return token; - } + => Symbol.Locations.First().FindToken(cancellationToken); public override SyntaxAnnotation IdentifierTokenAnnotation => _annotation; - public override bool CanBeCapturedByLocalFunction => true; - public override void AddIdentifierTokenAnnotationPair( MultiDictionary annotations, CancellationToken cancellationToken) { @@ -223,24 +177,13 @@ public override void AddIdentifierTokenAnnotationPair( } protected sealed class QueryVariableSymbol(IRangeVariableSymbol symbol, ITypeSymbol symbolType) - : NotMovableVariableSymbol(symbol, symbolType) + : NotMovableVariableSymbol( + symbol, symbolType, displayOrder: 2) { - public override int DisplayOrder => 2; + public override bool CanBeCapturedByLocalFunction => false; protected override int CompareTo(QueryVariableSymbol other) - { - var locationLeft = this.Symbol.Locations.First(); - var locationRight = other.Symbol.Locations.First(); - - Contract.ThrowIfFalse(locationLeft.IsInSource); - Contract.ThrowIfFalse(locationRight.IsInSource); - Contract.ThrowIfFalse(locationLeft.SourceTree == locationRight.SourceTree); - Contract.ThrowIfFalse(locationLeft.SourceSpan.Start != locationRight.SourceSpan.Start); - - return locationLeft.SourceSpan.Start - locationRight.SourceSpan.Start; - } - - public override bool CanBeCapturedByLocalFunction => false; + => DefaultCompareTo(this.Symbol, other.Symbol); } } } From 571c1030736dd178cbe6db6ba4a1088a63459397 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:01:00 -0800 Subject: [PATCH 19/32] Simplify code --- .../Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs index 904b861b9e8cb..e5a760b96ef88 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs @@ -109,7 +109,6 @@ protected static int DefaultCompareTo(ISymbol left, ISymbol right) Contract.ThrowIfFalse(locationLeft.IsInSource); Contract.ThrowIfFalse(locationRight.IsInSource); Contract.ThrowIfFalse(locationLeft.SourceTree == locationRight.SourceTree); - Contract.ThrowIfFalse(locationLeft.SourceSpan.Start != locationRight.SourceSpan.Start); return locationLeft.SourceSpan.Start - locationRight.SourceSpan.Start; } From b7e09706fbdf2aebe86bd850a5c351280ff29dfd Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:05:31 -0800 Subject: [PATCH 20/32] Simplify code --- .../MethodExtractor.VariableInfo.cs | 5 +---- .../MethodExtractor.VariableSymbol.cs | 22 +++++++++---------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs index 40fa837aa0d86..a217f5dd0629b 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableInfo.cs @@ -117,11 +117,8 @@ public SyntaxToken GetIdentifierTokenAtDeclaration(SyntaxNode node) public SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken) => _variableSymbol.GetOriginalIdentifierToken(cancellationToken); - public static void SortVariables(ArrayBuilder variables) - => variables.Sort(); - public int CompareTo(VariableInfo other) - => VariableSymbol.Compare(this._variableSymbol, other._variableSymbol); + => this._variableSymbol.CompareTo(other._variableSymbol); } } } diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs index e5a760b96ef88..30f4df93c84b2 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs @@ -2,11 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; using System.Linq; using System.Threading; -using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Shared.Extensions; -using Microsoft.CodeAnalysis.SymbolMapping; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.ExtractMethod; @@ -18,7 +17,7 @@ internal abstract partial class AbstractExtractMethodService< { internal abstract partial class MethodExtractor { - protected abstract class VariableSymbol + protected abstract class VariableSymbol : IComparable { /// /// Get the type of to use when generating code. May contain anonymous types in it. @@ -58,25 +57,26 @@ static bool IsCancellationToken(ITypeSymbol originalType) public abstract SyntaxAnnotation IdentifierTokenAnnotation { get; } public abstract SyntaxToken GetOriginalIdentifierToken(CancellationToken cancellationToken); + protected abstract int CompareToWorker(VariableSymbol other); + public abstract void AddIdentifierTokenAnnotationPair( MultiDictionary annotations, CancellationToken cancellationToken); - protected abstract int CompareTo(VariableSymbol right); - - public static int Compare(VariableSymbol left, VariableSymbol right) + public int CompareTo(VariableSymbol? other) { - if (left == right) + Contract.ThrowIfNull(other); + if (this == other) return 0; // CancellationTokens always go at the end of method signature. - return (left._isCancellationToken, right._isCancellationToken) switch + return (this._isCancellationToken, other._isCancellationToken) switch { (true, false) => 1, (false, true) => -1, // Then order by the general class of the variable (parameter, local, range-var). - _ when (left._displayOrder != right._displayOrder) => left._displayOrder - right._displayOrder, + _ when (this._displayOrder != other._displayOrder) => this._displayOrder - other._displayOrder, // Finally, compare within the general class of the variable. - _ => left.CompareTo(right), + _ => this.CompareToWorker(other), }; } @@ -96,7 +96,7 @@ protected abstract class VariableSymbol( protected override ISymbol GetSymbol() => this.Symbol; - protected sealed override int CompareTo(VariableSymbol right) + protected sealed override int CompareToWorker(VariableSymbol right) => this.CompareTo((TVariableSymbol)right); protected abstract int CompareTo(TVariableSymbol right); From 1a6f76e595c027954d2f7e252e80da5513612fde Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:08:00 -0800 Subject: [PATCH 21/32] Simplify code --- .../MethodExtractor.Analyzer.SymbolMapBuilder.cs | 10 +++------- .../Portable/ExtractMethod/MethodExtractor.Analyzer.cs | 5 ++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs index 1590db5c413bb..7b63382bf44da 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs @@ -27,10 +27,10 @@ private sealed class SymbolMapBuilder private readonly SemanticModel _semanticModel; private readonly ISyntaxFactsService _service; private readonly TextSpan _span; - private readonly Dictionary> _symbolMap = []; + private readonly MultiDictionary _symbolMap = []; private readonly CancellationToken _cancellationToken; - public static Dictionary> Build( + public static MultiDictionary Build( ISyntaxFactsService service, SemanticModel semanticModel, SyntaxNode root, @@ -74,11 +74,7 @@ private void Visit(SyntaxNode node) var symbolInfo = _semanticModel.GetSymbolInfo(token, _cancellationToken); foreach (var sym in symbolInfo.GetAllSymbols()) - { - // add binding result to map - var list = _symbolMap.GetOrAdd(sym, _ => []); - list.Add(token); - } + _symbolMap.Add(sym, token); } } } diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 8337fd60eec1c..4e358c39a5db7 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -309,11 +309,10 @@ private OperationStatus CheckAsyncMethodRefOutParameters(IList par return OperationStatus.SucceededStatus; } - private Dictionary> GetSymbolMap() + private MultiDictionary GetSymbolMap() { var context = SelectionResult.GetContainingScope(); - var symbolMap = SymbolMapBuilder.Build(this.SyntaxFacts, this.SemanticModel, context, SelectionResult.FinalSpan, CancellationToken); - return symbolMap; + return SymbolMapBuilder.Build(this.SyntaxFacts, this.SemanticModel, context, SelectionResult.FinalSpan, CancellationToken); } private ImmutableArray MarkVariableInfosToUseAsReturnValueIfPossible(ImmutableArray variableInfo) From ef140ca5cabc4ecd6f2588edb2b3d15c85e323f5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:11:43 -0800 Subject: [PATCH 22/32] Switch to a multi dictionary --- .../ExtractMethod/MethodExtractor.Analyzer.cs | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 4e358c39a5db7..7dc72f7fad5f1 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -249,7 +249,7 @@ private bool IsInExpressionOrHasReturnStatement() => SelectionResult.IsExtractMethodOnExpression || ContainsReturnStatementInSelectedCode(); private OperationStatus GetOperationStatus( - Dictionary> symbolMap, + MultiDictionary symbolMap, IList parameters, IList failedVariables, bool unsafeAddressTakenUsed, @@ -412,7 +412,7 @@ private static ImmutableArray GetMethodParameters(Dictionary> symbolMap, + MultiDictionary symbolMap, bool isInPrimaryConstructorBaseType, out Dictionary variableInfoMap, out List failedVariables) @@ -594,7 +594,7 @@ private static void AddVariableToMap(IDictionary variable private bool TryGetVariableStyle( bool bestEffort, - Dictionary> symbolMap, + MultiDictionary symbolMap, ISymbol symbol, ITypeSymbol type, bool captured, @@ -649,9 +649,10 @@ private bool TryGetVariableStyle( } private bool IsWrittenInsideForFrameworkValueType( - Dictionary> symbolMap, ISymbol symbol, bool writtenInside) + MultiDictionary symbolMap, ISymbol symbol, bool writtenInside) { - if (!symbolMap.TryGetValue(symbol, out var tokens)) + var tokens = symbolMap[symbol]; + if (tokens.Count == 0) return writtenInside; var semanticFacts = this.SemanticFacts; @@ -805,24 +806,21 @@ private static void AppendMethodTypeParameterFromConstraint(SortedDictionary> symbolMap, IDictionary sortedMap) + private static void AppendMethodTypeParameterUsedDirectly(MultiDictionary symbolMap, IDictionary sortedMap) { - foreach (var pair in symbolMap.Where(p => p.Key.Kind == SymbolKind.TypeParameter)) + foreach (var typeParameter in symbolMap.Keys.OfType()) { - var typeParameter = (ITypeParameterSymbol)pair.Key; - if (typeParameter.DeclaringMethod == null || - sortedMap.ContainsKey(typeParameter.Ordinal)) + if (typeParameter.DeclaringMethod != null && + !sortedMap.ContainsKey(typeParameter.Ordinal)) { - continue; + sortedMap[typeParameter.Ordinal] = typeParameter; } - - sortedMap[typeParameter.Ordinal] = typeParameter; } } private ImmutableArray GetMethodTypeParametersInConstraintList( IDictionary variableInfoMap, - IDictionary> symbolMap, + MultiDictionary symbolMap, SortedDictionary sortedMap) { // find starting points @@ -906,7 +904,7 @@ private static ImmutableArray GetMethodTypeParametersInDec return [.. sortedMap.Values]; } - private OperationStatus CheckReadOnlyFields(Dictionary> symbolMap) + private OperationStatus CheckReadOnlyFields(MultiDictionary symbolMap) { if (ReadOnlyFieldAllowed()) return OperationStatus.SucceededStatus; From 04b07b04fddd26e648d2d0b29d8e8e9ef260b970 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:15:37 -0800 Subject: [PATCH 23/32] Delete unnecessary type --- ...thodExtractor.Analyzer.SymbolMapBuilder.cs | 83 ------------------- .../ExtractMethod/MethodExtractor.Analyzer.cs | 23 ++++- 2 files changed, 22 insertions(+), 84 deletions(-) delete mode 100644 src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs deleted file mode 100644 index 7b63382bf44da..0000000000000 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.SymbolMapBuilder.cs +++ /dev/null @@ -1,83 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -#nullable disable - -using System.Collections.Generic; -using System.Threading; -using Microsoft.CodeAnalysis.LanguageService; -using Microsoft.CodeAnalysis.Shared.Extensions; -using Microsoft.CodeAnalysis.Text; -using Roslyn.Utilities; - -namespace Microsoft.CodeAnalysis.ExtractMethod; - -internal abstract partial class AbstractExtractMethodService< - TStatementSyntax, - TExecutableStatementSyntax, - TExpressionSyntax> -{ - internal abstract partial class MethodExtractor - { - protected abstract partial class Analyzer - { - private sealed class SymbolMapBuilder - { - private readonly SemanticModel _semanticModel; - private readonly ISyntaxFactsService _service; - private readonly TextSpan _span; - private readonly MultiDictionary _symbolMap = []; - private readonly CancellationToken _cancellationToken; - - public static MultiDictionary Build( - ISyntaxFactsService service, - SemanticModel semanticModel, - SyntaxNode root, - TextSpan span, - CancellationToken cancellationToken) - { - Contract.ThrowIfNull(semanticModel); - Contract.ThrowIfNull(service); - Contract.ThrowIfNull(root); - - var builder = new SymbolMapBuilder(service, semanticModel, span, cancellationToken); - builder.Visit(root); - - return builder._symbolMap; - } - - private SymbolMapBuilder( - ISyntaxFactsService service, - SemanticModel semanticModel, - TextSpan span, - CancellationToken cancellationToken) - { - _semanticModel = semanticModel; - _service = service; - _span = span; - _cancellationToken = cancellationToken; - } - - private void Visit(SyntaxNode node) - { - foreach (var token in node.DescendantTokens()) - { - if (token.IsMissing || - token.Width() <= 0 || - !_service.IsIdentifier(token) || - !_span.Contains(token.Span) || - _service.IsNameOfNamedArgument(token.Parent)) - { - continue; - } - - var symbolInfo = _semanticModel.GetSymbolInfo(token, _cancellationToken); - foreach (var sym in symbolInfo.GetAllSymbols()) - _symbolMap.Add(sym, token); - } - } - } - } - } -} diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 7dc72f7fad5f1..fb1cde2f19eae 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -311,8 +311,29 @@ private OperationStatus CheckAsyncMethodRefOutParameters(IList par private MultiDictionary GetSymbolMap() { + var symbolMap = new MultiDictionary(); + + var semanticModel = this.SemanticModel; + var syntaxFacts = this.SyntaxFacts; var context = SelectionResult.GetContainingScope(); - return SymbolMapBuilder.Build(this.SyntaxFacts, this.SemanticModel, context, SelectionResult.FinalSpan, CancellationToken); + + foreach (var token in context.DescendantTokens()) + { + if (token.IsMissing || + token.Width() <= 0 || + !this.SelectionResult.FinalSpan.Contains(token.Span) || + !syntaxFacts.IsIdentifier(token) || + syntaxFacts.IsNameOfNamedArgument(token.Parent)) + { + continue; + } + + var symbolInfo = semanticModel.GetSymbolInfo(token, this.CancellationToken); + foreach (var sym in symbolInfo.GetAllSymbols()) + symbolMap.Add(sym, token); + } + + return symbolMap; } private ImmutableArray MarkVariableInfosToUseAsReturnValueIfPossible(ImmutableArray variableInfo) From 56e9b8eea1aaa8ce445b0c9e557dd1143ff88b88 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:29:29 -0800 Subject: [PATCH 24/32] rename and reorder --- .../ExtractMethod/MethodExtractor.Analyzer.cs | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index fb1cde2f19eae..53c4c9870391d 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -124,7 +124,7 @@ public AnalyzerResult Analyze() // check whether the selection contains "&" over a symbol exist var unsafeAddressTakenUsed = dataFlowAnalysisData.UnsafeAddressTaken.Intersect(variableInfoMap.Keys).Any(); - var (parameters, returnType, returnsByRef, variablesToUseAsReturnValue) = GetSignatureInformation(variableInfoMap); + var (variables, variablesToUseAsReturnValue, returnType, returnsByRef) = GetSignatureInformation(variableInfoMap); (returnType, var awaitTaskReturn) = AdjustReturnType(returnType); @@ -135,12 +135,12 @@ public AnalyzerResult Analyze() // check various error cases var operationStatus = GetOperationStatus( - symbolMap, parameters, failedVariables, unsafeAddressTakenUsed, returnType.ContainsAnonymousType(), containsAnyLocalFunctionCallNotWithinSpan); + symbolMap, variables, failedVariables, unsafeAddressTakenUsed, returnType.ContainsAnonymousType(), containsAnyLocalFunctionCallNotWithinSpan); return new AnalyzerResult( typeParametersInDeclaration, typeParametersInConstraintList, - parameters, + variables, variablesToUseAsReturnValue, returnType, returnsByRef, @@ -207,26 +207,27 @@ private ITypeSymbol UnwrapTaskIfNeeded(ITypeSymbol returnType) return (returnType, awaitTaskReturn: false); } - private (ImmutableArray parameters, ITypeSymbol returnType, bool returnsByRef, ImmutableArray variablesToUseAsReturnValue) + private (ImmutableArray parameters, ImmutableArray variablesToUseAsReturnValue, ITypeSymbol returnType, bool returnsByRef) GetSignatureInformation(Dictionary variableInfoMap) { + var allVariableInfos = variableInfoMap.Values.Order().ToImmutableArray(); + if (this.IsInExpressionOrHasReturnStatement()) { // check whether current selection contains return statement - var parameters = GetMethodParameters(variableInfoMap); var (returnType, returnsByRef) = SelectionResult.GetReturnTypeInfo(this.CancellationToken); - return (parameters, returnType, returnsByRef, []); + return (allVariableInfos, [], returnType, returnsByRef); } else { // no return statement - var parameters = MarkVariableInfosToUseAsReturnValueIfPossible(GetMethodParameters(variableInfoMap)); - var variablesToUseAsReturnValue = parameters.WhereAsArray(v => v.UseAsReturnValue); + allVariableInfos = MarkVariableInfosToUseAsReturnValueIfPossible(allVariableInfos); + var variablesToUseAsReturnValue = allVariableInfos.WhereAsArray(v => v.UseAsReturnValue); var returnType = GetReturnType(variablesToUseAsReturnValue); - return (parameters, returnType, returnsByRef: false, variablesToUseAsReturnValue); + return (allVariableInfos, variablesToUseAsReturnValue, returnType, returnsByRef: false); } ITypeSymbol GetReturnType(ImmutableArray variablesToUseAsReturnValue) @@ -250,7 +251,7 @@ private bool IsInExpressionOrHasReturnStatement() private OperationStatus GetOperationStatus( MultiDictionary symbolMap, - IList parameters, + ImmutableArray variables, IList failedVariables, bool unsafeAddressTakenUsed, bool returnTypeHasAnonymousType, @@ -258,7 +259,7 @@ private OperationStatus GetOperationStatus( { var readonlyFieldStatus = CheckReadOnlyFields(symbolMap); - var namesWithAnonymousTypes = parameters.Where(v => v.OriginalTypeHadAnonymousTypeOrDelegate).Select(v => v.Name ?? string.Empty); + var namesWithAnonymousTypes = variables.Where(v => v.OriginalTypeHadAnonymousTypeOrDelegate).Select(v => v.Name ?? string.Empty); if (returnTypeHasAnonymousType) { namesWithAnonymousTypes = namesWithAnonymousTypes.Concat("return type"); @@ -275,7 +276,7 @@ private OperationStatus GetOperationStatus( ? OperationStatus.UnsafeAddressTaken : OperationStatus.SucceededStatus; - var asyncRefOutParameterStatus = CheckAsyncMethodRefOutParameters(parameters); + var asyncRefOutParameterStatus = CheckAsyncMethodRefOutParameters(variables); var variableMapStatus = failedVariables.Count == 0 ? OperationStatus.SucceededStatus @@ -419,14 +420,6 @@ private int GetIndexOfVariableInfoToUseAsReturnValue( return -1; } - private static ImmutableArray GetMethodParameters(Dictionary variableInfoMap) - { - var list = new FixedSizeArrayBuilder(variableInfoMap.Count); - list.AddRange(variableInfoMap.Values); - list.Sort(); - return list.MoveToImmutable(); - } - /// When false, variables whose data flow is not understood /// will be returned in . When true, we assume any /// variable we don't understand has From 1c3acffb396758ab26feb2e90f12db5afe6a7d40 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:33:09 -0800 Subject: [PATCH 25/32] in progress --- .../Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 53c4c9870391d..640ddff485030 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -217,7 +217,8 @@ private ITypeSymbol UnwrapTaskIfNeeded(ITypeSymbol returnType) // check whether current selection contains return statement var (returnType, returnsByRef) = SelectionResult.GetReturnTypeInfo(this.CancellationToken); - return (allVariableInfos, [], returnType, returnsByRef); + var variablesToUseAsReturnValue = allVariableInfos.WhereAsArray(v => v.UseAsReturnValue); + return (allVariableInfos, variablesToUseAsReturnValue, returnType, returnsByRef); } else { From 033dab6591caa5fddde050085e4d30aac33b8c62 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 12:40:02 -0800 Subject: [PATCH 26/32] move comptuation --- .../ExtractMethod/MethodExtractor.Analyzer.cs | 19 +++++++++---------- .../MethodExtractor.AnalyzerResult.cs | 3 +-- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs index 640ddff485030..3fcb005366170 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs @@ -124,7 +124,8 @@ public AnalyzerResult Analyze() // check whether the selection contains "&" over a symbol exist var unsafeAddressTakenUsed = dataFlowAnalysisData.UnsafeAddressTaken.Intersect(variableInfoMap.Keys).Any(); - var (variables, variablesToUseAsReturnValue, returnType, returnsByRef) = GetSignatureInformation(variableInfoMap); + + var (variables, returnType, returnsByRef) = GetSignatureInformation(variableInfoMap); (returnType, var awaitTaskReturn) = AdjustReturnType(returnType); @@ -141,7 +142,6 @@ public AnalyzerResult Analyze() typeParametersInDeclaration, typeParametersInConstraintList, variables, - variablesToUseAsReturnValue, returnType, returnsByRef, awaitTaskReturn, @@ -207,28 +207,27 @@ private ITypeSymbol UnwrapTaskIfNeeded(ITypeSymbol returnType) return (returnType, awaitTaskReturn: false); } - private (ImmutableArray parameters, ImmutableArray variablesToUseAsReturnValue, ITypeSymbol returnType, bool returnsByRef) - GetSignatureInformation(Dictionary variableInfoMap) + private (ImmutableArray finalOrderedVariableInfos, ITypeSymbol returnType, bool returnsByRef) + GetSignatureInformation(Dictionary symbolMap) { - var allVariableInfos = variableInfoMap.Values.Order().ToImmutableArray(); + var allVariableInfos = symbolMap.Values.Order().ToImmutableArray(); if (this.IsInExpressionOrHasReturnStatement()) { // check whether current selection contains return statement var (returnType, returnsByRef) = SelectionResult.GetReturnTypeInfo(this.CancellationToken); - var variablesToUseAsReturnValue = allVariableInfos.WhereAsArray(v => v.UseAsReturnValue); - return (allVariableInfos, variablesToUseAsReturnValue, returnType, returnsByRef); + return (allVariableInfos, returnType, returnsByRef); } else { // no return statement - allVariableInfos = MarkVariableInfosToUseAsReturnValueIfPossible(allVariableInfos); - var variablesToUseAsReturnValue = allVariableInfos.WhereAsArray(v => v.UseAsReturnValue); + var finalOrderedVariableInfos = MarkVariableInfosToUseAsReturnValueIfPossible(allVariableInfos); + var variablesToUseAsReturnValue = finalOrderedVariableInfos.WhereAsArray(v => v.UseAsReturnValue); var returnType = GetReturnType(variablesToUseAsReturnValue); - return (allVariableInfos, variablesToUseAsReturnValue, returnType, returnsByRef: false); + return (finalOrderedVariableInfos, returnType, returnsByRef: false); } ITypeSymbol GetReturnType(ImmutableArray variablesToUseAsReturnValue) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.AnalyzerResult.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.AnalyzerResult.cs index 51343bc4b1a56..58fc20398691e 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.AnalyzerResult.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.AnalyzerResult.cs @@ -25,7 +25,6 @@ protected sealed class AnalyzerResult( ImmutableArray typeParametersInDeclaration, ImmutableArray typeParametersInConstraintList, ImmutableArray variables, - ImmutableArray variablesToUseAsReturnValue, ITypeSymbol returnType, bool returnsByRef, bool awaitTaskReturn, @@ -36,7 +35,7 @@ protected sealed class AnalyzerResult( { public ImmutableArray MethodTypeParametersInDeclaration { get; } = typeParametersInDeclaration; public ImmutableArray MethodTypeParametersInConstraintList { get; } = typeParametersInConstraintList; - public ImmutableArray VariablesToUseAsReturnValue { get; } = variablesToUseAsReturnValue; + public ImmutableArray VariablesToUseAsReturnValue { get; } = variables.WhereAsArray(v => v.UseAsReturnValue); /// /// used to determine whether static can be used From d4c50634c8cd4c7ec6d4b87031d868652c8a8cc7 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 13:00:01 -0800 Subject: [PATCH 27/32] be more consistent --- .../CSharpSelectionResult.StatementResult.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs index ab373de64c22a..e241cb3db84b2 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs @@ -48,7 +48,6 @@ public override SyntaxNode GetContainingScope() n is AccessorDeclarationSyntax or LocalFunctionStatementSyntax or BaseMethodDeclarationSyntax or - AccessorDeclarationSyntax or AnonymousFunctionExpressionSyntax or CompilationUnitSyntax); } @@ -70,7 +69,13 @@ protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInf _ => throw ExceptionUtilities.UnexpectedValue(node), }; - case MethodDeclarationSyntax methodDeclaration: + case LocalFunctionStatementSyntax localFunction: + { + var method = semanticModel.GetRequiredDeclaredSymbol(localFunction, cancellationToken); + return (method.ReturnType, method.ReturnsByRef); + } + + case BaseMethodDeclarationSyntax methodDeclaration: { var method = semanticModel.GetRequiredDeclaredSymbol(methodDeclaration, cancellationToken); return (method.ReturnType, method.ReturnsByRef); From 8c4ea433682f5485742ecde1eb257a606a5bd26e Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 13:01:27 -0800 Subject: [PATCH 28/32] be more consistent --- .../ExtractMethod/CSharpSelectionResult.StatementResult.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs index e241cb3db84b2..cbac6bbfae5b1 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs @@ -41,7 +41,6 @@ public override bool ContainingScopeHasAsyncKeyword() public override SyntaxNode GetContainingScope() { - Contract.ThrowIfNull(SemanticDocument); Contract.ThrowIfTrue(IsExtractMethodOnExpression); return GetFirstTokenInSelection().GetRequiredParent().AncestorsAndSelf().First(n => @@ -54,8 +53,6 @@ AnonymousFunctionExpressionSyntax or protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfoWorker(CancellationToken cancellationToken) { - Contract.ThrowIfTrue(IsExtractMethodOnExpression); - var node = GetContainingScope(); var semanticModel = SemanticDocument.SemanticModel; From bba35459b8261f4a2a10130c2a4e83d03e3d8571 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 17:38:16 -0800 Subject: [PATCH 29/32] Remove another unnecessary annotation container --- .../ExtractMethod/CSharpMethodExtractor.cs | 10 +---- .../Portable/ExtractMethod/InsertionPoint.cs | 45 ------------------- .../MethodExtractor.CodeGenerator.cs | 26 ++++++----- .../Portable/ExtractMethod/MethodExtractor.cs | 37 ++++++++------- .../VisualBasicMethodExtractor.vb | 9 +--- 5 files changed, 40 insertions(+), 87 deletions(-) delete mode 100644 src/Features/Core/Portable/ExtractMethod/InsertionPoint.cs diff --git a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs index 2a847849479d5..50625f6f9a33e 100644 --- a/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs +++ b/src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.cs @@ -23,8 +23,8 @@ internal sealed partial class CSharpMethodExtractor( SelectionResult result, ExtractMethodGenerationOptions options, bool localFunction) : MethodExtractor(result, options, localFunction) { - protected override CodeGenerator CreateCodeGenerator(AnalyzerResult analyzerResult) - => CSharpCodeGenerator.Create(this.OriginalSelectionResult, analyzerResult, this.Options, this.LocalFunction); + protected override CodeGenerator CreateCodeGenerator(SelectionResult selectionResult, AnalyzerResult analyzerResult) + => CSharpCodeGenerator.Create(selectionResult, analyzerResult, this.Options, this.LocalFunction); protected override AnalyzerResult Analyze(CancellationToken cancellationToken) { @@ -170,12 +170,6 @@ await semanticDocument.WithSyntaxRootAsync(result.Root, cancellationToken).Confi result); } - protected override Task GenerateCodeAsync(InsertionPoint insertionPoint, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken) - { - var codeGenerator = CSharpCodeGenerator.Create(selectionResult, analyzeResult, options, this.LocalFunction); - return codeGenerator.GenerateAsync(insertionPoint, cancellationToken); - } - protected override AbstractFormattingRule GetCustomFormattingRule(Document document) => FormattingRule.Instance; diff --git a/src/Features/Core/Portable/ExtractMethod/InsertionPoint.cs b/src/Features/Core/Portable/ExtractMethod/InsertionPoint.cs deleted file mode 100644 index 36cea7e39f7de..0000000000000 --- a/src/Features/Core/Portable/ExtractMethod/InsertionPoint.cs +++ /dev/null @@ -1,45 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using System.Linq; -using Roslyn.Utilities; - -namespace Microsoft.CodeAnalysis.ExtractMethod; - -internal sealed class InsertionPoint -{ - private readonly SyntaxAnnotation _annotation; - private readonly Lazy _context; - - public InsertionPoint(SemanticDocument document, SyntaxAnnotation annotation) - { - Contract.ThrowIfNull(document); - Contract.ThrowIfNull(annotation); - - SemanticDocument = document; - _annotation = annotation; - _context = CreateLazyContextNode(); - } - - public SemanticDocument SemanticDocument { get; } - - public SyntaxNode GetRoot() - => SemanticDocument.Root; - - public SyntaxNode? GetContext() - => _context.Value; - - public InsertionPoint With(SemanticDocument document) - => new(document, _annotation); - - private Lazy CreateLazyContextNode() - => new(ComputeContextNode, isThreadSafe: true); - - private SyntaxNode? ComputeContextNode() - { - var root = SemanticDocument.Root; - return root.GetAnnotatedNodesAndTokens(_annotation).SingleOrDefault().AsNode(); - } -} diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs index f52a53b8e0611..0ddbaad18683a 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs @@ -37,6 +37,8 @@ protected abstract class CodeGenerator /// public abstract OperationStatus> GetNewMethodStatements( SyntaxNode insertionPointNode, CancellationToken cancellationToken); + + public abstract Task GenerateAsync(CancellationToken cancellationToken); } protected abstract partial class CodeGenerator : CodeGenerator @@ -99,10 +101,15 @@ protected abstract Task PerformFinalTriviaFixupAsync( SemanticDocument newDocument, CancellationToken cancellationToken); #endregion - public async Task GenerateAsync(InsertionPoint insertionPoint, CancellationToken cancellationToken) + private static SyntaxNode GetInsertionPoint(SemanticDocument document) + => document.Root.GetAnnotatedNodes(InsertionPointAnnotation).Single(); + + public sealed override async Task GenerateAsync(CancellationToken cancellationToken) { - var newMethodDefinition = GenerateMethodDefinition(insertionPoint.GetContext(), cancellationToken); - var callSiteDocument = await InsertMethodAndUpdateCallSiteAsync(insertionPoint, newMethodDefinition, cancellationToken).ConfigureAwait(false); + var semanticDocument = SelectionResult.SemanticDocument; + var insertionPoint = GetInsertionPoint(semanticDocument); + var newMethodDefinition = GenerateMethodDefinition(insertionPoint, cancellationToken); + var callSiteDocument = await InsertMethodAndUpdateCallSiteAsync(semanticDocument, newMethodDefinition, cancellationToken).ConfigureAwait(false); // For nullable reference types, we can provide a better experience by reducing use of nullable // reference types after a method is done being generated. If we can determine that the method never @@ -119,10 +126,9 @@ public async Task GenerateAsync(InsertionPoint insertionPoint, } private async Task InsertMethodAndUpdateCallSiteAsync( - InsertionPoint insertionPoint, IMethodSymbol newMethodDefinition, CancellationToken cancellationToken) + SemanticDocument document, IMethodSymbol newMethodDefinition, CancellationToken cancellationToken) { - var document = this.SemanticDocument.Document; - var codeGenerationService = document.GetLanguageService(); + var codeGenerationService = document.GetRequiredLanguageService(); // First, update the callsite with the call to the new method. var outermostCallSiteContainer = GetOutermostCallSiteContainerToProcess(cancellationToken); @@ -130,7 +136,7 @@ private async Task InsertMethodAndUpdateCallSiteAsync( var rootWithUpdatedCallSite = this.SemanticDocument.Root.ReplaceNode( outermostCallSiteContainer, await GenerateBodyForCallSiteContainerAsync( - insertionPoint.GetContext(), outermostCallSiteContainer, cancellationToken).ConfigureAwait(false)); + GetInsertionPoint(document), outermostCallSiteContainer, cancellationToken).ConfigureAwait(false)); // Then insert the local-function/method into the updated document that contains the updated callsite. var documentWithUpdatedCallSite = await this.SemanticDocument.WithSyntaxRootAsync(rootWithUpdatedCallSite, cancellationToken).ConfigureAwait(false); @@ -151,7 +157,7 @@ SyntaxNode InsertLocalFunction() var localMethod = codeGenerationService.CreateMethodDeclaration(newMethodDefinition, CodeGenerationDestination.Unspecified, info, cancellationToken); // Find the destination for the local function after the callsite has been fixed up. - var destination = insertionPoint.With(documentWithUpdatedCallSite).GetContext(); + var destination = GetInsertionPoint(documentWithUpdatedCallSite); var updatedDestination = codeGenerationService.AddStatements(destination, [localMethod], info, cancellationToken); var finalRoot = documentWithUpdatedCallSite.Root.ReplaceNode(destination, updatedDestination); @@ -160,10 +166,10 @@ SyntaxNode InsertLocalFunction() SyntaxNode InsertNormalMethod() { - var syntaxKinds = document.GetLanguageService(); + var syntaxKinds = document.GetRequiredLanguageService(); // Find the destination for the new method after the callsite has been fixed up. - var mappedMember = insertionPoint.With(documentWithUpdatedCallSite).GetContext(); + var mappedMember = GetInsertionPoint(documentWithUpdatedCallSite); mappedMember = mappedMember.Parent?.RawKind == syntaxKinds.GlobalStatement ? mappedMember.Parent : mappedMember; diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs index 290fa6595e71b..cbe016def449b 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs @@ -29,6 +29,8 @@ internal abstract partial class MethodExtractor( ExtractMethodGenerationOptions options, bool localFunction) { + public static readonly SyntaxAnnotation InsertionPointAnnotation = new(); + protected readonly SelectionResult OriginalSelectionResult = selectionResult; protected readonly ExtractMethodGenerationOptions Options = options; protected readonly bool LocalFunction = localFunction; @@ -38,9 +40,9 @@ internal abstract partial class MethodExtractor( protected abstract SyntaxNode GetInsertionPointNode(AnalyzerResult analyzerResult, CancellationToken cancellationToken); protected abstract Task PreserveTriviaAsync(SyntaxNode root, CancellationToken cancellationToken); - protected abstract CodeGenerator CreateCodeGenerator(AnalyzerResult analyzerResult); - protected abstract Task GenerateCodeAsync( - InsertionPoint insertionPoint, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken); + protected abstract CodeGenerator CreateCodeGenerator(SelectionResult selectionResult, AnalyzerResult analyzerResult); + //protected abstract Task GenerateCodeAsync( + // SemanticDocument document, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken); protected abstract SyntaxToken? GetInvocationNameToken(IEnumerable tokens); protected abstract AbstractFormattingRule GetCustomFormattingRule(Document document); @@ -63,7 +65,7 @@ public ExtractMethodResult ExtractMethod(OperationStatus initialStatus, Cancella return ExtractMethodResult.Fail(canAddStatus); cancellationToken.ThrowIfCancellationRequested(); - var codeGenerator = this.CreateCodeGenerator(analyzeResult); + var codeGenerator = this.CreateCodeGenerator(this.OriginalSelectionResult, analyzeResult); var statements = codeGenerator.GetNewMethodStatements(insertionPointNode, cancellationToken); if (statements.Status.Failed) @@ -73,18 +75,22 @@ public ExtractMethodResult ExtractMethod(OperationStatus initialStatus, Cancella status, async cancellationToken => { - var (analyzedDocument, insertionPoint) = await GetAnnotatedDocumentAndInsertionPointAsync( + var analyzedDocument = await GetAnnotatedDocumentAndInsertionPointAsync( originalSemanticDocument, analyzeResult, insertionPointNode, cancellationToken).ConfigureAwait(false); var triviaResult = await PreserveTriviaAsync(analyzedDocument.Root, cancellationToken).ConfigureAwait(false); cancellationToken.ThrowIfCancellationRequested(); - var generatedCode = await GenerateCodeAsync( - insertionPoint.With(triviaResult.SemanticDocument), - (SelectionResult)OriginalSelectionResult.With(triviaResult.SemanticDocument), - analyzeResult, - Options, - cancellationToken).ConfigureAwait(false); + var generator = this.CreateCodeGenerator( + OriginalSelectionResult.With(triviaResult.SemanticDocument), + analyzeResult); + + var generatedCode = await generator.GenerateAsync(cancellationToken).ConfigureAwait(false); + //GenerateCodeAsync( + // triviaResult.SemanticDocument, + // OriginalSelectionResult.With(triviaResult.SemanticDocument), + // analyzeResult, + // cancellationToken).ConfigureAwait(false); var afterTriviaRestored = await triviaResult.ApplyAsync(generatedCode, cancellationToken).ConfigureAwait(false); cancellationToken.ThrowIfCancellationRequested(); @@ -167,7 +173,7 @@ bool CanAddTo(Document document, SyntaxNode insertionPointNode, out OperationSta return (formattedDocument, finalInvocationNameToken == default ? null : finalInvocationNameToken); } - private static async Task<(SemanticDocument analyzedDocument, InsertionPoint insertionPoint)> GetAnnotatedDocumentAndInsertionPointAsync( + private static async Task GetAnnotatedDocumentAndInsertionPointAsync( SemanticDocument document, AnalyzerResult analyzeResult, SyntaxNode insertionPointNode, @@ -177,21 +183,18 @@ bool CanAddTo(Document document, SyntaxNode insertionPointNode, out OperationSta foreach (var variable in analyzeResult.Variables) variable.AddIdentifierTokenAnnotationPair(tokenMap, cancellationToken); - var insertionPointAnnotation = new SyntaxAnnotation(); - var finalRoot = document.Root.ReplaceSyntax( nodes: [insertionPointNode], // intentionally using 'n' (new) here. We want to see any updated sub tokens that were updated in computeReplacementToken - computeReplacementNode: (o, n) => n.WithAdditionalAnnotations(insertionPointAnnotation), + computeReplacementNode: (o, n) => n.WithAdditionalAnnotations(InsertionPointAnnotation), tokens: tokenMap.Keys, computeReplacementToken: (o, n) => o.WithAdditionalAnnotations(tokenMap[o]), trivia: null, computeReplacementTrivia: null); var finalDocument = await document.WithSyntaxRootAsync(finalRoot, cancellationToken).ConfigureAwait(false); - var insertionPoint = new InsertionPoint(finalDocument, insertionPointAnnotation); - return (finalDocument, insertionPoint); + return finalDocument; } private ImmutableArray GetFormattingRules(Document document) diff --git a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb index 937c5c9d61fbd..de8f167f9db7a 100644 --- a/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb +++ b/src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.vb @@ -17,8 +17,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod MyBase.New(result, options, localFunction:=False) End Sub - Protected Overrides Function CreateCodeGenerator(analyzerResult As AnalyzerResult) As CodeGenerator - Return VisualBasicCodeGenerator.Create(Me.OriginalSelectionResult, analyzerResult, Me.Options) + Protected Overrides Function CreateCodeGenerator(selectionResult As SelectionResult, analyzerResult As AnalyzerResult) As CodeGenerator + Return VisualBasicCodeGenerator.Create(selectionResult, analyzerResult, Me.Options) End Function Protected Overrides Function Analyze(cancellationToken As CancellationToken) As AnalyzerResult @@ -57,11 +57,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod result) End Function - Protected Overrides Async Function GenerateCodeAsync(insertionPoint As InsertionPoint, selectionResult As SelectionResult, analyzeResult As AnalyzerResult, options As ExtractMethodGenerationOptions, cancellationToken As CancellationToken) As Task(Of SemanticDocument) - Dim generator = VisualBasicCodeGenerator.Create(selectionResult, analyzeResult, options) - Return Await generator.GenerateAsync(insertionPoint, cancellationToken).ConfigureAwait(False) - End Function - Protected Overrides Function GetCustomFormattingRule(document As Document) As AbstractFormattingRule Return FormattingRule.Instance End Function From cf2eaf2672f5acb2ce02dfcbbe47c02a050f1a5f Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 17:41:25 -0800 Subject: [PATCH 30/32] delete --- src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs index cbe016def449b..be502c65e1b26 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs @@ -41,8 +41,6 @@ internal abstract partial class MethodExtractor( protected abstract Task PreserveTriviaAsync(SyntaxNode root, CancellationToken cancellationToken); protected abstract CodeGenerator CreateCodeGenerator(SelectionResult selectionResult, AnalyzerResult analyzerResult); - //protected abstract Task GenerateCodeAsync( - // SemanticDocument document, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken); protected abstract SyntaxToken? GetInvocationNameToken(IEnumerable tokens); protected abstract AbstractFormattingRule GetCustomFormattingRule(Document document); From 48d5703d92589664febd8634d4b3631faefca56b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 17:41:54 -0800 Subject: [PATCH 31/32] delete --- src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs index be502c65e1b26..3a85618330fe5 100644 --- a/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs +++ b/src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs @@ -82,13 +82,7 @@ public ExtractMethodResult ExtractMethod(OperationStatus initialStatus, Cancella var generator = this.CreateCodeGenerator( OriginalSelectionResult.With(triviaResult.SemanticDocument), analyzeResult); - var generatedCode = await generator.GenerateAsync(cancellationToken).ConfigureAwait(false); - //GenerateCodeAsync( - // triviaResult.SemanticDocument, - // OriginalSelectionResult.With(triviaResult.SemanticDocument), - // analyzeResult, - // cancellationToken).ConfigureAwait(false); var afterTriviaRestored = await triviaResult.ApplyAsync(generatedCode, cancellationToken).ConfigureAwait(false); cancellationToken.ThrowIfCancellationRequested(); From a5fe5d15d69058f3dabadc02fc30e712099432af Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 3 Jan 2025 18:04:04 -0800 Subject: [PATCH 32/32] Siplify --- .../Portable/ExtractMethod/SelectionResult.cs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs index 04cf65466d1e4..fff4998dbcc29 100644 --- a/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs +++ b/src/Features/Core/Portable/ExtractMethod/SelectionResult.cs @@ -229,20 +229,16 @@ public bool IsEndOfSelectionReachable() /// private (TExecutableStatementSyntax firstStatement, TExecutableStatementSyntax lastStatement) GetFlowAnalysisNodeRange() { - var first = this.GetFirstStatement(); - var last = this.GetLastStatement(); - - // single statement case - if (first == last || - first.Span.Contains(last.Span)) + if (this.IsExtractMethodOnSingleStatement) { + var first = this.GetFirstStatement(); return (first, first); } - - // multiple statement case - var firstUnderContainer = this.GetFirstStatementUnderContainer(); - var lastUnderContainer = this.GetLastStatementUnderContainer(); - return (firstUnderContainer, lastUnderContainer); + else + { + // multiple statement case + return (this.GetFirstStatementUnderContainer(), this.GetLastStatementUnderContainer()); + } } ///