Implement inline rename

[vasily-kirichenko]

This is early WIP of implementing Roslyn-powered Inline Rename.

[dsyme]

Wow. Fantastic to see this in progress!

[forki]

💋

[vasily-kirichenko]

@forki :) But it's not a small task, as it's turned out...

[forki]

@vasily-kirichenko I know. that's why you are doing it ;-)

[vasily-kirichenko]

I've stuck with this a bit. That's how it works at the moment:

What I think about it:

• The (green) tags are calculated properly - they are shifted and increased in size as they should.
• Tags and their content is not the same as real Document content because
  • Old code is not shifted (look at the + sign, for example)
  • Preview dialog says "No changes"
• I think C#/VB editor modify AST directly, which then automatically reflected visually on documents.

If this is all true, I'm afraid we have to modify Documents manually, which raises other questions:

• In what place we should do it? Candidates:
  • https://github.com/Microsoft/visualfsharp/pull/1932/files#diff-6ae7d2d36babc0c71812363409b9f718R142
  • https://github.com/Microsoft/visualfsharp/pull/1932/files#diff-6ae7d2d36babc0c71812363409b9f718R153
  • https://github.com/Microsoft/visualfsharp/pull/1932/files#diff-6ae7d2d36babc0c71812363409b9f718R196
• Would it work for not opened Documents? If it does not, is it OK to open them, change, then close?

[Pilchie]

Tagging @jasonmalinowski and @dpoeschl for comments about how this works.

[vladima]

I'll try to take a look today

[vasily-kirichenko]

BIG thanks to @vladima for fixing this!

It works like this now:

What I have to finish:

• add some validation, like we have in VFPT
• add tests

[vasily-kirichenko]

@vladima I found an interesting bug:

[dsyme]

@vasily-kirichenko Looks like there are quite a few unrelated changes in this branch? Or do you need to merge with master? thx

[vasily-kirichenko]

@dsyme You are right. Done.

I will investigate the bug later.

[dsyme]

~28 failures, e.g.

 1) Failed : Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.ColorizationServiceTests.InactiveCode("Inactive Code1*)le","excluded code")

[vasily-kirichenko]

I've reverted the tryClassifyAtPosition fix, all should be green now.

[vasily-kirichenko]

The bug is a tricky one. If there are no text in the file after the renaming symbol, it pops up immediately, after deleting single character:

If there is text after it, then it appears later, depending on the length of trailing text:

I've compared our implementation to XAML one http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices.Xaml/Features/InlineRename/XamlEditorInlineRenameService.cs and found no differences. I've tested renaming in XAML editor, it works OK.

Any ideas?

[dsyme]

@vasily-kirichenko This looks wrong (rename on a property e.Message replaces all text of the expression)

[vasily-kirichenko]

@dsyme yes, it's because FCS returns wrong ranges for qualified symbols. I had to work it around in Document Highlight service as well (it was copied from VFPT) and @vladima did something similar in this PR, too.

I think it would be really great to fix it at FSC level.

[cartermp]

I think it would be really great to fix it at FSC level.

Agreed, since this is cropping up in more than one feature area

[vasily-kirichenko]

@dsyme could you comment on this? I mean is it by design:

• System|.DateTime.Now => System
• System.DateTime|.Now => DateTime
• System.DateTime.Now| => System.DateTime.Now

?

[cartermp]

@vasily-kirichenko Something to consider either for this PR or a future one is how to deal with conflicts and preventing bad renames.

For example:

let getFirstFromTwople (t: int*int) = t |> fst
let getFirstFromThreeple (t: int*int*int) = t |> fst
let getFirstFromFourple (t: int*int*int*int) = t |> fst

I can currently inline rename fst to FST, which would result in a compile error, assuming FST either doesn't exist or isn't an inlined function which works for tuples up to a certain size.

Another thing to consider is the Big Scary Warning Text for renaming something into something else in scope which has the same name, like in C#/VB:

As an aside, I've been playing with this today and showing it off to people in the office. It's really, really great work. Kudos 😄

[vasily-kirichenko]

@cartermp yes, I already thought about this. We have quite good non scope aware name validation in VFPT, but it requires porting significant amount of code, including our own higher level wrapper over the lever for nicer qualified names parsing (or maybe not and we can use classifyAtPosition). What about name conflicts in same scope, I don't know how to get all names for current one. Never done this.

However, I'd prefer to fix the bug (I have absolutely no idea how to do it) and make it solution wide (for all F# projects, making it work with other project types is a different unexplored area, however, the Xamarin guys have done it somehow, so we must be able to do it, too).

[cartermp]

@vasily-kirichenko Makes sense.