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

ckipp01 · 2022-03-01T15:00:16Z

This PR adds in the functionality to add in backticks when needed when
giving back completions and also correctly returning completions when
the user has already started typing a backtick.

Hopefully this will fix the issues in the REPL and also rid the need of Ammonite and Metals to both have custom logic to handle this. Some of the work in here is based off stuff we have in Metals, which itself borrows from Ammonite. There are also parts of @tgodzik's work from #13369 in here and @alexarchambault's work in the early commits of #11794.

Fixes: #4406, #14006

This PR adds in the functionality to add in backticks when needed when giving back completions and also correctly returning completions when the user has already started typing a backtick. Fixes: scala#4406, scala#14006

ckipp01 · 2022-03-01T15:02:50Z

compiler/src/dotty/tools/dotc/interactive/Completion.scala

+         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


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?

compiler/src/dotty/tools/dotc/interactive/Completion.scala

ckipp01 · 2022-03-01T15:07:13Z

Beauty shot showing that it works

ckipp01 · 2022-03-01T15:57:36Z

Also note, I was working on #12514 but it introduces other issues. I'm able to detect the <error> in the import and that it was caused by an unclosed backtick, but I'm unsure what the compiler should actually return here. In the case given in the actual issue import scala.util.chaining.`sc there is no need for the back tick here, so while I can get it to return the scalaUtilChainingOps, it's not backticked, because it doesn't need to be. In this scenario what's actually desirable? Should we detect that they are trying to backtick something that isn't needed and then backtick that completion item when it's returned? That might work smoother but I do find it a bit odd mainly because then what do you expect if you try to do a completion with .`a which returns 10 members, should you backtick all ten of those members? Again that seems odd to me.

tgodzik · 2022-03-02T09:33:44Z

Also note, I was working on #12514 but it introduces other issues. I'm able to detect the <error> in the import and that it was caused by an unclosed backtick, but I'm unsure what the compiler should actually return here. In the case given in the actual issue import scala.util.chaining.`sc there is no need for the back tick here, so while I can get it to return the scalaUtilChainingOps, it's not backticked, because it doesn't need to be. In this scenario what's actually desirable? Should we detect that they are trying to backtick something that isn't needed and then backtick that completion item when it's returned? That might work smoother but I do find it a bit odd mainly because then what do you expect if you try to do a completion with .`a which returns 10 members, should you backtick all ten of those members? Again that seems odd to me.

I would even remove the backtick if it's no needed. I think it's a rare scenario that someone starts with a backtick if they actually don't need it.

tgodzik

Some minor comments, but it looks like a good way to go. Thanks!

compiler/src/dotty/tools/dotc/interactive/Completion.scala

compiler/src/dotty/tools/repl/ReplDriver.scala

prolativ · 2022-03-02T09:11:48Z

language-server/test/dotty/tools/languageserver/CompletionTest.scala

+           |    case `back-tick`
+           |    case `match`
+           |  
+           |  val x = Bar.`back${m1}"""


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

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

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.

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

compiler/src/dotty/tools/repl/ReplDriver.scala

prolativ · 2022-03-02T09:33:30Z

compiler/src/dotty/tools/dotc/interactive/Completion.scala

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

+      // We special case Select here because we want to determine if the name


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

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?

compiler/src/dotty/tools/dotc/interactive/Completion.scala

prolativ · 2022-03-02T10:19:58Z

I checked out to this branch and played with it a bit in REPL. Having this code defined

object Foo:
  val foo = 1
  val `foo-bar` = 2
  val `bar` = 3
  val `match` = 4
  val `has space` = 5

val `foo-baz` = 6

I tried completions with prefixes:
Foo.fo -> suggests foo but not `foo-bar`
Foo.`fo -> suggests `foo-bar` but not `foo`
Foo.`ba -> doesn't suggest anything
Foo.`has (with a trailing space) -> doesn't suggest anything
`fo -> doesn't suggest anything

prolativ · 2022-03-02T10:23:53Z

Even if we include backticks in labels of Completions I think it would be better to strip them when displaying the list of suggestions in REPL (similarly to how this is displayed in Metals)

This commit addresses the situation where you are trying to use a backtick when it's not needed. For example if you have: ```scala object Foo: case `bar` case foo Foo.`b ``` You still expect to get a completion bar being backticked. This also ensure that if you try ``Foo.`fo`` you also still get a completion.

ckipp01 · 2022-03-02T13:41:05Z

I checked out to this branch and played with it a bit in REPL. Having this code defined
object Foo:
  val foo = 1
  val `foo-bar` = 2
  val `bar` = 3
  val `match` = 4
  val `has space` = 5

val `foo-baz` = 6
I tried completions with prefixes: Foo.fo -> suggests foo but not `foo-bar` Foo.`fo -> suggests `foo-bar` but not `foo` Foo.`ba -> doesn't suggest anything Foo.`has (with a trailing space) -> doesn't suggest anything `fo -> doesn't suggest anything

So I've addressed most of these but still have one or two more to go.

Foo.fo -> suggests foo but not foo-bar

This is actually the REPL trying to be smart and recognizing that what you have doesn't start with a ` so it's not going to offer you that. However I added a test showing that it is actually returned.

Foo.fo -> suggests foo-barbut notfoo`

This one is now fixed, and really now anytime there is a backtick even when not needed it will still complete it and add the backticks.

Foo.`ba -> doesn't suggest anything

This one is also now fixed

Foo.`has (with a trailing space) -> doesn't suggest anything

This one I addressed above in a comment as it's a whole other beast that might have to be fixed separately since the issue is totally different with pathTo not returning anything.

`fo -> doesn't suggest anything

~~This one isn't yet fixed but I'll add another commit to address it.~~ So I looked into this one a bit, and I'm actually confused at what do do. For example if this is fo then you get a nice Ident(fo) to deal with. However, the result of pathTo for `fo is crazy. For example, take the following code:

object Foo:
  val `foo-bar` = 1

  `fo${m1}

What's returned here from pathTo is:

List(TypeDef(Foo$,Template(DefDef(<init>,List(List()),TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Unit)],EmptyTree),List(Apply(Select(New(TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class lang)),class Object)]),<init>),List())),ValDef(_,SingletonTypeTree(Ident(Foo)),EmptyTree),List(ValDef(foo-bar,TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Int)],Literal(Constant(1)))))), PackageDef(Ident(<empty>),List(ValDef(Foo,Ident(Foo$),Apply(Select(New(Ident(Foo$)),<init>),List())), TypeDef(Foo$,Template(DefDef(<init>,List(List()),TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Unit)],EmptyTree),List(Apply(Select(New(TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class lang)),class Object)]),<init>),List())),ValDef(_,SingletonTypeTree(Ident(Foo)),EmptyTree),List(ValDef(foo-bar,TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Int)],Literal(Constant(1)))))))))```

This removes the backticks when you are seeing all the available options, but still includes them in the actual value of the completion.

ckipp01 · 2022-03-02T13:55:16Z

Even if we include backticks in labels of Completions I think it would be better to strip them when displaying the list of suggestions in REPL (similarly to how this is displayed in Metals)

Done

som-snytt · 2022-03-02T15:07:50Z

The scala 2 ticket is from 2013. scala/bug#6919

I'm unable to add heart emoji reaction more than once.

prolativ · 2022-03-02T15:54:20Z

The problem with Foo.`has (with a trailing space) indeed seems to be deeper. It looks like the parser strips whitespaces from the end of the input so the position of the cursor during completion is outside of the parsed tree. Similarly there are no completions for Foo. (with a trailing space).

Could you elaborate on the problem in REPL for Foo.fo? It seems to work correctly in case of Foo.ma, where is suggests Foo.`match` . However if you add val maatch = 7 as a member of Foo then only maatch is suggested and match isn't

ckipp01 · 2022-03-02T18:59:26Z

Could you elaborate on the problem in REPL for Foo.fo? It seems to work correctly in case of Foo.ma, where is suggests Foo.`match`. However if you add val maatch = 7 as a member of Foo then only maatch is suggested and match isn't

Sure, you can see it illustrated in the backticksCompleteBoth test. So in the case of Foo.fo both foo and `foo-bar` are being returned, but since jline sees that you aren't starting with a ` it just filters it out and only offers foo. The same things happens with the match example. It actually does return both maatch and `match` but since you aren't starting with a ` jline filters the `match` out. This seems to differ when there is only one completion option returned and why trying to complete Foo.ma will complete `match` when it's the only completion item returned. All that to say, this seems to be something that jline is doing, not something that the completion logic that these changes are doing. I can try to dig into that further in the future, but trying to limit what I all touch in this.

tgodzik · 2022-03-03T19:52:21Z

What's returned here from pathTo is:

I looks like the enclosing class is only returned, I think it's a case that no actual Ident is being created and it's something we might want to fix in the parser. We should return an Ident with an error name in that case. We could do it in a follow up though.

tgodzik

Looks awesome! I would say that any improvement in case of backticks is a good way forward, but I would not focus on this as it is a super niche use case with some of the edge cases.

ckipp01 · 2022-03-03T20:50:35Z

Looks awesome! I would say that any improvement in case of backticks is a good way forward, but I would not focus on this as it is a super niche use case with some of the edge cases.

Cool, glad to hear this 😄 . I spent a big chunk of the day today looking at the edge cases here and worked a bit with @bishabosha on it as well. The issue seems to be that when we encounter unclosed backticks in the parser depending on where they are, we create various different things. So there are two more cases locally I actually have a fix for, but it's more of hack than a fix so I really don't know how I feel about them. The situations are as follows:

val `foo-bar` = 3

val x = `foo@@

`foo here ends up being a Literal(Constant(null)). So I can capture that in the various places, but it's messy. Furthermore if this is a Type instead of a Term like:

type Foo = `Lis@@

`Lis here will end up being a TypeDef(Foo,Ident(<error>)).

When you are just doing

val `foo-bar` = 3

`foo

`foo here doesn't find anything so pathTo just returns the entire document.

So as you can see, it ends up adding a ton of special cases into the completion logic which really clogs it all up to capture all these situations.

One thing we tried was to actually tag this with a StickyKey attachment in the parser so that we could just check later on, but for some reason the attachment was no longer there when I was checking for it.

All that to say, I think maybe this should be fixed in the parser and try to unify the way we handle unclosed backticks in a way that can easily be determined later on in a similar way. The fix that is here currently should take care of probably the majority of cases users will actually hit on, but these niche ones are difficult. Ha, I've been hitting more walls than expected with this 😅

tgodzik · 2022-03-04T06:31:58Z

@prolativ what do you say that we merge this as it's already a great improvement? We should but the remaining edge cases into an issue.

bishabosha · 2022-03-04T17:20:29Z

We had a small discussion and it could be worth trying to make a new Token for an unfinished backticked identifier, which we can then make a valid tree for

ckipp01 · 2022-03-07T09:46:25Z

@prolativ what do you say that we merge this as it's already a great improvement? We should but the remaining edge cases into an issue.

I'm all for this, and yea I can extract the remaining edgecases out into issues with details since I think the way forward is what @bishabosha recommends, but I'd rather not mix that with this pr.

prolativ · 2022-03-07T10:28:32Z

Ok. I started to dig a bit into the corner case with

object Foo:
  val foo = 1
  val `foo-bar` = 2
  
Foo.`fo<TAB>

myself but indeed this seems very tricky so let's get this merged first

fix(completions): add backticks when needed in completions

f61c6c8

This PR adds in the functionality to add in backticks when needed when giving back completions and also correctly returning completions when the user has already started typing a backtick. Fixes: scala#4406, scala#14006

ckipp01 commented Mar 1, 2022

View reviewed changes

ckipp01 linked an issue Mar 1, 2022 that may be closed by this pull request

completion of back-ticked members does not give backticks in REPL #14006

Closed

tgodzik added the area:ide label Mar 2, 2022

tgodzik requested changes Mar 2, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/interactive/Completion.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/interactive/Completion.scala Show resolved Hide resolved

compiler/src/dotty/tools/repl/ReplDriver.scala Outdated Show resolved Hide resolved

prolativ reviewed Mar 2, 2022

View reviewed changes

dwijnand assigned prolativ Mar 2, 2022

fix: don't show backticks in display on repl.

5e5866b

This removes the backticks when you are seeing all the available options, but still includes them in the actual value of the completion.

ckipp01 requested review from tgodzik and prolativ March 2, 2022 14:22

tgodzik approved these changes Mar 3, 2022

View reviewed changes

prolativ approved these changes Mar 7, 2022

View reviewed changes

prolativ merged commit 3c5dbc3 into scala:main Mar 7, 2022

ckipp01 deleted the backTickedOff branch March 7, 2022 11:24

This was referenced Mar 8, 2022

fix: adjust tests for 3.2.0-NIGHTLY scalameta/metals#3687

Merged

Missing completions when give a backticked prefix in various positions #14647

Open

Kordyjan added this to the 3.1.3 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ckipp01 commented Mar 1, 2022 •

edited

Loading

ckipp01 Mar 1, 2022

ckipp01 commented Mar 1, 2022

ckipp01 commented Mar 1, 2022 •

edited

Loading

tgodzik commented Mar 2, 2022

tgodzik left a comment

prolativ Mar 2, 2022

ckipp01 Mar 2, 2022 •

edited

Loading

tgodzik Mar 3, 2022

prolativ Mar 2, 2022

ckipp01 Mar 2, 2022

prolativ commented Mar 2, 2022

prolativ commented Mar 2, 2022

ckipp01 commented Mar 2, 2022 •

edited

Loading

ckipp01 commented Mar 2, 2022

som-snytt commented Mar 2, 2022

prolativ commented Mar 2, 2022

ckipp01 commented Mar 2, 2022 •

edited

Loading

tgodzik commented Mar 3, 2022

tgodzik left a comment

ckipp01 commented Mar 3, 2022 •

edited

Loading

tgodzik commented Mar 4, 2022

bishabosha commented Mar 4, 2022

ckipp01 commented Mar 7, 2022

prolativ commented Mar 7, 2022

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

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

Conversation

ckipp01 commented Mar 1, 2022 • edited Loading

ckipp01 Mar 1, 2022

Choose a reason for hiding this comment

ckipp01 commented Mar 1, 2022

ckipp01 commented Mar 1, 2022 • edited Loading

tgodzik commented Mar 2, 2022

tgodzik left a comment

Choose a reason for hiding this comment

prolativ Mar 2, 2022

Choose a reason for hiding this comment

ckipp01 Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

tgodzik Mar 3, 2022

Choose a reason for hiding this comment

prolativ Mar 2, 2022

Choose a reason for hiding this comment

ckipp01 Mar 2, 2022

Choose a reason for hiding this comment

prolativ commented Mar 2, 2022

prolativ commented Mar 2, 2022

ckipp01 commented Mar 2, 2022 • edited Loading

ckipp01 commented Mar 2, 2022

som-snytt commented Mar 2, 2022

prolativ commented Mar 2, 2022

ckipp01 commented Mar 2, 2022 • edited Loading

tgodzik commented Mar 3, 2022

tgodzik left a comment

Choose a reason for hiding this comment

ckipp01 commented Mar 3, 2022 • edited Loading

tgodzik commented Mar 4, 2022

bishabosha commented Mar 4, 2022

ckipp01 commented Mar 7, 2022

prolativ commented Mar 7, 2022

ckipp01 commented Mar 1, 2022 •

edited

Loading

ckipp01 commented Mar 1, 2022 •

edited

Loading

ckipp01 Mar 2, 2022 •

edited

Loading

ckipp01 commented Mar 2, 2022 •

edited

Loading

ckipp01 commented Mar 2, 2022 •

edited

Loading

ckipp01 commented Mar 3, 2022 •

edited

Loading