Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement textDocument/rename #1048

Merged
merged 5 commits into from Nov 11, 2019
Merged

Implement textDocument/rename #1048

merged 5 commits into from Nov 11, 2019

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Nov 6, 2019

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:

  1. When apply is renamed we add full name to each invocation

renamed

  1. Symbols can't be renamed while compiling, since we need full and correct semanticDB information - an error will pop up as a message.

  2. Methods ending with colon can only be renamed to another name ending with colon.

  3. Renaming an overriden method will rename both parent symbol and current symbol:
    rename-inherit

  4. Symbols from outside workspace cannot be renamed.

  5. Companion objects and classes are renamed.
    companion

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

@tgodzik tgodzik requested a review from olafurpg Nov 6, 2019
@tgodzik tgodzik force-pushed the tgodzik:add-rename branch from 9d4e433 to f1a8e69 Nov 6, 2019
@tgodzik tgodzik marked this pull request as ready for review Nov 6, 2019
@tgodzik tgodzik force-pushed the tgodzik:add-rename branch 2 times, most recently from cf495de to 792ae25 Nov 6, 2019
Copy link
Member

olafurpg left a comment

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 👍

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

This comment has been minimized.

Copy link
@olafurpg
tests/unit/src/test/scala/tests/RenameSuite.scala Outdated Show resolved Hide resolved
tests/unit/src/test/scala/tests/RenameSuite.scala Outdated Show resolved Hide resolved
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member

gabro commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 7, 2019

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

This comment has been minimized.

Copy link

mdedetrich commented Nov 10, 2019

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...

tgodzik added 2 commits Oct 29, 2019
…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 tgodzik force-pushed the tgodzik:add-rename branch from 3ba4c7b to b488b21 Nov 10, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Nov 10, 2019

@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
@tgodzik tgodzik force-pushed the tgodzik:add-rename branch from b488b21 to 344f826 Nov 10, 2019
@tgodzik tgodzik requested a review from olafurpg Nov 11, 2019
Copy link
Member

olafurpg left a comment

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

tests/unit/src/main/scala/tests/TestRanges.scala Outdated Show resolved Hide resolved
|/a/src/main/scala/a/Main.scala
|package a
|object Main{
| val other = new <<Oth@@er>>()

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 11, 2019

Member

Should we consider erroring in this case?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 11, 2019

Author Collaborator

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?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 11, 2019

Author Collaborator

I added a warning in a case like that.

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Nov 11, 2019

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.

Copy link
Member

olafurpg left a comment

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.

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

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Nov 11, 2019

@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

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 11, 2019

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

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 11, 2019

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

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Nov 11, 2019

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.

@tgodzik tgodzik merged commit bc6d85c into scalameta:master Nov 11, 2019
8 of 9 checks passed
8 of 9 checks passed
Windows unit tests Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@tgodzik tgodzik deleted the tgodzik:add-rename branch Nov 11, 2019
@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Nov 11, 2019

Really excited to start using this! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.