feature: Replace synthetic decorations with inlay hints #5983

jkciesluk · 2023-12-28T13:07:52Z

No description provided.

kasiaMarek

Sometimes hover on inlay hints feels broken.
this is line 107 in Scala 3 PcCollector
https://github.com/scalameta/metals/assets/26606662/9e7af9a2-e38d-4fef-9763-9678d2e3112c
Also it doesn't update hover if you move from one inlay hint to another but both of those things may be a VSCode issue.

But in other cases hover info seems to be missing, e.g. on List in Scala 3.

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala

kasiaMarek · 2024-01-08T12:22:39Z

metals/src/main/scala/scala/meta/internal/metals/InlayHintResolveProvider.scala

+  ): Future[InlayHint] = {
+    scala.util.Try {
+      val data = inlayHint.getData().asInstanceOf[JsonArray]
+      getLabelParts(inlayHint).zip(parseData(data))


Wouldn't it be a bit cleaner to use create a class for what you're parsing and use CommonMtagsEnrichments#decodeJson?

I tried many ways for parsing this, but didn't manage to make anything cleaner work, I had issues with trying to decode optional values or Either.

I thought that org.eclipse.lsp4j.jsonrpc.messages.Either would solve this.

mtags-shared/src/main/scala/scala/meta/internal/mtags/CommonMtagsEnrichments.scala

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

mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala

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

metals-bench/src/main/scala/bench/InlayHintsBench.scala

kasiaMarek · 2024-01-08T17:18:26Z

tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala

-           |""".stripMargin,
+      _ <- server.assertInlayHints(
+        "MyTests.sc",
+        s"""|#!/usr/bin/env -S scala-cli shebang --java-opt -Xms256m --java-opt -XX:MaxRAMPercentage=80 


Do you need to pass the whole file input, won't that be available in buffers in testing server?

I think most tested methods do it this way.

I've changed the signature so the tests should be more readable.

tgodzik · 2024-01-11T19:56:06Z

mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java

@@ -167,7 +168,7 @@ public abstract CompletableFuture<List<TextEdit>> convertToNamedArguments(Offset
 	/**
 	 * Returns decorations for missing type adnotations, inferred type parameters, implicit parameters and conversions.
 	 */
-	public CompletableFuture<List<SyntheticDecoration>> syntheticDecorations(SyntheticDecorationsParams params) {


This will break mima, if we have an older mtags that is no longer release this will throw an exception.

We could do two things:

have a way to translate older syntheticDecorations to inlay hints (even if worse or not complete) <- this would be the best option

just deprecate it and have inlay hints return nothing as a default like now (this will make the decorations stop working when using older Scala versions)

Anyway I would not remove the old method

have a way to translate older syntheticDecorations to inlay hints (even if worse or not complete) <- this would be the best option

How should we approach this? In Compilers.inlayHints, if pc.inlayHints returns empty results, try to run pc.syntheticDecorations and transform the result to inlayHints?

Actually, we've added syntheticDecorations to the Presentation Compiler in the last release, so the only scala versions for which decorations would stop working are these for which we drop support in the next release (assuming in would contain inlayHints)?

We also have to take into account the compiler itself, which uses the same interface currently. So we should at least not remove the method. But we might not bother with translating decorations to inlay hints then.

tgodzik · 2024-01-11T19:56:42Z

mtags-interfaces/src/main/java/scala/meta/pc/SyntheticDecoration.java

@@ -1,12 +0,0 @@
-package scala.meta.pc;


Let's not remove it, mima should, but I think I haven't updated it previously.

tgodzik

Great work! I left some comments about the interfaces that should be handled properly before merging.

tgodzik · 2024-01-11T21:14:07Z

mtags-shared/src/main/scala/scala/meta/internal/pc/InlayHints.scala

+object InlayHints {
+  def empty: InlayHints = InlayHints(Nil, Set.empty)
+
+  def makeLabelParts(


Does this method adds the additional [, ] parts? Could add a comment and might be better to name it properly.

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

tgodzik · 2024-01-11T21:24:08Z

tests/unit/src/test/scala/tests/inlayHints/InlayHintsHoverSuite.scala

+       |  implicit val ttt: Set[Int] = Set(1)
+       |}
+       |""".stripMargin,
+    // TODO: We should show info about renames (`val ttt: S[Int]` instead of `val ttt: Set[Int]`)


That should be shown properly with the recent changes by @kasiaMarek right?

It's not possible to make it work now, because we the tooltip we show, comes from hover on a ttt definition. On ttt definition there is no information about a rename from Set to S.

But I think thats not a big issue

Ok sure, we can leave it for now, not super important. I guess we would need to handle the resolve request in the compiler to make that work. And it's maybe not worth it.

Especially that the user will see the rename + what it dealiases to, so maybe that is better.

Yes, and if there are different renames in hint position and symbol definition position, the logic could get quite messy

I feel like this could be handled if we were passing two positions to hover, one of the symbol we want to hover on, and the other one where the hover info will actually be shown, so the context for the second one could be used for printing. But it may be too optimistic to assume it will just work, and definitely not worth to consider in this PR. I still think we should create an issue considering this, we can give low-priority and leave it be, but it will be there for sake of documentation.

tgodzik

Let's rebase and merge @jkciesluk ! I think we should be good to go

Backports changes from Metals: scalameta/metals#5983

igor-ramazanov · 2024-03-06T09:56:07Z

Just wanted to say thank you for this PR, appreciate.

This will greatly help people working in less mainstream editors like Kakoune, Helix or Zed.

Thanks.

jkciesluk force-pushed the inlay-hints branch 2 times, most recently from 3ec6048 to df32716 Compare January 3, 2024 08:51

jkciesluk marked this pull request as ready for review January 3, 2024 09:07

jkciesluk mentioned this pull request Jan 3, 2024

[WIP] Inlay hints support #5572

Closed

4 tasks

jkciesluk requested review from kasiaMarek and tgodzik January 4, 2024 13:50

kasiaMarek reviewed Jan 8, 2024

View reviewed changes

jkciesluk force-pushed the inlay-hints branch 2 times, most recently from 9ea107d to a7de426 Compare January 11, 2024 16:02

tgodzik reviewed Jan 11, 2024

View reviewed changes

jkciesluk force-pushed the inlay-hints branch 4 times, most recently from 3a35fbc to c9b3015 Compare January 15, 2024 14:20

filipwiech mentioned this pull request Feb 15, 2024

Add Helix documentation #6100

Merged

tgodzik approved these changes Feb 19, 2024

View reviewed changes

jkciesluk added 6 commits February 19, 2024 10:50

feature: Replace synthetic decorations with inlay hints

853ffd0

improvement: add mkLabel method on LabelParts

db284da

improvement: Add handling renamed symbols in inlay hints

f33b9be

improvement: Make InlayHints tests more readable

e59e50e

improvement: Don't show inlay hints for error types

7d04660

bugfix: Don't remove syntheticDecorations from mtags-interfaces

15d6de1

jkciesluk force-pushed the inlay-hints branch from c9b3015 to 15d6de1 Compare February 19, 2024 09:51

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

jkciesluk merged commit f530681 into scalameta:main Feb 19, 2024
27 checks passed

tgodzik mentioned this pull request Feb 19, 2024

strip long type hints in "show type" feature scalameta/metals-feature-requests#372

Open

jkciesluk added a commit to jkciesluk/dotty that referenced this pull request Feb 21, 2024

improvement: Replace synthetic decorations with inlay hints

94b85c1

Backports changes from Metals: scalameta/metals#5983

jkciesluk mentioned this pull request Feb 21, 2024

improvement: Replace synthetic decorations with inlay hints scala/scala3#19754

Merged

jkciesluk removed the backport-scala3-pc label Feb 21, 2024

jkciesluk added a commit to jkciesluk/dotty that referenced this pull request Feb 21, 2024

improvement: Replace synthetic decorations with inlay hints

1f8175d

Backports changes from Metals: scalameta/metals#5983

jkciesluk added a commit to scala/scala3 that referenced this pull request Feb 22, 2024

improvement: Replace synthetic decorations with inlay hints

be8abfa

Backports changes from Metals: scalameta/metals#5983

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Replace synthetic decorations with inlay hints #5983

feature: Replace synthetic decorations with inlay hints #5983

jkciesluk commented Dec 28, 2023

kasiaMarek left a comment

kasiaMarek Jan 8, 2024

jkciesluk Jan 11, 2024

kasiaMarek Jan 11, 2024

kasiaMarek Jan 8, 2024

jkciesluk Jan 11, 2024

tgodzik Jan 11, 2024

jkciesluk Jan 12, 2024

jkciesluk Jan 12, 2024

tgodzik Jan 12, 2024

tgodzik Jan 11, 2024

tgodzik left a comment

tgodzik Jan 11, 2024

tgodzik Jan 11, 2024

jkciesluk Jan 12, 2024

tgodzik Jan 12, 2024

tgodzik Jan 12, 2024

jkciesluk Jan 12, 2024

kasiaMarek Jan 12, 2024

tgodzik left a comment

igor-ramazanov commented Mar 6, 2024

feature: Replace synthetic decorations with inlay hints #5983

feature: Replace synthetic decorations with inlay hints #5983

Conversation

jkciesluk commented Dec 28, 2023

kasiaMarek left a 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

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

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

tgodzik left a comment

Choose a reason for hiding this comment

igor-ramazanov commented Mar 6, 2024