-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Instantiate Type Vars in completion labels of extension methods #18914
Conversation
presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionSuite.scala
Outdated
Show resolved
Hide resolved
This PR needs a rewrite due to how type inference works in Scala 3. 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: The reason behind it is that in Scala 3 type inference works on a whole application chain at the same time. So writing 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") |
da6c055
to
2d7ad61
Compare
2d7ad61
to
bcd7fe6
Compare
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 |
After this PR, there will be another one that will make Ignored tests pass. I didn't want to make this one even bigger. |
presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionSuite.scala
Show resolved
Hide resolved
presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionKeywordSuite.scala
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/CompletionPos.scala
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/CompletionPos.scala
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala
Outdated
Show resolved
Hide resolved
case (_: untpd.ImportOrExport) :: _ => Mode.ImportOrExport | ||
case _ => Mode.None | ||
|
||
val completionKind: Mode = |
There was a problem hiding this comment.
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..
There was a problem hiding this 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.
presentation-compiler/src/main/dotty/tools/pc/IndexedContext.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/CompletionValue.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/CompletionPos.scala
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala
Outdated
Show resolved
Hide resolved
presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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]
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]
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: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: