Skip to content
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

Instantiate Type Vars in completion labels of extension methods #18914

Merged

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Nov 13, 2023

This PR fixes the completion labels for extension methods and old style completion methods.

It required an API change of Completion.scala as it didn't contain enough information to properly create labels on presentation compiler side. Along with this following parts were rewritten to avoid recomputation:

  • computation of adjustedPath which is necessary for extension construct completions,
  • computation of prefix is now unified across the metals and presentation compilers,
  • completionKind was changed, and kind Scope, Member are now part of completion mode.

The biggest change is basically added support for old style extension methods.
I found out that the type var was not instantiated after ImplicitSearch computation, and was only calculated at later stages. I don't have enough knowledge about Inference and Typer to know whether this is a bug or rather an intended behaviour but I managed to solve it without touching the typer:

      def tryToInstantiateTypeVars(conversionTarget: SearchSuccess): Type =
        try
          val typingCtx = ctx.fresh
          inContext(typingCtx):
            val methodRefTree = ref(conversionTarget.ref, needLoad = false)
            val convertedTree = ctx.typer.typedAheadExpr(untpd.Apply(untpd.TypedSplice(methodRefTree), untpd.TypedSplice(qual) :: Nil))
            Inferencing.fullyDefinedType(convertedTree.tpe, "", pos)
        catch
          case error => conversionTarget.tree.tpe // fallback to not fully defined type

@rochala rochala marked this pull request as draft November 15, 2023 15:22
@rochala
Copy link
Contributor Author

rochala commented Nov 15, 2023

This PR needs a rewrite due to how type inference works in Scala 3.
In the snippet:

implicit class LUtil[T](xs: List[T]):
  def test1(x: T): List[T] = ???
extension [T](xs: List[T]) def test2(x: T): List[T] = ???

List(1,2,3).tes@@

The returned completions should be:
test1(x: T >: Int): List[T >: Int]
test2(x: T >: Int): List[T >: Int]

The reason behind it is that in Scala 3 type inference works on a whole application chain at the same time. So writing List(1, 2, 3).test("test") will compile and have type of List[Int | String]. It works that way only in apply chain, but in this issue, where completions don't have instantiated type vars it applies to both scenarios.

Both of them are basically chains:

List(1,2,3).test1("string") // LUtil[Int | String](List.apply[Int]([1, 2, 3 : Int]*)).test1("string")
List(1,2,3).test2("string") // test2[Int | String](List.apply[Int]([1, 2, 3 : Int]*))("string")

@rochala rochala force-pushed the instantiated-type-var-in-completion-labels branch from da6c055 to 2d7ad61 Compare January 23, 2024 12:56
@rochala rochala force-pushed the instantiated-type-var-in-completion-labels branch from 2d7ad61 to bcd7fe6 Compare January 23, 2024 13:22
@rochala
Copy link
Contributor Author

rochala commented Jan 23, 2024

After consultations, I've come to conclusion that type vars should stay as they are so:

implicit class LUtil[T](xs: List[T]):
  def test1(x: T): List[T] = ???
extension [T](xs: List[T]) def test2(x: T): List[T] = ???

List(1,2,3).tes@@

should return test1(x: Int): List[Int]

@rochala
Copy link
Contributor Author

rochala commented Jan 23, 2024

After this PR, there will be another one that will make Ignored tests pass. I didn't want to make this one even bigger.

@rochala rochala marked this pull request as ready for review January 23, 2024 13:25
@rochala rochala requested a review from tgodzik January 23, 2024 13:25
case (_: untpd.ImportOrExport) :: _ => Mode.ImportOrExport
case _ => Mode.None

val completionKind: Mode =
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this would be a bit more readable if the cases were merged with completionSymbolKind. I'm guessing this part came from metals but this division feels a bit odd..

Copy link
Contributor

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

Great job!
Some nitpicks, otherwise looks good.

Copy link
Contributor

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

LGTM

@rochala rochala merged commit 95266f2 into scala:main Feb 1, 2024
19 checks passed
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
@tgodzik tgodzik added the area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools label May 8, 2024
WojciechMazur pushed a commit that referenced this pull request Jun 28, 2024
This PR fixes the completion labels for extension methods and old style
completion methods.

It required an API change of `Completion.scala` as it didn't contain
enough information to properly create labels on presentation compiler
side. Along with this following parts were rewritten to avoid
recomputation:
- computation of adjustedPath which is necessary for extension construct
completions,
- computation of prefix is now unified across the metals and
presentation compilers,
- completionKind was changed, and kind Scope, Member are now part of
completion mode.


The biggest change is basically added support for old style extension
methods.
I found out that the type var was not instantiated after
`ImplicitSearch` computation, and was only calculated at later stages. I
don't have enough knowledge about Inference and Typer to know whether
this is a bug or rather an intended behaviour but I managed to solve it
without touching the typer:
```scala

      def tryToInstantiateTypeVars(conversionTarget: SearchSuccess): Type =
        try
          val typingCtx = ctx.fresh
          inContext(typingCtx):
            val methodRefTree = ref(conversionTarget.ref, needLoad = false)
            val convertedTree = ctx.typer.typedAheadExpr(untpd.Apply(untpd.TypedSplice(methodRefTree), untpd.TypedSplice(qual) :: Nil))
            Inferencing.fullyDefinedType(convertedTree.tpe, "", pos)
        catch
          case error => conversionTarget.tree.tpe // fallback to not fully defined type
```
[Cherry-picked 95266f2]
WojciechMazur pushed a commit that referenced this pull request Jun 30, 2024
This PR fixes the completion labels for extension methods and old style
completion methods.

It required an API change of `Completion.scala` as it didn't contain
enough information to properly create labels on presentation compiler
side. Along with this following parts were rewritten to avoid
recomputation:
- computation of adjustedPath which is necessary for extension construct
completions,
- computation of prefix is now unified across the metals and
presentation compilers,
- completionKind was changed, and kind Scope, Member are now part of
completion mode.


The biggest change is basically added support for old style extension
methods.
I found out that the type var was not instantiated after
`ImplicitSearch` computation, and was only calculated at later stages. I
don't have enough knowledge about Inference and Typer to know whether
this is a bug or rather an intended behaviour but I managed to solve it
without touching the typer:
```scala

      def tryToInstantiateTypeVars(conversionTarget: SearchSuccess): Type =
        try
          val typingCtx = ctx.fresh
          inContext(typingCtx):
            val methodRefTree = ref(conversionTarget.ref, needLoad = false)
            val convertedTree = ctx.typer.typedAheadExpr(untpd.Apply(untpd.TypedSplice(methodRefTree), untpd.TypedSplice(qual) :: Nil))
            Inferencing.fullyDefinedType(convertedTree.tpe, "", pos)
        catch
          case error => conversionTarget.tree.tpe // fallback to not fully defined type
```
[Cherry-picked 95266f2]
WojciechMazur added a commit that referenced this pull request Jul 1, 2024
…hods" to LTS (#20879)

Backports #18914 to the LTS branch.

PR submitted by the release tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants