Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConvertToNamedArguments code action #3971

Merged
merged 22 commits into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ final class CodeActionProvider(
new PatternMatchRefactor(trees),
new RewriteBracesParensCodeAction(trees),
new ExtractValueCodeAction(trees, buffers),
new CreateCompanionObjectCodeAction(trees, buffers)
new CreateCompanionObjectCodeAction(trees, buffers),
new ConvertToNamedArguments(trees)
)

def codeActions(
Expand Down
16 changes: 16 additions & 0 deletions metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,22 @@ class Compilers(
}
}.getOrElse(Future.successful(Nil.asJava))

def convertToNamedArguments(
position: TextDocumentPositionParams,
argIndices: ju.List[Integer],
token: CancelToken
): Future[ju.List[TextEdit]] = {
withPCAndAdjustLsp(position) { (pc, pos, adjust) =>
pc.convertToNamedArguments(
CompilerOffsetParams.fromPos(pos, token),
argIndices
).asScala
.map { edits =>
adjust.adjustTextEdits(edits)
}
}
}.getOrElse(Future.successful(Nil.asJava))

def implementAbstractMembers(
params: TextDocumentPositionParams,
token: CancelToken
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1983,6 +1983,25 @@ class MetalsLanguageServer(
} yield ().asInstanceOf[Object]
}

case ServerCommands.ConvertToNamedArguments(
ServerCommands.ConvertToNamedArgsRequest(position, argIndices)
) =>
CancelTokens.future { token =>
val uri = position.getTextDocument().getUri()
for {
edits <- compilers.convertToNamedArguments(
position,
argIndices,
token
)
if (!edits.isEmpty())
workspaceEdit = new l.WorkspaceEdit(Map(uri -> edits).asJava)
_ <- languageClient
.applyEdit(new ApplyWorkspaceEditParams(workspaceEdit))
.asScala
} yield ().asInstanceOf[Object]
}

case ServerCommands.ExtractMemberDefinition(textDocumentParams) =>
val data = ExtractMemberDefinitionData(textDocumentParams)
val future = for {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import ch.epfl.scala.{bsp4j => b}
import org.eclipse.lsp4j.Location
import org.eclipse.lsp4j.TextDocumentIdentifier
import org.eclipse.lsp4j.TextDocumentPositionParams
import java.{util => ju}

/**
* LSP commands supported by the Metals language server.
Expand Down Expand Up @@ -467,6 +468,21 @@ 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 GotoLog = new Command(
"goto-log",
"Check logs",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package scala.meta.internal.metals.codeactions

import scala.concurrent.ExecutionContext
import scala.concurrent.Future

import scala.meta.Term
import scala.meta.Tree
import scala.meta.internal.metals.CodeAction
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.metals.ServerCommands
import scala.meta.internal.parsing.Trees
import scala.meta.pc.CancelToken

import org.eclipse.{lsp4j => l}

class ConvertToNamedArguments(trees: Trees) extends CodeAction {

import ConvertToNamedArguments._
override val kind: String = l.CodeActionKind.RefactorRewrite

def firstApplyWithUnnamedArgs(
term: Option[Tree]
): Option[ApplyTermWithArgIndices] = {
term match {
case Some(apply: Term.Apply) =>
val argIndices = apply.args.zipWithIndex.collect {
case (arg, index)
if !arg.isInstanceOf[Term.Assign] && !arg
.isInstanceOf[Term.Block] =>
index
}
if (argIndices.isEmpty) firstApplyWithUnnamedArgs(apply.parent)
else Some(ApplyTermWithArgIndices(apply, argIndices))
case Some(t) => firstApplyWithUnnamedArgs(t.parent)
case _ => None
}
}

override def contribute(params: l.CodeActionParams, token: CancelToken)(
implicit ec: ExecutionContext
): Future[Seq[l.CodeAction]] = {

val path = params.getTextDocument().getUri().toAbsolutePath
val range = params.getRange()

val maybeApply = for {
term <- trees.findLastEnclosingAt[Term.Apply](path, range.getStart())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

apply <- firstApplyWithUnnamedArgs(Some(term))
} yield apply

maybeApply
.map { apply =>
{
val codeAction = new l.CodeAction(title(apply.app.fun.syntax))
codeAction.setKind(l.CodeActionKind.RefactorRewrite)
val position = new l.TextDocumentPositionParams(
params.getTextDocument(),
new l.Position(apply.app.pos.endLine, apply.app.pos.endColumn)
)
codeAction.setCommand(
ServerCommands.ConvertToNamedArguments.toLSP(
ServerCommands
.ConvertToNamedArgsRequest(
position,
apply.argIndices.map(new Integer(_)).asJava
)
)
)
Future.successful(Seq(codeAction))
}
}
.getOrElse(Future.successful(Nil))
}
}

object ConvertToNamedArguments {
case class ApplyTermWithArgIndices(app: Term.Apply, argIndices: List[Int])
def title(funcName: String) = s"Convert $funcName to named arguments"
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ public abstract class PresentationCompiler {
*/
public abstract CompletableFuture<List<TextEdit>> insertInferredType(OffsetParams params);

/**
* Return named arguments for the apply method that encloses the given position.
*/
public abstract CompletableFuture<List<TextEdit>> convertToNamedArguments(OffsetParams params, List<Integer> argIndices);

/**
* The text contents of the fiven file changed.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package scala.meta.internal.pc

import scala.meta.pc.OffsetParams

import org.eclipse.{lsp4j => l}

final class ConvertToNamedArgumentsProvider(
val compiler: MetalsGlobal,
params: OffsetParams,
argIndices: Set[Int]
) {

import compiler._
def convertToNamedArguments: List[l.TextEdit] = {
val unit = addCompilationUnit(
code = params.text(),
filename = params.uri().toString(),
cursor = None
)

val typedTree = typedTreeAt(unit.position(params.offset))
tanishiking marked this conversation as resolved.
Show resolved Hide resolved
typedTree match {
case Apply(fun, args) =>
args.zipWithIndex
.zip(fun.tpe.params)
.collect {
case ((arg, index), param) if argIndices.contains(index) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tanishiking tanishiking Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val position = arg.pos.toLSP
position.setEnd(position.getStart())
new l.TextEdit(position, s"${param.nameString} = ")
}
}
case _ => Nil
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,20 @@ case class ScalaPresentationCompiler(
}
}

override def convertToNamedArguments(
params: OffsetParams,
argIndices: ju.List[Integer]
): CompletableFuture[ju.List[TextEdit]] = {
val empty: ju.List[TextEdit] = new ju.ArrayList[TextEdit]()
compilerAccess.withInterruptableCompiler(empty, params.token) { pc =>
new ConvertToNamedArgumentsProvider(
pc.compiler(),
params,
argIndices.asScala.map(_.toInt).toSet
).convertToNamedArguments.asJava
}
}

override def autoImports(
name: String,
params: OffsetParams
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ case class ScalaPresentationCompiler(
.asJava
}

override def convertToNamedArguments(
params: OffsetParams,
argIndices: ju.List[Integer]
): CompletableFuture[ju.List[l.TextEdit]] =
CompletableFuture.completedFuture(Nil.asJava)

override def selectionRange(
params: ju.List[OffsetParams]
): CompletableFuture[ju.List[l.SelectionRange]] =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package tests.pc

import java.net.URI

import scala.meta.internal.jdk.CollectionConverters._
import scala.meta.internal.metals.CompilerOffsetParams
import scala.meta.internal.metals.TextEdits

import munit.Location
import munit.TestOptions
import org.eclipse.{lsp4j => l}
import tests.BaseCodeActionSuite

class ConvertToNamedArgumentsSuite extends BaseCodeActionSuite {

override protected def requiresScalaLibrarySources: Boolean = true

checkEdit(
"scala-std-lib",
"""|object A{
| val a = <<scala.math.max(1, 2)>>
|}""".stripMargin,
List(0, 1),
"""|object A{
| val a = scala.math.max(x = 1, y = 2)
|}""".stripMargin
)

def checkEdit(
name: TestOptions,
original: String,
argIndices: List[Int],
expected: String,
compat: Map[String, String] = Map.empty
)(implicit location: Location): Unit =
test(name) {
val edits = convertToNamedArgs(original, argIndices)
val (code, _, _) = params(original)
val obtained = TextEdits.applyEdits(code, edits)
assertNoDiff(obtained, getExpected(expected, compat, scalaVersion))
}

def convertToNamedArgs(
original: String,
argIndices: List[Int],
filename: String = "file:/A.scala"
): List[l.TextEdit] = {
val (code, _, offset) = params(original)
val result = presentationCompiler
.convertToNamedArguments(
CompilerOffsetParams(URI.create(filename), code, offset, cancelToken),
argIndices.map(new Integer(_)).asJava
)
.get()
result.asScala.toList
}

}
23 changes: 17 additions & 6 deletions tests/unit/src/main/scala/tests/TestingServer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -1161,7 +1168,8 @@ final class TestingServer(
filename: String,
query: String,
root: AbsolutePath,
kind: List[String]
kind: List[String],
filterAction: l.CodeAction => Boolean
): Future[(List[l.CodeAction], String)] =
for {
(_, params) <- codeActionParams(
Expand All @@ -1173,10 +1181,13 @@ final class TestingServer(
if (kind.nonEmpty) kind.asJava else null
)
)
codeActions <- server.codeAction(params).asScala
codeActions <- server
.codeAction(params)
.asScala
.map(_.asScala.filter(filterAction))
} yield (
codeActions.asScala.toList,
codeActions.map(_.getTitle()).asScala.mkString("\n")
codeActions.toList,
codeActions.map(_.getTitle()).mkString("\n")
)

def assertHighlight(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@ import scala.meta.internal.metals.{BuildInfo => V}
import munit.Location
import munit.TestOptions
import tests.BaseLspSuite
import org.eclipse.lsp4j.CodeAction

abstract class BaseCodeActionLspSuite(suiteName: String)
extends BaseLspSuite(suiteName) {
abstract class BaseCodeActionLspSuite(
suiteName: String,
filterAction: CodeAction => Boolean = _ => true
) extends BaseLspSuite(suiteName) {

protected val scalaVersion: String = V.scala213

Expand Down Expand Up @@ -83,7 +86,8 @@ abstract class BaseCodeActionLspSuite(suiteName: String)
path,
changeFile(input),
expectedActions,
kind
kind,
filterAction = filterAction
)
.recover {
case _: Throwable if expectError => Nil
Expand Down
Loading