bugfix: Add name field to distinguish different action data

Also refactor another code action to detect possible conflicts I was hoping to have it cleaner, but using a stable id between codeAction request and resolve one is safest. I also check if everything is not null, so we should be safe even if someone forgets to set the name properly.
scalameta · Nov 29, 2024 · 5711c31 · 5711c31
1 parent 5fd0c0a
commit 5711c31
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 66 deletions.
diff --git a/metals/src/main/scala/scala/meta/internal/metals/ServerCommands.scala b/metals/src/main/scala/scala/meta/internal/metals/ServerCommands.scala
@@ -1,6 +1,5 @@
 package scala.meta.internal.metals
 
-import java.{util => ju}
 import javax.annotation.Nullable
 
 import scala.meta.internal.metals.newScalaFile.NewFileTypes
@@ -629,21 +628,6 @@ object ServerCommands {
        |""".stripMargin,
   )
 
-  final case class ConvertToNamedArgsRequest(
-      position: TextDocumentPositionParams,
-      argIndices: ju.List[Integer],
-  )
-  val ConvertToNamedArguments =
-    new ParametrizedCommand[ConvertToNamedArgsRequest](
-      "convert-to-named-arguments",
-      "Convert positional arguments to named ones",
-      """|Whenever a user chooses code action to convert to named arguments, this command is later run to
-         |determine the parameter names of all unnamed arguments and insert names at the correct locations.
-         |""".stripMargin,
-      """|Object with [TextDocumentPositionParams](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocumentPositionParams) of the target Apply and `numUnnamedArgs` (int)
-         |""".stripMargin,
-    )
-
   val InsertInferredMethod =
     new ParametrizedCommand[TextDocumentPositionParams](
       "insert-inferred-method",
@@ -751,7 +735,6 @@ object ServerCommands {
       CancelCompile,
       CascadeCompile,
       CleanCompile,
-      ConvertToNamedArguments,
       CopyWorksheetOutput,
       DiscoverMainClasses,
       DiscoverTestSuites,

diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeAction.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeAction.scala
@@ -3,7 +3,9 @@ package scala.meta.internal.metals.codeactions
 import scala.annotation.nowarn
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
+import scala.reflect.ClassTag
 
+import scala.meta.internal.metals.JsonParser
 import scala.meta.internal.metals.MetalsEnrichments._
 import scala.meta.internal.metals.ParametrizedCommand
 import scala.meta.pc.CancelToken
@@ -18,6 +20,33 @@ trait CodeAction {
    */
   def kind: String
 
+  private def className = this.getClass().getSimpleName()
+  protected trait CodeActionResolveData {
+    this: Product =>
+
+    /**
+     * Name is neccessary to identify what code action is being resolved.
+     *
+     * Gson will attempt to fill this field during deserialization,
+     * but if it's the wrong data the name will be wrong.
+     */
+    val codeActionName: String = className
+
+    def notNullFields: Boolean = this.productIterator.forall(_ != null)
+  }
+
+  protected def parseData[T <: CodeActionResolveData](
+      codeAction: l.CodeAction
+  )(implicit clsTag: ClassTag[T]): Option[T] = {
+    val parser = new JsonParser.Of[T]
+    codeAction.getData() match {
+      case parser.Jsonized(data)
+          if data.codeActionName == className && data.notNullFields =>
+        Some(data)
+      case _ => None
+    }
+  }
+
   /**
    * The CodeActionId for this code action, if applicable. CodeActionId is only
    * used for code actions that require the use of the presentation compiler.

diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
@@ -41,7 +41,7 @@ final class CodeActionProvider(
     new CreateCompanionObjectCodeAction(trees, buffers),
     new ExtractMethodCodeAction(trees, compilers),
     new InlineValueCodeAction(trees, compilers, languageClient),
-    new ConvertToNamedArguments(trees, compilers, languageClient),
+    new ConvertToNamedArguments(trees, compilers),
     new FlatMapToForComprehensionCodeAction(trees, buffers),
     new MillifyDependencyCodeAction(buffers),
     new MillifyScalaCliDependencyCodeAction(buffers),

diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/ConvertToNamedArguments.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/ConvertToNamedArguments.scala
@@ -8,9 +8,8 @@ import scala.meta.Term
 import scala.meta.Tree
 import scala.meta.XtensionSyntax
 import scala.meta.internal.metals.Compilers
+import scala.meta.internal.metals.JsonParser.XtensionSerializableToJson
 import scala.meta.internal.metals.MetalsEnrichments._
-import scala.meta.internal.metals.ServerCommands
-import scala.meta.internal.metals.clients.language.MetalsLanguageClient
 import scala.meta.internal.metals.codeactions.CodeAction
 import scala.meta.internal.metals.codeactions.CodeActionBuilder
 import scala.meta.internal.metals.logging
@@ -22,39 +21,40 @@ import org.eclipse.{lsp4j => l}
 class ConvertToNamedArguments(
     trees: Trees,
     compilers: Compilers,
-    languageClient: MetalsLanguageClient,
 ) extends CodeAction {
 
   import ConvertToNamedArguments._
+
+  case class ConvertToNamedArgsData(
+      position: l.TextDocumentPositionParams,
+      argIndices: java.util.List[Integer],
+  ) extends CodeActionResolveData
+
   override val kind: String = l.CodeActionKind.RefactorRewrite
 
-  override type CommandData = ServerCommands.ConvertToNamedArgsRequest
-
-  override def command: Option[ActionCommand] = Some(
-    ServerCommands.ConvertToNamedArguments
-  )
-
-  override def handleCommand(
-      data: ServerCommands.ConvertToNamedArgsRequest,
-      token: CancelToken,
-  )(implicit ec: ExecutionContext): Future[Unit] = {
-    val uri = data.position.getTextDocument().getUri()
-    for {
-      edits <- compilers.convertToNamedArguments(
-        data.position,
-        data.argIndices,
-        token,
-      )
-      _ = logging.logErrorWhen(
-        edits.isEmpty(),
-        s"Could not find the correct names for arguments at ${data.position} with indices ${data.argIndices.asScala
-            .mkString(",")}",
-      )
-      workspaceEdit = new l.WorkspaceEdit(Map(uri -> edits).asJava)
-      _ <- languageClient
-        .applyEdit(new l.ApplyWorkspaceEditParams(workspaceEdit))
-        .asScala
-    } yield ()
+  override def resolveCodeAction(codeAction: l.CodeAction, token: CancelToken)(
+      implicit ec: ExecutionContext
+  ): Option[Future[l.CodeAction]] = {
+    parseData[ConvertToNamedArgsData](codeAction) match {
+      case Some(data) =>
+        val uri = data.position.getTextDocument().getUri()
+        val result = for {
+          edits <- compilers.convertToNamedArguments(
+            data.position,
+            data.argIndices,
+            token,
+          )
+          _ = logging.logErrorWhen(
+            edits.isEmpty(),
+            s"Could not find the correct names for arguments at ${data.position} with indices ${data.argIndices.asScala
+                .mkString(",")}",
+          )
+          workspaceEdit = new l.WorkspaceEdit(Map(uri -> edits).asJava)
+          _ = codeAction.setEdit(workspaceEdit)
+        } yield codeAction
+        Some(result)
+      case None => None
+    }
   }
 
   private def getTermWithArgs(
@@ -146,19 +146,16 @@ class ConvertToNamedArguments(
             params.getTextDocument(),
             new l.Position(apply.app.pos.endLine, apply.app.pos.endColumn),
           )
-          val command =
-            ServerCommands.ConvertToNamedArguments.toLsp(
-              ServerCommands
-                .ConvertToNamedArgsRequest(
-                  position,
-                  apply.argIndices.map(Integer.valueOf).asJava,
-                )
+          val data =
+            ConvertToNamedArgsData(
+              position,
+              apply.argIndices.map(Integer.valueOf).asJava,
             )
 
           val codeAction = CodeActionBuilder.build(
             title = title(methodName(apply.app, isFirst = true)),
             kind = l.CodeActionKind.RefactorRewrite,
-            command = Some(command),
+            data = Some(data.toJsonObject),
           )
 
           Future.successful(Seq(codeAction))

diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/ExtractMethodCodeAction.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/ExtractMethodCodeAction.scala
@@ -9,7 +9,6 @@ import scala.meta.Template
 import scala.meta.Term
 import scala.meta.Tree
 import scala.meta.internal.metals.Compilers
-import scala.meta.internal.metals.JsonParser
 import scala.meta.internal.metals.JsonParser.XtensionSerializableToJson
 import scala.meta.internal.metals.MetalsEnrichments._
 import scala.meta.internal.metals.logging
@@ -23,24 +22,20 @@ class ExtractMethodCodeAction(
     trees: Trees,
     compilers: Compilers,
 ) extends CodeAction {
-  ExtractMethodCodeAction
 
-  private val parser = new JsonParser.Of[ExtractMethodParams]
-
-  private case class ExtractMethodParams(
+  private case class ExtractMethodData(
       param: l.TextDocumentIdentifier,
       range: l.Range,
       extractPosition: l.Position,
-  )
+  ) extends CodeActionResolveData
 
   override def kind: String = l.CodeActionKind.RefactorExtract
 
   override def resolveCodeAction(codeAction: l.CodeAction, token: CancelToken)(
       implicit ec: ExecutionContext
   ): Option[Future[l.CodeAction]] = {
-    val data = codeAction.getData()
-    data match {
-      case parser.Jsonized(data) =>
+    parseData[ExtractMethodData](codeAction) match {
+      case Some(data) =>
         val doc = data.param
         val uri = doc.getUri()
         val modifiedCodeAction = for {
@@ -109,7 +104,7 @@ class ExtractMethodCodeAction(
             head.pos.toLsp.getStart(),
             expr.pos.toLsp.getEnd(),
           )
-          val data = ExtractMethodParams(
+          val data = ExtractMethodData(
             params.getTextDocument(),
             exprRange,
             defnPos.pos.toLsp.getStart(),