[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

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?

[ilya-biryukov]

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

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.

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 isSporadicCompletion(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.

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 CompletionSporadicError : public llvm::ErrorInfo<CancelledError> {
+public:
+  void log(llvm::raw_ostream &OS) const override {
+    OS << "ignored 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) {
-  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;
-                             for (const auto &R : List->Completions) {
-                               CompletionItem C = R.render(CCOpts);
-                               C.kind = adjustKindToCapability(
-                                   C.kind, SupportedCompletionItemKinds);
-                               LSPList.items.push_back(std::move(C));
-                             }
-                             return Reply(std::move(LSPList));
-                           },
-                           std::move(Reply)));
+  if (isSporadicCompletion(Params))
+    return Reply(llvm::make_error<CompletionSporadicError>());
+  Server->codeComplete(
+      Params.textDocument.uri.file(), Params.position, CCOpts,
+          [this](decltype(Reply) Reply, Expected<CodeCompleteResult> List) {
+              C.kind =
+                  adjustKindToCapability(C.kind, SupportedCompletionItemKinds);
 }
 
 void ClangdLSPServer::onSignatureHelp(const TextDocumentPositionParams &Params,
                                       Callback<SignatureHelp> Reply) {
   Server->signatureHelp(Params.textDocument.uri.file(), Params.position,
                         std::move(Reply));
 }
 
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::isSporadicCompletion(
+    const CompletionParams &Params) const {
+  StringRef Trigger = Params.context.triggerCharacter;
+  if (Params.context.triggerKind != CompletionTriggerKind::TriggerCharacter ||
+      (Trigger != ">" && Trigger != ":"))
+    return false;
+
+  auto Code = DraftMgr.getDraft(Params.textDocument.uri.file());
+  if (!Code)
+    return false; // completion code will log the error for untracked doc.
+
+  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 false;
+  }
+  if (*Offset < 2)
+    return true;
+
+  if (Trigger == ">")
+    return (*Code)[*Offset - 2] != '-'; // trigger only on '->'.
+  if (Trigger == ":")
+    return (*Code)[*Offset - 2] != ':'; // trigger only on '::'.
+  assert(false && "unhandled trigger character");
+  return false;
+}
+
 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) {

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;

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");