[clangd] Check preceding char when completion triggers on ':' or '>'

[ilya-biryukov]

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 :^'.

[ilya-biryukov]

This still needs tests, will add them before committing the change. However, the implementation should be good for review.

[hokein]

The code looks mostly good to me.

clangd/ClangdLSPServer.cpp

789 ↗

(On Diff #179261)

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 ↗

(On Diff #179261)

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?

[ilya-biryukov]

• Address comments
• Added a test

clangd/ClangdLSPServer.cpp

811 ↗

(On Diff #179261)

Done. Note that bar:/*comment*/: is not actually a qualifier, i.e. a single token cannot be split in half by the comment.
But the comment is totally valid: use the lexer would allow to filter out more things, e.g. we don't want to auto-trigger inside a comment, etc.

[hokein]

clangd/ClangdLSPServer.cpp

811 ↗

(On Diff #179261)

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 ↗

(On Diff #179844)

nit: would be nicer if we add a comment illustrating what this request tests for. For this case, it is a->^, IIUC.

26 ↗

(On Diff #179844)

sortText is implementation details, maybe use {{.*}}size, the same below.

[jkorous]

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

[hokein]

In D55994#1343669, @jkorous wrote:

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.

[ilya-biryukov]

In D55994#1343669, @jkorous wrote:

This looks like a work around LSP imperfection indeed.

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

[jkorous]

@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

[ilya-biryukov]

In D55994#1344717, @jkorous wrote:

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

clang-tools-extra/trunk/clangd/ClangdLSPServer.h

private:
   void onDocumentFormatting(const DocumentFormattingParams &,
                             Callback<std::vector<TextEdit>>);
   // The results are serialized 'vector<DocumentSymbol>' if
   // SupportsHierarchicalDocumentSymbol is true and 'vector<SymbolInformation>'
   // otherwise.
   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 &,
                         Callback<std::vector<Location>>);
   void onReference(const ReferenceParams &, Callback<std::vector<Location>>);
   void onSwitchSourceHeader(const TextDocumentIdentifier &,
                             Callback<std::string>);
   void onDocumentHighlight(const TextDocumentPositionParams &,
                            Callback<std::vector<DocumentHighlight>>);
   void onFileEvent(const DidChangeWatchedFilesParams &);
   void onCommand(const ExecuteCommandParams &, Callback<llvm::json::Value>);
   void onWorkspaceSymbol(const WorkspaceSymbolParams &,
                          Callback<std::vector<SymbolInformation>>);
   void onRename(const RenameParams &, Callback<WorkspaceEdit>);
   void onHover(const TextDocumentPositionParams &,
                Callback<llvm::Optional<Hover>>);
   void onChangeConfiguration(const DidChangeConfigurationParams &);
   void onSymbolInfo(const TextDocumentPositionParams &,
                     Callback<std::vector<SymbolDetails>>);
 
   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.
   void reparseOpenedFiles();
   void applyConfiguration(const ConfigurationSettings &Settings);
 
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 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) {
   for (auto &S : Syms) {
     S.kind = adjustKindToCapability(S.kind, Kinds);
     adjustSymbolKinds(S.children, Kinds);
   }
 }
Reply(json::Object{
              json::Object{
                  {"firstTriggerCharacter", "}"},
                  {"moreTriggerCharacter", {}},
              }},
             {"codeActionProvider", true},
             {"completionProvider",
              json::Object{
                  {"resolveProvider", false},
+                 // We do extra checks for '>' and ':' in completion to only
+                 // trigger on '->' and '::'.
                  {"triggerCharacters", {".", ">", ":"}},
              }},
             {"signatureHelpProvider",
              json::Object{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
else {
     std::vector<Command> Commands;
     for (const auto &Action : Actions)
       if (auto Command = asCommand(Action))
         Commands.push_back(std::move(*Command));
     Reply(json::Array(Commands));
   }
 }
 
-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(
           [this](decltype(Reply) Reply, Expected<CodeCompleteResult> List) {
             if (!List)
               return Reply(List.takeError());
             CompletionList LSPList;
             LSPList.isIncomplete = List->HasMore;
std::vector<Fix> ClangdLSPServer::getFixes(StringRef File,
   const auto &DiagToFixItsMap = DiagToFixItsIter->second;
   auto FixItsIter = DiagToFixItsMap.find(D);
   if (FixItsIter == DiagToFixItsMap.end())
     return {};
 
   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);
   std::vector<Diagnostic> LSPDiagnostics;
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
     toLSPDiags(Diag, URI, DiagOpts,
                [&](clangd::Diagnostic Diag, ArrayRef<Fix> Fixes) {

clang-tools-extra/trunk/clangd/Protocol.h

struct TextDocumentPositionParams {
   /// The text document.
   TextDocumentIdentifier textDocument;
 
   /// The position inside the text document.
   Position position;
 };
 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,
 };
 
 struct MarkupContent {
   MarkupKind kind = MarkupKind::PlainText;
   std::string value;

clang-tools-extra/trunk/clangd/Protocol.cpp

 }
 
 bool fromJSON(const json::Value &Params, TextDocumentPositionParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          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:
     return "plaintext";
   case MarkupKind::Markdown:
     return "markdown";
   }
   llvm_unreachable("Invalid MarkupKind");

clang-tools-extra/trunk/test/clangd/completion-auto-trigger.test

+# 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"}