Add ConvertToNamedArguments code action

[camgraff]

@ maintainers thanks for all the work you do! This is my first PR, I appreciate any feedback 🙂

Fixes #4059

Adds a code action which converts all positional arguments in the enclosing apply method to named args.

TODOS/open questions:
[ ] - Scala 3 support
[ ] - tests. I started by trying to extend BasicCodeActionLspSuite, but ran into this error with the presentation compiler. I'd appreciate some direction here :)

Caused by: java.lang.AbstractMethodError: Missing implementation of resolved method 'abstract java.util.concurrent.CompletableFuture convertToNamedArguments(scala.meta.pc.OffsetParams, int)' of abstract class scala.meta.pc.PresentationCompiler.

[ ] - Currently, if there are no args in the enclosing apply the code action is not available which means if my cursor were on params.text() in the gif above, I'd see no code action. Maybe we'd want to consider the next enclosing apply (ie addCompilationUnit) as a candidate?
[ ] - There's probably some edge cases that I haven't considered. One is when the apply is inside a block which can't have named args.

[tanishiking]

@camgraff This is really great!!! Thank you very much for working on this.

> tests. I started by trying to extend BasicCodeActionLspSuite, but ran into this error with the presentation compiler. I'd appreciate some direction here :)

I think this is because there's no implementation in scala3/....../ScalaPresentationCompiler.scala, you have to add something like this

  override def convertToNamedArguments(
      params: OffsetParams,
      numUnnamedArgs: Int
  ): CompletableFuture[ju.List[TextEdit]] = ???

---

> Scala3 Support

• For Scala3 support, my WIP implementation of show-parameter-name decoration PR may help :)
  • BTW, I'm planning to implement this feature based on Inlay Hint that enables us to insert parameter names to the code easily. However, this CodeAction is still pretty useful because
    • This CodeAction can insert names for all arguments, while we have to insert one by one with inlay hint.
    • Inlay hint might not be available for some editors, in that case, we have to provide this kind of features as CodeAction anyway.

---

Other improvement

Also, there's something to improve:

ConvertToNamedArguments#contribute will run every time users open / edit a file. Every time users open / edit a file, Metals calculate the code actions for all the Applys in the file. This may slow down the overall performance of the Metals server.

We should separate showing a code action tooltip and constructing the TextEdit part. For example InsertInferredType code action do the thing:

• InsertInferredType#contribute construct codeActions for a file, but it doesn't contain TextEdit, it has Command instead.
• When users run the codeAction, Metals will receive the command and construct the TextEdit
  • Here is the endpoint for the command.

---

> There's probably some edge cases that I haven't considered. One is when the apply is inside a block which can't have named args.

I think you don't have to worry too much about these edge cases :) If users add parameter names and compile failed, users can immediately notice it doesn't work and undo the code action.

There might be other edge cases, but we can improve this feature iteratively 👍

---

> Maybe we'd want to consider the next enclosing apply (ie, addCompilationUnit) as a candidate?

Sounds good! However, I'm concerned that it may introduce complexities.

In the scenario of this gif, it looks useful, but if the no-parameter method call is surrounded by another method call far away from the inner method call, falling back to the surrounding method call might be confusing for users.
I would go without this feature (next enclosing apply), at least in this PR, see the reactions from users. If many people agree it would help, consider falling back to the next enclosing apply. I left a comment https://github.com/scalameta/metals/pull/3971/files#r886297262

[tanishiking]

Also, I think it's ok to work on Scala3 support in another PR if you want :)

[camgraff]

Thanks @tanishiking for the pointers! I've made the code action delegate to a ServerCommand to return the edits. I've also excluded Term.Block args from the action and added logic to consider the next enclosing Term.Apply if the current one has no unnamed args.

[camgraff]

Looks like many of the LSP code action tests are failing due to this action now being returned by the server. Is the best way to fix this just going through and updating expectedActions for each failing test?

[tanishiking]

@camgraff Thank you for working on this! I'll take a look today

[tanishiking]

@camgraff

Is the best way to fix this just going through and updating expectedActions for each failing test?

Good point!

option1

Yes, basically we can fix those test failures by updating expectedActions as follows. And, as you may notice check method has selectedActionIndex parameter to select a code action to apply from the received list of code actions 👍

However, I personally don't think this is not the best way to fix failing tests (even though we'd been doing that).
In this way, we end up testing we receive convert to named arg code action from the server in ExtractValueLspSuite, which should focus only on extract value code action.
I believe tests should assert only things they're really interested in (in this case, ExtractValueLspSuite should exclusively verify server returns extract value code action) whether the server returns other actions or not).

diff --git a/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala b/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala
index fc5373cb2e..0868d10afd 100644
--- a/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala
+++ b/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala
@@ -1,6 +1,7 @@
 package tests.codeactions
 
 import scala.meta.internal.metals.codeactions.ExtractValueCodeAction
+import scala.meta.internal.metals.codeactions.ConvertToNamedArguments
 
 class ExtractValueLspSuite
     extends BaseCodeActionLspSuite("extractValueRewrite") {
@@ -15,7 +16,8 @@ class ExtractValueLspSuite
        |
        |}
        |""".stripMargin,
-    ExtractValueCodeAction.title,
+    s"""|${ExtractValueCodeAction.title}
+        |${ConvertToNamedArguments.title}""".stripMargin,
     """|object Main {
        |  def method2(i: Int) = ???
        |  def method1(s: String): Unit = {

---

option2

Instead of updating expectedActions, I prefer to filter code actions so that tests can focus on things they're interested in.
Like this:

diff --git a/tests/unit/src/main/scala/tests/TestingServer.scala b/tests/unit/src/main/scala/tests/TestingServer.scala
index 41c22befc1..7553009606 100644
--- a/tests/unit/src/main/scala/tests/TestingServer.scala
+++ b/tests/unit/src/main/scala/tests/TestingServer.scala
@@ -1131,10 +1131,17 @@ final class TestingServer(
       query: String,
       expected: String,
       kind: List[String],
-      root: AbsolutePath = workspace
+      root: AbsolutePath = workspace,
+      filterAction: l.CodeAction => Boolean = _ => true
   )(implicit loc: munit.Location): Future[List[l.CodeAction]] =
     for {
-      (codeActions, codeActionString) <- codeAction(filename, query, root, kind)
+      (codeActions, codeActionString) <- codeAction(
+        filename,
+        query,
+        root,
+        kind,
+        filterAction
+      )
     } yield {
       Assertions.assertNoDiff(codeActionString, expected)
       codeActions
@@ -1175,7 +1182,8 @@ final class TestingServer(
       filename: String,
       query: String,
       root: AbsolutePath,
-      kind: List[String]
+      kind: List[String],
+      filterAction: l.CodeAction => Boolean = _ => true
   ): Future[(List[l.CodeAction], String)] =
     for {
       (_, params) <- codeActionParams(
@@ -1188,10 +1196,13 @@ final class TestingServer(
         )
       )
       codeActions <- server.codeAction(params).asScala
-    } yield (
-      codeActions.asScala.toList,
-      codeActions.map(_.getTitle()).asScala.mkString("\n")
-    )
+    } yield {
+      val actions = codeActions.asScala.filter(filterAction)
+      (
+        actions.toList,
+        actions.map(_.getTitle()).mkString("\n")
+      )
+    }
 
   def assertHighlight(
       filename: String,
diff --git a/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala b/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala
index ccd3c9c8e7..522d3b30a7 100644
--- a/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala
+++ b/tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala
@@ -7,6 +7,7 @@ import scala.meta.internal.metals.{BuildInfo => V}
 
 import munit.Location
 import munit.TestOptions
+import org.eclipse.{lsp4j => l}
 import tests.BaseLspSuite
 
 abstract class BaseCodeActionLspSuite(suiteName: String)
@@ -97,7 +98,8 @@ abstract class BaseCodeActionLspSuite(suiteName: String)
       extraOperations: => Unit = (),
       fileName: String = "A.scala",
       changeFile: String => String = identity,
-      expectError: Boolean = false
+      expectError: Boolean = false,
+      filterAction: l.CodeAction => Boolean = _ => true
   )(implicit loc: Location): Unit = {
     val scalacOptionsJson =
       if (scalacOptions.nonEmpty)
@@ -133,7 +135,8 @@ abstract class BaseCodeActionLspSuite(suiteName: String)
               path,
               changeFile(input),
               expectedActions,
-              kind
+              kind,
+              filterAction = filterAction
             )
             .recover {
               case _: Throwable if expectError => Nil
diff --git a/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala b/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala
index fc5373cb2e..e3942ee4f1 100644
--- a/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala
+++ b/tests/unit/src/test/scala/tests/codeactions/ExtractValueLspSuite.scala
@@ -24,7 +24,8 @@ class ExtractValueLspSuite
        |  }
        |
        |}
-       |""".stripMargin
+       |""".stripMargin,
+    filterAction = action => action.getTitle() == ExtractValueCodeAction.title
   )
 
   check(

[tanishiking]

I left several comments on the test, and some nitpick comments, but overall it looks good to me!

[tgodzik]

This looks great! There are few tests failing, but I think it's fine to add the code action in most of them. I wouldn't filter it out everywhere (maybe just in case of noAction), since there might be situations that we show the code action in too many places

[tgodzik]

Does the compiler have any idea that there is a named parameter here? This might actually be the case, but just wanted to double check. We wouldn't need to calculate it before in that case. Otherwise, that's probably the right approach.

[tanishiking]

Looks like ArgCompletions check if the argument is named one or not like

metals/mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala

Lines 29 to 38 in 2cee762

	lazy val isNamed: Set[Name] = apply.args.iterator
	.filterNot(_ == ident)
	.zip(baseParams.iterator)
	.map {
	case (AssignOrNamedArg(Ident(name), _), _) =>
	name
	case (_, param) =>
	param.name
	}
	.toSet

I haven't confirmed yet, but we might be able to check it's named argument or not without indices.

edit: sorry, this is from Scala2

[camgraff]

I had tried this originally, but it seems like the compiler doesn't pick up on NamedArg. e.g. when I trigger the code action on

addCompilationUnit(
      params.text(),
      filename = params.uri().toString(),
      cursor  = None
    )

and add some logging, it shows the args as:

ConvertToNamedArgumentsProvider.scala:25 a: Apply(
  fun = Select(
    qualifier = Select(qualifier = This(qual = ConvertToNamedArgumentsProvider), name = params),
    name = text
  ),
  args = List()
)
ConvertToNamedArgumentsProvider.scala:26 a.isInstanceOf[NamedArg]: false
ConvertToNamedArgumentsProvider.scala:25 a: Apply(
  fun = Select(
    qualifier = Apply(
      fun = Select(
        qualifier = Select(qualifier = This(qual = ConvertToNamedArgumentsProvider), name = params),
        name = uri
      ),
      args = List()
    ),
    name = toString
  ),
  args = List()
)
ConvertToNamedArgumentsProvider.scala:26 a.isInstanceOf[NamedArg]: false
ConvertToNamedArgumentsProvider.scala:25 a: Select(qualifier = Ident(name = scala), name = None)
ConvertToNamedArgumentsProvider.scala:26 a.isInstanceOf[NamedArg]: false

[tgodzik]

Maybe we could add this in a specific test case? I thinking that it actually helps us to see where the new action pops up and double check if it actually shows up there correctly.

[camgraff]

sounds good, I will move it to check()

[tgodzik]

Suggested change

	term <- trees.findLastEnclosingAt[Term.Apply](path, range.getStart())
	term <- trees.findLastEnclosingAt[Term.Apply](path, range.getStart())
	if !term.fun.encloses(range)

So that we don't get the code action when at for example F<<>>uture.succesfull(1)

[tanishiking]

LGTM! great job!

[tanishiking]

Let's wait for @tgodzik review, BTW, it would be awesome if you can squash commits into more meaningful granularities, at least squashing "scalafmt", "scalafix", and "add/fix tests" commits would be helpful.

[tgodzik]

LGTM!

[tgodzik]

And of course I forgot to squash 🤦‍♂️ Maybe we should set it as a default