-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #5460: Improve completion of import nodes #5476
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
Conversation
Why are Java module classes excluded ? In term position, if I'm writing |
Ah that's a good point, this is indeed broken at the moment I'm trying to avoid duplicates when doing |
language-server/test/dotty/tools/languageserver/CompletionTest.scala
Outdated
Show resolved
Hide resolved
language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala
Show resolved
Hide resolved
@smarter I've moved everything related to completions out of The actual logic of all the different steps needed to get all the completion candidates is unchanged, though. I've removed |
Could we have an |
That's already the case. I don't think it'd be cleaner to use an |
@smarter More comments on this PR? |
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.
We still need to treat _
specially: if I write:
object Foo {
val _foo = 1
}
import Foo._
Then completions will suggest _foo
and vscode will accept the completion when pressing enter
When completing imports, we don't want to show duplicates for instance when the user does: import java.io.FileDesc<tab> When completing imports, we filter out the Java module classes, to keep only the classes. This avoids showing twice `java.io.FileDescriptor`.
When completing imports, and several symbols with the same name are possible options, we show only the type symbols.
When available, we display the formatted documentation for the imported symbol. For type symbols, we display the full name of the symbol, and for term symbols we show their type.
This commit moves all completion-related methods to their own file and cleans it up.
When the user writes `qualifier.<tab>`, we get the tree corresponding to `qualifier.<error>`. When encountering the error, we were not doing any completion, but we should consider both terms and types in this context.
@smarter I've added |
@@ -795,12 +796,17 @@ object DottyLanguageServer { | |||
|
|||
val label = sym.name.show | |||
val item = new lsp4j.CompletionItem(label) | |||
item.setDetail(sym.info.widenTermRefExpr.show) | |||
val detail = if (sym.isType) sym.showFullName else sym.info.widenTermRefExpr.show |
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.
Idea: instead of just showing the full name we could show the kind of the symbol too, e.g. "class a.b.c.Foo", "val x", ... and in the case where there's more than one symbol with the same name, we could show both at the same time "class and object a.b.c.Foo", ... We can reuse the code used in the REPL when printing a declaration.
The parser tells us whether the error happened in term or type position, so we can use this to provide better completion.
Stable terms can be part of the path to a type, and should be shown in completion when writing a type.
I just noticed that considering stable terms when completing types make Java classes appear twice... Let's always remove duplicate and prefer the types over the terms? |
Sure. |
Doesn't have to be done now but I think what we should do eventually is make our own custom Completion data type and return a case class Completion(label: String, description: String, documentation: String, symbols: List[String]) This way we can handle completions with no symbol (wildcards, structural types) and multiple symbols (both a class and an object exist, ...) in a uniform way. For example when there's both a class and an object, we could have the |
No description provided.