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

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")

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
 }

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 <<word2>> = "world"
+       |
+       |  myMethod(
+       |    word1,
+       |    <<word2>>,
+       |    s3 = word1,
+       |    s4 = <<word2>>,
+       |  )
+       |}
+       |""".stripMargin,
+    newName = "NewSymbol",
+    scalaVersion = Some(V.scala3),
+  )
+
 }

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 ()
     }
   }