fix: Instantiate type parameters for parametric extension methods in signature help #25041
Conversation
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
Thanks for the contribution, I think we should be able to reuse some methods from the compiler for this.
| if shown.contains("Any") || shown.contains("Nothing") || shown.contains("error") then param | ||
| else | ||
| param match | ||
| case p: Signatures.MethodParam => p.copy(tpe = shown) |
There was a problem hiding this comment.
This only changes a single parameter and leave all the other type parameters unchanged. We should change every instance of a generic type parameter. I think this might be too complex a code to do that, we should be able to use something along the line sof asSeenFrom, though not 100% sure about the specifics.
There was a problem hiding this comment.
Thanks for the suggestion.
In the latest push, I focused on ensuring the output is visually correct (instantiating the types properly) by using the ShortenedTypePrinter.
I kept the manual refinement logic for now to ensure I didn't break the existing behaviour while fixing the bug, but I agree asSeenFrom or similar would be cleaner. I can look into refactoring this to be simpler in a follow-up or next iteration.
| s"""Found documentation for scala/Option#fold(). | ||
| |fold[B](ifEmpty: => B)(f: Int => B): B | ||
| | ^^^^^^^^^^^ | ||
| |fold[B](ifEmpty: => B)(f: Int => String): B |
There was a problem hiding this comment.
| |fold[B](ifEmpty: => B)(f: Int => String): B | |
| |fold[String](ifEmpty: => String)(f: Int => String): String |
All the B should change, otherwise it looks like B is a totally separate parameter.
There was a problem hiding this comment.
Fixed in the latest commit.
I updated the test expectations so that all type parameters are instantiated in the expected output strings (e.g. fold[String]... => String).
dorodb-web22
force-pushed
the
fix/parametric-extension-signature
branch
from
5ea5491 to
e78c5de
Compare
| case _ => false | ||
| } | ||
|
|
||
| enclosingApply match |
There was a problem hiding this comment.
I suggested using asSeenFrom methods and alike. This approach seems very manual and fragile. We should instead of fixing the created signature, fix the code that creates it.
There was a problem hiding this comment.
Thanks for the pointers.
I refactored the logic into Signatures.scala
using asSeenFrom and appliedTo as suggested. I also added the regression test for the parametric extension case.
The diff looks large mostly because I had to update the existing test expectations in SignatureHelpSuite to match the corrected signatures (e.g. showing [Int] instead of [T])
zielinsky
left a comment
zielinsky
left a comment
There was a problem hiding this comment.
Just like @tgodzik metioned above, this fix should be made where the signature is created (Signatures.signatureHelp), not after. Additionally, these changes do not test the original bug mentioned in the description 😕 (could you add an appropriate test for this case?)
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
There is a lot of unrelated changes, I am afraid this might be too complex topic if you're using LLMs for it. And if not I would probably suggest tackling another issue anyway.
| |foo[K, V]()(x: Int): Unit | ||
| | ^^^^^^ | ||
| |foo[Any, Any]()(x: Int): Unit | ||
| | ^^^^^^ |
There was a problem hiding this comment.
This no longer points correctly to the parameter
| |} | ||
| |""".stripMargin, | ||
| "foo[K, V](): Unit" | ||
| "foo[K, V](): (): Unit" |
There was a problem hiding this comment.
| "foo[K, V](): (): Unit" | |
| "foo[K, V](): Unit" |
| """|test[A](x: A): Foo[A] | ||
| | ^^^^ | ||
| """|test[Int](x: Int): Foo[A] | ||
| | ^^^^^^ |
| | test(@@) | ||
| |""".stripMargin, | ||
| "test(): (Int, Int)" | ||
| """|test(): (Int, Int) |
| |""".stripMargin, | ||
| """|Test(x: Int, y: Int) | ||
| | ^^^^^^ | ||
| | ^^^^^^ |
| | | ||
| | @@ | ||
| |} | ||
| | } |
dorodb-web22
force-pushed
the
fix/parametric-extension-signature
branch
3 times, most recently
from
312af4b to
ddc64ba
Compare
dorodb-web22
force-pushed
the
fix/parametric-extension-signature
branch
from
ddc64ba to
1238df7
Compare
3 participants
Description
This PR fixes an issue where type arguments for parametric extension methods were not correctly rendered in signature help. Previously, they would appear as the generic type parameter (e.g.,
p: T) instead of the instantiated type (e.g.,p: Int).Context
This issue was originally reported in the Metals repository (scalameta/metals#7911). Upon review, the maintainers indicated that the fix should be implemented upstream in the Scala 3 Presentation Compiler.
Changes
refineSignaturesby checking the method application's instantiated types.AnyorNothing), preserving the original signature in unconstrained cases.parametric-extensionto SignatureHelpSuite.scala and updated existing test expectations to reflect more precise type information.Verification
extension [T](<x: T>) def foo(p: T)correctly displaysfoo(p: Int)when invoked on anInt.SignatureHelpSuitepass.@tgodzik