Use symbol index to populate completion results for qualfified IDs e.g. "nx::A^".
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
- Merge remote-tracking branch 'origin/master' into index-completion
- Fix merge with origin/master.
A few drive-by comments, will look deeper if I have time but don't wait for me.
clangd/Position.cpp | ||
---|---|---|
34 ↗ | (On Diff #127118) | This is deep magic (how it handles npos) and I'd like to replace it with something more explicit |
15 ↗ | (On Diff #127096) | This is kind of note-to-self, but there are things to fix here
|
clangd/Position.h | ||
1 ↗ | (On Diff #127096) | This seems like a really specific name - one possible generalization is "utilities that examine source code directly" - SourceCode.h? |
Hope you don't mind me piling on... let me know!
Mostly readability, but i'd also like to reduce the scope w.r.t namespace completion.
clangd/CodeComplete.cpp | ||
---|---|---|
90 ↗ | (On Diff #127118) | toCompletionItemKind? getKindOfSymbol doesn't match either the input or output, and they have very similar names... (yes, the names above are bad too - feel free to fix them!) |
103 ↗ | (On Diff #127118) | LSP has struct (not in protocol yet) fine for these to be FIXME |
111 ↗ | (On Diff #127118) | this seems wrong? I would have thought references are similar to c++ references? TypeAlias seems like it could default to class, for Using is complicated (would need to look at the subtype) - maybe just unknown? |
114 ↗ | (On Diff #127118) | conversionfunction might be rather operator, not sure |
121 ↗ | (On Diff #127118) | LSP has enum constant, not in protocol.h yet |
131 ↗ | (On Diff #127118) | destructor seems more like an operator or method |
280 ↗ | (On Diff #127118) | qualified |
282 ↗ | (On Diff #127118) | nit: 'info' is just noise. Specifier is vague too - consider calling this SpecifiedScope, putting the emphasis on the user interaction. |
283 ↗ | (On Diff #127118) | There's a lot of sema/ast details here, wedged in the middle of the code that deals with candidates and scoring. Can you forward declare this, and move this implementation and the other details somewhere lower? |
283 ↗ | (On Diff #127118) | "create" doesn't give any more semantics than a constructor would. Maybe extractCompletionScope? (This could also be a free function) |
310 ↗ | (On Diff #127118) | The written/resolved distinction is tangled up with the various fields relating to the written form. Could you pull out a struct here like: (This might be a reasonable candidate for `SourceCode.h) Then this struct can become struct ScopeSpecifier { SourceRange Written, std::string Resolved } |
322 ↗ | (On Diff #127118) | nit: symbol is impossibly vague here - since we're always talking about symbols and it's hard to know you mean index::Symbol. I'd call this indexCompletionItem, but just toCompletionItem seems fine too |
325 ↗ | (On Diff #127118) | as discussed offline: fuzzy matching and rewriting qualifiers is a can of worms. Among other things, this means that the first step doesn't have to rewrite qualifiers at all, the second only needs to insert them, and we defer thorny cases until the third. |
347 ↗ | (On Diff #127118) | we're a bit lax about the "method name should start with a verb" rule, but this one I actually found hard to parse. completeWithIndex? (since there's no unqualified case yet, and there's no need to guess about how the code would be structured at this point) |
362 ↗ | (On Diff #127118) | suggest we do this in a structured way instead: |
373 ↗ | (On Diff #127118) | Please look for names that will tell the reader what the role of this thing is, and only add comments that provide extra information. SemaCompletionInfo accurately describes most of the types in this file! Best i can come up with is NameToComplete or LeadingText - not great :-( |
375 ↗ | (On Diff #127118) | the partial identifier that is being completed, without qualifiers |
380 ↗ | (On Diff #127118) | It's not clear what's to fix here - isn't non-qualified ID completion already represented by {Filter, None}? |
397 ↗ | (On Diff #127118) | nit: auto seems a lot easier to read here? |
Here are a few more comments.
clangd/CodeComplete.cpp | ||
---|---|---|
329 ↗ | (On Diff #127118) | NIT: FIXME(ioeric)? |
334 ↗ | (On Diff #127118) | NIT: FIXME(ioeric)? |
380 ↗ | (On Diff #127118) | NIT: FIXME(ioeric)? |
847 ↗ | (On Diff #127096) | NIT: FIXME(ioeric)? |
clangd/CodeComplete.h | ||
64 ↗ | (On Diff #127118) | Given the comment, maybe we want to pass it as a separate parameter instead? |
- Address comments in CodeComplete.cpp
- Split index changes into a separate patch.
- Merged with patch D41351.
- Remove Position.* files.
Thanks for the review!
clangd/CodeComplete.cpp | ||
---|---|---|
111 ↗ | (On Diff #127118) | TypeAlias could be more than class I think? And they are references (to types) to some extend. Anyhow, this is copied from the conversion function above. I'll leave a FIXME for now. |
114 ↗ | (On Diff #127118) | Operator is not in the protocol either. I leave a FIXME and will clean it up in followup. |
283 ↗ | (On Diff #127118) | This is moved to a standalone function extractCompletionScope. |
310 ↗ | (On Diff #127118) | Got rid of ranges as we don't need any them in this revision. |
334 ↗ | (On Diff #127118) | Does sortText have to be non-empty? I thought the empty string would make all candidates equally scored? I think we want either sensible score or (for now) no score at all. |
373 ↗ | (On Diff #127118) | NameToComplete sounds reasonable. |
847 ↗ | (On Diff #127096) | Didn't realize logger is ready to use. Switched to use logger. |
clangd/CodeComplete.h | ||
64 ↗ | (On Diff #127118) | @sammccall suggested this approach in the previous prototype patch. I also ended up preferring this approach because:
|
clangd/CodeComplete.cpp | ||
---|---|---|
334 ↗ | (On Diff #127118) | LS says that if sortText is empty, label should be used. We're probably fine here, you're right. |
clangd/CodeComplete.h | ||
64 ↗ | (On Diff #127118) |
We do have it as a parameter to codeComplete method, the fact that it's wrapped in a struct does not make much difference.
unit-testing code can easily wrap any interface, this shouldn't be a problem.
I'm looking at CodeCompleteOptions as a configurable user preference rather than a struct, containing all required inputs of codeComplete. I don't think SymbolIndex is in line with other fields that this struct contains. |
clangd/CodeComplete.cpp | ||
---|---|---|
111 ↗ | (On Diff #127118) | Yeah, this is the problem I was alluding to before - LSP encourages us to be very specific in completion kinds - but we can't always be. Class seems like the best guess to me, but FIXME is also fine for now. |
283 ↗ | (On Diff #127118) | Thanks for extracting the function. These index-related details are still out of place - they split the two chunks of code that mostly do sema-based completion. |
clangd/CodeComplete.h | ||
64 ↗ | (On Diff #127118) | So yeah, this is my fault... codeComplete currently has 9(!) arguments. Different people will have different reactions to this, mine is to do almost anything to avoid a 10th :-)
This doesn't match my experience at all. Can you suggest how to test this logic without adding parameters to at least ClangdServer::codeComplete and the codeComplete test helpers?
As of today, the dynamic index is the only index clangd supports. There's nothing to accept in the constructor, it's entirely owned by ClangdServer. |
LGTM. I still think that we should move the SymbolIndex out of the struct, but don't want to block this patch.
clangd/CodeComplete.h | ||
---|---|---|
64 ↗ | (On Diff #127118) |
I do agree this is unfortunate and I'm not at all opposed to cleaning that up. By removing parameters that we don't need or grouping them into a struct that has saner default parameters or in a different way. Both 9 and 10 seem equally bad to me. My point is that SymbolIndex should not be part of CodeCompleteOptions only because it makes it easier to pass it around.
Exactly, I would go with the test helper.
We do have implementations of the index in the tests that we pass around. That being said, I don't want this change to be blocked by my opinion on this matter. This is a minor thing, compared to all the other changes in the patch. Just wanted to make a point that this field totally feels out of place. |
clangd/CodeComplete.h | ||
---|---|---|
64 ↗ | (On Diff #127118) | Thanks for the feedback! I added a FIXME for this. I am happy to clean this up when we have a saner way to config code completion in clangd :) |