Add convertToNamedParameters support for Scala 3 #4131

jkciesluk · 2022-07-11T12:17:15Z

Closes #4059
Add Scala 3 support for #3971

tanishiking

Thank you for working on this @jkciesluk !

This implementation converts all Apply to named arguments instead of selected Apply, we have to convert only the selected one (in the following case, we should convert only the arguments of foo method).

We should add unit test that has multiple Apply

Also, we would need to copy some test suite from https://github.com/scalameta/metals/blob/ec89eac187c5c9e1358a8229ebac85b64a71a8a9/tests/unit/src/test/scala/tests/codeactions/ConvertToNamedArgumentsLspSuite.scala to https://github.com/scalameta/metals/blob/ec89eac187c5c9e1358a8229ebac85b64a71a8a9/tests/slow/src/test/scala/tests/feature/Scala3CodeActionLspSuite.scala (I'm wondering we can cross run those LSP test 🤔 but not this time)

tanishiking · 2022-07-12T02:09:45Z

metals/src/main/scala/scala/meta/internal/metals/codeactions/ConvertToNamedArguments.scala

-      .map(ScalaVersions.isScala3Version)
-      .getOrElse(false)
-    if (isScala3) Future.successful(Nil)
-    else {


tanishiking · 2022-07-12T02:26:19Z

mtags/src/main/scala-3/scala/meta/internal/pc/ConvertToNamedArgumentsProvider.scala

+              .foreach(result.addOne(_))
+          case t =>
+            traverseChildren(t)
+    traverser.traverse(tree)(using newctx)


Instead of traversing the whole tree, it would be useful to check the enclosing tree (of the given position (via param)) using Interactive.pathTo or Interactive.enclosingTree

Hi, thank you for help!
Using pathTo works. I've also copied tests to https://github.com/scalameta/metals/blob/ec89eac187c5c9e1358a8229ebac85b64a71a8a9/tests/slow/src/test/scala/tests/feature/Scala3CodeActionLspSuite.scala and extended them by testing multiple Apply.

tanishiking

LGTM!

tanishiking · 2022-07-13T04:39:22Z

You may have to rebase on main and run scalafmt for #4050

tgodzik

Looks good! Just very minor remarks about the tests

tgodzik · 2022-07-13T16:11:30Z

tests/slow/src/test/scala/tests/feature/Scala3CodeActionLspSuite.scala

@@ -328,6 +338,264 @@ class Scala3CodeActionLspSuite
    expectNoDiagnostics = false,
  )

+  check(


We can really just add one test (first one looks good), since we only want to test the branch when we have Scala 3 dialect. All the other cases are already handled in the ConvertToNamedArgumentsLspSuite.

Anything specific to Scala 3 we can test in cross suite.

The reason is for this comment is that these tests each can last up to a couple of seconds and it's harsh on out CI.

Ah, right.

All the other cases are already handled in the ConvertToNamedArgumentsLspSuite.
ConvertToNamedArgumentsLspSuite test against Scala 2.13, it doesn't verify anything against Scala 3.

Actually, those tests that verify the behavior (rather than the first one) should be in PC tests instead of xxxLSPSuite.
Sorry, we should did this in #3971

For example, Implementing all abstract member code action
have two (three?) test suite

cross test in pc (presentation compiler)

metals/tests/cross/src/test/scala/tests/pc/AutoImplementAbstractMembersSuite.scala

Line 14 in 53249aa

class AutoImplementAbstractMembersSuite extends BaseCodeActionSuite {

These tests verify the behavior of code action, but it doesn't check the code action is available or not. Those test anyway invoke the code action even if the user won't see it's available.

These tests run against all Scala versions

You can run those tests with the specific version by ++3.1.3 cross/testOnly tests.pc.AutoImplementAbstractMemberSuite.

ImplementAbstractMemberLspSuite

metals/tests/unit/src/test/scala/tests/codeactions/ImplementAbstractMembersLspSuite.scala

Lines 5 to 6 in 53249aa

class ImplementAbstractMembersLspSuite

extends BaseCodeActionLspSuite("implementAbstractMembers") {

These test verify the code actions are available for users when we have a certain error (missing implementation error)

This test runs against only Scala 2.13

Scala3CodeActionLspSuite

metals/tests/slow/src/test/scala/tests/feature/Scala3CodeActionLspSuite.scala

Lines 332 to 358 in 53249aa

check(

"given-object-creation",

"""|package a

|

|trait Foo:

| def foo(x: Int): Int

| def bar(x: String): String

|

|given <<Foo>> with {}

|""".stripMargin,

s"""|${ImplementAbstractMembers.title}

|""".stripMargin,

"""|package a

|

|trait Foo:

| def foo(x: Int): Int

| def bar(x: String): String

|

|given Foo with {

|

| override def foo(x: Int): Int = ???

|

| override def bar(x: String): String = ???

|

|}

|""".stripMargin,

)

This test check the code action is available in Scala3. We test this because if the Scala3 compiler has different error message than Scala2, we might need to change the regex to capture the error message for missing implementation error.

This test runs against only Scala3.

So, we should migrate most of ConvertToNamedArgumentsLspSuite into pc suite (that we haven't yet implemented). but it can be done in another PR :)

So what is the difference between:

cross test in pc (presentation compiler) which have ignoreScala2 tag

slow/Scala3CodeActionLspSuite

I think I still don't get it.

Scala3CodeActionLspSuite will run the entire server, while cross tests will only invoke the compiler, which should be much faster.

It's probably fine to test if the workflow works for Scala 3, which confirms one new path for testing, while we can test anything specific to Scala 3 compiler in the cross tests.

tgodzik · 2022-07-13T16:12:35Z

tests/slow/src/test/scala/tests/feature/Scala3CodeActionLspSuite.scala

 import tests.codeactions.BaseCodeActionLspSuite

 class Scala3CodeActionLspSuite
    extends BaseCodeActionLspSuite("cross-code-actions") {

  override protected val scalaVersion: String = BuildInfo.scala3
+  val filterAction: CodeAction => Boolean = { act: CodeAction =>


Why is this needed? I see that we anyway ConvertToNamedArguments action showing up below?

tgodzik

LGTM!

tgodzik

LGTM!

tanishiking requested changes Jul 12, 2022

View reviewed changes

tanishiking reviewed Jul 12, 2022

View reviewed changes

tanishiking approved these changes Jul 13, 2022

View reviewed changes

jkciesluk force-pushed the namedparams branch from ff5b9a8 to aa67e22 Compare July 13, 2022 14:53

tgodzik requested changes Jul 13, 2022

View reviewed changes

jkciesluk requested a review from tgodzik July 14, 2022 16:36

tgodzik approved these changes Jul 15, 2022

View reviewed changes

jkciesluk and others added 6 commits July 18, 2022 14:47

Add convertToNamedParameters support for Scala 3

6b12780

Fix finding enclosing tree

9fc3ba7

Add tests to scala3codeactions, fix multiple parameters list

f3d1fe5

Add tests to scala3codeactions, fix multiple parameters list

2b13066

Remove unnecessary tests

b27e171

chore: Run scalafix

ea9012e

tgodzik force-pushed the namedparams branch from 9502978 to 30cc0f4 Compare July 18, 2022 12:51

Fixes after rebasing

79c9113

tgodzik force-pushed the namedparams branch from 30cc0f4 to 79c9113 Compare July 18, 2022 12:53

tgodzik merged commit d97fcd2 into scalameta:main Jul 18, 2022

jkciesluk deleted the namedparams branch August 9, 2022 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add convertToNamedParameters support for Scala 3 #4131

Add convertToNamedParameters support for Scala 3 #4131

jkciesluk commented Jul 11, 2022 •

edited by tanishiking

Loading

tanishiking left a comment •

edited

Loading

tanishiking Jul 12, 2022

tanishiking Jul 12, 2022

jkciesluk Jul 12, 2022

tanishiking left a comment

tanishiking commented Jul 13, 2022

tgodzik left a comment

tgodzik Jul 13, 2022

tanishiking Jul 15, 2022

kpodsiad Jul 15, 2022 •

edited

Loading

tgodzik Jul 15, 2022

tgodzik Jul 15, 2022

tgodzik Jul 13, 2022

tgodzik left a comment

tgodzik left a comment

	class ImplementAbstractMembersLspSuite
	extends BaseCodeActionLspSuite("implementAbstractMembers") {

	check(
	"given-object-creation",
	"""\|package a
	\|
	\|trait Foo:
	\| def foo(x: Int): Int
	\| def bar(x: String): String
	\|
	\|given <<Foo>> with {}
	\|""".stripMargin,
	s"""\|${ImplementAbstractMembers.title}
	\|""".stripMargin,
	"""\|package a
	\|
	\|trait Foo:
	\| def foo(x: Int): Int
	\| def bar(x: String): String
	\|
	\|given Foo with {
	\|
	\| override def foo(x: Int): Int = ???
	\|
	\| override def bar(x: String): String = ???
	\|
	\|}
	\|""".stripMargin,
	)

Add convertToNamedParameters support for Scala 3 #4131

Add convertToNamedParameters support for Scala 3 #4131

Conversation

jkciesluk commented Jul 11, 2022 • edited by tanishiking Loading

tanishiking left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanishiking left a comment

Choose a reason for hiding this comment

tanishiking commented Jul 13, 2022

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpodsiad Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

jkciesluk commented Jul 11, 2022 •

edited by tanishiking

Loading

tanishiking left a comment •

edited

Loading

kpodsiad Jul 15, 2022 •

edited

Loading