fix(completions): add backticks when needed in completions

[ckipp01]

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

[ckipp01]

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?

[ckipp01]

Beauty shot showing that it works

[ckipp01]

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]

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!

[prolativ]

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

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

[ckipp01]

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.

[tgodzik]

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

[prolativ]

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

[ckipp01]

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?

[prolativ]

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]

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)

[ckipp01]

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

[ckipp01]

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]

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

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

[prolativ]

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]

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]

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]

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]

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

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]

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

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