From 260e9b90b46ba4e5fc790e63467f5c668c1a7bfc Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Tue, 24 Sep 2024 17:15:55 +0200 Subject: [PATCH] bugfix: Exclude comma if included in symbol range Some previous versions of Scala 3 would included the final comma in a parameters list, this should help out with that case. Versions from 3.5.2 and onwards work correctly. I also reworked the tests to try and rename in each place a symbol is expected, which should catch more errors and need less test cases. I will follow up with changes to RenameLspSuite --- .../meta/internal/rename/RenameProvider.scala | 13 ++- project/V.scala | 23 +++-- .../tests/feature/RenameCrossLspSuite.scala | 27 ++++++ .../main/scala/tests/BaseRenameLspSuite.scala | 83 +++++++++++++------ 4 files changed, 105 insertions(+), 41 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala index 8c8043eb118..07f869aec32 100644 --- a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala @@ -174,7 +174,7 @@ final class RenameProvider( def foundName = occ.range .flatMap(rng => rng.inString(textDocument.text)) - .map(_.stripBackticks) + .map(_.stripBackticks.stripSuffix(",")) occ.symbol.isLocal || foundName.contains(realName) } @@ -591,9 +591,10 @@ final class RenameProvider( val isExplicitVarSetter = name.exists(nm => nm.endsWith("_=") || nm.endsWith("_=`")) val isBackticked = name.exists(_.isBackticked) + val commaIncluded = name.exists(_.endsWith(",")) lazy val symbolName = - if (!isExplicitVarSetter) nameString.stripSuffix("_=") + if (!isExplicitVarSetter) nameString.stripSuffix("_=").stripSuffix(",") else nameString lazy val realName = @@ -622,10 +623,14 @@ final class RenameProvider( range } + val withoutComma = if (commaIncluded) { + withoutBacktick.withEndCharacter(withoutBacktick.endCharacter - 1) + } else withoutBacktick + val realRange = if (isExplicitVarSetter && !newName.endsWith("_=")) - withoutBacktick.withEndCharacter(withoutBacktick.endCharacter - 2) - else withoutBacktick + withoutComma.withEndCharacter(withoutComma.endCharacter - 2) + else withoutComma Some(realRange) } else { scribe.warn(s"Name doesn't match for $symbolName at $range") diff --git a/project/V.scala b/project/V.scala index f181d059d3a..c23defb3d79 100644 --- a/project/V.scala +++ b/project/V.scala @@ -188,16 +188,15 @@ object V { def deprecatedScalaVersions = deprecatedScala2Versions ++ deprecatedScala3Versions - val quickPublishScalaVersions = - Set( - bazelScalaVersion, - scala211, - sbtScala, - scala212, - ammonite212Version, - scala213, - ammonite213Version, - scala3, - ammonite3Version, - ).toList ++ scala3RC.toList + val quickPublishScalaVersions = Set( + bazelScalaVersion, + scala211, + sbtScala, + scala212, + ammonite212Version, + scala213, + ammonite213Version, + scala3, + ammonite3Version, + ).toList ++ scala3RC.toList } diff --git a/tests/slow/src/test/scala/tests/feature/RenameCrossLspSuite.scala b/tests/slow/src/test/scala/tests/feature/RenameCrossLspSuite.scala index 4f018c19673..946e591d3bc 100644 --- a/tests/slow/src/test/scala/tests/feature/RenameCrossLspSuite.scala +++ b/tests/slow/src/test/scala/tests/feature/RenameCrossLspSuite.scala @@ -65,4 +65,31 @@ class RenameCrossLspSuite extends BaseRenameLspSuite("rename-cross") { scalaVersion = Some(V.scala3), ) + renamed( + "ends-comma", + """|/a/src/main/scala/a/Main.scala + |package a + |object main { + | def myMethod( + | s1: String, + | s2: String, + | s3: String, + | s4: String, + | ) = s1 ++ s2 ++ s3 ++ s4 + | + | val word1 = "hello" + | val <> = "world" + | + | myMethod( + | word1, + | <>, + | s3 = word1, + | s4 = <>, + | ) + |} + |""".stripMargin, + newName = "NewSymbol", + scalaVersion = Some(V.scala3), + ) + } diff --git a/tests/unit/src/main/scala/tests/BaseRenameLspSuite.scala b/tests/unit/src/main/scala/tests/BaseRenameLspSuite.scala index f6900414300..c1e59fc9e1a 100644 --- a/tests/unit/src/main/scala/tests/BaseRenameLspSuite.scala +++ b/tests/unit/src/main/scala/tests/BaseRenameLspSuite.scala @@ -2,6 +2,7 @@ package tests import scala.concurrent.Future +import scala.meta.internal.metals.MetalsEnrichments.XtensionString import scala.meta.internal.pc.Identifier import munit.Location @@ -60,31 +61,41 @@ abstract class BaseRenameLspSuite(name: String) extends BaseLspSuite(name) { expectedError: Boolean = false, metalsJson: Option[String] = None, )(implicit loc: Location): Unit = { - test(name, maxRetry = 3) { + test(name, maxRetry = { if (isCI) 3 else 0 }) { cleanWorkspace() val allMarkersRegex = "(<<|>>|@@|##.*##)" val files = FileLayout.mapFromString(input) - val expectedName = Identifier.backtickWrap(newName) - val expectedFiles = files.map { case (file, code) => - fileRenames.getOrElse(file, file) -> { - val expected = if (!notRenamed) { - code - .replaceAll("\\<\\<\\S*\\>\\>", expectedName) - .replaceAll("(##|@@)", "") - } else { - code.replaceAll(allMarkersRegex, "") + + val expectedFiles = (renamedTo: String) => + files.map { case (file, code) => + fileRenames.getOrElse(file, file) -> { + val expectedName = Identifier.backtickWrap(renamedTo) + val expected = if (!notRenamed) { + code + .replaceAll("\\<\\<\\S*\\>\\>", expectedName) + .replaceAll("(##|@@)", "") + } else { + code.replaceAll(allMarkersRegex, "") + } + "\n" + breakingChange(expected) } - "\n" + breakingChange(expected) } - } - val (filename, edit) = files + val singleRename = files .find(_._2.contains("@@")) - .getOrElse { - throw new IllegalArgumentException( - "No `@@` was defined that specifies cursor position" - ) - } + + val allRenameLocations = singleRename match { + case None => + files.flatMap { case (file, code) => + code.indicesOf("<<").map { ind => + val updated = + code.substring(0, ind + 3) + "@@" + code.substring(ind + 3) + (file, updated, newName + ind) + } + + } + case Some((filename, edit)) => List((filename, edit, newName)) + } val openedFiles = files.keySet.diff(nonOpened) val fullInput = input.replaceAll(allMarkersRegex, "") @@ -110,13 +121,35 @@ abstract class BaseRenameLspSuite(name: String) extends BaseLspSuite(name) { server.didChange(file) { code => "\n" + code } } } - _ <- server.assertRename( - filename, - edit.replaceAll("(<<|>>|##.*##)", ""), - expectedFiles, - files.keySet, - newName, - ) + allRenamed = allRenameLocations.map { case (filename, edit, renameTo) => + () => + server + .assertRename( + filename, + edit.replaceAll("(<<|>>|##.*##)", ""), + expectedFiles(renameTo), + files.keySet, + renameTo, + ) + .flatMap { + // Revert all files to initial state + _ => + Future + .sequence { + files.map { case (file, code) => + server.didSave(file)(_ => + code.replaceAll(allMarkersRegex, "") + ) + }.toList + + } + .map(_ => ()) + + } + } + _ <- allRenamed.foldLeft(Future.unit) { case (acc, next) => + acc.flatMap(_ => next()) + } } yield () } }