-
Notifications
You must be signed in to change notification settings - Fork 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
Context-sensitive IDE completions #3960
Conversation
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.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:
- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Added" instead of "Add")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
I have one issue that's still remaining: When doing selection completions, let's say following
we often get a completion on an error tree Is there a way to get round this? I.e. force the client to give us the latest source? |
Separate TODO: The REPL should use the new completer in Interactive instead of the old one. |
An error tree created by |
ctx | ||
} | ||
|
||
def contextOfPath(path: List[Tree])(implicit ctx: Context): Context = path match { |
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.
This looks very fragile. Since Typer already has to do a similar job for typedIdent to work, I was wondering if instead we could make a subclass of Typer that:
- Skips trees outside of the current position
- As soon as it reaches a leaf, find all completions and then exit.
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.
That's pretty much how scalac's PC works. It's a possibility, but would be a much larger rewrite compared to what we have. Other downsides: (1) it could be a lot slower, depending how much needs to be typechecked. (2) we create new symbols, which might confuse things. (3) Generally, more entanglement of normal compiler and presentation code.
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.
(2) we create new symbols, which might confuse things.
But a ReTyper doesn't create symbols, does it? It also should be possible to not retypecheck anything and rely on the existing typechecked tree.
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.
That's true. Retypers don't create symbols. But I think messing with them will ultimately be more fragile. It comes down to whether we can coax a 3000 line module to do something quite different than what it was intended for, or whether we write a 40 line method that extracts the information directly. I was actually quite pleased how straightforward that was. True, there are some small differences, i.e. we ignore the "in supercall" distinctions. But I don't think these matter. And it's not clear that a retyper approach would not have similar differences.
That's not what I am seeing. I a see a lot of trees where the selector is literally " It does not happen if I come from a full identifier like |
Sort of related, let's think about how the parser enable completions like I worked around this in the completion work I did for the 2.12 REPL by inserting some dummy text at the cursor, but you then need to account for the length of that text when looking at positions. So maybe the parser should be told the cursor location directly in interactive mode, and promote unexpected keywords to idents or selections. |
Couldn't we just do that all the time? |
Also, during my REPL work, I added a layer of abstraction over the scope- and member-completion APIs, |
I guess that's an implementation choice. So long as |
I did some more investigation. If I type e.g. It's not so bad, since we will simply return all completions from the For the same reason typing |
This allows the compiler to succeed even if the project is not fully built. It also gives navigation capability into directly dependent files.
Strangely, it seems that at least for simple dependencies, the IDE already knows about classes even if class name != source file name. @smarter Do you have an idea why?
Use less state: just a single field that can be either a tree or a tree provider This should make it easy to integrate sourfe files in IDE. The refactoring required a major refactoring in ReadTastyTreesFromClasses, which was a mess before. Hopefully it's not right. There are still "spurious" warnings coming from classes loaded twice. These will require a refactoring of FromTastyTest to avoid. They do not happen if the FromTasty frontend is invoked stand-alone.
1. Since Symbol.tree no longer automatically reads positions (it should not, really, that's what we have contexts for!), ReadPositions has to be turned on in therelevant tests. 2. Disabled simpleClass which fails again because wrong class file ends up being decompiled.
... so that it can be shared with SourcefileLoader in the future.
Various tweaks necessary so that symbols can be loaded from source.
After having implemented the interface between IDE and SymbolLoaders it turns out lateUnits is not needed, and lateFiles is used only internally.
- Don't fail if a class file is not found - Consult name tables of Tasty info for a possible match before loading tree. - Do the same for rename
Found to be usused by invoking findReferences, yay!
Following "name all the things" mantra.
We need that to be able to establish precise contexts.
Collect them all in NamerContextOps. Previously some were also in Typer and in Context itself.
This can cause MergeErrors which should be caught and ignored.
When invoking "goto-definition" on a definition node itself, we now return all overriding or overridden definitions, in all sources. Previously, we returned only overriding definitions in the same source. But it's arguably more useful to know about overriding definitions in other sources because these are harder to find manually. Performance, is not an issue because it's only invoked if one is already on a definition, which should be relatively rare.
Revert to the original definition. For a while this was changed so that a tree would match if its source symbol matched the required symbol but this yields to many trees (e.g. primary constructor of a class in addition to the class itself).
Revert to the original definition. For a while this was changed so that a tree would match if its source symbol matched the required symbol but this yields to many trees (e.g. primary constructor of a class in addition to the class itself). (reverted from commit 349d79b)
858d52c
to
0519aca
Compare
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 improvements!
@@ -65,7 +66,8 @@ class DottyLanguageServer extends LanguageServer | |||
myDrivers = new mutable.HashMap | |||
for (config <- configs) { | |||
val classpathFlags = List("-classpath", (config.classDirectory +: config.dependencyClasspath).mkString(File.pathSeparator)) | |||
val settings = defaultFlags ++ config.compilerArguments.toList ++ classpathFlags | |||
val sourcepathFlags = List("-sourcepath", config.sourceDirectories.mkString(File.pathSeparator), "-scansource") | |||
val settings = defaultFlags ++ config.compilerArguments.toList ++ classpathFlags ++ sourcepathFlags |
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.
When running launchIDE on Dotty we get:
[error] Flag -sourcepath set repeatedly
Because -sourcepath
is part of the settings we set for dotty-library in our build. This could be solved by not adding -sourcepath
when it's already set, or by appending source directories to an existing -sourcepath
found in config.compilerArguments
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.
Not sure what is the right thing to do here. I was assuming config.sourceDirectories gives us all the info. Does it make sense to include the dotty-library sourcepath?
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.
sourceDirectories does give us all the info, it's just that when compiling dotty-library itself we already use -sourcepath in our build to avoid accidentally linking against the dotty-library on the classpath. In general we cannot assume that -sourcepath is not part of compilerArguments
.
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.
Ah, OK, so we can just filter out the existing sourcepath, right?
val gadts: Printer = noPrinter | ||
val hk: Printer = noPrinter | ||
val implicits: Printer = noPrinter | ||
val implicitsDetailed: Printer = noPrinter | ||
val inlining: Printer = noPrinter | ||
val interactiv: Printer = new Printer |
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.
Should be noPrinter
case None => | ||
val idSet = mutable.SortedSet[String]() | ||
tree.foreachSubTree { | ||
case tree: tpd.NameTree => idSet += tree.name.toString |
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.
Should be restricted to SimpleName
, like in DottyUnpickler#mightContain
I think.
|
||
( sym == tree.symbol | ||
|| sym.exists && sym == sourceSymbol(tree.symbol) | ||
|| include != 0 && sym.name == tree.symbol.name && sym.owner != tree.symbol.owner |
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.
sym.owner
will crash if sym
is NoSymbol
private def safely[T](op: => List[T]): List[T] = | ||
try op catch { case ex: TypeError => Nil } | ||
|
||
/** Get possible completions from tree at `pos` | ||
* | ||
* @return offset and list of symbols for possible completions | ||
*/ | ||
// deprecated |
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.
Add a TODO for removing this
*/ | ||
def include(sym: Symbol) = | ||
sym.name.startsWith(prefix) && | ||
!sym.name.toString.drop(prefix.length).contains('$') && |
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.
Can't we just check if the name kind is SimpleName after a stripModuleClassSuffix
?
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 want to exclude all compiler-generated names, not just module names.
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.
But all these names will have a kind different from SimpleName, no?
addMember(imp.site, name) | ||
addMember(imp.site, name.toTypeName) | ||
} | ||
for (renamed <- imp.reverseMapping.keys) addImport(renamed) |
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 don't think this can work since the name renamed
doesn't exist in imp.site
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.
Good point. I removed the code and added a FIXME. Will open a separate issue.
|
||
if (ctx.owner.isClass) { | ||
for (mbr <- accessibleMembers(ctx.owner.thisType)) | ||
addMember(ctx.owner.thisType, mbr.name) |
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.
Equivalent to addAccessibleMembers(ctx.owner.thisType)
def contextOfStat(stats: List[Tree], stat: Tree, exprOwner: Symbol, ctx: Context): Context = stats match { | ||
case Nil => | ||
ctx | ||
case first :: _ if first eq stat => |
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.
Why is the first statement handled specially?
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.
It's not. There's a tail recursion in the third clause.
} | ||
localCtx | ||
case tree @ Template(constr, parents, self, _) => | ||
if ((constr :: self :: parents).exists(nested `eq` _)) ctx |
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.
Any reason to not use .contains(nested)
here?
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.
None. I overlooked that tree comparison is by eq
anyway.
In |
Yes, I think that's because these are different projects. I am not sure what needs to be done to also search external projects. Opening a fresh issue. |
Right, but to be conservative I would keep it and append our sourceDirectories to it if it already exists
|
The problem is that uncontrolled source directories can be surprising and expensive. So I'd rather keep a tight lid on it and not include some random stuff on the classpath. |
This provides support for full context-sensitive completions. Local scopes and imports are considered. Member completions take possible implicit conversions into account.
Based on #3949.