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

Fix #5460: Improve completion of import nodes #5476

Merged
merged 14 commits into from Nov 23, 2018

Conversation

Projects
None yet
3 participants
@Duhemm
Copy link
Contributor

Duhemm commented Nov 20, 2018

No description provided.

@Duhemm Duhemm added the area:ide label Nov 20, 2018

@smarter

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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 Duhemm force-pushed the dotty-staging:fix/5460 branch from 8b3432e to eba580c Nov 20, 2018

@Duhemm Duhemm force-pushed the dotty-staging:fix/5460 branch from 5f8d2d4 to 12fa20c Nov 21, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

Duhemm commented Nov 22, 2018

@smarter More comments on this PR?

@smarter
Copy link
Member

smarter left a comment

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

Duhemm added some commits Nov 20, 2018

Exclude Java module class in completion in imports
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`.
Prefer types over terms in import completions
When completing imports, and several symbols with the same name are
possible options, we show only the type symbols.
Improve display of completion results
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.

@Duhemm Duhemm force-pushed the dotty-staging:fix/5460 branch from 3a9aeb3 to 3ba7e02 Nov 23, 2018

Duhemm added some commits Nov 21, 2018

Refactor completions, add interactive.Completion
This commit moves all completion-related methods to their own file and
cleans it up.
Fix completion without prefix
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 Duhemm force-pushed the dotty-staging:fix/5460 branch from 3ba7e02 to daaccee Nov 23, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

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 :

def typedImport(imp: untpd.Import, sym: Symbol)(implicit ctx: Context): Import = track("typedImport") {
val expr1 = typedExpr(imp.expr, AnySelectionProto)
checkStable(expr1.tpe, imp.expr.pos)
if (!ctx.isAfterTyper) checkRealizable(expr1.tpe, imp.expr.pos)
assignType(cpy.Import(imp)(expr1, imp.selectors), sym)
}

/** Check that type `tp` is stable. */
def checkStable(tp: Type, pos: Position)(implicit ctx: Context): Unit =
if (!tp.isStable) ctx.error(ex"$tp is not stable", pos)

@@ -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

This comment has been minimized.

@smarter

smarter Nov 23, 2018

Member

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.

@Duhemm Duhemm force-pushed the dotty-staging:fix/5460 branch from daaccee to ace5092 Nov 23, 2018

Duhemm added some commits Nov 23, 2018

Extract name kind from `ERROR`
The parser tells us whether the error happened in term or type position,
so we can use this to provide better completion.
Show stable terms when completing types
Stable terms can be part of the path to a type, and should be shown in
completion when writing a type.
@Duhemm

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

smarter commented Nov 23, 2018

Sure.

@Duhemm Duhemm force-pushed the dotty-staging:fix/5460 branch from db87741 to 89f850a Nov 23, 2018

@smarter

This comment has been minimized.

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 lampepfl:master Nov 23, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@Duhemm Duhemm deleted the dotty-staging:fix/5460 branch Nov 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment