Ask user when renaming overriding symbols

When renaming a symbol that is overriding another symbol, we now ask the
user whether we should rename the base symbol or only this member.

Author: Duhemm

language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala

@@ -351,9 +351,7 @@ class DottyLanguageServer extends LanguageServer
     val uriTrees = driver.openedTrees(uri)
     val pos = sourcePosition(driver, uri, params.getPosition)
     val path = Interactive.pathTo(uriTrees, pos)
-    val syms = Interactive.enclosingSourceSymbols(path, pos).flatMap { sym =>
-      sym :: sym.allOverriddenSymbols.toList
-    }
+    val syms = Interactive.enclosingSourceSymbols(path, pos)
     val newName = params.getNewName
 
     def findRenamedReferences(trees: List[SourceTree], syms: List[Symbol], withName: Name): List[SourceTree] = {
@@ -374,13 +372,25 @@ class DottyLanguageServer extends LanguageServer
           findRenamedReferences(uriTrees, syms, nameTree.name)
 
         case _ =>
-          val names = syms.map(_.name.sourceModuleName).toSet
+          val (include, allSymbols) =
+            if (syms.exists(_.allOverriddenSymbols.nonEmpty)) {
+              showMessageRequest(MessageType.Info,
+                RENAME_OVERRIDDEN_QUESTION,
+                List(
+                  RENAME_OVERRIDDEN    -> (() => (Include.all, syms.flatMap(s => s :: s.allOverriddenSymbols.toList))),
+                  RENAME_NO_OVERRIDDEN -> (() => (Include.all.except(Include.overridden), syms)))
+              ).get.getOrElse((Include.empty, List.empty[Symbol]))
+            } else {
+              (Include.all, syms)
+            }
+
+          val names = allSymbols.map(_.name.sourceModuleName).toSet
           val trees = names.flatMap(name => driver.allTreesContaining(name.toString)).toList
-          syms.flatMap { sym =>
+          allSymbols.flatMap { sym =>
             Interactive.findTreesMatching(trees,
-                                          Include.all,
-                                          sym,
-                                          t => names.exists(Interactive.sameName(t.name, _)))
+              include,
+              sym,
+              t => names.exists(Interactive.sameName(t.name, _)))
           }
       }
 
@@ -549,13 +559,40 @@ class DottyLanguageServer extends LanguageServer
     }
   }
 
-
+  /**
+   * Send a `window/showMessageRequest` to the client, asking to choose between `choices`, and
+   * perform the associated operation.
+   *
+   * @param tpe     The type of the request
+   * @param message The message accompanying the request
+   * @param choices The choices and their associated operation
+   * @return A future that will complete with the result of executing the action corresponding to
+   *         the user's response.
+   */
+  private def showMessageRequest[T](tpe: MessageType,
+                                    message: String,
+                                    choices: List[(String, () => T)]): CompletableFuture[Option[T]] = {
+    val options = choices.map((title, _) => new MessageActionItem(title))
+    val request = new ShowMessageRequestParams(options.asJava)
+    request.setMessage(message)
+    request.setType(tpe)
+
+    client.showMessageRequest(request).thenApply { (answer: MessageActionItem) =>
+      choices.find(_._1 == answer.getTitle).map {
+        case (_, action) => action()
+      }
+    }
+  }
 }
 
 object DottyLanguageServer {
   /** Configuration file normally generated by sbt-dotty */
   final val IDE_CONFIG_FILE = ".dotty-ide.json"
 
+  final val RENAME_OVERRIDDEN_QUESTION = "Do you want to rename the base member, or only this member?"
+  final val RENAME_OVERRIDDEN= "Rename the base member"
+  final val RENAME_NO_OVERRIDDEN = "Rename only this member"
+
   /** Convert an lsp4j.Position to a SourcePosition */
   def sourcePosition(driver: InteractiveDriver, uri: URI, pos: lsp4j.Position): SourcePosition = {
     val actualPosition =

language-server/test/dotty/tools/languageserver/RenameTest.scala

@@ -216,19 +216,23 @@ class RenameTest {
   }
 
   @Test def renameOverridden: Unit = {
-    def testRename(m: CodeMarker) =
+    def testRename(m: CodeMarker, expectations: Set[CodeRange], withOverridden: Option[Boolean]) =
       withSources(
         code"""class A { def ${m1}foo${m2}: Int = 0 }
                class B extends A { override def ${m3}foo${m4}: Int = 1 }
                class C extends A { override def ${m5}foo${m6}: Int = 2 }"""
-      ).rename(m, "NewName", Set(m1 to m2, m3 to m4, m5 to m6))
-
-    testRename(m1)
-    testRename(m2)
-    testRename(m3)
-    testRename(m4)
-    testRename(m5)
-    testRename(m6)
+      ).rename(m, "NewName", expectations, withOverridden)
+
+    testRename(m1, Set(m1 to m2, m3 to m4, m5 to m6), withOverridden = None)
+    testRename(m2, Set(m1 to m2, m3 to m4, m5 to m6), withOverridden = None)
+    testRename(m3, Set(m1 to m2, m3 to m4, m5 to m6), withOverridden = Some(true))
+    testRename(m4, Set(m1 to m2, m3 to m4, m5 to m6), withOverridden = Some(true))
+    testRename(m5, Set(m1 to m2, m3 to m4, m5 to m6), withOverridden = Some(true))
+    testRename(m6, Set(m1 to m2, m3 to m4, m5 to m6), withOverridden = Some(true))
+    testRename(m3, Set(m3 to m4), withOverridden = Some(false))
+    testRename(m4, Set(m3 to m4), withOverridden = Some(false))
+    testRename(m5, Set(m5 to m6), withOverridden = Some(false))
+    testRename(m6, Set(m5 to m6), withOverridden = Some(false))
 
   }
 

language-server/test/dotty/tools/languageserver/util/CodeTester.scala

@@ -89,14 +89,20 @@ class CodeTester(projects: List[Project]) {
    * Performs a workspace-wide renaming of the symbol under `marker`, verifies that the positions to
    * update match `expected`.
    *
-   * @param marker   The position from which to ask for renaming.
-   * @param newName  The new name to give to the symbol.
-   * @param expected The expected positions to change.
+   * @param marker         The position from which to ask for renaming.
+   * @param newName        The new name to give to the symbol.
+   * @param expected       The expected positions to change.
+   * @param withOverridden If `None`, do not expect the server to ask whether to include overridden
+   *                       symbol. Otherwise, wait for this question from the server and include
+   *                       overridden symbols if this is true.
    *
    * @see dotty.tools.languageserver.util.actions.CodeRename
    */
-  def rename(marker: CodeMarker, newName: String, expected: Set[CodeRange]): this.type =
-    doAction(new CodeRename(marker, newName, expected)) // TODO apply changes to the sources and positions
+  def rename(marker: CodeMarker,
+             newName: String,
+             expected: Set[CodeRange],
+             withOverridden: Option[Boolean] = None): this.type =
+    doAction(new CodeRename(marker, newName, expected, withOverridden)) // TODO apply changes to the sources and positions
 
   /**
    * Queries for all the symbols referenced in the source file in `marker`, verifies that they match

language-server/test/dotty/tools/languageserver/util/actions/CodeRename.scala

@@ -2,8 +2,13 @@ package dotty.tools.languageserver.util.actions
 
 import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.{CodeRange, PositionContext}
+import dotty.tools.languageserver.DottyLanguageServer.{RENAME_OVERRIDDEN, RENAME_NO_OVERRIDDEN}
 
-import org.junit.Assert.{assertEquals, assertNull}
+import org.junit.Assert.{assertEquals, assertNull, fail}
+
+import org.eclipse.lsp4j.{MessageActionItem, ShowMessageRequestParams}
+
+import java.util.concurrent.CompletableFuture
 
 import scala.collection.JavaConverters._
 
@@ -17,10 +22,31 @@ import scala.collection.JavaConverters._
  */
 class CodeRename(override val marker: CodeMarker,
                  newName: String,
-                 expected: Set[CodeRange]) extends ActionOnMarker {
+                 expected: Set[CodeRange],
+                 withOverridden: Option[Boolean]) extends ActionOnMarker {
+
+  private final val TIMEOUT_MS = 10000
 
   override def execute(): Exec[Unit] = {
-    val results = server.rename(marker.toRenameParams(newName)).get()
+    val query = server.rename(marker.toRenameParams(newName))
+
+    withOverridden.foreach { includeOverridden =>
+      var question: (ShowMessageRequestParams, CompletableFuture[MessageActionItem]) = null
+      val startTime = System.currentTimeMillis()
+      do {
+        Thread.sleep(50)
+        question = client.requests.get.headOption.orNull
+      } while (question == null && System.currentTimeMillis() - startTime < TIMEOUT_MS)
+
+      if (question == null) fail("The server didn't ask about overridden symbols.")
+
+      val answerStr = if (includeOverridden) RENAME_OVERRIDDEN else RENAME_NO_OVERRIDDEN
+      val action = question._1.getActions.asScala.find(_.getTitle == answerStr).get
+      question._2.complete(action)
+    }
+
+    val results = query.get()
+
     val changes = results.getChanges.asScala.mapValues(_.asScala.toSet.map(ch => (ch.getNewText, ch.getRange)))
     val expectedChanges = expected.groupBy(_.file.uri).mapValues(_.map(range => (newName, range.toRange)))
 

language-server/test/dotty/tools/languageserver/util/server/TestClient.scala

@@ -23,6 +23,7 @@ class TestClient extends WorksheetClient {
   val diagnostics = new Log[PublishDiagnosticsParams]
   val telemetry = new Log[Any]
   val worksheetOutput = new Log[WorksheetRunOutput]
+  val requests = new Log[(ShowMessageRequestParams, CompletableFuture[MessageActionItem])]
 
   override def logMessage(message: MessageParams) = {
     log += message
@@ -37,8 +38,9 @@ class TestClient extends WorksheetClient {
   }
 
   override def showMessageRequest(requestParams: ShowMessageRequestParams) = {
-    log += requestParams
-    new CompletableFuture[MessageActionItem]
+    val reply = new CompletableFuture[MessageActionItem]
+    requests += ((requestParams, reply))
+    reply
   }
 
   override def publishDiagnostics(diagnosticsParams: PublishDiagnosticsParams) = {