Skip to content

fix(completions): add backticks when needed in completions #14594

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

Merged
merged 3 commits into from
Mar 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 55 additions & 6 deletions compiler/src/dotty/tools/dotc/interactive/Completion.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import dotty.tools.dotc.core.StdNames.nme
import dotty.tools.dotc.core.SymDenotations.SymDenotation
import dotty.tools.dotc.core.TypeError
import dotty.tools.dotc.core.Types.{ExprType, MethodOrPoly, NameFilter, NoType, TermRef, Type}
import dotty.tools.dotc.parsing.Tokens
import dotty.tools.dotc.util.Chars
import dotty.tools.dotc.util.SourcePosition

import scala.collection.mutable
Expand Down Expand Up @@ -78,8 +80,8 @@ object Completion {
* Inspect `path` to determine the completion prefix. Only symbols whose name start with the
* returned prefix should be considered.
*/
def completionPrefix(path: List[untpd.Tree], pos: SourcePosition): String =
path match {
def completionPrefix(path: List[untpd.Tree], pos: SourcePosition)(using Context): String =
path match
case (sel: untpd.ImportSelector) :: _ =>
completionPrefix(sel.imported :: Nil, pos)

Expand All @@ -88,13 +90,22 @@ object Completion {
completionPrefix(selector :: Nil, pos)
}.getOrElse("")

// We special case Select here because we want to determine if the name
Copy link
Contributor

Choose a reason for hiding this comment

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

What about identifiers that are available directly in the current scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I'm actually working with figuring this out for Ident as well. However one difference I noticed that I don't fully get is that the name of a Select is a nospan when it's an <error>, however the name of an Ident when it's an <error> has a span. This makes getting the prefix different for each since here in the Select it's a bit hacky. How come one <error> is a no span and the other isn't?

// is an error due to an unclosed backtick.
case (select: untpd.Select) :: _ if (select.name == nme.ERROR) =>
val content = select.source.content()
content.lift(select.nameSpan.start) match
case Some(char) if char == '`' =>
content.slice(select.nameSpan.start, select.span.end).mkString
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if there is better way to get the full span of the actual name here, since when you attempt to complete Foo.`back it gives you a Select(Ident(Bar),<error>) with the error being a nospan. So in order to "cheat" this a bit and actually get what was passed in to use as a prefix I get the nameSpan.start and the select.span.end which gives me `back which is exactly what I want here. Is there any better way to do this?

case _ =>
""
case (ref: untpd.RefTree) :: _ =>
if (ref.name == nme.ERROR) ""
else ref.name.toString.take(pos.span.point - ref.span.point)

case _ =>
""
}
end completionPrefix

/** Inspect `path` to determine the offset where the completion result should be inserted. */
def completionOffset(path: List[Tree]): Int =
Expand All @@ -105,7 +116,11 @@ object Completion {

private def computeCompletions(pos: SourcePosition, path: List[Tree])(using Context): (Int, List[Completion]) = {
val mode = completionMode(path, pos)
val prefix = completionPrefix(path, pos)
val rawPrefix = completionPrefix(path, pos)

val hasBackTick = rawPrefix.headOption.contains('`')
val prefix = if hasBackTick then rawPrefix.drop(1) else rawPrefix

val completer = new Completer(mode, prefix, pos)

val completions = path match {
Expand All @@ -120,16 +135,49 @@ object Completion {
}

val describedCompletions = describeCompletions(completions)
val backtickedCompletions =
describedCompletions.map(completion => backtickCompletions(completion, hasBackTick))

val offset = completionOffset(path)

interactiv.println(i"""completion with pos = $pos,
| prefix = ${completer.prefix},
| term = ${completer.mode.is(Mode.Term)},
| type = ${completer.mode.is(Mode.Type)}
| results = $describedCompletions%, %""")
(offset, describedCompletions)
| results = $backtickCompletions%, %""")
(offset, backtickedCompletions)
}

def backtickCompletions(completion: Completion, hasBackTick: Boolean) =
if hasBackTick || needsBacktick(completion.label) then
completion.copy(label = s"`${completion.label}`")
else
completion

// This borrows from Metals, which itself borrows from Ammonite. This uses
// the same approach, but some of the utils that already exist in Dotty.
// https://github.com/scalameta/metals/blob/main/mtags/src/main/scala/scala/meta/internal/mtags/KeywordWrapper.scala
// https://github.com/com-lihaoyi/Ammonite/blob/73a874173cd337f953a3edc9fb8cb96556638fdd/amm/util/src/main/scala/ammonite/util/Model.scala
private def needsBacktick(s: String) =
val chunks = s.split("_", -1)

val validChunks = chunks.zipWithIndex.forall { case (chunk, index) =>
chunk.forall(Chars.isIdentifierPart) ||
(chunk.forall(Chars.isOperatorPart) &&
index == chunks.length - 1 &&
!(chunks.lift(index - 1).contains("") && index - 1 == 0))
}

val validStart =
Chars.isIdentifierStart(s(0)) || chunks(0).forall(Chars.isOperatorPart)

val valid = validChunks && validStart && !keywords.contains(s)

!valid
end needsBacktick

private lazy val keywords = Tokens.keywords.map(Tokens.tokenString)

/**
* Return the list of code completions with descriptions based on a mapping from names to the denotations they refer to.
* If several denotations share the same name, each denotation will be transformed into a separate completion item.
Expand Down Expand Up @@ -382,6 +430,7 @@ object Completion {
private def include(denot: SingleDenotation, nameInScope: Name)(using Context): Boolean =
val sym = denot.symbol


nameInScope.startsWith(prefix) &&
sym.exists &&
completionsFilter(NoType, nameInScope) &&
Expand Down
11 changes: 10 additions & 1 deletion compiler/src/dotty/tools/repl/JLineTerminal.scala
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ final class JLineTerminal extends java.io.Closeable {
def currentToken: TokenData /* | Null */ = {
val source = SourceFile.virtual("<completions>", input)
val scanner = new Scanner(source)(using ctx.fresh.setReporter(Reporter.NoReporter))
var lastBacktickErrorStart: Option[Int] = None

while (scanner.token != EOF) {
val start = scanner.offset
val token = scanner.token
Expand All @@ -126,7 +128,14 @@ final class JLineTerminal extends java.io.Closeable {

val isCurrentToken = cursor >= start && cursor <= end
if (isCurrentToken)
return TokenData(token, start, end)
return TokenData(token, lastBacktickErrorStart.getOrElse(start), end)


// we need to enclose the last backtick, which unclosed produces ERROR token
if (token == ERROR && input(start) == '`') then
lastBacktickErrorStart = Some(start)
else
lastBacktickErrorStart = None
}
null
}
Expand Down
9 changes: 8 additions & 1 deletion compiler/src/dotty/tools/repl/ReplDriver.scala
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,19 @@ class ReplDriver(settings: Array[String],
state.copy(context = run.runContext)
}

private def stripBackTicks(label: String) =
if label.startsWith("`") && label.endsWith("`") then
label.drop(1).dropRight(1)
else
label

/** Extract possible completions at the index of `cursor` in `expr` */
protected final def completions(cursor: Int, expr: String, state0: State): List[Candidate] = {
def makeCandidate(label: String) = {

new Candidate(
/* value = */ label,
/* displ = */ label, // displayed value
/* displ = */ stripBackTicks(label), // displayed value
/* group = */ null, // can be used to group completions together
/* descr = */ null, // TODO use for documentation?
/* suffix = */ null,
Expand Down
58 changes: 58 additions & 0 deletions compiler/test/dotty/tools/repl/TabcompleteTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,62 @@ class TabcompleteTests extends ReplTest {
tabComplete("import quoted.* ; def fooImpl(using Quotes): Expr[Int] = { import quotes.reflect.* ; TypeRepr.of[Int].s"))
}

@Test def backticked = initially {
assertEquals(
List(
"!=",
"##",
"->",
"==",
"__system",
"`back-tick`",
"`match`",
"asInstanceOf",
"dot_product_*",
"ensuring",
"eq",
"equals",
"foo",
"formatted",
"fromOrdinal",
"getClass",
"hashCode",
"isInstanceOf",
"ne",
"nn",
"notify",
"notifyAll",
"synchronized",
"toString",
"valueOf",
"values",
"wait",
"→"
),
tabComplete("""|enum Foo:
| case `back-tick`
| case `match`
| case foo
| case dot_product_*
| case __system
|
|Foo."""stripMargin))
}


@Test def backtickedAlready = initially {
assertEquals(
List(
"`back-tick`"
),
tabComplete("""|enum Foo:
| case `back-tick`
| case `match`
| case foo
| case dot_product_*
| case __system
|
|Foo.`bac"""stripMargin))
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -1023,4 +1023,102 @@ class CompletionTest {
|class Foo[A]{ self: Futu${m1} => }""".withSource
.completion(m1, expected)
}

@Test def backticks: Unit = {
val expected = Set(
("getClass", Method, "[X0 >: Foo.Bar.type](): Class[? <: X0]"),
("ensuring", Method, "(cond: Boolean): A"),
("##", Method, "=> Int"),
("nn", Method, "=> Foo.Bar.type"),
("==", Method, "(x$0: Any): Boolean"),
("ensuring", Method, "(cond: Boolean, msg: => Any): A"),
("ne", Method, "(x$0: Object): Boolean"),
("valueOf", Method, "($name: String): Foo.Bar"),
("equals", Method, "(x$0: Any): Boolean"),
("wait", Method, "(x$0: Long): Unit"),
("hashCode", Method, "(): Int"),
("notifyAll", Method, "(): Unit"),
("values", Method, "=> Array[Foo.Bar]"),
("→", Method, "[B](y: B): (A, B)"),
("!=", Method, "(x$0: Any): Boolean"),
("fromOrdinal", Method, "(ordinal: Int): Foo.Bar"),
("asInstanceOf", Method, "[X0] => X0"),
("->", Method, "[B](y: B): (A, B)"),
("wait", Method, "(x$0: Long, x$1: Int): Unit"),
("`back-tick`", Field, "Foo.Bar"),
("notify", Method, "(): Unit"),
("formatted", Method, "(fmtstr: String): String"),
("ensuring", Method, "(cond: A => Boolean, msg: => Any): A"),
("wait", Method, "(): Unit"),
("isInstanceOf", Method, "[X0] => Boolean"),
("`match`", Field, "Foo.Bar"),
("toString", Method, "(): String"),
("ensuring", Method, "(cond: A => Boolean): A"),
("eq", Method, "(x$0: Object): Boolean"),
("synchronized", Method, "[X0](x$0: X0): X0")
)
code"""object Foo:
| enum Bar:
| case `back-tick`
| case `match`
|
| val x = Bar.${m1}"""
.withSource.completion(m1, expected)
}

@Test def backticksPrefix: Unit = {
val expected = Set(
("`back-tick`", Field, "Foo.Bar"),
)
code"""object Foo:
| enum Bar:
| case `back-tick`
| case `match`
|
| val x = Bar.`back${m1}"""
Copy link
Contributor

Choose a reason for hiding this comment

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

How about something that contains a space? E.g.

case `has space`
...
val x = Bar.`has ${m1}

Copy link
Member Author

@ckipp01 ckipp01 Mar 2, 2022

Choose a reason for hiding this comment

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

So this one is tricky. Mainly because when we trigger a completion here and we look at the path that is returned for Interactive.pathTo, it's actually empty because it no longer thinks the position you are looking at is part of anything. I have no idea how to get around that. Bar.`has${m1} works fine and even Bar.`has s${m1}, but not with only the preceding space.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it's a really niche case and I would just ignore it. It's highly unlikely that someone will want a completion on a space

.withSource.completion(m1, expected)
}

@Test def backticksSpace: Unit = {
val expected = Set(
("`has space`", Field, "Foo.Bar"),
)
code"""object Foo:
| enum Bar:
| case `has space`
|
| val x = Bar.`has s${m1}"""
.withSource.completion(m1, expected)
}

@Test def backticksCompleteBoth: Unit = {
val expected = Set(
("formatted", Method, "(fmtstr: String): String"),
("`foo-bar`", Field, "Int"),
("foo", Field, "Int")
)
code"""object Foo:
| object Bar:
| val foo = 1
| val `foo-bar` = 2
| val `bar` = 3
|
| val x = Bar.fo${m1}"""
.withSource.completion(m1, expected)
}

@Test def backticksWhenNotNeeded: Unit = {
val expected = Set(
("`formatted`", Method, "(fmtstr: String): String"),
("`foo-bar`", Field, "Int"),
("`foo`", Field, "Int")
)
code"""object Foo:
| object Bar:
| val foo = 1
| val `foo-bar` = 2
|
| val x = Bar.`fo${m1}"""
.withSource.completion(m1, expected)
}
}