RenameSymbol request allows collisions (#11892)

close #11658 Changelog: - add: `DefinitionAlreadyExists` error when the definition with the provided name already exists # Important Notes In the GUI it looks like this https://github.com/user-attachments/assets/824e7881-81bf-4547-a74a-7532753db614
2024-12-18 19:41:32 +03:00 · 2024-12-17 18:32:21 +03:00 · 2024-12-17 18:32:21 +03:00 · 909afff9b8
commit 909afff9b8
parent 30075d26bf
7 changed files with 372 additions and 7 deletions
--- a/docs/language-server/protocol-language-server.md
+++ b/docs/language-server/protocol-language-server.md
@ -238,6 +238,8 @@ transport formats, please look [here](./protocol-architecture).
  - [`ExpressionNotFoundError`](#expressionnotfounderror)
  - [`FailedToApplyEdits`](#failedtoapplyedits)
  - [`RefactoringNotSupported`](#refactoringnotsupported)
+  - [`ProjectRenameFailed`](#projectrenamefailed)
+  - [`DefinitionAlreadyExists`](#definitionalreadyexists)

 <!-- /MarkdownTOC -->

@ -3151,7 +3153,8 @@ type RefactoringRenameProjectResult = null;

 #### Errors

-None
+- [`ProjectRenameFailed`](#projectrenamefailed) to signal that the project
+  rename operation has failed.

 ### `refactoring/renameSymbol`

@ -3231,6 +3234,8 @@ interface RefactoringRenameSymbolResult {
  operation was not able to apply generated edits.
 - [`RefactoringNotSupported`](#refactoringnotsupported) to signal that the
  refactoring of the given expression is not supported.
+- [`DefinitionAlreadyExists`](#definitionalreadyexists) to signal that the
+  definition with the provided name already exists in scope.

 ### `refactoring/projectRenamed`

@ -5911,6 +5916,28 @@ Signals that the refactoring of the given expression is not supported.
 }
 ```

+### `ProjectRenameFailed`
+
+Signals that the project rename failed.
+
+```typescript
+"error" : {
+  "code" : 9004,
+  "message" : "Project rename failed [<oldName>, <newName>]"
+}
+```
+
+### `DefinitionAlreadyExists`
+
+Signals that the definition with the provided name already exists in the scope.
+
+```typescript
+"error" : {
+  "code" : 9005,
+  "message" : "Definition [<name>] already exists"
+}
+```
+
 ### `AiHttpError`

 Signals about an error during the processing of AI http respnse.
--- a/engine/language-server/src/main/scala/org/enso/languageserver/refactoring/RefactoringApi.scala
+++ b/engine/language-server/src/main/scala/org/enso/languageserver/refactoring/RefactoringApi.scala
@ -77,4 +77,7 @@ object RefactoringApi {
  case class ProjectRenameFailed(oldName: String, newName: String)
      extends Error(9004, s"Project rename failed [$oldName, $newName]")

+  case class DefinitionAlreadyExists(name: String)
+      extends Error(9005, s"Definition [$name] already exists")
+
 }
--- a/engine/language-server/src/main/scala/org/enso/languageserver/refactoring/RenameFailureMapper.scala
+++ b/engine/language-server/src/main/scala/org/enso/languageserver/refactoring/RenameFailureMapper.scala
@ -15,6 +15,9 @@ object RenameFailureMapper {
      case error: Api.SymbolRenameFailed.ExpressionNotFound =>
        RefactoringApi.ExpressionNotFoundError(error.expressionId)

+      case error: Api.SymbolRenameFailed.DefinitionAlreadyExists =>
+        RefactoringApi.DefinitionAlreadyExists(error.name)
+
      case error: Api.SymbolRenameFailed.FailedToApplyEdits =>
        RefactoringApi.FailedToApplyEdits(error.module)

--- a/engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala
+++ b/engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala
@ -1330,11 +1330,18 @@ object Runtime {
        *
        * @param expressionId the id of expression
        */
-
      @named("symbolRenameFailedExpressionNotFound")
      final case class ExpressionNotFound(expressionId: ExpressionId)
          extends SymbolRenameFailed.Error

+      /** Signals that the definition with the provided name already exists in the scope.
+        *
+        * @param name the definition name
+        */
+      @named("symbolRenameFailedDefinitionAlreadyExists")
+      final case class DefinitionAlreadyExists(name: String)
+          extends SymbolRenameFailed.Error
+
      /** Signals that it was unable to apply edits to the current module contents.
        *
        * @param module the module name
--- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/refactoring/IRUtils.scala
+++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/refactoring/IRUtils.scala
@ -2,8 +2,9 @@ package org.enso.compiler.refactoring

 import org.enso.compiler.core.Implicits.AsMetadata
 import org.enso.compiler.core.{ExternalID, IR, Identifier}
-import org.enso.compiler.core.ir.Name
+import org.enso.compiler.core.ir.{Expression, Name}
 import org.enso.compiler.core.ir.expression.Application
+import org.enso.compiler.core.ir.module.scope.definition.Method
 import org.enso.compiler.data.BindingsMap
 import org.enso.compiler.pass.analyse.DataflowAnalysis
 import org.enso.compiler.pass.resolve.MethodCalls
@ -31,6 +32,69 @@ trait IRUtils {
    None
  }

+  /** Find definitions with the provided name.
+    *
+    * @param ir the IR where to search the definition
+    * @param name the definition name to look for
+    * @return the list of definitions with the provided name
+    */
+  def findModuleDefinitions(ir: IR, name: String): Set[IR] = {
+    val builder = Set.newBuilder[IR]
+    IR.preorder(
+      ir,
+      {
+        case methodExplicit: Method.Explicit
+            if methodExplicit.methodName.name == name =>
+          builder.addOne(methodExplicit)
+        case _ =>
+      }
+    )
+    builder.result()
+  }
+
+  /** Find definitions with the provided name.
+    *
+    * @param scope the IR where to search the definition
+    * @param name the definition name to look for
+    * @return the list of definitions with the provided name
+    */
+  def findLocalDefinitions(scope: IR, name: String): Set[IR] = {
+    val builder = Set.newBuilder[IR]
+    IR.preorder(
+      scope,
+      {
+        case expressionBinding: Expression.Binding
+            if expressionBinding.name.name == name =>
+          builder.addOne(expressionBinding)
+        case _ =>
+      }
+    )
+    builder.result()
+  }
+
+  /** Get the [[Expression.Block]] containing the provided expression.
+    *
+    * @param scope the scope where to look
+    * @param expression the expression to look for
+    * @return the block containing the provided expression
+    */
+  def getExpressionBlock(
+    scope: IR,
+    expression: IR
+  ): Option[Expression.Block] = {
+    val blocksBuilder = Set.newBuilder[Expression.Block]
+    IR.preorder(
+      scope,
+      {
+        case block: Expression.Block => blocksBuilder.addOne(block)
+        case _                       =>
+      }
+    )
+    val blocks = blocksBuilder.result()
+
+    blocks.find(block => findById(block, expression.getId).isDefined)
+  }
+
  /** Find usages of a local defined in the body block.
    *
    * @param ir the syntax tree
@ -63,7 +127,7 @@ trait IRUtils {
    node: Name
  ): Option[Set[Name.Literal]] =
    for {
-      usages <- findDynamicUsages(ir, node)
+      usages <- findDynamicUsages(ir, node.name)
    } yield {
      usages.collect {
        case Application.Prefix(function: Name.Literal, args, _, _, _)
@ -117,16 +181,16 @@ trait IRUtils {
  /** Find usages of a dynamic dependency in the [[DataflowAnalysis]] metadata.
    *
    * @param ir the syntax tree
-    * @param node the name to look for
+    * @param name the name to look for
    * @return the list of usages of the given name in the `ir`
    */
  private def findDynamicUsages(
    ir: IR,
-    node: Name
+    name: String
  ): Option[Set[IR]] = {
    for {
      metadata <- ir.getMetadata(DataflowAnalysis)
-      key = DataflowAnalysis.DependencyInfo.Type.Dynamic(node.name, None)
+      key = DataflowAnalysis.DependencyInfo.Type.Dynamic(name, None)
      dependents <- metadata.dependents.get(key)
    } yield {
      dependents
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/RefactoringRenameJob.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/RefactoringRenameJob.scala
@ -58,6 +58,13 @@ final class RefactoringRenameJob(
              )
            )
            Seq()
+          case ex: RefactoringRenameJob.DefinitionAlreadyExists =>
+            reply(
+              Api.SymbolRenameFailed(
+                Api.SymbolRenameFailed.DefinitionAlreadyExists(ex.name)
+              )
+            )
+            Seq()
          case ex: RefactoringRenameJob.FailedToApplyEdits =>
            reply(
              Api.SymbolRenameFailed(
@ -95,6 +102,26 @@ final class RefactoringRenameJob(
        throw new RefactoringRenameJob.OperationNotSupported(expressionId)
      )

+    // check if global definition exists
+    methodDefinition.foreach { _ =>
+      val moduleDefs =
+        IRUtils.findModuleDefinitions(module.getIr, newSymbolName)
+      if (moduleDefs.nonEmpty) {
+        throw new RefactoringRenameJob.DefinitionAlreadyExists(newSymbolName)
+      }
+    }
+
+    // check if local definition exists
+    local.foreach { symbol =>
+      val scopeOpt = IRUtils.getExpressionBlock(module.getIr, symbol)
+      scopeOpt.foreach { scope =>
+        val localDefs = IRUtils.findLocalDefinitions(scope, newSymbolName)
+        if (localDefs.nonEmpty) {
+          throw new RefactoringRenameJob.DefinitionAlreadyExists(newSymbolName)
+        }
+      }
+    }
+
    def localUsages = local.flatMap(IRUtils.findLocalUsages(module.getIr, _))
    def methodDefinitionUsages = methodDefinition.flatMap(
      IRUtils.findModuleMethodUsages(module.getName, module.getIr, _)
@ -179,6 +206,9 @@ object RefactoringRenameJob {
  final private class ExpressionNotFound(val expressionId: UUID @ExternalID)
      extends Exception(s"Expression was not found by id [$expressionId].")

+  final private class DefinitionAlreadyExists(val name: String)
+      extends Exception(s"Definition [$name] already exists in scope")
+
  final private class FailedToApplyEdits(val module: String)
      extends Exception(s"Failed to apply edits to module [$module]")

--- a/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeRefactoringTest.scala
+++ b/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeRefactoringTest.scala
@ -1067,4 +1067,235 @@ class RuntimeRefactoringTest
    )
    context.consumeOut shouldEqual List()
  }
+
+  it should "fail to rename module method when module definition exists" in {
+    val contextId  = UUID.randomUUID()
+    val requestId  = UUID.randomUUID()
+    val moduleName = "Enso_Test.Test.Main"
+
+    val metadata    = new Metadata
+    val idFunction1 = metadata.addItem(31, 9)
+    val code =
+      """from Standard.Base import all
+        |
+        |function1 x = x + 1
+        |
+        |function2 = Nothing
+        |
+        |main =
+        |    operator1 = 41
+        |    operator2 = x -> Main.function1 x
+        |    IO.println (operator2 operator1)
+        |""".stripMargin.linesIterator.mkString("\n")
+    val contents = metadata.appendToCode(code)
+    val mainFile = context.writeMain(contents)
+
+    // create context
+    context.send(Api.Request(requestId, Api.CreateContextRequest(contextId)))
+    context.receive shouldEqual Some(
+      Api.Response(requestId, Api.CreateContextResponse(contextId))
+    )
+
+    // open file
+    context.send(
+      Api.Request(requestId, Api.OpenFileRequest(mainFile, contents))
+    )
+    context.receive shouldEqual Some(
+      Api.Response(Some(requestId), Api.OpenFileResponse)
+    )
+
+    // push main
+    context.send(
+      Api.Request(
+        requestId,
+        Api.PushContextRequest(
+          contextId,
+          Api.StackItem.ExplicitCall(
+            Api.MethodPointer(moduleName, moduleName, "main"),
+            None,
+            Vector()
+          )
+        )
+      )
+    )
+
+    context.receiveNIgnoreStdLib(2) should contain theSameElementsAs Seq(
+      Api.Response(requestId, Api.PushContextResponse(contextId)),
+      context.executionComplete(contextId)
+    )
+    context.consumeOut shouldEqual List("42")
+
+    // rename operator1
+    val newName = "function2"
+    context.send(
+      Api.Request(requestId, Api.RenameSymbol(moduleName, idFunction1, newName))
+    )
+    context.receiveNIgnoreStdLib(1) should contain theSameElementsAs Seq(
+      Api.Response(
+        requestId,
+        Api.SymbolRenameFailed(
+          Api.SymbolRenameFailed.DefinitionAlreadyExists(newName)
+        )
+      )
+    )
+    context.consumeOut shouldEqual List()
+  }
+
+  it should "fail to rename operator when local definition exists" in {
+    val contextId  = UUID.randomUUID()
+    val requestId  = UUID.randomUUID()
+    val moduleName = "Enso_Test.Test.Main"
+
+    val metadata    = new Metadata
+    val idOperator1 = metadata.addItem(42, 9)
+    val code =
+      """from Standard.Base import all
+        |
+        |main =
+        |    operator1 = 41
+        |    operator2 = operator1 + 1
+        |    IO.println operator2
+        |""".stripMargin.linesIterator.mkString("\n")
+    val contents = metadata.appendToCode(code)
+    val mainFile = context.writeMain(contents)
+
+    // create context
+    context.send(Api.Request(requestId, Api.CreateContextRequest(contextId)))
+    context.receive shouldEqual Some(
+      Api.Response(requestId, Api.CreateContextResponse(contextId))
+    )
+
+    // open file
+    context.send(
+      Api.Request(requestId, Api.OpenFileRequest(mainFile, contents))
+    )
+    context.receive shouldEqual Some(
+      Api.Response(Some(requestId), Api.OpenFileResponse)
+    )
+
+    // push main
+    context.send(
+      Api.Request(
+        requestId,
+        Api.PushContextRequest(
+          contextId,
+          Api.StackItem.ExplicitCall(
+            Api.MethodPointer(moduleName, moduleName, "main"),
+            None,
+            Vector()
+          )
+        )
+      )
+    )
+
+    context.receiveNIgnoreStdLib(2) should contain theSameElementsAs Seq(
+      Api.Response(requestId, Api.PushContextResponse(contextId)),
+      context.executionComplete(contextId)
+    )
+    context.consumeOut shouldEqual List("42")
+
+    // rename operator1
+    val newName = "operator2"
+    context.send(
+      Api.Request(requestId, Api.RenameSymbol(moduleName, idOperator1, newName))
+    )
+    context.receiveNIgnoreStdLib(1) should contain theSameElementsAs Seq(
+      Api.Response(
+        requestId,
+        Api.SymbolRenameFailed(
+          Api.SymbolRenameFailed.DefinitionAlreadyExists(newName)
+        )
+      )
+    )
+    context.consumeOut shouldEqual List()
+  }
+
+  it should "rename operator if the same definition exists in different method body" in {
+    val contextId  = UUID.randomUUID()
+    val requestId  = UUID.randomUUID()
+    val moduleName = "Enso_Test.Test.Main"
+    val newName    = "foobarbaz"
+
+    val metadata    = new Metadata
+    val idOperator1 = metadata.addItem(42, 9)
+    val code =
+      s"""from Standard.Base import all
+         |
+         |main =
+         |    operator1 = 41
+         |    operator2 = operator1 + 1
+         |    IO.println operator2
+         |
+         |test =
+         |    $newName = 42
+         |    $newName
+         |""".stripMargin.linesIterator.mkString("\n")
+    val contents = metadata.appendToCode(code)
+    val mainFile = context.writeMain(contents)
+
+    // create context
+    context.send(Api.Request(requestId, Api.CreateContextRequest(contextId)))
+    context.receive shouldEqual Some(
+      Api.Response(requestId, Api.CreateContextResponse(contextId))
+    )
+
+    // open file
+    context.send(
+      Api.Request(requestId, Api.OpenFileRequest(mainFile, contents))
+    )
+    context.receive shouldEqual Some(
+      Api.Response(Some(requestId), Api.OpenFileResponse)
+    )
+
+    // push main
+    context.send(
+      Api.Request(
+        requestId,
+        Api.PushContextRequest(
+          contextId,
+          Api.StackItem.ExplicitCall(
+            Api.MethodPointer(moduleName, moduleName, "main"),
+            None,
+            Vector()
+          )
+        )
+      )
+    )
+
+    context.receiveNIgnoreStdLib(2) should contain theSameElementsAs Seq(
+      Api.Response(requestId, Api.PushContextResponse(contextId)),
+      context.executionComplete(contextId)
+    )
+    context.consumeOut shouldEqual List("42")
+
+    // rename operator1
+    val expectedEdits = Vector(
+      TextEdit(
+        model.Range(model.Position(3, 4), model.Position(3, 13)),
+        newName
+      ),
+      TextEdit(
+        model.Range(model.Position(4, 16), model.Position(4, 25)),
+        newName
+      )
+    )
+    val expectedFileEdit = Api.FileEdit(
+      context.pkg.mainFile,
+      expectedEdits,
+      versionCalculator.evalVersion(contents).toHexString,
+      versionCalculator
+        .evalVersion(contents.replaceAll("operator1", newName))
+        .toHexString
+    )
+    context.send(
+      Api.Request(requestId, Api.RenameSymbol(moduleName, idOperator1, newName))
+    )
+    context.receiveNIgnoreStdLib(4) should contain theSameElementsAs Seq(
+      Api.Response(requestId, Api.SymbolRenamed(newName)),
+      Api.Response(None, expectedFileEdit),
+      TestMessages.pending(contextId, idOperator1),
+      context.executionComplete(contextId)
+    )
+    context.consumeOut shouldEqual List("42")
+  }
 }