Introduce using statement #30106

jnm2 · 2018-09-22T23:34:21Z

/cc @CyrusNajmabadi

...e/Portable/IntroduceUsingStatement/AbstractIntroduceUsingStatementCodeRefactoringProvider.cs

...arp/Portable/IntroduceUsingStatement/CSharpIntroduceUsingStatementCodeRefactoringProvider.cs

...e/Portable/IntroduceUsingStatement/AbstractIntroduceUsingStatementCodeRefactoringProvider.cs

CyrusNajmabadi

request changes.

src/EditorFeatures/CSharpTest/IntroduceUsingStatement/IntroduceUsingStatementTests.cs

src/Features/Core/Portable/CodeRefactoringHelpers.cs

CyrusNajmabadi · 2018-09-23T00:00:20Z

...e/Portable/IntroduceUsingStatement/AbstractIntroduceUsingStatementCodeRefactoringProvider.cs

+            CancellationToken cancellationToken)
+
+            where TStatementSyntax : SyntaxNode
+        {


an alternative approach that might would would be to get the parent, walk backward to find the last reference. That itself could be a generic helper elsewhere.

Then, when you have the node for the last reference, you simply ask for it's ancestor node that shares your parent. that would be your sibling node.

The granularity of the search is at the sibling statement level, so this wouldn't provide any additional performance boost overall. It doesn't matter within each sibling statement whether we walk forwards or backwards. In fact, perf would probably go down due to the extra bookkeeping.

It was not about a performance boost. it was about building this out of more general helpers with a simple and clear algorithm.

Specifically, all you'd need was a helper to find the last-ref of a local in a specified node. That seems generally useful and sane to have as a general helper.

Then you could simply use the result of that with exsiting code we have to give the specific answer of "last sibling statement".

Okay. I'm not ready to generalize yet. Can I keep this PR more concrete?

jnm2 · 2018-09-23T02:01:13Z

Pushed all interesting logic into the abstract class and got all pre-existing tests passing.

Next up: writing more test cases.

...arp/Portable/IntroduceUsingStatement/CSharpIntroduceUsingStatementCodeRefactoringProvider.cs

CyrusNajmabadi · 2018-09-23T04:23:35Z

...arp/Portable/IntroduceUsingStatement/CSharpIntroduceUsingStatementCodeRefactoringProvider.cs

+        protected override BlockSyntax WithStatements(BlockSyntax blockSyntax, SyntaxList<StatementSyntax> statements)
+        {
+            return blockSyntax.WithStatements(statements);
+        }


Note: CSharpIntroduceVariableService_IntroduceLocal has helpers for these (which also do the right thing for switch-cases). We should likely put those somewhere and use for both of these.

Note: SyntaxGenerator would be a good place for this as it already handles getting/adding statements to members.

@CyrusNajmabadi Ready to do this, but where would IsBlockLike go? SyntaxGenerator? C# SyntaxFacts is public API and internal members aren't accessible to this assembly. I looked at doing it on the syntax facts language service, but VB doesn't have standalone blocks so I'd have to do a ton of work to get statements from every kind of syntax.

I'm fine with SyntaxGenerator being used for this.

this does'nt seem to have been done.

this doesn't seem to have been done.

@CyrusNajmabadi I'm actually worried that my current methods aren't general enough for this. However, maybe this is the key.

src/Compilers/Core/Portable/Syntax/SyntaxList`1.cs

...e/Portable/IntroduceUsingStatement/AbstractIntroduceUsingStatementCodeRefactoringProvider.cs

jnm2 · 2018-11-02T00:32:17Z

@jinujoseph @JoeRobich I resolved the merge conflicts. Let me know if there's anything else you'd like me to do!

JoeRobich · 2018-11-02T00:59:03Z

@jnm2 After looking at the integration failures, it seems that the xlf files weren't rebuilt after making some resx changes. This is pretty common if you were fixing merge conflicts in the resx. Could you rebuild the solution and commit the rebuilt xlf files? Thanks!

…ations

jnm2 · 2018-11-02T01:16:30Z

Oops, fixed!

JoeRobich · 2018-11-02T01:22:27Z

@jnm2 Thanks! Assuming everything ends up green I will merge this in the morning.

While playing with it some more, a separate enhancement PR might be to remove the final usage of the disposable variable, if it is an invocation of Dispose().

jnm2 · 2018-11-02T01:29:43Z

@JoeRobich Yes, I thought about that! I couldn't imagine running into the scenario myself, so I thought I'd wait and see if people wanted it. (I wouldn't get as far as deciding to type .Dispose() before realizing that I want a using. In practice I decide I want a using statement the moment I learn that the variable I just declared is disposable.)

I think if we did this, we should place the ending brace where the .Dispose() call previously was and move any declarations that became surrounded before the using (while leaving their initialization where it was). So that could be a generally enhancement even before detecting an existing .Dispose() statement: separate declarations from initializers that became surrounded but are used outside the new using block.

JoeRobich · 2018-11-02T15:12:56Z

The integration test failure was the IPC Error - #29001 - Build logs

JoeRobich · 2018-11-02T15:18:29Z

Thanks @jnm2!

As for future enhancements, when deciding whether to move a declaration out, could we use a similar find last usage and check if it is going to fall within the using body? Even if not, I agree that would help ensure we don't break working code.

jnm2 · 2018-11-02T15:26:49Z

@JoeRobich Yes, we would want to do that.

…zyT-and-delegates * dotnet/dev16.0-preview2: (117 commits) Do not warn for double nullable supression (dotnet#30936) Add -shared and -keepalive compiler options (dotnet#30818) Introduce using statement (dotnet#30106) Skip BinaryFileErrorTest Skip ParseFileOnBinaryFile Merge fixup Add missing binding redirects to csc.exe.config Add comments for default namespace in workspace Fix variable name Merge fixups Add support for default namespace to workspace Addressing feedback from Cyrus Embed optimization data to external compiler dependencies Skip optimizing System.Threading.Tasks.Extensions Skip optimizing System.Threading.Tasks.Extensions Add cert for PKT=b03f5f7f11d50a3a RoslynTools 18526.1 Fix VS dependency deployment Fix msbuild test Deploy System.Runtime.CompilerServices.Unsafe ...

jnm2 requested review from a team as code owners September 22, 2018 23:34

jnm2 force-pushed the introduce_using_statement branch from d1dd366 to 08c88b5 Compare September 22, 2018 23:36