Continued Extract method cleanup (part 2) #76585

CyrusNajmabadi · 2024-12-30T19:32:30Z

Followup to #76577.

CyrusNajmabadi · 2025-01-02T20:55:24Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs

@@ -45,7 +46,7 @@ public override SyntaxNode GetContainingScope()
                return CSharpSyntaxFacts.Instance.GetRootStandaloneExpression(scope);
            }

-            public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnType()
+            public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken)


renamed for clarity.

CyrusNajmabadi · 2025-01-02T20:56:33Z

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

@@ -32,7 +32,7 @@ internal abstract partial class CSharpSelectionResult(
    {
        public static async Task<CSharpSelectionResult> CreateAsync(
            SemanticDocument document,
-            SelectionInfo selectionInfo,
+            FinalSelectionInfo selectionInfo,


broke this into two type InitialSelectionInfo and FinalSelectionInfo so that we can make the variables clearer and so that later parts of EM don't use data they shouldn't.

CyrusNajmabadi · 2025-01-02T20:56:58Z

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

@@ -210,37 +210,23 @@ public override ImmutableArray<StatementSyntax> GetOuterReturnStatements(SyntaxN
        }

        public override bool IsFinalSpanSemanticallyValidSpan(
-            TextSpan textSpan, ImmutableArray<StatementSyntax> returnStatements, CancellationToken cancellationToken)


very odd use of passing spans of selections around, when we have the actual data we need in nodes that we can use directly.

CyrusNajmabadi · 2025-01-02T20:57:54Z

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

@@ -25,29 +24,73 @@ internal sealed partial class CSharpSelectionValidator(
    {
        private readonly bool _localFunction = localFunction;

-        protected override SelectionInfo UpdateSelectionInfo(
-            SelectionInfo selectionInfo, StatementSyntax? firstStatement, StatementSyntax? lastStatement, CancellationToken cancellationToken)
+        protected override InitialSelectionInfo GetInitialSelectionInfo(CancellationToken cancellationToken)


moved up from teh bottom, but adjusted heavily for simplicity.

CyrusNajmabadi · 2025-01-02T20:58:31Z

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

-                    Status = new OperationStatus(succeeded: false, FeaturesResources.Selection_does_not_contain_a_valid_token),
-                    FirstTokenInOriginalSpan = firstTokenInSelection,
-                    LastTokenInOriginalSpan = lastTokenInSelection
-                };


instead of having the moved code have this pattern (where it repeats the failure case multiple times), instead it just computes the failure code, and then if that happens, it creates the infla failure object once.

CyrusNajmabadi · 2025-01-02T20:59:13Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

-                }
-
-                return isInExpressionOrHasReturnStatement;
-            }


worlds most convoluted || expression.

CyrusNajmabadi · 2025-01-02T20:59:36Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

@@ -670,7 +663,7 @@ private bool SelectionContainsOnlyIdentifierWithSameType(ITypeSymbol type)
                    return false;
                }

-                return type.Equals(SelectionResult.GetContainingScopeType());
+                return type.Equals(SelectionResult.GetReturnType(this.CancellationToken));


GetContainingScopeType was a terribly confusing name. it's now just called GetReturnType, which defers to GetReturnTypeInfo

CyrusNajmabadi · 2025-01-02T21:00:35Z

src/Features/Core/Portable/ExtractMethod/SelectionInfo.cs


        public SyntaxToken FirstTokenInFinalSpan { get; init; }
        public SyntaxToken LastTokenInFinalSpan { get; init; }

        public bool SelectionInExpression { get; init; }
-        public bool SelectionInSingleStatement { get; init; }


this doesn't need to be passed in. it can be determiend dynamically within this type. this also fixed an issue where the caller computed this in a different fashion from how this type computed it (a common issue in tnhe extract method codebase).

CyrusNajmabadi · 2025-01-02T21:00:46Z

src/Features/Core/Portable/ExtractMethod/SelectionInfo.cs

@@ -67,7 +103,7 @@ public SelectionType GetSelectionType()

            var firstStatement = this.FirstTokenInFinalSpan.GetRequiredAncestor<TExecutableStatementSyntax>();
            var lastStatement = this.LastTokenInFinalSpan.GetRequiredAncestor<TExecutableStatementSyntax>();
-            if (firstStatement == lastStatement || firstStatement.Span.Contains(lastStatement.Span))
+            if (firstStatement.Span.Contains(lastStatement.Span))


the latter check subsumes the former one.

CyrusNajmabadi · 2025-01-02T21:01:14Z

src/Features/Core/Portable/ExtractMethod/SelectionValidator.cs

@@ -105,15 +90,58 @@ private static bool IsValidStatementRange(
            return firstStatement != null && lastStatement != null;
        }

+        protected FinalSelectionInfo AssignFinalSpan(
+             InitialSelectionInfo initialSelectionInfo, FinalSelectionInfo finalSelectionInfo)
+        {


pulled up from C# and VB.

CyrusNajmabadi · 2025-01-02T21:01:23Z

src/Features/Core/Portable/ExtractMethod/SelectionValidator.cs

+            bool selectionInExpression,
+            SyntaxToken firstTokenInOriginalSpan,
+            SyntaxToken lastTokenInOriginalSpan,
+            CancellationToken cancellationToken)


pulled up from C# and VB.

CyrusNajmabadi · 2025-01-02T21:01:55Z

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionResult.vb

@@ -347,14 +349,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod
                End If

                ' check whether selection reaches the end of the container
-                Dim lastToken = Me.SemanticDocument.Root.FindToken(textSpan.End)
-                If lastToken.Kind = SyntaxKind.None Then


things that can't happen due to tree invariants.

CyrusNajmabadi · 2025-01-02T21:02:11Z

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionValidator.vb

@@ -18,36 +18,86 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod
                MyBase.New(document, textSpan)
            End Sub

-            Protected Overrides Function UpdateSelectionInfo(selectionInfo As SelectionInfo, firstStatement As StatementSyntax, lastStatement As StatementSyntax, cancellationToken As CancellationToken) As SelectionInfo
+            Protected Overrides Function GetInitialSelectionInfo(cancellationToken As CancellationToken) As InitialSelectionInfo
+                Dim root = Me.SemanticDocument.Root


simiklar to c#, this moved up from teh bottom.

CyrusNajmabadi · 2025-01-02T21:05:41Z

@JoeRobich @ToddGrun ptal. thanks! just one more cleanup pass after htis one.

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs

CyrusNajmabadi · 2025-01-02T21:10:18Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs

@@ -70,27 +66,22 @@ public override (ITypeSymbol returnType, bool returnsByRef) GetReturnType()
                switch (node)
                {
                    case AccessorDeclarationSyntax access:
-                        // property or event case
-                        if (access.Parent == null || access.Parent.Parent == null)


remove code that can't happen.

CyrusNajmabadi · 2025-01-02T21:10:53Z

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


-            var lastToken = this.SemanticDocument.Root.FindToken(textSpan.End);


very strange code that would recompute info already computed in earlier stages.

CyrusNajmabadi · 2025-01-02T21:11:05Z

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


-            var lastToken = this.SemanticDocument.Root.FindToken(textSpan.End);
-            if (lastToken.Kind() == SyntaxKind.None)


not possible with syntax invariants.

CyrusNajmabadi · 2025-01-02T21:11:51Z

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

+
+            var statusReason = CheckSpan();
+            if (statusReason is not null)
+                return InitialSelectionInfo.Failure(statusReason);


simpler approach of just computing failure reason, then returning failure object in that one case, instead of instantiating failure object (verbosely) over and over again.

CyrusNajmabadi · 2025-01-02T21:12:01Z

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

+                return InitialSelectionInfo.Failure(statusReason);
+
+            return CreateInitialSelectionInfo(
+                selectionInExpression, firstTokenInSelection, lastTokenInSelection, cancellationToken);


call to shared code for VB and C#.

CyrusNajmabadi · 2025-01-02T21:13:06Z

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

+        }
+
+        protected override FinalSelectionInfo UpdateSelectionInfo(
+            InitialSelectionInfo initialSelectionInfo,


initialselection is now passed in data. FinalSelection is what is passed out. the domains are kept separated.

src/Features/Core/Portable/ExtractMethod/SelectionInfo.cs

ToddGrun · 2025-01-02T22:03:44Z

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

            selectionInfo = AdjustFinalTokensBasedOnContext(selectionInfo, model, cancellationToken);
-            selectionInfo = AssignFinalSpan(selectionInfo);
-            selectionInfo = ApplySpecialCases(selectionInfo, SemanticDocument.SyntaxTree.Options, _localFunction);
+            selectionInfo = AssignFinalSpan(initialSelectionInfo, selectionInfo);


initialSelectionInfo

Seems odd that this and the next line pass in initialSelectionInfo

never mind, selectionInfo is also passed in

they're doing some work comparing the initial to the final they've computed so far. (yes, odd).

ToddGrun

CyrusNajmabadi added 7 commits December 30, 2024 10:06

in progress

502768c

in progress

fa05f16

in progress

4035085

Share more code

547024c

Share code

c52bc5e

Switch to executable statements

cc913b5

Simplify

48bc3d2

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 30, 2024

dotnet-policy-service bot added the VSCode label Dec 30, 2024

CyrusNajmabadi added 2 commits December 30, 2024 11:41

in progress

4d1c7f5

Simplify api

751e1cc

CyrusNajmabadi mentioned this pull request Dec 30, 2024

Cleanup Extract method code (part 3) #76587

Merged

CyrusNajmabadi added 2 commits January 2, 2025 08:41

Merge branch 'extractMethodCleanup2' into extractMethodCleanup4

ed44ed0

Merge branch 'main' into extractMethodCleanup4

3ee94ca

CyrusNajmabadi commented Jan 2, 2025

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review January 2, 2025 21:04

CyrusNajmabadi requested a review from a team as a code owner January 2, 2025 21:04

CyrusNajmabadi requested review from JoeRobich and ToddGrun January 2, 2025 21:05

CyrusNajmabadi commented Jan 2, 2025

View reviewed changes

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs Show resolved Hide resolved

CyrusNajmabadi commented Jan 2, 2025

View reviewed changes

CyrusNajmabadi enabled auto-merge January 2, 2025 21:16

ToddGrun reviewed Jan 2, 2025

View reviewed changes

src/Features/Core/Portable/ExtractMethod/SelectionInfo.cs Show resolved Hide resolved

CyrusNajmabadi requested a review from ToddGrun January 2, 2025 21:47

ToddGrun reviewed Jan 2, 2025

View reviewed changes

ToddGrun approved these changes Jan 2, 2025

View reviewed changes

CyrusNajmabadi merged commit 6b25538 into dotnet:main Jan 2, 2025
25 checks passed

dotnet-policy-service bot added this to the Next milestone Jan 2, 2025

CyrusNajmabadi deleted the extractMethodCleanup4 branch January 2, 2025 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continued Extract method cleanup (part 2) #76585

Continued Extract method cleanup (part 2) #76585

CyrusNajmabadi commented Dec 30, 2024

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi commented Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

ToddGrun Jan 2, 2025

ToddGrun Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

ToddGrun left a comment


		var lastToken = this.SemanticDocument.Root.FindToken(textSpan.End);


		var lastToken = this.SemanticDocument.Root.FindToken(textSpan.End);
		if (lastToken.Kind() == SyntaxKind.None)

Continued Extract method cleanup (part 2) #76585

Continued Extract method cleanup (part 2) #76585

Conversation

CyrusNajmabadi commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment