Only run completion when we were trigerred on '->' and '::', otherwise
send an error code in return.
To avoid automatically invoking completions in cases like 'a >^' or
'a ? b :^'.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 26321 Build 26320: arc lint + arc unit
Event Timeline
This still needs tests, will add them before committing the change. However, the implementation should be good for review.
The code looks mostly good to me.
clangd/ClangdLSPServer.cpp | ||
---|---|---|
789 | The method name is a bit confusing to me, I didn't get its meaning at the first glance and without reading its comment. Maybe shouldRunCompletion? | |
811 | Checking Offset is not always right for rare cases like (bar:/*commment*/:), a robust way is to use lexer and get the token at the current position, but I don't think it is worth (these rare cases should not happen when people write code). Maybe add a comment documenting the limitation? |
- Address comments
- Added a test
clangd/ClangdLSPServer.cpp | ||
---|---|---|
811 | Done. Note that bar:/*comment*/: is not actually a qualifier, i.e. a single token cannot be split in half by the comment. |
clangd/ClangdLSPServer.cpp | ||
---|---|---|
811 | Yeah, you are right a single token cannot be split in half by the comment. Thanks for the clarification. | |
test/clangd/completion-auto-trigger.test | ||
13 | nit: would be nicer if we add a comment illustrating what this request tests for. For this case, it is a->^, IIUC. | |
26 | sortText is implementation details, maybe use {{.*}}size, the same below. |
This looks like a work around LSP imperfection indeed.
Are you going to push for change in LSP? Something like CompletionOptions/triggerCharacters -> CompletionOptions/triggerStrings?
export interface CompletionOptions { /** * The server provides support to resolve additional * information for a completion item. */ resolveProvider?: boolean; /** * The characters that trigger completion automatically. */ triggerCharacters?: string[]; }
https://microsoft.github.io/language-server-protocol/specification
I think LSP uses a single character as a trigger character (see CompletionTriggerKind::TriggerCharacter). The string type of triggerCharacter is misleading, a more correct type is char I believe, but char is not the basic type in JSON schema.
Totally.
Are you going to push for change in LSP? Something like CompletionOptions/triggerCharacters -> CompletionOptions/triggerStrings?
This would workaround this particular problem, but I don't expect the triggerStrings to cover all the use-cases we care about in the long-term.
A few examples that need semantic knowledge:
- Auto-completions inside a comment
int test() { std::^ // <-- should trigger here // std::^ <-- this is inside a comment, shouldn't trigger here as clang won't provide any useful results anyway. }
- Trailing return types
auto foo() ->^ int {}// <-- should not trigger here
@hokein I think you are correct. I meant that it would be possible to make LSP more generic by using trigger strings instead of trigger characters..
@ilya-biryukov Yes, that's true. I would still expect some performance gain in more restrictive filtering on client's side - not sure how big though. Anyway, seems like LSP folks think character + trigger context is good enough. For example here:
https://github.com/Microsoft/language-server-protocol/issues/138
That would definitely mean less completion requests from the client, I doubt it would be the bottleneck for performance, though.
Thanks for the pointers, an insightful conversation on almost the same issue there.
The method name is a bit confusing to me, I didn't get its meaning at the first glance and without reading its comment. Maybe shouldRunCompletion?