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.

Sign up for GitHub

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
Merged

Respect client capabilities for completion item snippets #1057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c40a21
Respect client capabilities for completion item snippets
gabro Nov 10, 2019
03c3f68
Address review feedback
gabro Nov 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import ch.epfl.scala.bsp4j.ScalaBuildTarget
import ch.epfl.scala.bsp4j.ScalacOptionsItem
import java.util.Collections
import java.util.concurrent.ScheduledExecutorService
import org.eclipse.lsp4j.InitializeParams
import org.eclipse.lsp4j.CompletionItem
import org.eclipse.lsp4j.CompletionList
import org.eclipse.lsp4j.CompletionParams
Expand Down Expand Up @@ -40,7 +41,8 @@ class Compilers(
search: SymbolSearch,
embedded: Embedded,
statusBar: StatusBar,
sh: ScheduledExecutorService
sh: ScheduledExecutorService,
initializeParams: Option[InitializeParams]
)(implicit ec: ExecutionContextExecutorService)
extends Cancelable {
val plugins = new CompilerPlugins()
Expand Down Expand Up @@ -247,7 +249,11 @@ class Compilers(
.withExecutorService(ec)
.withScheduledExecutorService(sh)
.withConfiguration(
config.compilers.copy(_symbolPrefixes = userConfig().symbolPrefixes)
config.compilers.copy(
_symbolPrefixes = userConfig().symbolPrefixes,
isCompletionSnippetsEnabled =
initializeParams.supportsCompletionSnippets
)
)

def newCompiler(
Expand Down
10 changes: 10 additions & 0 deletions metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,16 @@ object MetalsEnrichments
)
} yield hierarchicalDocumentSymbolSupport.booleanValue).getOrElse(false)

def supportsCompletionSnippets: Boolean =
(for {
params <- initializeParams
capabilities <- Option(params.getCapabilities)
textDocument <- Option(capabilities.getTextDocument)
completion <- Option(textDocument.getCompletion)
completionItem <- Option(completion.getCompletionItem)
snippetSupport <- Option(completionItem.getSnippetSupport())
} yield snippetSupport.booleanValue).getOrElse(false)

}

implicit class XtensionPromise[T](promise: Promise[T]) {
Expand Down
3 changes: 2 additions & 1 deletion metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ class MetalsLanguageServer(
),
embedded,
statusBar,
sh
sh,
Option(params)
)
)
doctor = new Doctor(
Expand Down
5 changes: 5 additions & 0 deletions mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompilerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ enum OverrideDefFormat {
*/
boolean isSignatureHelpDocumentationEnabled();

/**
* Returns true if completions can contain snippets.
*/
boolean isCompletionSnippetsEnabled();

/**
* The maximum delay for requests to respond.
*
Expand Down
16 changes: 12 additions & 4 deletions mtags/src/main/scala/scala/meta/internal/pc/CompletionProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class CompletionProvider(
)
val pos = unit.position(params.offset)
val isSnippet = isSnippetEnabled(pos, params.text())
val clientSupportsSnippets =
compiler.metalsConfig.isCompletionSnippetsEnabled()
val (i, completion, editRange, query) = safeCompletionsAt(pos)
val start = inferIdentStart(pos, params.text())
val end = inferIdentEnd(pos, params.text())
Expand Down Expand Up @@ -88,13 +90,13 @@ class CompletionProvider(
item.setDetail(detail)
}
val templateSuffix =
if (!isSnippet) ""
if (!isSnippet || !clientSupportsSnippets) ""
else if (completion.isNew &&
r.sym.dealiased.requiresTemplateCurlyBraces) " {}"
else ""

val typeSuffix =
if (!isSnippet) ""
if (!isSnippet || !clientSupportsSnippets) ""
else if (completion.isType && r.sym.hasTypeParams) "[$0]"
else if (completion.isNew && r.sym.hasTypeParams) "[$0]"
else ""
Expand All @@ -111,7 +113,11 @@ class CompletionProvider(
item.setFilterText(symbolName)
}

item.setInsertTextFormat(InsertTextFormat.Snippet)
if (clientSupportsSnippets) {
item.setInsertTextFormat(InsertTextFormat.Snippet)
} else {
item.setInsertTextFormat(InsertTextFormat.PlainText)
}

r match {
case i: TextEditMember =>
Expand Down Expand Up @@ -151,7 +157,9 @@ class CompletionProvider(
case head :: Nil if head.forall(_.isImplicit) =>
() // Don't set ($0) snippet for implicit-only params.
case _ =>
item.setTextEdit(textEdit(baseLabel + "($0)"))
if (clientSupportsSnippets) {
item.setTextEdit(textEdit(baseLabel + "($0)"))
}
metalsConfig
.parameterHintsCommand()
.asScala
Expand Down
42 changes: 32 additions & 10 deletions mtags/src/main/scala/scala/meta/internal/pc/Completions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import scala.meta.internal.tokenizers.Chars
*/
trait Completions { this: MetalsGlobal =>

val clientSupportsSnippets: Boolean =
metalsConfig.isCompletionSnippetsEnabled()

/**
* A member for symbols on the classpath that are not in scope, produced via workspace/symbol.
*/
Expand Down Expand Up @@ -765,11 +768,11 @@ trait Completions { this: MetalsGlobal =>
if (text.charAt(lit.pos.start - 1) != 's')
List(new l.TextEdit(lit.pos.withEnd(lit.pos.start).toLSP, "s"))
else Nil
val dolarEdits = for {
val dollarEdits = for {
i <- lit.pos.start to (lit.pos.end - CURSOR.length())
if text.charAt(i) == '$' && i != interpolator.dollar
} yield new l.TextEdit(pos.source.position(i).withEnd(i).toLSP, "$")
interpolatorEdit ++ dolarEdits
interpolatorEdit ++ dollarEdits
}

def newText(sym: Symbol): String = {
Expand All @@ -793,6 +796,7 @@ trait Completions { this: MetalsGlobal =>

val filter: String =
text.substring(lit.pos.start, pos.point - interpolator.name.length)

override def contribute: List[Member] = {
metalsScopeMembers(pos).collect {
case s: ScopeMember
Expand Down Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val editText = allParams.zipWithIndex
.collect {
case (param, index) if !param.hasDefault =>
Expand Down Expand Up @@ -1202,7 +1206,11 @@ trait Completions { this: MetalsGlobal =>
private def signature = printer.defaultMethodSignature()
private def edit = new l.TextEdit(
range,
s"$filterText$signature = $${0:???}"
if (clientSupportsSnippets) {
s"$filterText$signature = $${0:???}"
} else {
s"$filterText$signature = ???"
}
)
}

Expand Down Expand Up @@ -1259,7 +1267,11 @@ trait Completions { this: MetalsGlobal =>
"match",
new l.TextEdit(
editRange,
"match {\n\tcase$0\n}"
if (clientSupportsSnippets) {
"match {\n\tcase$0\n}"
} else {
"match"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
),
completionsSymbol("match"),
label = Some("match"),
Expand All @@ -1273,7 +1285,11 @@ trait Completions { this: MetalsGlobal =>
tail
.map(_.edit.getNewText())
.mkString(
s"match {\n\t${head.edit.getNewText} $$0\n\t",
if (clientSupportsSnippets) {
s"match {\n\t${head.edit.getNewText} $$0\n\t"
} else {
s"match {\n\t${head.edit.getNewText}\n\t"
},
"\n\t",
"\n}"
)
Expand Down Expand Up @@ -1408,7 +1424,10 @@ trait Completions { this: MetalsGlobal =>
if (definitions.isTupleType(parents.selector)) {
result += new TextEditMember(
"case () =>",
new l.TextEdit(editRange, "case ($0) =>"),
new l.TextEdit(
editRange,
if (clientSupportsSnippets) "case ($0) =>" else "case () =>"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
parents.selector.typeSymbol,
label = Some(s"case ${parents.selector} =>"),
command = metalsConfig.parameterHintsCommand().asScala
Expand Down Expand Up @@ -1466,8 +1485,10 @@ trait Completions { this: MetalsGlobal =>
val label = s"case $pattern =>"
new TextEditMember(
filterText = label,
edit =
new l.TextEdit(editRange, label + (if (isSnippet) " $0" else "")),
edit = new l.TextEdit(
editRange,
label + (if (isSnippet && clientSupportsSnippets) " $0" else "")
),
sym = sym,
label = Some(label),
additionalTextEdits = autoImports
Expand All @@ -1482,7 +1503,8 @@ trait Completions { this: MetalsGlobal =>
s"case _: $name",
new l.TextEdit(
editRange,
if (isSnippet) s"case $${0:_}: $name$suffix => "
if (isSnippet && clientSupportsSnippets)
s"case $${0:_}: $name$suffix => "
else s"case _: $name$suffix =>"
),
sym,
Expand Down
6 changes: 3 additions & 3 deletions mtags/src/main/scala/scala/meta/internal/pc/MetalsGlobal.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,11 +573,11 @@ class MetalsGlobal(

def snippetCursor: String = sym.paramss match {
case Nil =>
"$0"
if (clientSupportsSnippets) "$0" else ""
case Nil :: Nil =>
"()$0"
if (clientSupportsSnippets) "()$0" else "()"
case _ =>
"($0)"
if (clientSupportsSnippets) "($0)" else ""
}

def isDefined: Boolean =
Expand Down
1 change: 1 addition & 0 deletions mtags/src/main/scala/scala/meta/internal/pc/PresentationCompilerConfigImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ case class PresentationCompilerConfigImpl(
isCompletionItemDocumentationEnabled: Boolean = true,
isHoverDocumentationEnabled: Boolean = true,
isSignatureHelpDocumentationEnabled: Boolean = true,
isCompletionSnippetsEnabled: Boolean = true,
isCompletionItemResolve: Boolean = true,
timeoutDelay: Long = 20,
timeoutUnit: TimeUnit = TimeUnit.SECONDS
Expand Down
76 changes: 76 additions & 0 deletions tests/cross/src/test/scala/tests/pc/CompletionSnippetNegSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package tests.pc

import tests.BaseCompletionSuite
import scala.meta.pc.PresentationCompilerConfig
import scala.meta.internal.pc.PresentationCompilerConfigImpl

object CompletionSnippetNegSuite extends BaseCompletionSuite {

override def config: PresentationCompilerConfig =
PresentationCompilerConfigImpl(
isCompletionSnippetsEnabled = false
)

checkSnippet(
"member",
"""
|object Main {
| List.appl@@
|}
|""".stripMargin,
"""|apply
|unapplySeq
|""".stripMargin,
compat = Map(
"2.13" ->
// the second apply is from scala/collection/BuildFrom#apply(), introduced in 2.13
"""|apply
|unapplySeq
|apply
|""".stripMargin
)
)

checkSnippet(
"scope",
"""
|object Main {
| printl@@
|
|}
|""".stripMargin,
"""|println()
|println
|""".stripMargin
)

checkSnippet(
"java-nullary",
"""
|class Foo {
| override def toString = "Foo"
|}
|object Main {
| new Foo().toStrin@@
|
|}
|""".stripMargin,
// even if `Foo.toString` is nullary, it overrides `Object.toString()`
// which is a Java non-nullary method with an empty parameter list.
"""|toString()
|""".stripMargin
)

checkSnippet(
"type",
s"""|object Main {
| val x: scala.IndexedSe@@
|}
|""".stripMargin,
// It's expected to have two separate results, one for `object IndexedSeq` and one for `type IndexedSeq[T]`.
"""|IndexedSeq
|IndexedSeq
|""".stripMargin
)

}