Skip to content

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

Merged
merged 14 commits into from
Nov 23, 2018

Conversation

Duhemm
Copy link
Contributor

@Duhemm Duhemm commented Nov 20, 2018

No description provided.

@smarter
Copy link
Member

smarter commented Nov 20, 2018

Why are Java module classes excluded ? In term position, if I'm writing java.lang. I'd like completion to show me all the Java classes with static methods.

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 20, 2018

Ah that's a good point, this is indeed broken at the moment

I'm trying to avoid duplicates when doing import java.io.FileDescrip<tab>. This condition should be checked only if we're completing imports.

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 21, 2018

@smarter I've moved everything related to completions out of object Interactive and into object Completion. I've refactored it to make it easier to follow, as before everything was crammed in one method.

The actual logic of all the different steps needed to get all the completion candidates is unchanged, though.

I've removed termOnly and typeOnly in favor of a Mode which can be either Term, Type, Import or None. Mode.None will indicate that we cannot provide any suggestion for completions (for instance, trying to autocomplete a renaming import), so we don't have to worry about trying to complete at offset = 0.

@smarter
Copy link
Member

smarter commented Nov 21, 2018

Mode.None will indicate that we cannot provide any suggestion for completions (for instance, trying to autocomplete a renaming import)

Could we have an Option[CompletionMode] instead, since in the None case we could just early return with an empty list ?

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 22, 2018

Could we have an Option[CompletionMode] instead, since in the None case we could just early return with an empty list ?

That's already the case. I don't think it'd be cleaner to use an Option, but I'm happy to change it if you prefer.

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 22, 2018

@smarter More comments on this PR?

Copy link
Member

@smarter smarter left a 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.
@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 23, 2018

@smarter I've added _ to the list of completions when in import mode and qual.tpe.isStable. It looks like this is the check that is when typing the import. I don't know whether it should check realizability, nor why the check is skipped after typer :

https://github.com/lampepfl/dotty/blob/30b8b1ce174a2d0a2d05e5106c862d3c499c7908/compiler/src/dotty/tools/dotc/typer/Typer.scala#L1717-L1722

https://github.com/lampepfl/dotty/blob/30b8b1ce174a2d0a2d05e5106c862d3c499c7908/compiler/src/dotty/tools/dotc/typer/Checking.scala#L580-L582

@@ -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
Copy link
Member

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.
@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 23, 2018

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?

@smarter
Copy link
Member

smarter commented Nov 23, 2018

Sure.

@smarter
Copy link
Member

smarter commented Nov 23, 2018

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 List[CompletionItem] instead of a `List[Symbol], it could be defined a bit like this:

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 documentation field contain the documentation for both.

@Duhemm Duhemm merged commit 9a9aa31 into scala:master Nov 23, 2018
@Duhemm Duhemm deleted the fix/5460 branch November 23, 2018 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants