[clangd] Add textDocument/signatureHelp

[rwols]

Summary

Makes clangd respond to a client's "textDocument/signatureHelp" request by
presenting function/method overloads.
Still need to add tests.
Also, there is duplicate code in clangd::codeComplete and clangd::signatureHelp
now, maybe refactor this to a common function?

[ilya-biryukov]

Thanks for the patch. Overall looks good, just a few comments.
Could you also add some tests?

clangd/ClangdServer.h

243 ↗

(On Diff #115874)

Mentions "code completion" in the comment.

clangd/ClangdUnit.cpp

536 ↗

(On Diff #115874)

Code in clangd uses FIXME instead of TODO.

589 ↗

(On Diff #115874)

Ignoring CK_Optional chunks leads to weird results. They represent parameters that have default arguments. For example,

int foo(int a, int b = 10, int c = 20);

int test() {
  foo(10, 20, 30); // signatureHelp shows foo(int a) here, which is confusing.
}

691 ↗

(On Diff #115874)

Most of this function is almost identical to clangd::codeComplete. They only differ in some of the CodeCompleteOpts flags and CodeCompletionConsumers they use, right?

Maybe extract a helper function that runs code completion and reuse it for both clangd::codeCompleteand clangd::signatureHelp?

[rwols]

• Update doxygen comment to say "signature help" instead of "code complete",
• Refactor: put the FillDocumentation member function outside of CompletionItemsCollector so that SignatureHelpCollector can also use it.
• Refactor: put the FillSortText member function outside of CompletionItemsCollector because it has nothing to do with CompletionItemsCollector.
• Refactor: Make a templated function called invokeClangAction that will invoke the given CodeCompleteConsumer type. Both clangd::codeComplete and clangd::signatureHelp now call this function.
• Present optional/defaulted parameters too (using CodeCompletionString::CK_Optional chunks).

[rwols]

• Add signature-help.test
• Modify formatting.test to make the test pass. The test failed because clangd's response to the initialize request was changed to advertise signature help trigger characters, so the expected content length changed.

[rwols]

clangd/ClangdLSPServer.cpp

92 ↗

(On Diff #116466)

This required me to adjust the formatting.test file.

clangd/ClangdUnit.cpp

650 ↗

(On Diff #116466)

Not sure if these template names are according to style.

test/clangd/signature-help.test

39 ↗

(On Diff #116466)

When there are multiple defaulted parameters after each other, the CK_Optional chunk consists of *all* of those parameters, instead of a CK_Optional chunk per parameter. This might require us to dive into SemaCodeComplete.cpp to fix this. I'm just leaving it as-is right now.

[ilya-biryukov]

Sorry for late response.

clangd/ClangdUnit.cpp

610 ↗

(On Diff #116466)

Are we first concatenating the CodeCompletionString inside optional chunks and then trying to parse them again here?
Can we extract ProcessChunks function and recursively call it with Chunk.Optional->Chunks?

650 ↗

(On Diff #116466)

It's ok. See my comment below, though, it looks like we can avoid making this function template in the first place.

652 ↗

(On Diff #116466)

Maybe rename to invokeCodeCompletion? invokeClangAction seems too generic.

705 ↗

(On Diff #116466)

Maybe receive CodeCompleteConsumer and not make function template?
We can return whether calling an action succeeded instead.

test/clangd/signature-help.test

39 ↗

(On Diff #116466)

But does CodeCompletionString inside Chunk->Optional contain those extra parameters as a separate chunk?

[rwols]

• Updated to latest upstream revision.
• Fix optional/default parameter handling: each default parameter now has its own label (thanks, ilya!).
• Remove the templated "invokeClangAction" function and use polymorphism with CodeCompleteConsumer instead. The function is now called "invokeCodeComplete".

[rwols]

There were some failing tests, probably because we use an extra digit for the sortText property now. I haven't touched those tests.

clangd/ClangdUnit.cpp

610 ↗

(On Diff #116466)

Yes, this was hokey. Fixed now (see the function getOptionalParameters)

test/clangd/signature-help.test

39 ↗

(On Diff #116466)

Yes, they do. Thanks for the idea!

[rwols]

This comment was removed by rwols.

[rwols]

clangd/Protocol.h

458 ↗

(On Diff #117261)

@malaperle I copied the sentences from the protocol markdown file over here, but judging from your comment here that might a problem. Do you suggest to change this or is this okay as-is?

[rwols]

clangd/ClangdUnit.cpp

442 ↗

(On Diff #117261)

I seem to have inadvertently used the old fillSortText during the merge from upstream, let me fix that (this is why tests were failing probably).

[rwols]

• Fix fillSortText: used the old version, now using new version from upstream
• Fix initialize-params.test and initialize-params-invalid.test to account for signature help
• All tests pass now

[ilya-biryukov]

clangd/ClangdUnit.cpp

742 ↗

(On Diff #117290)

Duplicated line sneaked into commit. It looks like the intention was to set IncludeCodePatterns.

Since we need to fill up CodeCompleteOpts in all callers when creating CodeCompletionConsumer anyway, maybe it's better to accept CodeCompleteOpts as a parameter?
Otherwise there's a high chance to forget syncing some of CodeCompleteOpts flags with the ones from CodeCompletionConsumer when changing the code later.

[rwols]

• Update invokeCodeComplete function to also take a CodeCompleteOptions, we assign it to the CompilerInstance's FrontendOpts.CodeCompleteOpts.
• Remove unused variable in SignatureHelpCollector::ProcessOverloadCandidate.

[malaperle]

clangd/Protocol.h

458 ↗

(On Diff #117261)

I'm not a lawyer and I have very limited understanding of how these things work but I would maybe reword the longer ones and keep it a short summary of a few words. I'll ask around what my other open source colleagues think though.

[rwols]

Reword the documentation of the signature help structures in Protocol.h

[rwols]

Thinking ahead, it's probably best if we change the behavior of SnippetCompletionItemCollector as follows:

1. If it finds a callable, provide the name of the callable, followed by an opening parenthesis, followed by $0, followed by a closing parenthesis.
2. If it finds a code pattern, use the old behavior.

The point of (1) is that it should trigger textDocument/signatureHelp in the client immediately upon insertion of the completion. For example, if we have this function:

int foo(int x, int y);

Then the insertText would be foo($0). This causes the cursor to end up in-between the parentheses. Then it'll trigger textDocument/signatureHelp and that'll provide the parameter info.

The point of (2) is that we don't want to apply (1) for things like for-loop completions, or if-else constructs.

[ilya-biryukov]

LGTM. Do you need my help in landing this?

1. If it finds a callable, provide the name of the callable, followed by an opening parenthesis, followed by $0, followed by a closing parenthesis.

So that we don't clutter the source code with function parameters? This makes sense.
But I think some clients we might want to support don't have signatureHelp, so we may want to have a flag to switch between these two behaviors.

Another case where this might be bad is overloaded functions. I may choose one overload in completion, but signatureHelp will initially point into a different one.

clangd/Protocol.h

458 ↗

(On Diff #117261)

I'm not a lawyer either, but I believe it does not matter whether you reword things or not, you're still bound to comply with the license when doing any derived work.

[francisco.lopes]

In D38048#887960, @ilya-biryukov wrote:

Another case where this might be bad is overloaded functions. I may choose one overload in completion, but signatureHelp will initially point into a different one.

Hi, I'd just like to comment on this aspect to inform how this is done is different ways for Vim.

Original YouCompleteMe, for example, still doesn't support parameter hints, so it just offers function prototypes in the completion menu, for functions foo(int, char) and
foo(void *, int), the two prototypes gets listed and not mattering what function the user selects the text to be inserted will always just be the same function name, foo.

I didn't find this useful because it cluttered the menu with many overloads that the user may not be in fact looking for, it may be targeting for another function but it gets
like ten long overloads of another function that's ranked first according to what the user typed.

I also didn't find it useful, in Vim, to show full prototypes in the pop up menu because for C++ they tend to be rather long.

So I changed YouCompleteMe this way:

• Just show unique function names in the popup menu (they're tagged as functions).
• When the user selects a given function, the overloads are shown in a separate preview window instead, which for me is more sane for showing long information.
• When the user goes about filling the call site with arguments, the overloads in the preview window are dynamically updated showing the current argument the user is at, for the remaining compatible overloads.
• The overloads get reduced while the user fill arguments as well as there's type resolution of generic functions along the way.

These pictures show how this happens:

• https://s3.amazonaws.com/f.cl.ly/items/1e2F0A123h331c1G0L0R/SadBart.gif
• https://imgur.com/a/OqFaI

They just don't happen illustrate the reduction of multiple overloads because I didn't put multiple ones at the time of the screen capture.

This is just to illustrate some different completion approach that tries to be inline with libclang offerings.

[ilya-biryukov]

@francisco.lopes, thanks for sharing your experience, I definitely like the final UX you achieved, we should aim for something similar in clangd.
It also looks like @rwols's idea of how things should look like is very close to your implementation.

I do think that showing signatures for non-overloaded functions in completion may still be useful. Overloaded functions can be grouped by name and include some indicators that there are more overloads (something like foo(int) void (+5 overloads).
How do you choose return type for the completion item if function is overloaded? Show it only when it's the same for all overloads? Go with one of the return types?

[francisco.lopes]

In D38048#889176, @ilya-biryukov wrote:

@francisco.lopes, thanks for sharing your experience, I definitely like the final UX you achieved, we should aim for something similar in clangd.
It also looks like @rwols's idea of how things should look like is very close to your implementation.

Nice!

I do think that showing signatures for non-overloaded functions in completion may still be useful. Overloaded functions can be grouped by name and include some indicators that there are more overloads (something like foo(int) void (+5 overloads).
How do you choose return type for the completion item if function is overloaded? Show it only when it's the same for all overloads? Go with one of the return types?

In fact it didn't happen to me to think about the return information in the popup menu at the time, so it was not changed!, and it should just show one return type from one of the overloads randomly (I must never look at return types...).
I do think the (+ 5 overloads) approach is a good one too, though I prefer to not oversize the menu width with parameters.

It will be cool if clangd and its Language Server Protocol implementation manages to have feature parity with this, personally I'd be looking in migrating from YouCompleteMe to a LSP solution that's more modular and lightweight, possibly bringing to it what gets implemented here myself.

[rwols]

In D38048#887960, @ilya-biryukov wrote:

LGTM. Do you need my help in landing this?

Yes, I don't have commit access, go ahead and merge. If you find any merge conflicts let me know I can probably fix that.
Since we're throwing around gifs for fun here's how signatureHelp looks in practice for Sublime Text:

[francisco.lopes]

In D38048#889568, @rwols wrote:

Since we're throwing around gifs for fun here's how signatureHelp looks in practice for Sublime Text

Very cool!

Makes me wish to finish implementing https://reviews.llvm.org/D6880. There were some special cases I didn't cover. For variadic parameter list I had no idea how to tackle well, but I also didn't touch constructors in the initialization list, so, there are no tips when initializing members in the initialization list, but I remember this requiring maybe just a bit of additional effort to implement, so if there's interest in having that, it may be worth taking a look :-)

[ilya-biryukov]

@francisco.lopes, I've surely noticed there are a bunch of places where signatureHelp does not show up while I was playing around with this change.
It would be great to have it available in more cases.

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

private:
   void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
                                  StringRef ID, JSONOutput &Out) override;
   void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
                             JSONOutput &Out) override;
   void onCodeAction(CodeActionParams Params, StringRef ID,
                     JSONOutput &Out) override;
   void onCompletion(TextDocumentPositionParams Params, StringRef ID,
                     JSONOutput &Out) override;
+  void onSignatureHelp(TextDocumentPositionParams Params, StringRef ID,
+                       JSONOutput &Out) override;
   void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
                         JSONOutput &Out) override;
   void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
                             JSONOutput &Out) override;
   void onFileEvent(const DidChangeWatchedFilesParams &Params) override;
 
   std::vector<clang::tooling::Replacement>
   getFixIts(StringRef File, const clangd::Diagnostic &D);

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

Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID +
       R"(,"result":{"capabilities":{
           "textDocumentSync": 1,
           "documentFormattingProvider": true,
           "documentRangeFormattingProvider": true,
           "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
           "codeActionProvider": true,
           "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
+          "signatureHelpProvider": {"triggerCharacters": ["(",","]},
           "definitionProvider": true
         }}})");
   if (IP.rootUri && !IP.rootUri->file.empty())
     Server.setRootPath(IP.rootUri->file);
   else if (IP.rootPath && !IP.rootPath->empty())
     Server.setRootPath(*IP.rootPath);
 }
 
void ClangdLSPServer::onCompletion(TextDocumentPositionParams Params,
   }
   if (!Completions.empty())
     Completions.pop_back();
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":[)" + Completions + R"(]})");
 }
 
+void ClangdLSPServer::onSignatureHelp(TextDocumentPositionParams Params,
+                                      StringRef ID, JSONOutput &Out) {
+  const auto SigHelp = SignatureHelp::unparse(
+      Server
+          .signatureHelp(
+              Params.textDocument.uri.file,
+              Position{Params.position.line, Params.position.character})
+          .Value);
+  Out.writeMessage(R"({"jsonrpc":"2.0","id":)" + ID.str() + R"(,"result":)" +
+                   SigHelp + "}");
+}
+
 void ClangdLSPServer::onGoToDefinition(TextDocumentPositionParams Params,
                                        StringRef ID, JSONOutput &Out) {
 
   auto Items = Server
                    .findDefinitions(Params.textDocument.uri.file,
                                     Position{Params.position.line,
                                              Params.position.character})
                    .Value;

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

public:
   /// This method should only be called for currently tracked files. However, it
   /// is safe to call removeDocument for \p File after this method returns, even
   /// while returned future is not yet ready.
   std::future<Tagged<std::vector<CompletionItem>>>
   codeComplete(PathRef File, Position Pos,
                llvm::Optional<StringRef> OverridenContents = llvm::None,
                IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
 
+  /// Provide signature help for \p File at \p Pos. If \p OverridenContents is
+  /// not None, they will used only for signature help, i.e. no diagnostics
+  /// update will be scheduled and a draft for \p File will not be updated. If
+  /// \p OverridenContents is None, contents of the current draft for \p File
+  /// will be used. If \p UsedFS is non-null, it will be overwritten by
+  /// vfs::FileSystem used for signature help. This method should only be called
+  /// for currently tracked files.
+  Tagged<SignatureHelp>
+  signatureHelp(PathRef File, Position Pos,
+                llvm::Optional<StringRef> OverridenContents = llvm::None,
+                IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
+
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
   Tagged<std::vector<Location>> findDefinitions(PathRef File, Position Pos);
 
   /// Helper function that returns a path to the corresponding source file when
   /// given a header file and vice versa.
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
   /// Run formatting for \p Rng inside \p File.

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

ClangdServer::codeComplete(PathRef File, Position Pos,
 
   auto Future = Task.get_future();
   // FIXME(ibiryukov): to reduce overhead for wrapping the same callable
   // multiple times, ClangdScheduler should return future<> itself.
   WorkScheduler.addToFront([](PackagedTask Task) { Task(); }, std::move(Task));
   return Future;
 }
 
+Tagged<SignatureHelp>
+ClangdServer::signatureHelp(PathRef File, Position Pos,
+                            llvm::Optional<StringRef> OverridenContents,
+                            IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
+  std::string DraftStorage;
+  if (!OverridenContents) {
+    auto FileContents = DraftMgr.getDraft(File);
+    assert(FileContents.Draft &&
+           "signatureHelp is called for non-added document");
+
+    DraftStorage = std::move(*FileContents.Draft);
+    OverridenContents = DraftStorage;
+  }
+
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+  if (UsedFS)
+    *UsedFS = TaggedFS.Value;
+
+  std::shared_ptr<CppFile> Resources = Units.getFile(File);
+  assert(Resources && "Calling signatureHelp on non-added file");
+
+  auto Preamble = Resources->getPossiblyStalePreamble();
+  auto Result = clangd::signatureHelp(File, Resources->getCompileCommand(),
+                                      Preamble ? &Preamble->Preamble : nullptr,
+                                      *OverridenContents, Pos, TaggedFS.Value,
+                                      PCHs, Logger);
+  return make_tagged(std::move(Result), TaggedFS.Tag);
+}
+
 std::vector<tooling::Replacement> ClangdServer::formatRange(PathRef File,
                                                             Range Rng) {
   std::string Code = getDocument(File);
 
   size_t Begin = positionToOffset(Code, Rng.start);
   size_t Len = positionToOffset(Code, Rng.end) - Begin;
   return formatCode(Code, File, {tooling::Range(Begin, Len)});
 }

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

 /// Get code completions at a specified \p Pos in \p FileName.
 std::vector<CompletionItem>
 codeComplete(PathRef FileName, tooling::CompileCommand Command,
              PrecompiledPreamble const *Preamble, StringRef Contents,
              Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
              std::shared_ptr<PCHContainerOperations> PCHs,
              bool SnippetCompletions, clangd::Logger &Logger);
 
+/// Get signature help at a specified \p Pos in \p FileName.
+SignatureHelp signatureHelp(PathRef FileName, tooling::CompileCommand Command,
+                            PrecompiledPreamble const *Preamble,
+                            StringRef Contents, Position Pos,
+                            IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+                            std::shared_ptr<PCHContainerOperations> PCHs,
+                            clangd::Logger &Logger);
+
 /// Get definition of symbol at a specified \p Pos.
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
                                       clangd::Logger &Logger);
 
 /// For testing/debugging purposes. Note that this method deserializes all
 /// unserialized Decls, so use with care.
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS);
 
 } // namespace clangd
 } // namespace clang
 #endif

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

static int getSeverity(DiagnosticsEngine::Level L) {
   case DiagnosticsEngine::Error:
     return 1;
   case DiagnosticsEngine::Ignored:
     return 0;
   }
   llvm_unreachable("Unknown diagnostic level!");
 }
 
+/// Get the optional chunk as a string. This function is possibly recursive.
+///
+/// The parameter info for each parameter is appended to the Parameters.
+std::string
+getOptionalParameters(const CodeCompletionString &CCS,
+                      std::vector<ParameterInformation> &Parameters) {
+  std::string Result;
+  for (const auto &Chunk : CCS) {
+    switch (Chunk.Kind) {
+    case CodeCompletionString::CK_Optional:
+      assert(Chunk.Optional &&
+             "Expected the optional code completion string to be non-null.");
+      Result += getOptionalParameters(*Chunk.Optional, Parameters);
+      break;
+    case CodeCompletionString::CK_VerticalSpace:
+      break;
+    case CodeCompletionString::CK_Placeholder:
+      // A string that acts as a placeholder for, e.g., a function call
+      // argument.
+      // Intentional fallthrough here.
+    case CodeCompletionString::CK_CurrentParameter: {
+      // A piece of text that describes the parameter that corresponds to
+      // the code-completion location within a function call, message send,
+      // macro invocation, etc.
+      Result += Chunk.Text;
+      ParameterInformation Info;
+      Info.label = Chunk.Text;
+      Parameters.push_back(std::move(Info));
+      break;
+    }
+    default:
+      Result += Chunk.Text;
+      break;
+    }
+  }
+  return Result;
+}
+
 llvm::Optional<DiagWithFixIts> toClangdDiag(StoredDiagnostic D) {
   auto Location = D.getLocation();
   if (!Location.isValid() || !Location.getManager().isInMainFile(Location))
     return llvm::None;
 
   Position P;
   P.line = Location.getSpellingLineNumber() - 1;
   P.character = Location.getSpellingColumnNumber();
std::string escapeSnippet(const llvm::StringRef Text) {
   for (const auto Character : Text) {
     if (Character == '$' || Character == '}' || Character == '\\')
       Result.push_back('\\');
     Result.push_back(Character);
   }
   return Result;
 }
 
-class CompletionItemsCollector : public CodeCompleteConsumer {
+std::string getDocumentation(const CodeCompletionString &CCS) {
+  // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
+  // information in the documentation field.
+  std::string Result;
+  const unsigned AnnotationCount = CCS.getAnnotationCount();
+  if (AnnotationCount > 0) {
+    Result += "Annotation";
+    if (AnnotationCount == 1) {
+      Result += ": ";
+    } else /* AnnotationCount > 1 */ {
+      Result += "s: ";
+    }
+    for (unsigned I = 0; I < AnnotationCount; ++I) {
+      Result += CCS.getAnnotation(I);
+      Result.push_back(I == AnnotationCount - 1 ? '\n' : ' ');
+    }
+  }
+  // Add brief documentation (if there is any).
+  if (CCS.getBriefComment() != nullptr) {
+    if (!Result.empty()) {
+      // This means we previously added annotations. Add an extra newline
+      // character to make the annotations stand out.
+      Result.push_back('\n');
+    }
+    Result += CCS.getBriefComment();
+  }
+  return Result;
+}
 
+class CompletionItemsCollector : public CodeCompleteConsumer {
 public:
   CompletionItemsCollector(const CodeCompleteOptions &CodeCompleteOpts,
                            std::vector<CompletionItem> &Items)
       : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
         Items(Items),
         Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
         CCTUInfo(Allocator) {}
 
private:
   ProcessCodeCompleteResult(const CodeCompletionResult &Result,
                             const CodeCompletionString &CCS) const {
 
     // Adjust this to InsertTextFormat::Snippet iff we encounter a
     // CK_Placeholder chunk in SnippetCompletionItemsCollector.
     CompletionItem Item;
     Item.insertTextFormat = InsertTextFormat::PlainText;
 
-    FillDocumentation(CCS, Item);
+    Item.documentation = getDocumentation(CCS);
 
     // Fill in the label, detail, insertText and filterText fields of the
     // CompletionItem.
     ProcessChunks(CCS, Item);
 
     // Fill in the kind field of the CompletionItem.
     Item.kind = getKind(Result.CursorKind);
 
     FillSortText(CCS, Item);
 
     return Item;
   }
 
   virtual void ProcessChunks(const CodeCompletionString &CCS,
                              CompletionItem &Item) const = 0;
 
-  void FillDocumentation(const CodeCompletionString &CCS,
-                         CompletionItem &Item) const {
-    // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
-    // information in the documentation field.
-    const unsigned AnnotationCount = CCS.getAnnotationCount();
-    if (AnnotationCount > 0) {
-      Item.documentation += "Annotation";
-      if (AnnotationCount == 1) {
-        Item.documentation += ": ";
-      } else /* AnnotationCount > 1 */ {
-        Item.documentation += "s: ";
-      }
-      for (unsigned I = 0; I < AnnotationCount; ++I) {
-        Item.documentation += CCS.getAnnotation(I);
-        Item.documentation.push_back(I == AnnotationCount - 1 ? '\n' : ' ');
-      }
-    }
-
-    // Add brief documentation (if there is any).
-    if (CCS.getBriefComment() != nullptr) {
-      if (!Item.documentation.empty()) {
-        // This means we previously added annotations. Add an extra newline
-        // character to make the annotations stand out.
-        Item.documentation.push_back('\n');
-      }
-      Item.documentation += CCS.getBriefComment();
-    }
-  }
-
   static int GetSortPriority(const CodeCompletionString &CCS) {
     int Score = CCS.getPriority();
     // Fill in the sortText of the CompletionItem.
     assert(Score <= 99999 && "Expecting code completion result "
                              "priority to have at most 5-digits");
 
     const int Penalty = 100000;
     switch (static_cast<CXAvailabilityKind>(CCS.getAvailability())) {
for (const auto &Chunk : CCS) {
         // platform).
         Item.insertText += Chunk.Text;
         // Don't even add a space to the label.
         break;
       }
     }
   }
 }; // SnippetCompletionItemsCollector
-} // namespace
 
-std::vector<CompletionItem>
-clangd::codeComplete(PathRef FileName, tooling::CompileCommand Command,
+class SignatureHelpCollector final : public CodeCompleteConsumer {
+
+public:
+  SignatureHelpCollector(const CodeCompleteOptions &CodeCompleteOpts,
+                         SignatureHelp &SigHelp)
+      : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
+        SigHelp(SigHelp),
+        Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
+        CCTUInfo(Allocator) {}
+
+  void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg,
+                                 OverloadCandidate *Candidates,
+                                 unsigned NumCandidates) override {
+    SigHelp.signatures.reserve(NumCandidates);
+    // FIXME(rwols): How can we determine the "active overload candidate"?
+    // Right now the overloaded candidates seem to be provided in a "best fit"
+    // order, so I'm not too worried about this.
+    SigHelp.activeSignature = 0;
+    assert(CurrentArg <= std::numeric_limits<int>::max() &&
+           "too many arguments");
+    SigHelp.activeParameter = static_cast<int>(CurrentArg);
+    for (unsigned I = 0; I < NumCandidates; ++I) {
+      const auto &Candidate = Candidates[I];
+      const auto *CCS = Candidate.CreateSignatureString(
+          CurrentArg, S, *Allocator, CCTUInfo, true);
+      assert(CCS && "Expected the CodeCompletionString to be non-null");
+      SigHelp.signatures.push_back(ProcessOverloadCandidate(Candidate, *CCS));
+    }
+  }
+
+  GlobalCodeCompletionAllocator &getAllocator() override { return *Allocator; }
+
+  CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
+
+private:
+  SignatureInformation
+  ProcessOverloadCandidate(const OverloadCandidate &Candidate,
+                           const CodeCompletionString &CCS) const {
+    SignatureInformation Result;
+    const char *ReturnType = nullptr;
+
+    Result.documentation = getDocumentation(CCS);
+
+    for (const auto &Chunk : CCS) {
+      switch (Chunk.Kind) {
+      case CodeCompletionString::CK_ResultType:
+        // A piece of text that describes the type of an entity or,
+        // for functions and methods, the return type.
+        assert(!ReturnType && "Unexpected CK_ResultType");
+        ReturnType = Chunk.Text;
+        break;
+      case CodeCompletionString::CK_Placeholder:
+        // A string that acts as a placeholder for, e.g., a function call
+        // argument.
+        // Intentional fallthrough here.
+      case CodeCompletionString::CK_CurrentParameter: {
+        // A piece of text that describes the parameter that corresponds to
+        // the code-completion location within a function call, message send,
+        // macro invocation, etc.
+        Result.label += Chunk.Text;
+        ParameterInformation Info;
+        Info.label = Chunk.Text;
+        Result.parameters.push_back(std::move(Info));
+        break;
+      }
+      case CodeCompletionString::CK_Optional: {
+        // The rest of the parameters are defaulted/optional.
+        assert(Chunk.Optional &&
+               "Expected the optional code completion string to be non-null.");
+        Result.label +=
+            getOptionalParameters(*Chunk.Optional, Result.parameters);
+        break;
+      }
+      case CodeCompletionString::CK_VerticalSpace:
+        break;
+      default:
+        Result.label += Chunk.Text;
+        break;
+      }
+    }
+    if (ReturnType) {
+      Result.label += " -> ";
+      Result.label += ReturnType;
+    }
+    return Result;
+  }
+
+  SignatureHelp &SigHelp;
+  std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
+  CodeCompletionTUInfo CCTUInfo;
+
+}; // SignatureHelpCollector
+
+bool invokeCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
+                        const CodeCompleteOptions &Options, PathRef FileName,
+                        const tooling::CompileCommand &Command,
                         PrecompiledPreamble const *Preamble, StringRef Contents,
                         Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
                         std::shared_ptr<PCHContainerOperations> PCHs,
-                     bool SnippetCompletions, clangd::Logger &Logger) {
+                        clangd::Logger &Logger) {
   std::vector<const char *> ArgStrs;
   for (const auto &S : Command.CommandLine)
     ArgStrs.push_back(S.c_str());
 
   VFS->setCurrentWorkingDirectory(Command.Directory);
 
   std::unique_ptr<CompilerInvocation> CI;
   EmptyDiagsConsumer DummyDiagsConsumer;
bool invokeCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   auto Clang = prepareCompilerInstance(std::move(CI), Preamble,
                                        std::move(ContentsBuffer), PCHs, VFS,
                                        DummyDiagsConsumer);
   auto &DiagOpts = Clang->getDiagnosticOpts();
   DiagOpts.IgnoreWarnings = true;
 
   auto &FrontendOpts = Clang->getFrontendOpts();
   FrontendOpts.SkipFunctionBodies = true;
-
-  FrontendOpts.CodeCompleteOpts.IncludeGlobals = true;
-  FrontendOpts.CodeCompleteOpts.IncludeMacros = true;
-  FrontendOpts.CodeCompleteOpts.IncludeBriefComments = true;
-
+  FrontendOpts.CodeCompleteOpts = Options;
   FrontendOpts.CodeCompletionAt.FileName = FileName;
   FrontendOpts.CodeCompletionAt.Line = Pos.line + 1;
   FrontendOpts.CodeCompletionAt.Column = Pos.character + 1;
 
-  std::vector<CompletionItem> Items;
-  if (SnippetCompletions) {
-    FrontendOpts.CodeCompleteOpts.IncludeCodePatterns = true;
-    Clang->setCodeCompletionConsumer(new SnippetCompletionItemsCollector(
-        FrontendOpts.CodeCompleteOpts, Items));
-  } else {
-    FrontendOpts.CodeCompleteOpts.IncludeCodePatterns = false;
-    Clang->setCodeCompletionConsumer(new PlainTextCompletionItemsCollector(
-        FrontendOpts.CodeCompleteOpts, Items));
-  }
+  Clang->setCodeCompletionConsumer(Consumer.release());
 
   SyntaxOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
     Logger.log("BeginSourceFile() failed when running codeComplete for " +
                FileName);
-    return Items;
+    return false;
   }
-  if (!Action.Execute())
+  if (!Action.Execute()) {
     Logger.log("Execute() failed when running codeComplete for " + FileName);
+    return false;
+  }
 
   Action.EndSourceFile();
 
-  return Items;
+  return true;
+}
+
+} // namespace
+
+std::vector<CompletionItem>
+clangd::codeComplete(PathRef FileName, tooling::CompileCommand Command,
+                     PrecompiledPreamble const *Preamble, StringRef Contents,
+                     Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+                     std::shared_ptr<PCHContainerOperations> PCHs,
+                     bool SnippetCompletions, clangd::Logger &Logger) {
+  std::vector<CompletionItem> Results;
+  CodeCompleteOptions Options;
+  std::unique_ptr<CodeCompleteConsumer> Consumer;
+  Options.IncludeGlobals = true;
+  Options.IncludeMacros = true;
+  Options.IncludeBriefComments = true;
+  if (SnippetCompletions) {
+    Options.IncludeCodePatterns = true;
+    Consumer =
+        llvm::make_unique<SnippetCompletionItemsCollector>(Options, Results);
+  } else {
+    Options.IncludeCodePatterns = false;
+    Consumer =
+        llvm::make_unique<PlainTextCompletionItemsCollector>(Options, Results);
+  }
+  invokeCodeComplete(std::move(Consumer), Options, FileName, Command, Preamble,
+                     Contents, Pos, std::move(VFS), std::move(PCHs), Logger);
+  return Results;
+}
+
+SignatureHelp
+clangd::signatureHelp(PathRef FileName, tooling::CompileCommand Command,
+                      PrecompiledPreamble const *Preamble, StringRef Contents,
+                      Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+                      std::shared_ptr<PCHContainerOperations> PCHs,
+                      clangd::Logger &Logger) {
+  SignatureHelp Result;
+  CodeCompleteOptions Options;
+  Options.IncludeGlobals = false;
+  Options.IncludeMacros = false;
+  Options.IncludeCodePatterns = false;
+  Options.IncludeBriefComments = true;
+  invokeCodeComplete(llvm::make_unique<SignatureHelpCollector>(Options, Result),
+                     Options, FileName, Command, Preamble, Contents, Pos,
+                     std::move(VFS), std::move(PCHs), Logger);
+  return Result;
 }
 
 void clangd::dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
 llvm::Optional<ParsedAST>
 ParsedAST::Build(std::unique_ptr<clang::CompilerInvocation> CI,

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

struct CompletionItem {
   // command?: Command - An optional command that is executed *after* inserting
   //                     this completion.
   //
   // data?: any - A data entry field that is preserved on a completion item
   //              between a completion and a completion resolve request.
   static std::string unparse(const CompletionItem &P);
 };
 
+/// A single parameter of a particular signature.
+struct ParameterInformation {
+
+  /// The label of this parameter. Mandatory.
+  std::string label;
+
+  /// The documentation of this parameter. Optional.
+  std::string documentation;
+
+  static std::string unparse(const ParameterInformation &);
+};
+
+/// Represents the signature of something callable.
+struct SignatureInformation {
+
+  /// The label of this signature. Mandatory.
+  std::string label;
+
+  /// The documentation of this signature. Optional.
+  std::string documentation;
+
+  /// The parameters of this signature.
+  std::vector<ParameterInformation> parameters;
+
+  static std::string unparse(const SignatureInformation &);
+};
+
+/// Represents the signature of a callable.
+struct SignatureHelp {
+
+  /// The resulting signatures.
+  std::vector<SignatureInformation> signatures;
+
+  /// The active signature.
+  int activeSignature = 0;
+
+  /// The active parameter of the active signature.
+  int activeParameter = 0;
+
+  static std::string unparse(const SignatureHelp &);
+};
+
 } // namespace clangd
 } // namespace clang
 
 #endif

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

if (!CI.additionalTextEdits.empty()) {
     // Replace the trailing comma with right brace.
     Result.back() = ']';
   }
   Os.flush();
   // Label is required, so Result is guaranteed to have a trailing comma.
   Result.back() = '}';
   return Result;
 }
+
+std::string ParameterInformation::unparse(const ParameterInformation &PI) {
+  std::string Result = "{";
+  llvm::raw_string_ostream Os(Result);
+  assert(!PI.label.empty() && "parameter information label is required");
+  Os << R"("label":")" << llvm::yaml::escape(PI.label) << '\"';
+  if (!PI.documentation.empty())
+    Os << R"(,"documentation":")" << llvm::yaml::escape(PI.documentation)
+       << '\"';
+  Os << '}';
+  Os.flush();
+  return Result;
+}
+
+std::string SignatureInformation::unparse(const SignatureInformation &SI) {
+  std::string Result = "{";
+  llvm::raw_string_ostream Os(Result);
+  assert(!SI.label.empty() && "signature information label is required");
+  Os << R"("label":")" << llvm::yaml::escape(SI.label) << '\"';
+  if (!SI.documentation.empty())
+    Os << R"(,"documentation":")" << llvm::yaml::escape(SI.documentation)
+       << '\"';
+  Os << R"(,"parameters":[)";
+  for (const auto &Parameter : SI.parameters) {
+    Os << ParameterInformation::unparse(Parameter) << ',';
+  }
+  Os.flush();
+  if (SI.parameters.empty())
+    Result.push_back(']');
+  else
+    Result.back() = ']'; // Replace the last `,` with an `]`.
+  Result.push_back('}');
+  return Result;
+}
+
+std::string SignatureHelp::unparse(const SignatureHelp &SH) {
+  std::string Result = "{";
+  llvm::raw_string_ostream Os(Result);
+  assert(SH.activeSignature >= 0 &&
+         "Unexpected negative value for number of active signatures.");
+  assert(SH.activeParameter >= 0 &&
+         "Unexpected negative value for active parameter index");
+  Os << R"("activeSignature":)" << SH.activeSignature
+     << R"(,"activeParameter":)" << SH.activeParameter << R"(,"signatures":[)";
+  for (const auto &Signature : SH.signatures) {
+    Os << SignatureInformation::unparse(Signature) << ',';
+  }
+  Os.flush();
+  if (SH.signatures.empty())
+    Result.push_back(']');
+  else
+    Result.back() = ']'; // Replace the last `,` with an `]`.
+  Result.push_back('}');
+  return Result;
+}

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

public:
   virtual void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
                                           StringRef ID, JSONOutput &Out) = 0;
   virtual void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
                                          StringRef ID, JSONOutput &Out) = 0;
   virtual void onCodeAction(CodeActionParams Params, StringRef ID,
                             JSONOutput &Out) = 0;
   virtual void onCompletion(TextDocumentPositionParams Params, StringRef ID,
                             JSONOutput &Out) = 0;
+  virtual void onSignatureHelp(TextDocumentPositionParams Params, StringRef ID,
+                               JSONOutput &Out) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
                                 JSONOutput &Out) = 0;
   virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
                                     JSONOutput &Out) = 0;
   virtual void onFileEvent(const DidChangeWatchedFilesParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
                               ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

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

void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
 
     Callbacks.onCompletion(*TDPP, ID, Output);
   }
 
 private:
   ProtocolCallbacks &Callbacks;
 };
 
+struct SignatureHelpHandler : Handler {
+  SignatureHelpHandler(JSONOutput &Output, ProtocolCallbacks &Callbacks)
+      : Handler(Output), Callbacks(Callbacks) {}
+
+  void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+    auto TDPP = TextDocumentPositionParams::parse(Params, Output);
+    if (!TDPP) {
+      Output.log("Failed to decode TextDocumentPositionParams!\n");
+      return;
+    }
+    Callbacks.onSignatureHelp(*TDPP, ID, Output);
+  }
+
+private:
+  ProtocolCallbacks &Callbacks;
+};
+
 struct GotoDefinitionHandler : Handler {
   GotoDefinitionHandler(JSONOutput &Output, ProtocolCallbacks &Callbacks)
       : Handler(Output), Callbacks(Callbacks) {}
 
   void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
     auto TDPP = TextDocumentPositionParams::parse(Params, Output);
     if (!TDPP) {
       Output.log("Failed to decode TextDocumentPositionParams!\n");
Dispatcher.registerHandler(
       llvm::make_unique<TextDocumentFormattingHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
       "textDocument/codeAction",
       llvm::make_unique<CodeActionHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
       "textDocument/completion",
       llvm::make_unique<CompletionHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
+      "textDocument/signatureHelp",
+      llvm::make_unique<SignatureHelpHandler>(Out, Callbacks));
+  Dispatcher.registerHandler(
       "textDocument/definition",
       llvm::make_unique<GotoDefinitionHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
       "textDocument/switchSourceHeader",
       llvm::make_unique<SwitchSourceHeaderHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
       "workspace/didChangeWatchedFiles",
       llvm::make_unique<WorkspaceDidChangeWatchedFilesHandler>(Out, Callbacks));
 }

clang-tools-extra/trunk/test/clangd/formatting.test

 # RUN: clangd < %s | FileCheck %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 Content-Length: 125
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
-# CHECK: Content-Length: 466
 # CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
 # CHECK:   "textDocumentSync": 1,
 # CHECK:   "documentFormattingProvider": true,
 # CHECK:   "documentRangeFormattingProvider": true,
 # CHECK:   "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
 # CHECK:   "codeActionProvider": true,
 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
 # CHECK:   "definitionProvider": true

clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test

 # RUN: clangd -run-synchronously < %s | FileCheck %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 # Test with invalid initialize request parameters
 Content-Length: 142
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":"","rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
-# CHECK: Content-Length: 466
+# CHECK: Content-Length: 535
 # CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
 # CHECK:   "textDocumentSync": 1,
 # CHECK:   "documentFormattingProvider": true,
 # CHECK:   "documentRangeFormattingProvider": true,
 # CHECK:   "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
 # CHECK:   "codeActionProvider": true,
 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
+# CHECK:   "signatureHelpProvider": {"triggerCharacters": ["(",","]},
 # CHECK:   "definitionProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}

clang-tools-extra/trunk/test/clangd/initialize-params.test

 # RUN: clangd -run-synchronously < %s | FileCheck %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 # Test initialize request parameters with rootUri
 Content-Length: 143
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"file:///path/to/workspace","capabilities":{},"trace":"off"}}
-# CHECK: Content-Length: 466
+# CHECK: Content-Length: 535
 # CHECK: {"jsonrpc":"2.0","id":0,"result":{"capabilities":{
 # CHECK:   "textDocumentSync": 1,
 # CHECK:   "documentFormattingProvider": true,
 # CHECK:   "documentRangeFormattingProvider": true,
 # CHECK:   "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
 # CHECK:   "codeActionProvider": true,
 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
+# CHECK:   "signatureHelpProvider": {"triggerCharacters": ["(",","]},
 # CHECK:   "definitionProvider": true
 # CHECK: }}}
 #
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}

clang-tools-extra/trunk/test/clangd/signature-help.test

+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+
+# Start a session.
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+# Modify the document.
+Content-Length: 333
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"void foo(int x, int y);\nvoid foo(int x, float y);\nvoid foo(float x, int y);\nvoid foo(float x, float y);\nvoid bar(int x, int y = 0);\nvoid bar(float x = 0, int y = 42);\nint main() { foo("}}}
+
+# Ask for signature help.
+Content-Length: 151
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/signatureHelp","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":8,"character":9}}}
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"activeSignature":0,"activeParameter":0,"signatures":[
+# CHECK-DAG: {"label":"foo(float x, float y) -> void","parameters":[{"label":"float x"},{"label":"float y"}]}
+# CHECK-DAG: {"label":"foo(float x, int y) -> void","parameters":[{"label":"float x"},{"label":"int y"}]}
+# CHECK-DAG: {"label":"foo(int x, float y) -> void","parameters":[{"label":"int x"},{"label":"float y"}]}
+# CHECK-DAG: {"label":"foo(int x, int y) -> void","parameters":[{"label":"int x"},{"label":"int y"}]}
+# CHECK: ]}
+
+# Modify the document
+Content-Length: 333
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":2,"text":"void foo(int x, int y);\nvoid foo(int x, float y);\nvoid foo(float x, int y);\nvoid foo(float x, float y);\nvoid bar(int x, int y = 0);\nvoid bar(float x = 0, int y = 42);\nint main() { bar("}}}
+
+# Ask for signature help (this checks default argument handling).
+Content-Length: 151
+
+{"jsonrpc":"2.0","id":2,"method":"textDocument/signatureHelp","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":8,"character":9}}}
+# CHECK: {"jsonrpc":"2.0","id":2,"result":{"activeSignature":0,"activeParameter":0,"signatures":[
+# CHECK-DAG: {"label":"bar(int x, int y = 0) -> void","parameters":[{"label":"int x"},{"label":"int y = 0"}]}
+# CHECK-DAG: {"label":"bar(float x = 0, int y = 42) -> void","parameters":[{"label":"float x = 0"},{"label":"int y = 42"}]}
+# CHECK: ]}
+
+# Shutdown.
+Content-Length: 49
+
+{"jsonrpc":"2.0","id":100000,"method":"shutdown"}