Update Extract Method to support top-level statements by sharwell · Pull Request #44453 · dotnet/roslyn

sharwell · 2020-05-20T23:13:17Z

CyrusNajmabadi · 2020-05-20T23:34:13Z

src/Workspaces/CSharp/Portable/CodeGeneration/CSharpCodeGenerationService.cs

this block i don't quite understand. what's the case where the destinatino member is a statement itself? who is adding statements to a statement itself?

Top-level statements of a script are not part of a statement container. In this case, we have to create the container and move the original statement into it along with the new statements.

can you give a concrete example of the type of statement we are adding, and the statement it is being added to? it's not clear to me.

This behavior supports the following test:

roslyn/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractLocalFunctionTests.cs

Lines 4790 to 4808 in 6573bd1

[WorkItem(44260, "https://github.com/dotnet/roslyn/issues/44260")]

[Fact, Trait(Traits.Feature, Traits.Features.ExtractMethod)]

public async Task TopLevelStatement_ArgumentInInvocation_InInteractive()

{

var code = @"

System.Console.WriteLine([|""string""|]);

";

var expected =

@"{

System.Console.WriteLine({|Rename:NewMethod|}());

static string NewMethod()

{

return ""string"";

}

}";

await TestAsync(code, expected, TestOptions.Script.WithLanguageVersion(LanguageVersionExtensions.CSharp9), index: CodeActionIndex);

}

If we didn't wrap the generated code in a block, the generated function would be a method declaration as opposed to a local function declaration.

would it make sense to disable extract-local-function here instead? this really doesn't seem like a desirable change to me. Plus, the addition of the block seems like it can actually screw things up. For example, what if we had var v = [|1 + 1|];. Would we make a block that only contained the variable and the local-func, causing references to 'v' to not work?

It would make sense to disable, but that was a bit more complicated and only impacts script scenarios. I'll file a follow-up issue for it.

Fixes dotnet#44260

sharwell · 2020-06-10T03:12:29Z

@CyrusNajmabadi for review

mavasani · 2020-06-12T16:01:06Z

src/EditorFeatures/CSharpTest/ExtractMethod/ExtractMethodTests.cs

-}";
+}
+
+local = NewMethod();


Why did this ordering change? Should we offer 2 fixes here: 1. to insert in-place and 2. to insert at end of the current block?

This scenario is only valid for C# scripting, so I'm not sure it matters. It would be covered by https://github.com/dotnet/roslyn/pull/44453/files#r428859532.

It's going to be much easier to deal with these details when the tests are converted to the new test library.

sharwell · 2020-09-14T03:22:21Z

@CyrusNajmabadi let me know if you think this one is good to merge

CyrusNajmabadi · 2020-09-14T03:58:55Z

src/Workspaces/CSharp/Portable/CodeGeneration/CSharpCodeGenerationService.cs

            {
                return (accessorDeclaration.Body == null) ? destinationMember : Cast<TDeclarationNode>(accessorDeclaration.AddBodyStatements(StatementGenerator.GenerateStatements(statements).ToArray()));
            }
+            else if (destinationMember is CompilationUnitSyntax compilationUnit && options is null)


why the check for options is null?

This method had an existing caller that passed in CompilationUnitSyntax with options, and relied on the AddStatementsWorker fallback to handle those options. Right now I forget what caller that was.

➡️ Will submit a follow-up to clarify the condition.

CyrusNajmabadi · 2020-09-14T03:59:54Z

src/Workspaces/CSharp/Portable/CodeGeneration/CSharpCodeGenerationService.cs

+            else if (destinationMember is CompilationUnitSyntax compilationUnit && options is null)
+            {
+                // Insert the new global statement(s) at the end of any current global statements
+                var insertionIndex = compilationUnit.Members.LastIndexOf(memberDeclaration => memberDeclaration.IsKind(SyntaxKind.GlobalStatement)) + 1;


i would love it we doc'ed that LastIndexOf returns -1 on failure. that would make me feel much safer about this code :)

➡️ Will submit a follow-up to clarify.

CyrusNajmabadi · 2020-09-14T04:00:42Z

src/Workspaces/CSharp/Portable/CodeGeneration/CSharpCodeGenerationService.cs

+            {
+                // Insert the new global statement(s) at the end of any current global statements
+                var insertionIndex = compilationUnit.Members.LastIndexOf(memberDeclaration => memberDeclaration.IsKind(SyntaxKind.GlobalStatement)) + 1;
+                var wrappedStatements = StatementGenerator.GenerateStatements(statements).Select(generated => SyntaxFactory.GlobalStatement(generated)).ToArray();


.ToArray seems unnecessary. though it looks like there's a lot of that in this mtehod :)

Partially pattern matching with the rest of the method, but also easier to deal with when reading and debugging through.

CyrusNajmabadi

Overall i'm ok with this. A couple of tiny clarity questions. but they're not blocking (unless hte answers are very bizarre :))

See dotnet#44453

sharwell requested a review from a team as a code owner May 20, 2020 23:13

CyrusNajmabadi reviewed May 20, 2020

View reviewed changes

sharwell force-pushed the top-extract-method branch from df672de to 6573bd1 Compare May 21, 2020 00:27

Update Extract Method to support top-level statements

99397da

Fixes dotnet#44260

sharwell force-pushed the top-extract-method branch from 6573bd1 to 99397da Compare May 21, 2020 19:13

sharwell changed the base branch from features/SimplePrograms to master June 9, 2020 15:43

sharwell added the Area-IDE label Jun 12, 2020

mavasani reviewed Jun 12, 2020

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into extract-method

6f34b47

CyrusNajmabadi reviewed Sep 14, 2020

View reviewed changes

CyrusNajmabadi approved these changes Sep 14, 2020

View reviewed changes

sharwell merged commit 0457da1 into dotnet:master Sep 19, 2020

ghost added this to the Next milestone Sep 19, 2020

sharwell deleted the top-extract-method branch September 19, 2020 20:21

sharwell added a commit to sharwell/roslyn that referenced this pull request Sep 20, 2020

Improve comments for top-level statement insertion

ee2f290

See dotnet#44453

sharwell mentioned this pull request Sep 20, 2020

Improve comments for top-level statement insertion #47877

Merged

dibarbet modified the milestones: Next, 16.8.P4 Sep 21, 2020

timheuer mentioned this pull request Oct 11, 2021

Extract method fails on top-level programs #56969

Closed

	[WorkItem(44260, "https://github.com/dotnet/roslyn/issues/44260")]
	[Fact, Trait(Traits.Feature, Traits.Features.ExtractMethod)]
	public async Task TopLevelStatement_ArgumentInInvocation_InInteractive()
	{
	var code = @"
	System.Console.WriteLine([\|""string""\|]);
	";
	var expected =
	@"{
	System.Console.WriteLine({\|Rename:NewMethod\|}());

	static string NewMethod()
	{
	return ""string"";
	}
	}";

	await TestAsync(code, expected, TestOptions.Script.WithLanguageVersion(LanguageVersionExtensions.CSharp9), index: CodeActionIndex);
	}

Conversation

sharwell commented May 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Jun 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants