Implement textDocument/rename #1048

tgodzik · 2019-11-06T12:58:44Z

Previously, rename was not supported in any way. User would have to manually rename everything by hand and that could prove really tedious. Now, we offer renaming of any symbol with a small number of exclusions:

unapply
equals
hashCode
unary_!
!

We can also add/remove other exclusions if needed.

There are also some special cases and several approaches that need to be mentioned:

When apply is renamed we add full name to each invocation

Symbols can't be renamed while compiling, since we need full and correct semanticDB information - an error will pop up as a message.
Methods ending with colon can only be renamed to another name ending with colon.
Renaming an overriden method will rename both parent symbol and current symbol:
Symbols from outside workspace cannot be renamed.
Companion objects and classes are renamed.

Currently not working:

type reference such as classOf[MyClass] or [T <: MyClass]
we don't check operator precedence when they are renamed
we rename all parent's overriden methods by default - we might want to ask about that, however MessageRequest doesn't seem a pretty way to do it (might be mistaken).

Fixes #679

olafurpg

Wow! This looks amazing. Great job @tgodzik handling the tricky cases such as .apply, right-associate operators, hashCode and more. The implementation itself was easy to follow and the test cases are easy to read. A few thoughts

Did you look into implementing textDocument/prepareRename? https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#textDocument_prepareRename I believe it could help provide faster validation to the user but I'm not 100% sure how it works.

Can we use the documentChanges field in WorkspaceEdit to rename the files? One feature I love in IntelliJ is that it renames the file to match the classname when that class/companion is the only toplevel definition in the file.

A few minor suggestions for more test cases to add. Otherwise looks good to me 👍

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

olafurpg · 2019-11-07T04:36:32Z

tests/unit/src/test/scala/tests/RenameSuite.scala

+       |
+       |object Main {
+       |  val a = new Alphabet {
+       |    override def <<me@@thod>>(adf: String): Int = 321


tests/unit/src/test/scala/tests/RenameSuite.scala

tgodzik · 2019-11-07T10:32:37Z

Thanks @olafurpg for the review! I will try to address the comments soon, though I am thinking we should merge this after the next release so that we introduce implementation and test/run first. What do you think?

Did you look into implementing textDocument/prepareRename? https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#textDocument_prepareRename I believe it could help provide faster validation to the user but I'm not 100% sure how it works.

I am not sure if it helps us a lot. I was hoping there would be some kind of additional window or something like that - it would enable use to choose exact options for rename. However, currently it will do a very similar thing to what we do in rename. I will see if it helps us now.

Can we use the documentChanges field in WorkspaceEdit to rename the files? One feature I love in IntelliJ is that it renames the file to match the classname when that class/companion is the only toplevel definition in the file.

Och, I wanted to do that, but totally forgot. Should be simple enough to do.

gabro · 2019-11-07T10:38:27Z

I am not sure if it helps us a lot. I was hoping there would be some kind of additional window or something like that - it would enable use to choose exact options for rename. However, currently it will do a very similar thing to what we do in rename. I will see if it helps us now.

As far I as understand, prepareRename may enable us to reject a rename before the rename input is prompted to the user. With the current flow, the user will first input the new name, and then get an error, which can be slightly annoying.

tgodzik · 2019-11-07T10:50:02Z

I am not sure if it helps us a lot. I was hoping there would be some kind of additional window or something like that - it would enable use to choose exact options for rename. However, currently it will do a very similar thing to what we do in rename. I will see if it helps us now.

As far I as understand, prepareRename may enable us to reject a rename before the rename input is prompted to the user. With the current flow, the user will first input the new name, and then get an error, which can be slightly annoying.

I will check that! That would be much more useful indeed, haven't thought of that.

olafurpg · 2019-11-07T17:53:06Z

Yes, the idea with prepareRename would be to give the user faster feedback on when the rename is not supported, ideally, before they type out the new name. For example, the validation to ensure we don't rename during compilation, that we don't rename equals, etc.

olafurpg · 2019-11-07T17:55:30Z

I am thinking we should merge this after the next release so that we introduce implementation and test/run first. What do you think?

I'm not sure if that's necessary. From the looks of it, this PR is quite complete already with a solid implementation and test cases. I don't think we have to rush for the next release, it's OK if we wait a few more weeks before having a large stable release with a lot of the exciting stuff that's is almost ready (rename symbol, worksheets, breakpoint debugging, pants support)

mdedetrich · 2019-11-10T15:05:55Z

I'm not sure if that's necessary. From the looks of it, this PR is quite complete already with a solid implementation and test cases. I don't think we have to rush for the next release, it's OK if we wait a few more weeks before having a large stable release with a lot of the exciting stuff that's is almost ready (rename symbol, worksheets, breakpoint debugging, pants support)

I am getting so excited right now...

…anually rename everything by hand and that could prove really tedious. Now, we offer renaming of any symbol with a small number of exclusions: - unapply - equals - hashcode This also introduces finding parents of overriden symbols.

tgodzik · 2019-11-10T22:11:26Z

@olafurpg I added some more tests as suggested and file renaming in case that a top level symbol matches the file name.

Let me know if there is something more to fix!

Also, I added the prepareRename, but doesn't seem to help, since it's being currently ignored in VS Code :/ If we return null from textDocument/prepareRename it still tries to invoke textDocument/rename

…e and additional tests to address review feedback

olafurpg

Looking great! Just a few minor comments, otherwise looks good to me

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

tests/unit/src/main/scala/tests/TestRanges.scala

tests/unit/src/test/scala/tests/RenameSuite.scala

olafurpg · 2019-11-11T04:26:00Z

tests/unit/src/test/scala/tests/RenameSuite.scala

+       |/a/src/main/scala/a/Main.scala
+       |package a
+       |object Main{
+       |  val other = new <<Oth@@er>>()


Should we consider erroring in this case?

We could, but the other way would be to use Metals to at least rename Scala occurrences and rename the rest by hand or via different LSP.

I thinking maybe report a warning in that case?

I added a warning in a case like that.

tests/unit/src/test/scala/tests/RenameSuite.scala

tgodzik · 2019-11-11T12:34:52Z

There are 3 test cases that are not behaving correctly due to some issues in SemanticDB. I will create the issues there and add information about them to this PR.

@sswistun-vl might be able to work on some of those issues.

The macro-annotation test case might be probably fixed by a work around, which I will try to follow up with separately.

olafurpg

LGTM 👍 Awesome work @tgodzik! Having "rename symbol" is a major milestone for Metals, I'm super excited about this 😄

I think it's fine to merge this PR despite the known limitations for certain type parameters and named arguments. If we don't manage to fix those issues in SemanticDB before the next stable release, then we can at least document them on the website.

metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala

…the workspace and display a warning when renaming a symbol from a java file

tgodzik · 2019-11-11T16:45:26Z

@olafurpg Thanks again for the review! I fixed the issue with scalafix and added a warning when renaming a symbol from a java file.

I agree that we can merge this PR and rework the last issues later on. For sure some things will come up when people start using the feature.

olafurpg · 2019-11-11T17:07:22Z

I'm not sure why but the worksheet completion test seems flaky on Windows

X tests.worksheets.WorksheetLspSuite.completion 5755ms

Might be best to disable it for now and I'll open up a ticket that I will followup on later.

olafurpg · 2019-11-11T17:07:50Z

I can disable it in a followup PR, feel free to merge despite CI failure

tgodzik · 2019-11-11T18:17:45Z

I can disable it in a followup PR, feel free to merge despite CI failure

I can try to take a look at some point. I have everything setup on a Windows machine at home.

ckipp01 · 2019-11-11T18:28:16Z

Really excited to start using this! 🎉

tgodzik requested a review from olafurpg November 6, 2019 12:58

tgodzik force-pushed the add-rename branch from 9d4e433 to f1a8e69 Compare November 6, 2019 15:10

tgodzik marked this pull request as ready for review November 6, 2019 15:14

tgodzik force-pushed the add-rename branch 2 times, most recently from cf495de to 792ae25 Compare November 6, 2019 17:52

olafurpg requested changes Nov 7, 2019

View reviewed changes

tgodzik added 2 commits November 10, 2019 23:04

Add test case for renaming with pattern match

8bbe875

tgodzik force-pushed the add-rename branch from 3ba4c7b to b488b21 Compare November 10, 2019 22:06

Add prepareRenamed, rename for files when symbol matches the file nam…

344f826

…e and additional tests to address review feedback

tgodzik force-pushed the add-rename branch from b488b21 to 344f826 Compare November 10, 2019 23:25

tgodzik requested a review from olafurpg November 11, 2019 00:02

olafurpg requested changes Nov 11, 2019

View reviewed changes

Addressing review feedback and adding more tests

e426abb

olafurpg approved these changes Nov 11, 2019

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala Show resolved Hide resolved

Make sure we check if file with symbol to rename is contained inside …

32617da

…the workspace and display a warning when renaming a symbol from a java file

tgodzik merged commit bc6d85c into scalameta:master Nov 11, 2019

tgodzik deleted the add-rename branch November 11, 2019 18:18

olafurpg mentioned this pull request Nov 11, 2019

Disable flaky test on Window #1060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement textDocument/rename #1048

Implement textDocument/rename #1048

tgodzik commented Nov 6, 2019 •

edited

Loading

olafurpg left a comment •

edited

Loading

olafurpg Nov 7, 2019

tgodzik commented Nov 7, 2019

gabro commented Nov 7, 2019

tgodzik commented Nov 7, 2019

olafurpg commented Nov 7, 2019

olafurpg commented Nov 7, 2019

mdedetrich commented Nov 10, 2019

tgodzik commented Nov 10, 2019 •

edited

Loading

olafurpg left a comment

olafurpg Nov 11, 2019

tgodzik Nov 11, 2019 •

edited

Loading

tgodzik Nov 11, 2019

tgodzik commented Nov 11, 2019

olafurpg left a comment

tgodzik commented Nov 11, 2019

olafurpg commented Nov 11, 2019

olafurpg commented Nov 11, 2019

tgodzik commented Nov 11, 2019

ckipp01 commented Nov 11, 2019

Implement textDocument/rename #1048

Implement textDocument/rename #1048

Conversation

tgodzik commented Nov 6, 2019 • edited Loading

olafurpg left a comment • edited Loading

Choose a reason for hiding this comment

olafurpg Nov 7, 2019

Choose a reason for hiding this comment

tgodzik commented Nov 7, 2019

gabro commented Nov 7, 2019

tgodzik commented Nov 7, 2019

olafurpg commented Nov 7, 2019

olafurpg commented Nov 7, 2019

mdedetrich commented Nov 10, 2019

tgodzik commented Nov 10, 2019 • edited Loading

olafurpg left a comment

Choose a reason for hiding this comment

olafurpg Nov 11, 2019

Choose a reason for hiding this comment

tgodzik Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

tgodzik Nov 11, 2019

Choose a reason for hiding this comment

tgodzik commented Nov 11, 2019

olafurpg left a comment

Choose a reason for hiding this comment

tgodzik commented Nov 11, 2019

olafurpg commented Nov 11, 2019

olafurpg commented Nov 11, 2019

tgodzik commented Nov 11, 2019

ckipp01 commented Nov 11, 2019

tgodzik commented Nov 6, 2019 •

edited

Loading

olafurpg left a comment •

edited

Loading

tgodzik commented Nov 10, 2019 •

edited

Loading

tgodzik Nov 11, 2019 •

edited

Loading