Type annotation on code selection #3060

KacperFKorban · 2021-08-21T19:20:14Z

We are limited by frontend capabilities, so to display the information properly we have to use the type annotation as hover.

Things that may be controversial:

Hover for literals (Only for ranges now)
Expression type for every hover (Only for ranges now)
~~key binding here~~

vscode plugin changes: scalameta/metals-vscode#652

tests/cross/src/test/scala/tests/hover/HoverLiteralSuite.scala

ckipp01

So this is really rad @KacperFKorban, I will use this a ton!

Using nvim-metals, this seems to just work as expected out of the box.

Screen.Recording.2021-08-22.at.15.56.00.mov

With that in mind and looking at the code fully recognising I may have missed something, it seems that the biggest thing we are still looking for is the offset coming off of the end of the range. In my video above, that's all that is being sent in, the position at the end of the visual selection. Do we actually need the full range? Won't the type at the end point always be what we are after? If that's possible, then we don't actually need to break away from the LSP spec for hover by using our own class instead of the TextDocumentPositionParams.

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

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

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

KacperFKorban · 2021-08-22T19:10:50Z

So this is really rad @KacperFKorban, I will use this a ton!

Using nvim-metals, this seems to just work as expected out of the box.

Screen.Recording.2021-08-22.at.15.56.00.mov
With that in mind and looking at the code fully recognising I may have missed something, it seems that the biggest thing we are still looking for is the offset coming off of the end of the range. In my video above, that's all that is being sent in, the position at the end of the visual selection. Do we actually need the full range? Won't the type at the end point always be what we are after? If that's possible, then we don't actually need to break away from the LSP spec for hover by using our own class instead of the TextDocumentPositionParams.

Hmmm, can it be the case that nvim sends different position?

In vscode triggering the hover on the end of an expression doesn't work. Range on a term name seems to include the position before, but not the position after. Like here:

Technically endOffset - 1 can work for ranges, but it can bring problems with position requests.

ckipp01 · 2021-08-22T19:27:37Z

Hmmm, can it be the case that nvim sends different position?

in vscode triggering the hover on the end of an expression doesn't work. Range on a term name seems to include the position before, but not the position after. Like here:

Gotcha. I think it's partly because when nvim sends in the position from the selection range, it's still just a single point that often isn't actually after the symbol, since unlike VS Code, you can't really extend past the text like in your image.

Either way, great work on this 👍🏼 I'll for sure play around with this a bit more to see if anything will even need to be added for this to work the same way in nvim-metals as it does in VS Code.

KacperFKorban · 2021-08-23T08:07:46Z

@ckipp01 I thought of an example that won't work with just the end offset- method calls in infix notation.
e.g. for:

List("1", "2", "3").filter(<<_ contains "2">>)

the result will be Boolean.
and for:

List("1", "2", "3").filter(_ contains "2@@")

the result will be String.

ayoub-benali · 2021-08-27T12:15:26Z

Really cool feature ! What needs to be done for none VS code clients, say Sublime Text ? :)

tgodzik · 2021-08-27T15:44:33Z

Really cool feature ! What needs to be done for none VS code clients, say Sublime Text ? :)

You would need to send a range instead of position, similar to scalameta/metals-vscode#652 - this was actually discussed in the LSP github repo, but not sure if anything will change on that front. This is already being done for Rust and I think Haskell.

tgodzik

Thanks for the great work! I have a few comments, overall it would be awesome to limit the changes to only the code selection if possible.

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

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

mtags/src/main/scala/scala/meta/internal/metals/CompilerOffsetParams.scala

tests/cross/src/test/scala/tests/hover/HoverDefnSuite.scala

mtags/src/main/scala/scala/meta/internal/pc/HoverMarkup.scala

tgodzik

LGTM! Just one small typo, I will push the change and rebase on master

tests/mtest/src/main/scala/tests/TestHovers.scala

This reverts commit c457790.

This is a follow-up to the work done in scalameta#3060, but since positions is now optional, we need to ensure to use `getPosition` instead of position like we did here. This was causing the following exception to be throw: ``` Caused by: java.lang.IllegalArgumentException: Property must not be null: position at org.eclipse.lsp4j.util.Preconditions.checkNotNull(Preconditions.java:29) at org.eclipse.lsp4j.TextDocumentPositionParams.<init>(TextDocumentPositionParams.java:49) at scala.meta.internal.metals.Compilers.withPCAndAdjustLsp(Compilers.scala:594) at scala.meta.internal.metals.Compilers.hover(Compilers.scala:365) at scala.meta.internal.metals.MetalsLanguageServer.$anonfun$hover$1(MetalsLanguageServer.scala:1359) at scala.meta.internal.metals.CancelTokens$.future(CancelTokens.scala:38) at scala.meta.internal.metals.MetalsLanguageServer.hover(MetalsLanguageServer.scala:1357) ... 17 more ``` I'm surprised the test didn't catch this, because this shouldn't work at all without this change, but by quickly looking at the tests, it's wasn't clear to me right away why that was the case. I'll need to look further to understand the tests, but figured I'd send up this quick fix right away because now it won't work at all without it.

ayoub-benali · 2021-11-08T09:20:02Z

Really cool feature ! What needs to be done for none VS code clients, say Sublime Text ? :)

You would need to send a range instead of position, similar to scalameta/metals-vscode#652 - this was actually discussed in the LSP github repo, but not sure if anything will change on that front. This is already being done for Rust and I think Haskell.

@tgodzik Do you mind posting the link so I get the context ?

tgodzik · 2021-11-08T10:50:08Z

The discussion is happening here: microsoft/language-server-protocol#377

This is needed to handle scalameta/metals#3060 It should be handle by LSP package once microsoft/language-server-protocol#377 is resolved

bloznelis · 2023-05-07T13:44:03Z

Sorry for necrobumping, but a question for @ckipp01, did you manage to make it work properly in nvim-metals?

KacperFKorban mentioned this pull request Aug 21, 2021

Type annotation on code selection scalameta/metals-vscode#652

Merged

KacperFKorban requested review from tgodzik, dos65 and ckipp01 August 21, 2021 19:24

tgodzik reviewed Aug 21, 2021

View reviewed changes

tests/cross/src/test/scala/tests/hover/HoverLiteralSuite.scala Outdated Show resolved Hide resolved

KacperFKorban force-pushed the range-hover branch 2 times, most recently from 3f72351 to c0d2f7c Compare August 22, 2021 00:34

ckipp01 reviewed Aug 22, 2021

View reviewed changes

tgodzik requested changes Aug 30, 2021

View reviewed changes

KacperFKorban force-pushed the range-hover branch from 4f530cd to 4d956c8 Compare August 31, 2021 18:44

KacperFKorban requested review from tgodzik and ckipp01 September 2, 2021 08:35

tgodzik approved these changes Sep 24, 2021

View reviewed changes

tests/mtest/src/main/scala/tests/TestHovers.scala Outdated Show resolved Hide resolved

KacperFKorban added 12 commits September 24, 2021 18:07

Somewhat working version of range hover action

f8bfc46

Definition type on lhs hover

29740c0

Always display Expression Type on hovers

4816604

Remove debugs

8280aea

Move range hover tests to separate suite

a64630e

fmt

4feaa2c

Fix docstrings

d6484fd

scalafix

0cd3082

fix unit test

89609e2

deduplicate part of hover adjustment

55eb3ef

Revert "Always display Expression Type on hovers"

a6c8749

This reverts commit c457790.

fix tests and add some more for ranges

400dff1

Fix typo

b3fb8d0

tgodzik force-pushed the range-hover branch from 2bb35a9 to b3fb8d0 Compare September 24, 2021 16:07

Fix issues with 3.1.0 tests

94501a6

tgodzik merged commit 42ea48a into scalameta:main Sep 24, 2021

ckipp01 mentioned this pull request Sep 25, 2021

Add support for type annotation on code selection scalameta/nvim-metals#228

Closed

ckipp01 mentioned this pull request Oct 3, 2021

Avoid IllegalArgumentException when trying to get type of range. #3178

Merged

KacperFKorban deleted the range-hover branch November 8, 2021 12:42

ayoub-benali added a commit to scalameta/metals-sublime that referenced this pull request Nov 14, 2021

Send range with textDocument/hover

d806c9a

This is needed to handle scalameta/metals#3060 It should be handle by LSP package once microsoft/language-server-protocol#377 is resolved

ayoub-benali mentioned this pull request Nov 14, 2021

Send range with textDocument/hover scalameta/metals-sublime#45

Closed

1 task

ulugbekna mentioned this pull request Dec 2, 2021

Add a new custom command ocamllsp/hoverExtended with support for variable verbosity ocaml/ocaml-lsp#561

Merged

bloznelis mentioned this pull request May 10, 2023

feat: use current range for type_of_range() scalameta/nvim-metals#564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type annotation on code selection #3060

Type annotation on code selection #3060

KacperFKorban commented Aug 21, 2021 •

edited

Loading

ckipp01 left a comment •

edited

Loading

KacperFKorban commented Aug 22, 2021

ckipp01 commented Aug 22, 2021

KacperFKorban commented Aug 23, 2021

ayoub-benali commented Aug 27, 2021

tgodzik commented Aug 27, 2021

tgodzik left a comment

tgodzik left a comment

ayoub-benali commented Nov 8, 2021

tgodzik commented Nov 8, 2021

bloznelis commented May 7, 2023

Type annotation on code selection #3060

Type annotation on code selection #3060

Conversation

KacperFKorban commented Aug 21, 2021 • edited Loading

ckipp01 left a comment • edited Loading

Choose a reason for hiding this comment

KacperFKorban commented Aug 22, 2021

ckipp01 commented Aug 22, 2021

KacperFKorban commented Aug 23, 2021

ayoub-benali commented Aug 27, 2021

tgodzik commented Aug 27, 2021

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

ayoub-benali commented Nov 8, 2021

tgodzik commented Nov 8, 2021

bloznelis commented May 7, 2023

KacperFKorban commented Aug 21, 2021 •

edited

Loading

ckipp01 left a comment •

edited

Loading