Extract method feature doesn't respect selection or tabs/spaces preference

[CanisLupus]

Environment data

dotnet --info output:

.NET Command Line Tools (1.0.0-preview2-1-003177)

Product Information:
 Version:            1.0.0-preview2-1-003177
 Commit SHA-1 hash:  a2df9c2576

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.14393
 OS Platform: Windows
 RID:         win10-x64

(I installed .NET Core SDK 1.1.0 to get the output from the command line, otherwise it always said "Failed to spawn 'dotnet --info'")

VS Code version: 1.9.1
C# Extension version: 1.7.0

Description

Right now, running VSCode v1.9.1, I get a light bulb containing "extract method" to the left of a selected line or lines, but only if my selection is particular. I believe it is using the cursor position only and not what I have selected. Is this correct?

It's a bit hard to work with if it doesn't respect the selection. For example, if I want to extract the contents of an "else", I don't seem to be able to, unless it's a single statement. On the other hand, if I extract with the cursor on the "else" word, it will extract the full if-else (I guess this is expected... maybe ;)).

Furthermore, running extract method seems to disregard my preference of tabs vs spaces, always using spaces, and will also change the formatting of the extracted code (though maybe it runs format and this is expected behavior).

Thanks for reading!

[I originally commented on issue #317]

[DustinCampbell]

The tabs and spaces issue is tracked by OmniSharp/omnisharp-roslyn#759.

Is there a specific code sample you could provide along with how you expect it to work with various selections? I'd like to be sure we're on the same page for what should be fixed.

Note: Our Extract Method refactoring comes directly from Visual Studio, so we might be a bit limited in what we can change. However, I've also seen some weird behaviors in VS Code around Extract Method that differ from how it behaves in VS. So, I expect there are some bugs on our end.

[CanisLupus]

Sure. :) As the tabs issue is already documented I'll focus on the rest.

This selection should display the light bulb and be extracted as a method as expected:

Currently, it doesn't recognize it because the cursor isn't placed on any token.

---

These two equivalent selections should both show the light bulb and yield the same extraction as the example above:

The first was selected from bottom to top, with the cursor ending on the beginning of the if; the second is from top to bottom and ends on the last ;. As it stands, only the first shows the light bulb, and will extract the if-else only, ignoring the last two lines.

---

The cursor is placed somewhere on the else in this next example, or even selecting part of the word:

I would accept either the current behavior (extracting the whole if-else) or just the else. In the latter case, placing the cursor on the if or selecting the word or part of it should extract only the if and not the else. I'm not sure which of these options is more sensible. Both seem a bit confusing in different situations.

---

Selecting this line should extract it:

---

Selecting the same line including the curly brackets should extract everything inside the brackets (which has the same result as above) and not the if.

---

For weirder selections like this one, which are not valid code, it should probably fall back to the next largest selection that makes sense, which would be the whole if (!softSelection) and its contents.

---

This is similar, and should extract the whole if-else and the line after it, which is partially selected:

---

I believe this would be reasonable behavior, though there might exist more extreme cases I didn't approach here. I'm sad to hear that you would be limited in changing this. I hope this works. :)

As for the formatting of the extracted method, I didn't give an example but I can say that curly brackets move to the next line, which is something I usually only do when the body contains many lines. Is there anything you can do to keep the formatting? Is it always formatted when extracted? (well, at least the indentation should be, but I mean the code itself)

Thank you!

[DustinCampbell]

So, I was just trying to reproduce the issues you reported here, but I've been largely unsuccessful. I'm wondering if recent changes to VS Code have addressed some of the problems.

1. I get a light bulb for Extract Method, regardless of how I make my selection: whole line or not, top-to-bottom or bottom-to-top.

   

   

   

2. Selecting a line as you did also shows a light bulb for Extract Method.

   

3. Selecting the same line with its surrounding curly braces works as expected.

   

4. Partially selecting code works as expected.

   • Before:
     

   • After:
     

The only problem you demonstrated above that I can repro is the problem of having a partial selection across multiple statements:

However, that has the same problem in Visual Studio 2017 as well:

I believe that the issue here in with how Roslyn expands the selection to find "extractable" code. I suspect that the selection tries to expand to the block that includes the two statements (both the inner if-else and the assignment) and something fails when it tries to validate that selection. @Pilchie, are you aware of any known issues on Roslyn that would account for this?

@CanisLupus: Are you still experiencing the problems above? So far, I'm unable to find any issues that wouldn't also be reproducible in VS 2017, which would likely indicate that they're Roslyn bugs.

[CanisLupus]

Hi @DustinCampbell, thanks for checking back! You are correct. Something seems to have changed in VSCode because I can confirm that every example (except the last one) is working well for me now. This is great!

I wouldn't even have noticed the changes if you hadn't commented (I had given up on doing extracts). The last one not working is hardly a big problem; it's only slightly inconsistent with the rest.

So what's left is this, the tabs/spaces preference (if not taken care of already), and not changing my curly brackets' positions. ;) (maybe by adopting the indentation of the source as much as possible when extracting?)

[DustinCampbell]

I believe the tabs/spaces issue should be fixed in the next release. The braces can be controlled by a formatting option by including an omnisharp.json file in your workspace. You can control the braces with the various "NewLinesForBraces*" values (see the formatting schema here).

[CanisLupus]

Ah, thank you for your help, I appreciate the links. :) Those settings should take care of most of the formatting. In other cases I guess I could fix the extraction by hand afterwards (if an if/for/while has a lot of lines I tend to open curly brackets on a new line, otherwise I place them at the end of the line). It's not like I'm constantly extracting methods. ;)

[Pilchie]

I'm not aware of any issues around the selection expansion, but I can indeed reproduce this one, so I filed dotnet/roslyn#18656

[DustinCampbell]

Thanks @Pilchie. In that case, I think everything else here is covered now.