Continued Extract method cleanup #76577

CyrusNajmabadi · 2024-12-29T03:27:13Z

Followup to #76550

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

src/EditorFeatures/CSharpTest/ExtractMethod/SelectionValidatorTests.cs

@@ -1520,7 +1520,7 @@ public async Task AnonymousTypeMember1()
    {
        var code = """
            using System;
-            class C { void M() { {|r:var x = new { {|b:String|} = true }; |}} }
+            class C { void M() { {|r:var x = new { {|b:String|} = true };|} } }


note: the test previously were non-functional. they were not actually validating that the spans unless a special bit was set on a reseult avlue (which wasn't computed properly). I also remove this bit in a follow up pr because the whole "feature adds special out of band data, for tests to validate, but which doesn't impactthe feature itself" is an awful paradigm.

CyrusNajmabadi · 2025-01-02T16:13:23Z

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

@@ -16,16 +16,13 @@ namespace Microsoft.CodeAnalysis.CSharp.ExtractMethod;
 [method: ImportingConstructor]
 [method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
 internal sealed partial class CSharpExtractMethodService() : AbstractExtractMethodService<
-    CSharpExtractMethodService.CSharpSelectionValidator,
-    CSharpExtractMethodService.CSharpMethodExtractor,
-    CSharpExtractMethodService.CSharpSelectionResult,


got rid of theese generics. they are not needed, and just add type system complexity.

CyrusNajmabadi · 2025-01-02T16:13:38Z

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

-            {
-                var analyzer = new CSharpAnalyzer(selectionResult, localFunction, cancellationToken);
-                return analyzer.Analyze();
-            }


inlined these static construction methods at callsite.

CyrusNajmabadi · 2025-01-02T16:14:45Z

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

@@ -202,9 +190,25 @@ protected override SyntaxNode GetCallSiteContainerFromOutermostMoveInVariable(Ca
                return declStatement.Parent;
            }

+            private bool ShouldPutUnsafeModifier()


this used to be on the strongly typed CSharpXXX types, but were only ever called from a single location (here). so instead of needing all the generic type system wonkyness to pass in one of these subtypes, just to call special methods (not on the base type), i just moved the mtehods to the places that use them. simple, easy.

CyrusNajmabadi · 2025-01-02T16:15:06Z

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

-                var root = selectionResult.SemanticDocument.Root;
-                var result = preservationService.SaveTriviaAroundSelection(root, selectionResult.FinalSpan);
-                return new CSharpTriviaResult(
-                    await selectionResult.SemanticDocument.WithSyntaxRootAsync(result.Root, cancellationToken).ConfigureAwait(false),


static factories inlined to callsite.

CyrusNajmabadi · 2025-01-02T16:15:14Z

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

-            }
-
-            private CSharpTriviaResult(SemanticDocument document, ITriviaSavedResult result)
-                : base(document, result, (int)SyntaxKind.EndOfLineTrivia, (int)SyntaxKind.WhitespaceTrivia)


primary constructorified.

CyrusNajmabadi · 2025-01-02T16:16:19Z

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


        protected override SyntaxNode GetInsertionPointNode(
            AnalyzerResult analyzerResult, CancellationToken cancellationToken)
        {
-            var originalSpanStart = OriginalSelectionResult.OriginalSpan.Start;
+            var originalSpanStart = OriginalSelectionResult.FinalSpan.Start;


unified a lot of the behavior around which pieces of data we use. the user's original selection, or what we updated the selection to.

Note: the followup PR on this cleans this up much further, but separating out this data into two types so that the feature itself is very clear about if it's using the pre/post selection and why.

CyrusNajmabadi · 2025-01-02T16:16:53Z

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

@@ -24,10 +24,9 @@ internal abstract partial class CSharpSelectionResult
        private sealed class ExpressionResult(
            SemanticDocument document,
            SelectionType selectionType,
-            TextSpan originalSpan,


remove this. this should only be the starting info from the user. we don't want to pass this along as it's actively confusing as to when you'd use this over 'final span'.

CyrusNajmabadi · 2025-01-02T16:17:06Z

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

+        public override bool ContainsNonReturnExitPointsStatements(ImmutableArray<SyntaxNode> jumpsOutOfRegion)
+            => jumpsOutOfRegion.Any(n => n is not ReturnStatementSyntax);
+
+        public override ImmutableArray<StatementSyntax> GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray<SyntaxNode> jumpsOutOfRegion)


just a move.

CyrusNajmabadi · 2025-01-02T16:17:32Z

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

-            }
-
-            return false;
-        }


moved to common location. uses IBLockFacts to unify logic for C#/vb.

CyrusNajmabadi · 2025-01-02T16:18:03Z

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

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


we used to passa a lot of instance-state data to helper instance methods. i cleaned that up to access off of 'this' instead.

CyrusNajmabadi · 2025-01-02T16:19:11Z

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

-
-                var (firstStatement, lastStatement) = this.SelectionResult.GetFlowAnalysisNodeRange();
-                return this.SemanticModel.AnalyzeDataFlow(firstStatement, lastStatement);
-            }


a high level goal was to stop exposing the 'start/end nodes' and having callers then do things like AnalyzeDataFlow/AnalyzeControlFlow. By the end of hte cleanup, the nodes are not exposed, and that data is computed once and cached for all callers to use.

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

CyrusNajmabadi · 2025-01-02T16:20:29Z

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

@@ -226,5 +265,49 @@ protected static SyntaxNode AddAnnotations(SyntaxNode root, IEnumerable<(SyntaxN
            var tokenMap = pairs.GroupBy(p => p.Item1, p => p.Item2).ToDictionary(g => g.Key, g => g.ToArray());
            return root.ReplaceNodes(tokenMap.Keys, (o, n) => o.WithAdditionalAnnotations(tokenMap[o]));
        }
+
+        public OperationStatus ValidateSelectionResult(CancellationToken cancellationToken)


trying to move common logic when i can between c# and vb to a common location.

CyrusNajmabadi · 2025-01-02T16:20:36Z

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

+            return OperationStatus.SucceededStatus;
+        }
+
+        protected bool IsFinalSpanSemanticallyValidSpan(CancellationToken cancellationToken)


this is a move.

CyrusNajmabadi · 2025-01-02T16:21:04Z

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

            if (selectionInfo.Status.Failed)
                return (null, selectionInfo.Status);

+            TStatementSyntax? firstStatement = null;


this will get more cleaned up in followup. but the idea here is again just pulling logic up from C#/VB to common location.

CyrusNajmabadi · 2025-01-02T16:21:42Z

...es/VisualBasic/Portable/ExtractMethod/VisualBasicMethodExtractor.VisualBasicCodeGenerator.vb

@@ -158,12 +149,28 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ExtractMethod
                    Return declStatement.Parent
                End Function

+                Private Function IsUnderModuleBlock() As Boolean


vb specific helpers that were on VBSelectionResult, like with C#, these were called in one lcation. so i just moved the helpers right to that location.

CyrusNajmabadi · 2025-01-02T16:22:50Z

@JoeRobich @ToddGrun ptal. thanks :)

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

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

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

ToddGrun · 2025-01-02T19:07:11Z

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

@@ -316,18 +313,19 @@ protected ImmutableArray<TStatementSyntax> AppendReturnStatementIfNeeded(Immutab
            protected static HashSet<SyntaxAnnotation> CreateVariableDeclarationToRemoveMap(
                IEnumerable<VariableInfo> variables, CancellationToken cancellationToken)
            {
-                var annotations = new List<(SyntaxToken, SyntaxAnnotation)>();
+                var annotations = new MultiDictionary<SyntaxToken, SyntaxAnnotation>();


MultiDictionary

Need more coffee, what's the advantage of MultiDictionary in this case?

Oh, I see, MethodExtractor.GetAnnotatedDocumentAndInsertionPointAsync is simplified by changing AddIdentifierTokenAnnotationPair to use a MultiDictionary

It just more accurately reflects what the code was doing. It was building a list of K/V pairs, then later flattening to an IGrouping (or ILookup) of K to many Vs.

ToddGrun

CyrusNajmabadi added 4 commits December 28, 2024 18:27

Shuffle a few things

988a92e

Call the right overload

9a212ad

Simplify further

d2b41d1

Simplify further

412e22a

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

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

CyrusNajmabadi added 15 commits December 29, 2024 09:05

Remove use of cloning

67a613e

Simplify annotations

b297641

simplify

4c4c7dd

Simplify generics

9578118

Simplify generics

2893412

Simplify generics

da25b2f

Move helpers

b8addf2

move data flow analysis

cbc7ca8

simplify

40f32b8

Make private

f1e3461

Cleanup

652a504

Remove use of originalspan

e00bb87

remove unneeded data

432ba07

delete

c9f7a0c

Share code

d86f8a3

CyrusNajmabadi mentioned this pull request Dec 30, 2024

Continued Extract method cleanup (part 2) #76585

Merged

Merge branch 'main' into extractMethodCleanup2

f16cf64

CyrusNajmabadi marked this pull request as ready for review January 2, 2025 16:11

CyrusNajmabadi requested a review from a team as a code owner January 2, 2025 16:11

CyrusNajmabadi commented Jan 2, 2025

View reviewed changes

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

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

2b8801a

CyrusNajmabadi commented Jan 2, 2025

View reviewed changes

CyrusNajmabadi requested review from JoeRobich and ToddGrun January 2, 2025 16:22

ToddGrun reviewed Jan 2, 2025

View reviewed changes

src/Features/Core/Portable/ExtractMethod/SelectionResult.cs Outdated Show resolved Hide resolved

JoeRobich approved these changes Jan 2, 2025

View reviewed changes

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

CyrusNajmabadi and others added 2 commits January 2, 2025 10:24

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

cc6848f

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

Simplify

f11598a

CyrusNajmabadi enabled auto-merge January 2, 2025 18:26

ToddGrun reviewed Jan 2, 2025

View reviewed changes

ToddGrun approved these changes Jan 2, 2025

View reviewed changes

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

CyrusNajmabadi added a commit that referenced this pull request Jan 2, 2025

Continued Extract method cleanup (#76577)

ccbc092

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

CyrusNajmabadi deleted the extractMethodCleanup2 branch January 2, 2025 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continued Extract method cleanup #76577

Continued Extract method cleanup #76577

CyrusNajmabadi commented Dec 29, 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 Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

CyrusNajmabadi commented Jan 2, 2025

ToddGrun Jan 2, 2025

ToddGrun Jan 2, 2025

CyrusNajmabadi Jan 2, 2025

ToddGrun left a comment

Continued Extract method cleanup #76577

Continued Extract method cleanup #76577

Conversation

CyrusNajmabadi commented Dec 29, 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

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

ToddGrun left a comment

Choose a reason for hiding this comment