[clangd] Report position of opening paren in singature help

[ilya-biryukov]

Summary

Only accessible via the C++ API at the moment.

[ilya-biryukov]

• Format the code

[sammccall]

clangd/Protocol.h

832

As noted offline, I misread this as being *inside* the paren - could maybe be more explicit?

Position of the start of the argument list, including opening paren. e.g.
foo("first arg", 
   ^argListStart ^cursor

unittests/clangd/CodeCompleteTests.cpp

914

Hmm, I think this test would be easier to follow if tests 1-5 were written separately - it's hard to spot all the locations and how the code interacts.

As a bonus, no need to mess around with explicit positions and the failure message can just echo the test:

for (const char* Test : {
  R"cpp(
    int foo(int a, b, c);
    int main() { foo(foo$p^(foo(10, 10, 10), ^); }
  )cpp",
  ...
}) {
  EXPECT_EQ(signatures(Test).argListStart, Annotations(Test).point("p")) << Test;
}

[ilya-biryukov]

• Clarify the docs, add example.
• Parse each test separately.

[ilya-biryukov]

unittests/clangd/CodeCompleteTests.cpp

914

Thanks! It's way better this way!

clangd/CodeComplete.cpp

SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts,
       : CodeCompleteConsumer(CodeCompleteOpts,
                              /*OutputIsBinary=*/false),
         SigHelp(SigHelp),
         Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
         CCTUInfo(Allocator), Index(Index) {}
 
   void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg,
                                  OverloadCandidate *Candidates,
-                                 unsigned NumCandidates) override {
+                                 unsigned NumCandidates,
+                                 SourceLocation OpenParLoc) override {
+    assert(!OpenParLoc.isInvalid());
+    SourceManager &SrcMgr = S.getSourceManager();
+    OpenParLoc = SrcMgr.getFileLoc(OpenParLoc);
+    if (SrcMgr.isInMainFile(OpenParLoc))
+      SigHelp.argListStart = sourceLocToPosition(SrcMgr, OpenParLoc);
+    else
+      elog("Location oustide main file in signature help: {0}",
+           OpenParLoc.printToString(SrcMgr));
+
     std::vector<ScoredSignature> ScoredSignatures;
     SigHelp.signatures.reserve(NumCandidates);
     ScoredSignatures.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 <= (unsigned)std::numeric_limits<int>::max() &&

clangd/Protocol.h

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;
+
+  /// Position of the start of the argument list, including opening paren. e.g.
+  /// foo("first arg",   "second arg",
+  ///    ^-argListStart   ^-cursor
+  /// This is a clangd-specific extension, it is only available via C++ API and
+  /// not currently serialized for the LSP.
+  Position argListStart;
 };
 llvm::json::Value toJSON(const SignatureHelp &);
 
 struct RenameParams {
   /// The document that was opened.
   TextDocumentIdentifier textDocument;
 
   /// The position at which this request was sent.

unittests/clangd/CodeCompleteTests.cpp

#if 0
   // really provide useful results in excluded branches.
   par^
 #endif
     }
 )cpp");
 
   EXPECT_TRUE(Results.Completions.empty());
 }
-
-SignatureHelp signatures(StringRef Text,
+SignatureHelp signatures(StringRef Text, Position Point,
                          std::vector<Symbol> IndexSymbols = {}) {
   std::unique_ptr<SymbolIndex> Index;
   if (!IndexSymbols.empty())
     Index = memIndex(IndexSymbols);
 
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer::Options Opts = ClangdServer::optsForTest();
   Opts.StaticIndex = Index.get();
 
   ClangdServer Server(CDB, FS, DiagConsumer, Opts);
   auto File = testPath("foo.cpp");
+  runAddDocument(Server, File, Text);
+  return cantFail(runSignatureHelp(Server, File, Point));
+}
+
+SignatureHelp signatures(StringRef Text,
+                         std::vector<Symbol> IndexSymbols = {}) {
   Annotations Test(Text);
-  runAddDocument(Server, File, Test.code());
-  return cantFail(runSignatureHelp(Server, File, Test.point()));
+  return signatures(Test.code(), Test.point(), std::move(IndexSymbols));
 }
 
 MATCHER_P(ParamsAre, P, "") {
   if (P.size() != arg.parameters.size())
     return false;
   for (unsigned I = 0; I < P.size(); ++I)
     if (P[I] != arg.parameters[I].label)
       return false;
TEST(SignatureHelpTest, ActiveArg) {
   )cpp");
   EXPECT_THAT(Results.signatures,
               ElementsAre(Sig("baz(int a, int b, int c) -> int",
                               {"int a", "int b", "int c"})));
   EXPECT_EQ(0, Results.activeSignature);
   EXPECT_EQ(1, Results.activeParameter);
 }
 
+TEST(SignatureHelpTest, OpeningParen) {
+  llvm::StringLiteral Tests[] = {// Recursive function call.
+                                 R"cpp(
+    int foo(int a, int b, int c);
+    int main() {
+      foo(foo $p^( foo(10, 10, 10), ^ )));
+    })cpp",
+                                 // Functional type cast.
+                                 R"cpp(
+    struct Foo {
+      Foo(int a, int b, int c);
+    };
+    int main() {
+      Foo $p^( 10, ^ );
+    })cpp",
+                                 // New expression.
+                                 R"cpp(
+    struct Foo {
+      Foo(int a, int b, int c);
+    };
+    int main() {
+      new Foo $p^( 10, ^ );
+    })cpp",
+                                 // Macro expansion.
+                                 R"cpp(
+    int foo(int a, int b, int c);
+    #define FOO foo(
+
+    int main() {
+      // Macro expansions.
+      $p^FOO 10, ^ );
+    })cpp",
+                                 // Macro arguments.
+                                 R"cpp(
+    int foo(int a, int b, int c);
+    int main() {
+    #define ID(X) X
+      ID(foo $p^( foo(10), ^ ))
+    })cpp"};
+
+  for (auto Test : Tests) {
+    Annotations Code(Test);
+    EXPECT_EQ(signatures(Code.code(), Code.point()).argListStart,
+              Code.point("p"))
+        << "Test source:" << Test;
+  }
+}
+
 class IndexRequestCollector : public SymbolIndex {
 public:
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
             llvm::function_ref<void(const Symbol &)> Callback) const override {
     Requests.push_back(Req);
     return true;
   }