Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect client capabilities for completion item snippets #1057

Merged
merged 2 commits into from Nov 11, 2019

Conversation

@gabro
Copy link
Member

gabro commented Nov 10, 2019

Fixes #1055

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.

@gabro gabro force-pushed the gabro:fix-1055 branch from 5417e72 to c230d7d Nov 10, 2019
Copy link
Collaborator

tgodzik left a comment

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.

Copy link
Collaborator

tgodzik left a comment

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

Copy link
Member

olafurpg left a comment

Thank you for looking into this!

@bstaletic

This comment has been minimized.

Copy link

bstaletic commented Nov 11, 2019

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

@gabro gabro force-pushed the gabro:fix-1055 branch from 992b3e8 to 485e84c Nov 11, 2019
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 11, 2019

@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 gabro force-pushed the gabro:fix-1055 branch from 485e84c to 5f8a7e4 Nov 11, 2019
@@ -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 comment has been minimized.

Copy link
@gabro

gabro Nov 11, 2019

Author Member

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

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 11, 2019

Collaborator

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

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

This comment has been minimized.

Copy link
@gabro

gabro Nov 11, 2019

Author Member

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

This comment has been minimized.

Copy link
@bstaletic

bstaletic Nov 11, 2019

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 ).

This comment has been minimized.

Copy link
@gabro

gabro Nov 11, 2019

Author Member

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)

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

This comment has been minimized.

Copy link
@gabro

gabro Nov 11, 2019

Author Member

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)

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

This comment has been minimized.

Copy link
@gabro

gabro Nov 11, 2019

Author Member

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

@gabro gabro force-pushed the gabro:fix-1055 branch from 5f8a7e4 to 03c3f68 Nov 11, 2019
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 11, 2019

@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?

  1. s"$buffer.app<CURSOR>" -> no completions
  2. s"$buffer.app<CURSOR>" -> s"${buffer.append}<CURSOR>"
  3. 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

This comment has been minimized.

Copy link

bstaletic commented Nov 11, 2019

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

This comment has been minimized.

Copy link

bstaletic commented Nov 11, 2019

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

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 11, 2019

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

This comment has been minimized.

Copy link

puremourning commented Nov 11, 2019

@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

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 11, 2019

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.

Copy link
Collaborator

tgodzik left a comment

Thanks @gabro for fixing this! LGTM

@@ -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 comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 11, 2019

Collaborator

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

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Nov 11, 2019

@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 gabro merged commit 86a09f3 into scalameta:master Nov 11, 2019
9 checks passed
9 checks passed
Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@gabro gabro deleted the gabro:fix-1055 branch Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.