Author: ibiryukov Date: Thu Jan 3 05:37:12 2019 New Revision: 350304 URL: http://llvm.org/viewvc/llvm-project?rev=350304&view=rev Log: [clangd] Check preceding char when completion triggers on ':' or '>'
Summary: 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 :^'. Reviewers: hokein Reviewed By: hokein Subscribers: ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D55994 Added: clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp clang-tools-extra/trunk/clangd/ClangdLSPServer.h clang-tools-extra/trunk/clangd/Protocol.cpp clang-tools-extra/trunk/clangd/Protocol.h Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=350304&r1=350303&r2=350304&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Thu Jan 3 05:37:12 2019 @@ -22,6 +22,15 @@ using namespace llvm; namespace clang { namespace clangd { namespace { +class IgnoreCompletionError : public llvm::ErrorInfo<CancelledError> { +public: + void log(llvm::raw_ostream &OS) const override { + OS << "ignored auto-triggered completion, preceding char did not match"; + } + std::error_code convertToErrorCode() const override { + return std::make_error_code(std::errc::operation_canceled); + } +}; void adjustSymbolKinds(llvm::MutableArrayRef<DocumentSymbol> Syms, SymbolKindBitset Kinds) { @@ -310,6 +319,8 @@ void ClangdLSPServer::onInitialize(const {"completionProvider", json::Object{ {"resolveProvider", false}, + // We do extra checks for '>' and ':' in completion to only + // trigger on '->' and '::'. {"triggerCharacters", {".", ">", ":"}}, }}, {"signatureHelpProvider", @@ -612,8 +623,10 @@ void ClangdLSPServer::onCodeAction(const } } -void ClangdLSPServer::onCompletion(const TextDocumentPositionParams &Params, +void ClangdLSPServer::onCompletion(const CompletionParams &Params, Callback<CompletionList> Reply) { + if (!shouldRunCompletion(Params)) + return Reply(llvm::make_error<IgnoreCompletionError>()); Server->codeComplete( Params.textDocument.uri.file(), Params.position, CCOpts, Bind( @@ -773,6 +786,41 @@ std::vector<Fix> ClangdLSPServer::getFix return FixItsIter->second; } +bool ClangdLSPServer::shouldRunCompletion( + const CompletionParams &Params) const { + StringRef Trigger = Params.context.triggerCharacter; + if (Params.context.triggerKind != CompletionTriggerKind::TriggerCharacter || + (Trigger != ">" && Trigger != ":")) + return true; + + auto Code = DraftMgr.getDraft(Params.textDocument.uri.file()); + if (!Code) + return true; // completion code will log the error for untracked doc. + + // A completion request is sent when the user types '>' or ':', but we only + // want to trigger on '->' and '::'. We check the preceeding character to make + // sure it matches what we expected. + // Running the lexer here would be more robust (e.g. we can detect comments + // and avoid triggering completion there), but we choose to err on the side + // of simplicity here. + auto Offset = positionToOffset(*Code, Params.position, + /*AllowColumnsBeyondLineLength=*/false); + if (!Offset) { + vlog("could not convert position '{0}' to offset for file '{1}'", + Params.position, Params.textDocument.uri.file()); + return true; + } + if (*Offset < 2) + return false; + + if (Trigger == ">") + return (*Code)[*Offset - 2] == '-'; // trigger only on '->'. + if (Trigger == ":") + return (*Code)[*Offset - 2] == ':'; // trigger only on '::'. + assert(false && "unhandled trigger character"); + return true; +} + void ClangdLSPServer::onDiagnosticsReady(PathRef File, std::vector<Diag> Diagnostics) { auto URI = URIForFile::canonicalize(File, /*TUPath=*/File); Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=350304&r1=350303&r2=350304&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original) +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Thu Jan 3 05:37:12 2019 @@ -74,8 +74,7 @@ private: void onDocumentSymbol(const DocumentSymbolParams &, Callback<llvm::json::Value>); void onCodeAction(const CodeActionParams &, Callback<llvm::json::Value>); - void onCompletion(const TextDocumentPositionParams &, - Callback<CompletionList>); + void onCompletion(const CompletionParams &, Callback<CompletionList>); void onSignatureHelp(const TextDocumentPositionParams &, Callback<SignatureHelp>); void onGoToDefinition(const TextDocumentPositionParams &, @@ -98,6 +97,12 @@ private: std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D); + /// Checks if completion request should be ignored. We need this due to the + /// limitation of the LSP. Per LSP, a client sends requests for all "trigger + /// character" we specify, but for '>' and ':' we need to check they actually + /// produce '->' and '::', respectively. + bool shouldRunCompletion(const CompletionParams &Params) const; + /// Forces a reparse of all currently opened files. As a result, this method /// may be very expensive. This method is normally called when the /// compilation database is changed. Modified: clang-tools-extra/trunk/clangd/Protocol.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Protocol.cpp?rev=350304&r1=350303&r2=350304&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/Protocol.cpp (original) +++ clang-tools-extra/trunk/clangd/Protocol.cpp Thu Jan 3 05:37:12 2019 @@ -550,6 +550,29 @@ bool fromJSON(const json::Value &Params, O.map("position", R.position); } +bool fromJSON(const llvm::json::Value &Params, CompletionContext &R) { + json::ObjectMapper O(Params); + if (!O) + return false; + + int triggerKind; + if (!O.map("triggerKind", triggerKind)) + return false; + R.triggerKind = static_cast<CompletionTriggerKind>(triggerKind); + + if (auto *TC = Params.getAsObject()->get("triggerCharacter")) + return fromJSON(*TC, R.triggerCharacter); + return true; +} + +bool fromJSON(const llvm::json::Value &Params, CompletionParams &R) { + if (!fromJSON(Params, static_cast<TextDocumentPositionParams &>(R))) + return false; + if (auto *Context = Params.getAsObject()->get("context")) + return fromJSON(*Context, R.context); + return true; +} + static StringRef toTextKind(MarkupKind Kind) { switch (Kind) { case MarkupKind::PlainText: Modified: clang-tools-extra/trunk/clangd/Protocol.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Protocol.h?rev=350304&r1=350303&r2=350304&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/Protocol.h (original) +++ clang-tools-extra/trunk/clangd/Protocol.h Thu Jan 3 05:37:12 2019 @@ -775,6 +775,31 @@ struct TextDocumentPositionParams { }; bool fromJSON(const llvm::json::Value &, TextDocumentPositionParams &); +enum class CompletionTriggerKind { + /// Completion was triggered by typing an identifier (24x7 code + /// complete), manual invocation (e.g Ctrl+Space) or via API. + Invoked = 1, + /// Completion was triggered by a trigger character specified by + /// the `triggerCharacters` properties of the `CompletionRegistrationOptions`. + TriggerCharacter = 2, + /// Completion was re-triggered as the current completion list is incomplete. + TriggerTriggerForIncompleteCompletions = 3 +}; + +struct CompletionContext { + /// How the completion was triggered. + CompletionTriggerKind triggerKind = CompletionTriggerKind::Invoked; + /// The trigger character (a single character) that has trigger code complete. + /// Is undefined if `triggerKind !== CompletionTriggerKind.TriggerCharacter` + std::string triggerCharacter; +}; +bool fromJSON(const llvm::json::Value &, CompletionContext &); + +struct CompletionParams : TextDocumentPositionParams { + CompletionContext context; +}; +bool fromJSON(const llvm::json::Value &, CompletionParams &); + enum class MarkupKind { PlainText, Markdown, Added: clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test?rev=350304&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test (added) +++ clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test Thu Jan 3 05:37:12 2019 @@ -0,0 +1,106 @@ +# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s +{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}} +--- +{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"namespace ns { int ns_member; } struct vector { int size; static int default_capacity; };\nvoid test(vector *a, vector *b) {\n if (a > b) {} \n a->size = 10;\n\n a ? a : b;\n ns::ns_member = 10;\n}"}}} +--- +{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":9},"context":{"triggerKind":2,"triggerCharacter":">"}}} +# CHECK: "error": { +# CHECK-NEXT: "code": -32001, +# CHECK-NEXT: "message": "ignored auto-triggered completion, preceding char did not match" +# CHECK-NEXT: }, +# CHECK-NEXT: "id": 1, +--- +{"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":3,"character":5},"context":{"triggerKind":2,"triggerCharacter":">"}}} +# CHECK: "id": 2, +# CHECK-NEXT: "jsonrpc": "2.0" +# CHECK-NEXT: "result": { +# CHECK-NEXT: "isIncomplete": false, +# CHECK-NEXT: "items": [ +# CHECK-NEXT: { +# CHECK-NEXT: "detail": "int", +# CHECK-NEXT: "filterText": "size", +# CHECK-NEXT: "insertText": "size", +# CHECK-NEXT: "insertTextFormat": 1, +# CHECK-NEXT: "kind": 5, +# CHECK-NEXT: "label": " size", +# CHECK-NEXT: "sortText": "3eacccccsize", +# CHECK-NEXT: "textEdit": { +# CHECK-NEXT: "newText": "size", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 5, +# CHECK-NEXT: "line": 3 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 5, +# CHECK-NEXT: "line": 3 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "detail": "int", +# CHECK-NEXT: "filterText": "default_capacity", +# CHECK-NEXT: "insertText": "default_capacity", +# CHECK-NEXT: "insertTextFormat": 1, +# CHECK-NEXT: "kind": 10, +# CHECK-NEXT: "label": " default_capacity", +# CHECK-NEXT: "sortText": "3fd70a3ddefault_capacity", +# CHECK-NEXT: "textEdit": { +# CHECK-NEXT: "newText": "default_capacity", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 5, +# CHECK-NEXT: "line": 3 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 5, +# CHECK-NEXT: "line": 3 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +--- +{"jsonrpc":"2.0","id":3,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":5,"character":9},"context":{"triggerKind":2,"triggerCharacter":":"}}} +# CHECK: "error": { +# CHECK-NEXT: "code": -32001, +# CHECK-NEXT: "message": "ignored auto-triggered completion, preceding char did not match" +# CHECK-NEXT: }, +# CHECK-NEXT: "id": 3, +--- +{"jsonrpc":"2.0","id":4,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":6,"character":6},"context":{"triggerKind":2,"triggerCharacter":":"}}} +--- +# CHECK: "id": 4, +# CHECK-NEXT: "jsonrpc": "2.0" +# CHECK-NEXT: "result": { +# CHECK-NEXT: "isIncomplete": false, +# CHECK-NEXT: "items": [ +# CHECK-NEXT: { +# CHECK-NEXT: "detail": "int", +# CHECK-NEXT: "filterText": "ns_member", +# CHECK-NEXT: "insertText": "ns_member", +# CHECK-NEXT: "insertTextFormat": 1, +# CHECK-NEXT: "kind": 6, +# CHECK-NEXT: "label": " ns_member", +# CHECK-NEXT: "sortText": "3f2cccccns_member", +# CHECK-NEXT: "textEdit": { +# CHECK-NEXT: "newText": "ns_member", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 6, +# CHECK-NEXT: "line": 6 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 6, +# CHECK-NEXT: "line": 6 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +{"jsonrpc":"2.0","id":5,"method":"shutdown"} +--- +{"jsonrpc":"2.0","method":"exit"} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits