Respect client capabilities for completion item snippets #1057

gabro · 2019-11-10T21:03:46Z

Previously we always used snippets in completion items, regardless of whether the client declared that it supported them. Now we respect the capabilities declared by the client.

tgodzik

Thanks for looking into this @gabro ! It long overdue, since I totally forgot about this.

There are some more snippets in Completions.scala we need to change. Also, we should probably disable fillAllFields completion without snippets.

tgodzik

Forgot click the right checkbox :O I shouldn't work so late I think 😆

olafurpg

Thank you for looking into this!

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

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

bstaletic · 2019-11-11T08:53:37Z

Hello everyone. I tried out this patch briefly with ycmd and completions started working. @gabro Thanks again for working on this.

gabro · 2019-11-11T14:19:34Z

@tgodzik thanks for the review and woops, yes, I've missed a lot of cases from Completion.scala! I've addressed all the ones I could spot and taken some UX decisions (I'll comment inline) that would need some validation from you and @olafurpg.

Hello everyone. I tried out this patch briefly with ycmd and completions started working. @gabro Thanks again for working on this.

Thanks for testing this, @bstaletic!

gabro · 2019-11-11T14:28:47Z

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

@@ -985,7 +989,7 @@ trait Completions { this: MetalsGlobal =>
          allParams.exists(param => param.name.startsWith(prefix))
        val isExplicitlyCalled = suffix.startsWith(prefix)
        val hasParamsToFill = allParams.count(!_.hasDefault) > 1
-        if ((shouldShow || isExplicitlyCalled) && hasParamsToFill) {
+        if ((shouldShow || isExplicitlyCalled) && hasParamsToFill && clientSupportsSnippets) {


this disables the Autofill with default values completion when the client does not support snippets, as per @tgodzik. Please review that it makes sense and that we can't find a good completion that works without snippets

Let's disable it for now, I will think it over to see if it makes sense without the snippets.

gabro · 2019-11-11T14:29:45Z

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

+            if (clientSupportsSnippets) {
+              "match {\n\tcase$0\n}"
+            } else {
+              "match"


avoid the braces if we have no snippets. Inserting braces will result in a cursor out of place ({}<CURSOR>), which seems bad.

From my point of view, it is bad to end up with foo{}<CURSOR> after completion. I ran into a similar case with Apple's swift server and it forced me to do this - overriding completion algorithm for swift to strip () from foo(), even if there's something in between ( and ).

thanks for the feedback! I'm actively avoiding that pattern, except in some cases where the completion is still very useful and/or it's opt-in (e.g. it's not the default case)

gabro · 2019-11-11T14:32:01Z

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

+                  if (clientSupportsSnippets) {
+                    "match {\n\t${head.edit.getNewText} $$0\n\t"
+                  } else {
+                    "match {\n\t${head.edit.getNewText}\n\t"


in this case we're ok with an "annoying" cursor position (match {...}<CURSOR>) since the exhaustive match feature is quite useful regardless (and it's opt-in anyway)

gabro · 2019-11-11T14:32:27Z

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

-            new l.TextEdit(editRange, "case ($0) =>"),
+            new l.TextEdit(
+              editRange,
+              if (clientSupportsSnippets) "case ($0) =>" else "case () =>"


not brilliant, but again, it's opt-in.

gabro · 2019-11-11T15:13:29Z

@bstaletic curious to hear your input about one particular case: Scala allows interpolating strings with params and if you have a "simple" param you can do $param whereas if it's a "complicated" expression, you have to surround it with braces. Metals allows you to complete on the "simple" expression and it inserts braces for you:

// from
s"$buffer.app<CURSOR>"

//to
s"${buffer.append(<CURSOR>)}"

This is not possible without snippets, so what would you prefer as a language client?

s"$buffer.app<CURSOR>" -> no completions
s"$buffer.app<CURSOR>" -> s"${buffer.append}<CURSOR>"
s"$buffer.app<CURSOR>" -> s"$buffer.append" <- not what you want, this is potentially a bug

I'm leaning towards 1 but the current implementation in this PR would yield 2

bstaletic · 2019-11-11T15:24:40Z

I'm leaning towards 1 as well. If a user complains that such a case isn't getting completions I can point the user to the logs and say no completions are returned by metals.

The option 2 doesn't seem good for the same reason foo() doesn't. On the other hand, I've never written a line of scala.

Tell me if I understand option 3 and scala parsing rules correctly. Without braces $ in string interpolation refers to a single identifier. In case of s"$buffer.append (without braces) it would be parsed as s"${buffer}.append". In general case this might compile and is therefore impossible to catch with errors/warnings. If all of the above is correct, I'd definitely avoid option 3.

@puremourning Do you have anything to add?

bstaletic · 2019-11-11T15:30:15Z

To add to the above, I won't mind option 2 either. If option 1 is too much trouble, oh well, I won't lose any sleep over it.

gabro · 2019-11-11T15:31:05Z

Tell me if I understand option 3 and scala parsing rules correctly. Without braces $ in string interpolation refers to a single identifier. In case of s"$buffer.append (without braces) it would be parsed as s"${buffer}.append". In general case this might compile and is therefore impossible to catch with errors/warnings. If all of the above is correct, I'd definitely avoid option 3.

Correct.

Thanks for the feedback @bstaletic, I'll see whether option 1 is worth the hassle :)

puremourning · 2019-11-11T16:50:58Z

@puremourning Do you have anything to add?

Idiomatically it should be:

${buffer.app -> ${buffer.append, with the suggestions being of the form append, apple, appropriate
$buffer.app -> ${buffer.append, with the suggestions all being of the form {buffer.append, {buffer.apple, {buffer.appropriate

Thought I think it would be reasonable to only support the first one.

ycmd would handle the correct prefix selection (assuming compliant TextEdits are returned).

Basically, I don't think you should insert anything to the "right" for the word being completed, ever. Some servers love to try and insert ; or () or whatever, but this is almost always more wrong than right, or at least not consistent enough to be useful.

IM(ns?)HO !

gabro · 2019-11-11T17:57:11Z

Basically, I don't think you should insert anything to the "right" for the word being completed, ever. Some servers love to try and insert ; or () or whatever, but this is almost always more wrong than right, or at least not consistent enough to be useful.

Thanks for the feedback, that's the general direction I'm following when there's no snippet support in the client.

tgodzik

Thanks @gabro for fixing this! LGTM

tgodzik · 2019-11-11T20:34:25Z

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

@@ -985,7 +989,7 @@ trait Completions { this: MetalsGlobal =>
          allParams.exists(param => param.name.startsWith(prefix))
        val isExplicitlyCalled = suffix.startsWith(prefix)
        val hasParamsToFill = allParams.count(!_.hasDefault) > 1
-        if ((shouldShow || isExplicitlyCalled) && hasParamsToFill) {
+        if ((shouldShow || isExplicitlyCalled) && hasParamsToFill && clientSupportsSnippets) {


Let's disable it for now, I will think it over to see if it makes sense without the snippets.

gabro · 2019-11-11T21:26:39Z

@puremourning / @bstaletic in the end, is quite finicky to tweak that particular use case, and it's quite a niche use case so - for now - I'll merge this as is (option 2 above)

gabro force-pushed the fix-1055 branch from 5417e72 to c230d7d Compare November 10, 2019 21:04

Respect client capabilities for completion item snippets

2c40a21

gabro force-pushed the fix-1055 branch from c230d7d to 2c40a21 Compare November 10, 2019 21:05

gabro requested review from olafurpg and tgodzik November 10, 2019 21:05

gabro mentioned this pull request Nov 10, 2019

Metals returns snippets even when a client doesn't support them #1055

Closed

tgodzik approved these changes Nov 11, 2019

View reviewed changes

tgodzik requested changes Nov 11, 2019

View reviewed changes

olafurpg reviewed Nov 11, 2019

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala Show resolved Hide resolved

mtags/src/main/scala/scala/meta/internal/pc/CompletionProvider.scala Outdated Show resolved Hide resolved

olafurpg approved these changes Nov 11, 2019

View reviewed changes

gabro force-pushed the fix-1055 branch from 992b3e8 to 485e84c Compare November 11, 2019 14:18

gabro force-pushed the fix-1055 branch from 485e84c to 5f8a7e4 Compare November 11, 2019 14:26

gabro commented Nov 11, 2019

View reviewed changes

Address review feedback

03c3f68

gabro force-pushed the fix-1055 branch from 5f8a7e4 to 03c3f68 Compare November 11, 2019 14:41

tgodzik approved these changes Nov 11, 2019

View reviewed changes

gabro merged commit 86a09f3 into scalameta:master Nov 11, 2019

gabro deleted the fix-1055 branch November 11, 2019 21:26

ckipp01 mentioned this pull request Nov 15, 2019

Update emacs docs to include yasnippet #1068

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect client capabilities for completion item snippets #1057

Respect client capabilities for completion item snippets #1057

gabro commented Nov 10, 2019

tgodzik left a comment

tgodzik left a comment

olafurpg left a comment

bstaletic commented Nov 11, 2019

gabro commented Nov 11, 2019

gabro Nov 11, 2019

tgodzik Nov 11, 2019

gabro Nov 11, 2019

bstaletic Nov 11, 2019

gabro Nov 11, 2019

gabro Nov 11, 2019

gabro Nov 11, 2019

gabro commented Nov 11, 2019 •

edited

Loading

bstaletic commented Nov 11, 2019

bstaletic commented Nov 11, 2019

gabro commented Nov 11, 2019 •

edited

Loading

puremourning commented Nov 11, 2019 •

edited

Loading

gabro commented Nov 11, 2019

tgodzik left a comment

tgodzik Nov 11, 2019

gabro commented Nov 11, 2019

Respect client capabilities for completion item snippets #1057

Respect client capabilities for completion item snippets #1057

Conversation

gabro commented Nov 10, 2019

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

bstaletic commented Nov 11, 2019

gabro commented Nov 11, 2019

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

gabro commented Nov 11, 2019 • edited Loading

bstaletic commented Nov 11, 2019

bstaletic commented Nov 11, 2019

gabro commented Nov 11, 2019 • edited Loading

puremourning commented Nov 11, 2019 • edited Loading

gabro commented Nov 11, 2019

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented Nov 11, 2019

gabro commented Nov 11, 2019 •

edited

Loading

gabro commented Nov 11, 2019 •

edited

Loading

puremourning commented Nov 11, 2019 •

edited

Loading