Defer extract method mutation computation till the point the code action is invoked. #70477

CyrusNajmabadi · 2023-10-20T01:37:36Z

Followup to #70466.

Follow-up to #57838. The earlier work deferred only the final formatting and simplification work. This pull request defers additional work on computing the extracted method contents itself.

CyrusNajmabadi · 2023-10-20T14:51:58Z

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

-                var codeGenerator = Create(insertionPoint, selectionResult, analyzerResult, options, localFunction);
-                return codeGenerator.GenerateAsync(cancellationToken);
+                var codeGenerator = Create(selectionResult, analyzerResult, options, localFunction);
+                return codeGenerator.GenerateAsync(insertionPoint, cancellationToken);


insertionpoint is expensive (it forks the document). so it is now passed in as an arg, and only in the "actually create the new method" codepath.

CyrusNajmabadi · 2023-10-20T14:53:02Z

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

+                var statements = CreateMethodBody(insertionPointNode, cancellationToken);
+                var status = CheckActiveStatements(statements);
+                return status.With(statements.CastArray<SyntaxNode>());
+            }


code refactoring has to still make the new statements, to do some final applicability checks.

we migth be able to optimize this if it's still expensive to do less here in the codepath where we're just querying for applicability.

CyrusNajmabadi · 2023-10-20T14:54:37Z

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

            }

            protected override async Task<SyntaxNode> GenerateBodyForCallSiteContainerAsync(
+                SyntaxNode context,


this is the same as SyntaxNode insertionPointNode. but i acll it context in some places because that's the original name in the code. i'll fix up to make this all consistent in followup.

renamed in mop up.

CyrusNajmabadi · 2023-10-20T14:55:30Z

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


-                if (statements.Skip(1).Any())
+                if (statements.Length != 1)


this is intentionally not the same as before. the 0 and >=2 cases were handled the same way in the prior code in two separate branches. i merged them into one branch here.

CyrusNajmabadi · 2023-10-20T14:56:23Z

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

@@ -673,28 +666,26 @@ protected override StatementSyntax CreateAssignmentExpressionStatement(SyntaxTok
                    .WithUsingKeyword(usingKeyword);
            }

-            protected override async Task<GeneratedCode> CreateGeneratedCodeAsync(OperationStatus status, SemanticDocument newDocument, CancellationToken cancellationToken)
+            protected override async Task<GeneratedCode> CreateGeneratedCodeAsync(


actual generation of code no long involves 'status' as there's nothing to report once we've decided to generte.

CyrusNajmabadi · 2023-10-20T14:57:00Z

...Core/Portable/CodeRefactorings/ExtractMethod/AbstractExtractMethodCodeRefactoringProvider.cs


-            var actions = await GetCodeActionsAsync(document, textSpan, extractOptions, cleanupOptions, cancellationToken).ConfigureAwait(false);
+            var actions = await GetCodeActionsAsync(document, textSpan, extractOptions, cancellationToken).ConfigureAwait(false);


the cleanupoptinos are part of the extractoptions now.

CyrusNajmabadi · 2023-10-20T14:57:24Z

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


 namespace Microsoft.CodeAnalysis.ExtractMethod
 {
-    internal abstract class ExtractMethodResult
+    internal sealed class ExtractMethodResult


no subclassing here. just a simple result type.

CyrusNajmabadi · 2023-10-20T14:57:39Z

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

-        /// The name token for the invocation node that replaces the extracted code.
-        /// </summary>
-        public SyntaxToken? InvocationNameToken { get; }
+        private readonly AsyncLazy<(Document document, SyntaxToken? invocationNameToken)>? _lazyData;


computation fo the final doc, and token to rename, is done lazily and cached here.

CyrusNajmabadi · 2023-10-20T14:58:15Z

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

-
-            var annotation = new SyntaxAnnotation();
-
-            var root = await DocumentWithoutFinalFormatting.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);


all this logic moved inside MethodExtractor as the last thing it does in the lazy async callback it passes to the result object.

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

CyrusNajmabadi · 2023-10-20T14:59:53Z

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

-            cancellationToken.ThrowIfCancellationRequested();
+            return ExtractMethodResult.Success(
+                operationStatus,
+                async cancellationToken =>


this is the crux of hte change. first, we do frontload some statement checks, since tehy are required to know if we're showing the EM option or not. but everything after that moves into this lazy computation in the success object.

CyrusNajmabadi · 2023-10-20T15:00:03Z

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

-                GetFormattingRules(documentWithoutFinalFormatting),
-                generatedCode.MethodNameAnnotation,
-                generatedCode.MethodDefinitionAnnotation,
-                cancellationToken).ConfigureAwait(false);


inlined this method.

CyrusNajmabadi · 2023-10-20T15:00:23Z

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

+            Document document,
+            SyntaxToken? invocationNameToken,
+            CancellationToken cancellationToken)
+        {


this was the code that used to be in teh result type.

CyrusNajmabadi · 2023-10-20T15:00:46Z

src/Features/LanguageServer/Protocol/Features/Options/ExtractMethodOptionsStorage.cs

            ExtractOptions = globalOptions.GetExtractMethodOptions(languageServices.Language),
-            AddImportOptions = globalOptions.GetAddImportPlacementOptions(languageServices),
-            LineFormattingOptions = globalOptions.GetLineFormattingOptions(languageServices.Language)


the last two options are redundant. CodeCleanupOptions exposes it.

CyrusNajmabadi · 2023-10-20T15:01:27Z

@ToddGrun this is ready for review. A mop-up PR is coming after this with more code sharing, and fixing of nits.

ToddGrun · 2023-10-20T15:48:55Z

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

-            var (analyzedDocument, insertionPoint) = await GetAnnotatedDocumentAndInsertionPointAsync(
-                originalSemanticDocument, analyzeResult, insertionPointNode, cancellationToken).ConfigureAwait(false);
+            var statements = codeGenerator.GetNewMethodStatements(insertionPointNode, cancellationToken);
+            if (statements.Status.Failed())


if (statements.Status.Failed())

General question: Does the HasRefactoringsAsync call into this still need to call in for both local/method or are they going to give the same result?

they're different. you can be offered one, or the other, or both.

HasRefactoring would still be good though as we could stop once we know about one.

ToddGrun · 2023-10-20T16:01:43Z

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

@@ -776,7 +767,7 @@ static bool ReturnOperationBelongsToMethod(SyntaxNode returnOperationSyntax, Syn

                SemanticDocument CheckReturnOperations(
                    SyntaxNode node,
-                    OperationStatus<IMethodSymbol> methodSymbolResult,
+                    IMethodSymbol methodSymbol,


IMethodSymbol methodSymbol,

not seeing this used

can remove if that's the case :)

intresting. thsi wasn't used. @mavasani a potential bug with the unused parameter analyzer? note: this is in a local function.

ToddGrun

CyrusNajmabadi added 3 commits October 19, 2023 17:52

Simplify construction

af9bb14

defer computation

de4391c

defer computation

146d67e

CyrusNajmabadi requested a review from a team as a code owner October 20, 2023 01:37

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 20, 2023

CyrusNajmabadi and others added 7 commits October 19, 2023 19:04

in progrss

890d47f

in progrss

7160967

vb and c# sides

554b608

Working

33b9c11

simplify

515ed53

simplify

472228f

Merge branch 'main' into emPerf3

2fa8681

CyrusNajmabadi changed the title ~~WIP: Defer extract method computation.~~ Defer extract method mutation computation till the point the code action is invoked. Oct 20, 2023

CyrusNajmabadi commented Oct 20, 2023

View reviewed changes

src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs Show resolved Hide resolved

CyrusNajmabadi commented Oct 20, 2023

View reviewed changes

src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs Show resolved Hide resolved

CyrusNajmabadi commented Oct 20, 2023

View reviewed changes

src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs Show resolved Hide resolved

CyrusNajmabadi commented Oct 20, 2023

View reviewed changes

ToddGrun reviewed Oct 20, 2023

View reviewed changes

ToddGrun approved these changes Oct 20, 2023

View reviewed changes

CyrusNajmabadi enabled auto-merge October 20, 2023 16:10

CyrusNajmabadi merged commit d4a970d into dotnet:main Oct 20, 2023
23 of 24 checks passed

ghost added this to the Next milestone Oct 20, 2023

CyrusNajmabadi deleted the emPerf3 branch October 20, 2023 16:33

allisonchou mentioned this pull request Oct 24, 2023

[Automated] PRs inserted in VS build main-34224.107 #70544

Closed

jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defer extract method mutation computation till the point the code action is invoked. #70477

Defer extract method mutation computation till the point the code action is invoked. #70477

CyrusNajmabadi commented Oct 20, 2023 •

edited by sharwell

Loading

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi commented Oct 20, 2023

ToddGrun Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

ToddGrun Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

CyrusNajmabadi Oct 20, 2023

ToddGrun left a comment


		var actions = await GetCodeActionsAsync(document, textSpan, extractOptions, cleanupOptions, cancellationToken).ConfigureAwait(false);
		var actions = await GetCodeActionsAsync(document, textSpan, extractOptions, cancellationToken).ConfigureAwait(false);


		var annotation = new SyntaxAnnotation();

		var root = await DocumentWithoutFinalFormatting.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);

Defer extract method mutation computation till the point the code action is invoked. #70477

Defer extract method mutation computation till the point the code action is invoked. #70477

Conversation

CyrusNajmabadi commented Oct 20, 2023 • edited by sharwell Loading

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 Oct 20, 2023

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

CyrusNajmabadi commented Oct 20, 2023 •

edited by sharwell

Loading