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

Use exact match when computing file renames #1075

Merged
merged 6 commits into from Nov 18, 2019
Merged

Conversation

@gabro
Copy link
Member

gabro commented Nov 18, 2019

Fixes #1074

Previously we would rename files whose name partially matched the symbol being renamed. Now we compute those renames using an exact match, to avoid e.g. renaming a file named FooBar.scala when renaming the symbol Bar in that file.

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 18, 2019

@tgodzik I'm currently unable to run tests locally (they all fail, I haven't investigated, but as you know my Mac is cursed), so I'm using the CI to get some feedback

|object <<The@@Main>>
|""".stripMargin,
newName = "Tree",
fileRenames = Map.empty

This comment has been minimized.

Copy link
@gabro

gabro Nov 18, 2019

Author Member

I know this is the default, but I wanted to be explicit here

Copy link
Collaborator

tgodzik left a comment

Thanks! I somehow missed it despite remembering to check for exact matches a couple of lines below 😅

)

renamed(
"filename-exact-match-2",

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 18, 2019

Collaborator

😍 Thanks @gabro!

gabro added 2 commits Nov 18, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 18, 2019

@tgodzik I'm currently unable to run tests locally (they all fail, I haven't investigated, but as you know my Mac is cursed), so I'm using the CI to get some feedback

Let me know if you need help with that - I might be able to help? I had some issue that broke all tests previously.

@gabro gabro force-pushed the gabro:fix-1074 branch from eeea8d6 to d615570 Nov 18, 2019
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 18, 2019

The failure on Windows is relevant:

X tests.RenameSuite.across-targets 8002ms 
  java.nio.file.InvalidPathException: Illegal char <:> at index 4: file:///D:/a/metals/metals/tests/unit/target/e2e/rename/across-targets/a/src/main/scala/a/Main.scala
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 18, 2019

Although I can't reproduce in a REPL on macOS 🤔

scala> Paths.get("file:///D:/a/metals/metals/tests/unit/target/e2e/rename/across-targets/a/src/main/scala/a/Main.scala")
res4: java.nio.file.Path = file:/D:/a/metals/metals/tests/unit/target/e2e/rename/across-targets/a/src/main/scala/a/Main.scala
@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 18, 2019

@gabro We do AbsolutePath(Paths.get(new URI(file))) and it seems to work with the same input. Might be worth fixing this way.

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 18, 2019

Thanks for looking into this, I think we're hitting on this JDK bug https://bugs.openjdk.java.net/browse/JDK-8232925 (linked from quarkusio/quarkus#3592, which describes a similar problem). Adding the new URI step probably introduces URL encoding which fixes the issue. I'll give it a try and see if it fixes tests on Windows

Copy link
Member

olafurpg left a comment

Nice! Thank you @gabro for fixing this bug and @kubukoz for reporting!

str.desc.name.value + ".scala"
)
(str.desc.isType || str.desc.isTerm) &&
Paths.get(new URI(file)).filename == str.desc.name.value + ".scala"

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 18, 2019

Member

Since we know file is a URI, can we do this instead?

Suggested change
Paths.get(new URI(file)).filename == str.desc.name.value + ".scala"
file.endsWith(s"/${str.desc.name.value}.scala")

This comment has been minimized.

Copy link
@gabro

gabro Nov 18, 2019

Author Member

That's what @tgodzik suggested too. If you both think it's a good idea, I'll fix it this way!

@gabro gabro force-pushed the gabro:fix-1074 branch from bee823d to 0341a98 Nov 18, 2019
@@ -279,6 +279,29 @@ object RenameSuite extends BaseLspSuite("rename") {
Map("a/src/main/scala/a/Main.scala" -> "a/src/main/scala/a/Tree.scala")

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 18, 2019

Member

Unrelated to this PR, but I just noticed that this test suite should be called RenameLspSuite! cc/ @tgodzik

This comment has been minimized.

Copy link
@gabro

gabro Nov 18, 2019

Author Member

I can squeeze a fix in ;-)

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 18, 2019

Collaborator

You can use rename for the renaming of RenameLspSuite :O

This comment has been minimized.

Copy link
@olafurpg

This comment has been minimized.

Copy link
@gabro

gabro Nov 18, 2019

Author Member

Done!

I've renamed the object using the rename command, forgot to rename the file... but "rename" had my back! 😍

Copy link
Contributor

kubukoz left a comment

That was quick!

Thank you @gabro, very cool!

@@ -324,14 +323,14 @@ final class RenameProvider(
): MessageParams = {
val renamed = name.map(n => s"to $n").getOrElse("")
val message =
s"""|Cannot rename $old $renamed since it will change the semantics and
s"""|Cannot rename $old $renamed since it will change the semantics and

This comment has been minimized.

Copy link
@kubukoz

kubukoz Nov 18, 2019

Contributor

Double and

This comment has been minimized.

Copy link
@gabro

gabro Nov 18, 2019

Author Member

thanks :) I hadn't actually changed those lines in this PR, but I'll take the chance to fix it

@@ -342,8 +341,8 @@ final class RenameProvider(
): MessageParams = {
val renamed = name.map(n => s"to $n").getOrElse("")
val message =
s"""|Cannot rename from $old $renamed since it will change the semantics and
|and might break your code.
s"""|Cannot rename from $old $renamed since it will change the semantics and

This comment has been minimized.

Copy link
@kubukoz

kubukoz Nov 18, 2019

Contributor

from $old to $renamed?

gabro added 2 commits Nov 18, 2019
@gabro gabro merged commit cf7b952 into scalameta:master Nov 18, 2019
9 checks passed
9 checks passed
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
@gabro gabro deleted the gabro:fix-1074 branch Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.