SignatureHelp gives apply of the current symbol instead of enclosing apply

[ckipp01]

Compiler version

3.1.1

Explanation

I hit on this with Metals but then testing in Dotty with the SignatureHelp tests it suffers from the same issue, so the fix should probably be here.

Given the following code with the @@ signifying a trigger of the user to get signature help.

Li@@st(1, 2,  3)

The user will actually get the signature help of the apply on the List. That may not seem that problematic, but when you have something like this:

def doThing(myList: List[Int]) = ???

doThing(L@@ist(1, 2, 3))

The user is expecting to see the signature help for doThing, not the apply on List.

Example test

Here is what I used to test this:

  @Test def applyWhenYouDontWantIt: Unit = {
    val signature =
      S("apply[A]", Nil, List(List(P("elems", "A*"))), Some("CC[A]"))
    code"""|object O:
           |  def doThing(myList: List[Int]) = ???
           |  doThing(Li${m1}st(1, 2, 3)""".withSource
      .signatureHelp(m1, List(signature), Some(0), 0)
  }

Expectation

The test above will pass, when you really want it to fail and to be showing you the info for doThing meaning I care about the myList here.

[bishabosha]

I was a bit confused reading this, do you mean you still want to see the List.apply method, but with A substituted for Int, so rendering in metals like def apply(elems: Int*): List[Int]?

[ckipp01]

I was a bit confused reading this, do you mean you still want to see the List.apply method, but with A substituted for Int, so rendering in metals like def apply(elems: Int*): List[Int]?

No, I mean in the situation for doThing(Li${m1}st(1,2,3)) I would hope that the signature help would show me doThing(myList: List[Int]) with myList being highlighted. That should be the correct behavior here imo since that's the actual enclosing apply that we care about when triggering a signature help here. This is currently how it works for Scala 2 in Metals.

Whereas with Scala 3 triggering a signature help in the same position instead gives me the signature for the List.apply.

[prolativ]

Yeah, I didn't get it like this at the first moment either. So even in a case like

val x = foo1(foo2(foo3(foo4(foo5(0)))))

when you hover on foo5 you would expect to get the signature help only for foo1?

[ckipp01]

In Scala 2 the way that it's set up in Metals is that it will also go to the closest enclosing apply. So in the situation you have @prolativ if you triggered a signature help like so:

val x = foo1(foo2(foo3(foo4(fo${m1}o5(0)))))

This apply you actually care about would be the closest enclosing one meaning foo4.

Whereas currently in Scala 3 this shows:

However this isn't what is desired. The only time the signature for foo5 here should be shown is if you triggered it in:

val x = foo1(foo2(foo3(foo4(foo5(${m1})))))

Does this make sense?

[ckipp01]

Furthermore, this behavior also leads to other issues. For example

def doThing(myList: List[Int], second: Int) = ???

do${m1}Thing(List(1,2,3), 2)

Will result in:

Notice how it shows the second parameter is the current one? Signature help in this scenario shouldn't return anything since you're not in an apply. You see the same thing whenever you try to trigger it on List for example since the apply span covers the entire <<List(1, 2, 3)>> not just List(<<1, 2, 3>>. However you really only want signature help triggerable in the latter region.

[prolativ]

Ok, I think I get it now

[rochala]

Furthermore, this behavior also leads to other issues. For example

def doThing(myList: List[Int], second: Int) = ???

do${m1}Thing(List(1,2,3), 2)

Will result in:

Notice how it shows the second parameter is the current one? Signature help in this scenario shouldn't return anything since you're not in an apply. You see the same thing whenever you try to trigger it on List for example since the apply span covers the entire <<List(1, 2, 3)>> not just List(<<1, 2, 3>>. However you really only want signature help triggerable in the latter region.

@ckipp01 Should we remove signatureHelp when its triggered by position on toplevel function name such as doThi{m1}g(Nil, "") or rather make activeParameter an Option ( it will require changes in existing tools ).
In my opinion we should leave signatureHelp as is and just fix activeParameter selection but what's your opinion on that. I know that metals doesn't support such calls doThi{m1}g(Nil, "")

[ckipp01]

@ckipp01 Should we remove signatureHelp when its triggered by position on toplevel function name such as doThi{m1}g(Nil, "") or rather make activeParameter an Option ( it will require changes in existing tools ).
In my opinion we should leave signatureHelp as is and just fix activeParameter selection but what's your opinion on that. I know that metals doesn't support such calls doThi{m1}g(Nil, "")

So I'm most familiar with Metals and not sure how this works with other tooling, but in the scenario where the user triggers a signature help on doThi{m1}g(Nil, "") in Scala 2 with Metals, nothing is returned as you'd need to trigger it inside like doThig(Nil, {m1}""). I'm used to this behavior so my first reaction was that in your example, the entire signature help shouldn't provide anything, but then again, I see how it'd be useful. The idea of an activeParameter sort of implies that it's always being triggered when you have some sort of position in the argument list, where in your example, you don't since you're not inside. @tgodzik do you by chance have any thoughts on this?

[tgodzik]

I think we should only show the signature help when we are inside parenthesis, which is where it's most useful. The purpose of it is to be able to write the correct parameters. For just seeing the signature, we can use hover.

Isn't the issue here that we don't try to find the most recent enclosing apply, but use the identifier to calculate the signature help? We should make sure that we are actually inside the args.