Improve signature help by more stable position calculation + better named arg support #19214

rochala · 2023-12-06T15:27:16Z

Previously, signature help had unstable way of calculating active parameter inside the signature help. It is now changed to work better with erroneous trees such as unclosed openings.

It also adds a new feature for signature help, which will help user navigate inside named arguments. Furthermore, it will now find reordered named arguments, and display in the same way, by also marking the remaining parameters that they are now required to be named.

Example:

Screen.Recording.2023-12-06.at.15.55.02.mov

For the reviewers: I recommend testing it locally.

…tions

kasiaMarek

Also the unclosed opening works for params but not type params.

presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala

compiler/src/dotty/tools/dotc/util/Signatures.scala

presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala

jkciesluk

Great job! It fixes a lot of things, but I'm unsure if handling unclosed openings is needed, especially because it really complicates finding enclosing apply.

Could you add these tests:

def test[T, F](aaa: Int, bbb: T, ccc: F): T = ???
val x = test(1, ccc = 2, b@@)

def test2(aaa: Int, bbb: Int, ccc: Int): Int = ???
val x = test2(
    1, 
    @@
  )

def test2(aaa: Int, bbb: Int, ccc: Int): Int = ???
val x = test2(aaa = 2@@, ccc = 1)

def test2(aaa: Int, bbb: Int, ccc: Int): Int = ???
val x = test2(aaa = 2, @@  ccc = 1, bbb = 3)

Also, would it be possible to show in the case below that arguments in the first parameter list are reordered?

def test3(aaa: Int, bbb: Int, ccc: Int)(ddd: Int): Int = ???
val x = test3(bbb = 1, aaa = 2, ccc = 3)(d@@)

compiler/src/dotty/tools/dotc/util/Signatures.scala

presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala

…earch for curried methods

…e error recovery better, drop support for unclosed openings

rochala · 2023-12-28T17:19:38Z

The following changes have been made:

we will stop supporting Signature Helps for unclosed openings. It is too hard to recover from such errors reliably and to track where we are with the cursor. All modern editors will close opening on input. It simplified the code, and is the best way forward.
if there is an error within a definition return type parameter, we will now fall back to source, instead of print error,
tuples and functions are now supported when they are returned from methods.
this rewrite made me change a few things, and I took the chance and made it support clause interleaving. There is now a proper test suite for that. It required changing the way we represent Signatures.
argument reordering now works for all parameter lists, not only the currently applied one,
type parameters now also correctly display documentation

rochala · 2024-01-09T16:15:02Z

I've changed the PR to use ShortenedTypePrinter and fixed some other minor issues like correctly marking reordered argumentss or hiding synthetic parameter names.

In the future, Signature help should be changed to return symbols from the compiler instead of already printed values, and only print them later using shared API of ShortenedTypePrinter in the Presentation Compiler. Ideally we'd want a shared implementation for label printing with completions, hovers and other features.

jkciesluk

LGTM! Great work!

compiler/src/dotty/tools/dotc/reporting/messages.scala

compiler/src/dotty/tools/dotc/util/Signatures.scala

presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala

...tation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpInterleavingSuite.scala

presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpNamedArgsSuite.scala

kasiaMarek

Looks really good. 🚀 I feel like you improved naming of different variables/methods, thanks for that, I think it made things more readable.

kasiaMarek · 2024-01-16T13:12:56Z

presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpPatternSuite.scala

@@ -202,23 +202,6 @@ class SignatureHelpPatternSuite extends BaseSignatureHelpSuite:
         |""".stripMargin
    )

-  @Test def `pat4` =


Why did you delete this test?

A. It is not working any more due to changes to range.
B. It was a bit useless as it would not be automatically opened and triggered, also the information is not that helpful, especially if you have more than single Apply like
a :: b :: c :: Nil

kasiaMarek · 2024-01-16T13:27:17Z

presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala

@@ -323,9 +323,9 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
        |  } yield k
        |}
      """.stripMargin,
-      """|to(end: Int): scala.collection.immutable.Range.Inclusive
+      """|to(end: Int): Inclusive


Is it supposed to shorten the name this much?

No, it should only shorten packages. I'll change it in separate PR as this is not caused by these changes.

presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala

compiler/src/dotty/tools/dotc/util/Signatures.scala

kasiaMarek · 2024-01-17T13:19:46Z

language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala

-          }
-          .mkString("(", ")(", ")")
-    val tparamsLabel = if (signature.tparams.isEmpty) "" else signature.tparams.mkString("[", ", ", "]")
+      signature.paramss


I assume this whole duplication is because there is no good place for it, such that it could be used by both modules.

Dotty language server should actually be removed from the repository as it is no longer necessary. It is not so easy tho as the test coverage is different. This change was only made to make the tests pass

presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala

…amed arg support (#19214) Previously, signature help had unstable way of calculating active parameter inside the signature help. It is now changed to work better with erroneous trees such as unclosed openings. It also adds a new feature for signature help, which will help user navigate inside named arguments. Furthermore, it will now find reordered named arguments, and display in the same way, by also marking the remaining parameters that they are now required to be named. Example: https://github.com/lampepfl/dotty/assets/48657087/b181d2d5-60f0-46a5-b2df-a58aa5f07454 The following changes have been made: - we will stop supporting Signature Helps for unclosed openings. It is too hard to recover from such errors reliably and to track where we are with the cursor. All modern editors will close opening on input. It simplified the code, and is the best way forward. - if there is an error within a definition return type parameter, we will now fall back to source, instead of print error, - tuples and functions are now supported when they are returned from methods. - this rewrite made me change a few things, and I took the chance and made it support clause interleaving. There is now a proper test suite for that. It required changing the way we represent Signatures. - argument reordering now works for all parameter lists, not only the currently applied one, - type parameters now also correctly display documentation I've changed the PR to use ShortenedTypePrinter and fixed some other minor issues like correctly marking reordered argumentss or hiding synthetic parameter names. In the future, Signature help should be changed to return symbols from the compiler instead of already printed values, and only print them later using shared API of ShortenedTypePrinter in the Presentation Compiler. Ideally we'd want a shared implementation for label printing with completions, hovers and other features. [Cherry-picked 01171de]

…+ better named arg support" to LTS (#20845) Backports #19214 to the LTS branch. PR submitted by the release tooling. [skip ci]

rochala added 6 commits December 6, 2023 16:03

Add better signature helps for named args, fix various bugs with posi…

c465c09

…tions

Properly compute index when between the args

1a38639

Named args after cursor should properly detect param span

811d130

Improve detection of param, with unclosed params

f447ba2

Refactor SignatureHelpProvider

0426165

Remove deprecated code

a859a1a

rochala requested review from tgodzik, jkciesluk and kasiaMarek December 6, 2023 15:27

kasiaMarek reviewed Dec 7, 2023

View reviewed changes

jkciesluk suggested changes Dec 11, 2023

View reviewed changes

rochala added 2 commits December 22, 2023 20:23

Drop support for signature help with unclosed openings. Alternative s…

83e337e

…earch for curried methods

Add support for clause interleaving, simplify parameter ordering, mak…

13a0dcd

…e error recovery better, drop support for unclosed openings

rochala requested review from kasiaMarek and jkciesluk December 28, 2023 17:19

rochala added 3 commits January 9, 2024 10:46

Don't mark non reordered params as reordered

0a3181d

Add support for shortened type printer in Signature Help

018244f

Don't print synthetic param names

fa584ad

Select correct parameter when using is missing

676a594

jkciesluk approved these changes Jan 11, 2024

View reviewed changes

compiler/src/dotty/tools/dotc/reporting/messages.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/util/Signatures.scala Show resolved Hide resolved

Remove unnecessary added vals

6647c71

rochala commented Jan 12, 2024

View reviewed changes

presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala Outdated Show resolved Hide resolved

kasiaMarek reviewed Jan 16, 2024

View reviewed changes

...tation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpInterleavingSuite.scala Outdated Show resolved Hide resolved

kasiaMarek reviewed Jan 16, 2024

View reviewed changes

presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpNamedArgsSuite.scala Show resolved Hide resolved

kasiaMarek approved these changes Jan 17, 2024

View reviewed changes

Apply review changes

329a2eb

rochala enabled auto-merge (squash) January 19, 2024 13:24

rochala disabled auto-merge January 19, 2024 13:48

rochala mentioned this pull request Jan 19, 2024

chore: test changes for upcoming changes in nightly scalameta/metals#6035

Merged

rochala merged commit 01171de into scala:main Jan 19, 2024
19 checks passed

Kordyjan added this to the 3.4.1 milestone Feb 14, 2024

tgodzik added the area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools label May 8, 2024

WojciechMazur mentioned this pull request Jun 27, 2024

Backport "Improve signature help by more stable position calculation + better named arg support" to LTS #20845

Merged

WojciechMazur added a commit that referenced this pull request Jun 28, 2024

Backport "Improve signature help by more stable position calculation …

6dcfeab

…+ better named arg support" to LTS (#20845) Backports #19214 to the LTS branch. PR submitted by the release tooling. [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve signature help by more stable position calculation + better named arg support #19214

Improve signature help by more stable position calculation + better named arg support #19214

rochala commented Dec 6, 2023

kasiaMarek left a comment

jkciesluk left a comment

rochala commented Dec 28, 2023

rochala commented Jan 9, 2024

jkciesluk left a comment

kasiaMarek left a comment

kasiaMarek Jan 16, 2024

rochala Jan 19, 2024

kasiaMarek Jan 16, 2024

rochala Jan 19, 2024

kasiaMarek Jan 17, 2024

rochala Jan 19, 2024

Improve signature help by more stable position calculation + better named arg support #19214

Improve signature help by more stable position calculation + better named arg support #19214

Conversation

rochala commented Dec 6, 2023

kasiaMarek left a comment

Choose a reason for hiding this comment

jkciesluk left a comment

Choose a reason for hiding this comment

rochala commented Dec 28, 2023

rochala commented Jan 9, 2024

jkciesluk left a comment

Choose a reason for hiding this comment

kasiaMarek left a comment

Choose a reason for hiding this comment

kasiaMarek Jan 16, 2024

Choose a reason for hiding this comment

rochala Jan 19, 2024

Choose a reason for hiding this comment

kasiaMarek Jan 16, 2024

Choose a reason for hiding this comment

rochala Jan 19, 2024

Choose a reason for hiding this comment

kasiaMarek Jan 17, 2024

Choose a reason for hiding this comment

rochala Jan 19, 2024

Choose a reason for hiding this comment