improvement: Better inlay Hints for anonymous implicits #6176

jkciesluk · 2024-02-29T09:36:21Z

No description provided.

jkciesluk · 2024-03-01T09:24:27Z

tests/cross/src/test/scala/tests/pc/InlayHintsSuite.scala

@@ -111,7 +132,7 @@ class InlayHintsSuite extends BaseInlayHintsSuite {
       |object Main {
       |  implicit val imp: Int = 2
       |  def addOne(x: Int)(using one: Int)/*: Int<<scala/Int#>>*/ = x + one
-       |  val x/*: Int<<scala/Int#>>*/ = addOne(1)/*(imp<<(3:15)>>)*/
+       |  val x/*: Int<<scala/Int#>>*/ = addOne(1)/*(using imp<<(3:15)>>)*/


Should we also change the label for Scala 2? If so, any ideas?

Let's leave as is for Scala 2, using makes sense in case of Scala 3 but with Scala 2 the code doesn't usually contain anything and would be weird to have a decoration with invalid code show.

tgodzik

Looks good!

tgodzik · 2024-03-01T12:41:21Z

tests/cross/src/test/scala/tests/pc/InlayHintsSuite.scala

@@ -111,7 +132,7 @@ class InlayHintsSuite extends BaseInlayHintsSuite {
       |object Main {
       |  implicit val imp: Int = 2
       |  def addOne(x: Int)(using one: Int)/*: Int<<scala/Int#>>*/ = x + one
-       |  val x/*: Int<<scala/Int#>>*/ = addOne(1)/*(imp<<(3:15)>>)*/
+       |  val x/*: Int<<scala/Int#>>*/ = addOne(1)/*(using imp<<(3:15)>>)*/


Let's leave as is for Scala 2, using makes sense in case of Scala 3 but with Scala 2 the code doesn't usually contain anything and would be weird to have a decoration with invalid code show.

tgodzik · 2024-03-01T12:43:05Z

tests/cross/src/test/scala/tests/pc/InlayHintsSuite.scala

@@ -744,4 +814,37 @@ class InlayHintsSuite extends BaseInlayHintsSuite {
       |case class ErrorMessage(error)
       |""".stripMargin
  )
+
+  // NOTE: We don't show inlayHints for anonymous given instances


We can do it as an improvement later on,no?

rochala · 2024-03-01T13:24:46Z

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

@@ -77,13 +76,24 @@ class PcInlayHintsProvider(
          if params.implicitParameters() =>
        val labelParts = symbols.map(s => List(labelPart(s, s.decodedName)))
        val label =
-          if allImplicit then labelParts.separated("(", ", ", ")")
+          if allImplicit then labelParts.separated("(using ", ", ", ")")


How would this work with context bounds e.g def test[T: Ordering](x: T)(using Int) = ???

I think this is one test that should be added to ensure that we don't show evidence params (context bounds are appended to last implicit parameter list if it exists, and it always adds evidences as implicits).

Also when i use this like this:

def test[T: Ordering](x: T)(using Int) = ??? test(1)

I'd expect it to look like this:

def test[T: Ordering](x: T)(using Int) = ??? given test: Int = 5 test/*[Int]*/(1)/*(using test)*/

and when there are is no implicit param list then:

def test[T: Ordering](x: T) = ??? test/*[Int]*/(1)

rochala · 2024-03-01T13:33:00Z

tests/unit/src/test/resources/inlayHints3/example/GivenAlias.scala

@@ -28,7 +28,7 @@ trait Zg[T]:
 given Xg with
  def doX/*: Int<<scala/Int#>>*/ = 7

-given (using Xg): Yg with
+given (using /*x$1<<(30:13)>>: */Xg): Yg with


If I understand correctly, in these implicit params we will add compiler generated names, but is this really helpful to the end user? I don't see any use case in which this can help. If someone wants to use it as value later in code, it should rather be named implicit param.

I'd at least add option to hide those decorations as I see them as unnecessary noise.

Maybe we could rename the synthetic compiler generated names to something like <anonymous>? 🤔 The nice thing about these hints is that they provide a "go to definition" functionality, which can still be helpful. 🙂

I would keep x$1 since this an actual value that you can use. So best to be consistent with the compiler

But shouldn't you use summon[Xg] instead ?

I don't think it's necessary and it would harder to see where it's actually defined.

We could do if it shows up in both I think, if this feels a problems otherwise for you

The problem is in keeping those decorations minimal. There will be tons of them, and in my opinion we should focus on displaying actually useful information. This may work, but even in simple projects, it will be added to almost every method to e.g. pass execution context which most users won't name. As long as we can go from usage to source, it's fine. The fact that decoration is valid Scala code is not as important as it is to show useful info.

Coming back to it, you are assuming that some information will not be useful. And it might not be useful to you, but might be useful to someone else. Having the added name at definition fits very well with all the other decorations, where we add them in places that the compiler will add something itself. We also have options to turn off certain decorations, to toggle them etc. So if there is too much at any point, they can be easily hidden. Also this is not really an issue in cases that decorations don't show all together.

I still don't agree that this can be useful to anyone. Compiler will report error on ambiguity, and you can still find the source, I don't think anything else is necessary.

If we go with this logic there are tons of other scenarios in which we can show such decorations, even the context bounds so we also should adjust them to display where is this name coming from:

def test[T: Ordering](x: List[T])/*(using $evidence1: Ordering[T])*/ = x.sorted/*(using $evidence1)*/

Which I think right now would lead to the same issue of not knowing where $evidence1 comes from

def test[T: Ordering](x: List[T]) = x.sorted/*(using $evidence1)*/

and even in this simple example it will already look cluttered:

def test[T: Ordering](x: List[T])/*(using $evidence1: Ordering[T])*/ = test/*[T]*/(x)/*(using $evidence1)*/

so maybe another representation may help with this ?

def test[T: Ordering](x: List[T]) = x.sorted/*(using Ordering)*/

and how would it look in the scenario:

def test[T: Ordering](x: List[T]) = x.sorted trait Foo given Ordering[Foo] = ??? test(List.empty[Foo])

would it be:

def test[T: Ordering](x: List[T])/*(using $evidence1: Ordering[T])*/ = x.sorted/*(using $evidence1)*/ trait Foo given Ordering[Foo] = ??? test/*[Foo]*/(List.empty[Foo])/*(using given_Ordering_Foo)*/

? Maybe we should also consider something shorter:

test/*[Foo: Ordering]*/(List.empty[Foo])

and trying to go to definiton on Ordering would put you in proper definition ?

Nevertheless, these are just the ideas. If we add an option to configure it at that level, I'm fine with this change, but afaik at the moment we can only control to hide / display implicits all together. We could make it available via verbose setting and leave it normal with on, or even more customisation options, including other decorations.

I think that because users can precisely decide which decorations they find useful, there is no reason to remove this feature (inlay hints in anonymous given definitions), let's just hide it under a separate option.

Nevertheless, these are just the ideas. If we add an option to configure it at that level, I'm fine with this change, but afaik at the moment we can only control to hide / display implicits all together. We could make it available via verbose setting and leave it normal with on, or even more customisation options, including other decorations.

I am fine with this, in this PR we can only add using to implicit parameters and in a follow up decouple decorations for context bounds parameters and add decorations in definitions, both under separate options.

scalameta/metals#6176 scalameta/metals#6173

scalameta/metals#6176 scalameta/metals#6173 [Cherry-picked 3694d95]

jkciesluk added the backport-scala3-pc label Feb 29, 2024

jkciesluk force-pushed the anon-given branch from aaa891b to 340ecd4 Compare February 29, 2024 14:25

jkciesluk commented Mar 1, 2024

View reviewed changes

jkciesluk requested review from kasiaMarek, tgodzik and rochala March 1, 2024 09:24

tgodzik approved these changes Mar 1, 2024

View reviewed changes

rochala reviewed Mar 1, 2024

View reviewed changes

improvement: Better inlay Hints for anonymous implicits

fa155a5

jkciesluk force-pushed the anon-given branch from 340ecd4 to fa155a5 Compare March 6, 2024 11:09

jkciesluk merged commit f6a1de4 into scalameta:main Mar 7, 2024
20 of 26 checks passed

jkciesluk deleted the anon-given branch March 7, 2024 13:58

kasiaMarek mentioned this pull request Mar 11, 2024

Weekly metals backport scala/scala3#19921

Merged

kasiaMarek added a commit to scala/scala3 that referenced this pull request Mar 12, 2024

Weekly metals backport (#19921)

3694d95

scalameta/metals#6176 scalameta/metals#6173

jkciesluk mentioned this pull request Mar 13, 2024

Show using in synthetic inlay hints for using parameters #6148

Closed

kasiaMarek removed the backport-scala3-pc label Mar 15, 2024

WojciechMazur pushed a commit to scala/scala3 that referenced this pull request Jul 2, 2024

Weekly metals backport (#19921)

762b8a3

scalameta/metals#6176 scalameta/metals#6173 [Cherry-picked 3694d95]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: Better inlay Hints for anonymous implicits #6176

improvement: Better inlay Hints for anonymous implicits #6176

jkciesluk commented Feb 29, 2024

jkciesluk Mar 1, 2024

tgodzik Mar 1, 2024

tgodzik left a comment

tgodzik Mar 1, 2024

tgodzik Mar 1, 2024

rochala Mar 1, 2024 •

edited

Loading

rochala Mar 1, 2024 •

edited

Loading

filipwiech Mar 1, 2024 •

edited

Loading

tgodzik Mar 1, 2024

rochala Mar 1, 2024

tgodzik Mar 1, 2024

tgodzik Mar 1, 2024

rochala Mar 1, 2024

tgodzik Mar 6, 2024

rochala Mar 6, 2024

jkciesluk Mar 6, 2024

improvement: Better inlay Hints for anonymous implicits #6176

improvement: Better inlay Hints for anonymous implicits #6176

Conversation

jkciesluk commented Feb 29, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochala Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

rochala Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

filipwiech Mar 1, 2024 • 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

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

Choose a reason for hiding this comment

rochala Mar 1, 2024 •

edited

Loading

rochala Mar 1, 2024 •

edited

Loading

filipwiech Mar 1, 2024 •

edited

Loading