[clangd] Index UsingDecls

[kadircet]

Summary

D58340 enables indexing of USRs, this makes sure test in clangd are
aligned with the change

[kadircet]

• Update USR to include "UD"

[ilya-biryukov]

Could we also add a test for code completion to make sure we return the new decls there?

[hokein]

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

namespace abc {
int foo(int);
int foo(double);
};

namespace std {
using abc::foo;
}

void f() {
  std::^
}

[ilya-biryukov]

For context: @hokein mentioned problems in the clangd's code completion if we would index these symbols.
This patch in itself does not hurt much, users of the indexing API can decide how to deal with UsingDecl on their own, clangd is just one of the clients.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

That's true, but we're not sure how much this would hurt in practice. Currently we don't show any results from dynamic index for std::strcmp, which is arguably worse than showing an extra completion item for the using.

[hokein]

In D58341#1401212, @ilya-biryukov wrote:

For context: @hokein mentioned problems in the clangd's code completion if we would index these symbols.
This patch in itself does not hurt much, users of the indexing API can decide how to deal with UsingDecl on their own, clangd is just one of the clients.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

That's true, but we're not sure how much this would hurt in practice. Currently we don't show any results from dynamic index for std::strcmp, which is arguably worse than showing an extra completion item for the using.

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior? Is it possible for us to extend Sema to support it?

If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.

[ilya-biryukov]

In D58341#1401295, @hokein wrote:

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?

Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.

Is it possible for us to extend Sema to support it?

We could, but then we'd loose the signatures of the targets functions, which is sad :-(

If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.

The problem is that using-decls have multiple signatures. They can introduce more than one name into the scope, so the question is which one should we pick and how should we store them.
In any case, it feels like any solution we can come up with would require storing using declarations in the index in one form or the other, so this patch definitely makes sense: it gives us hooks we can use to handle usings in clangd.

[kadircet]

• Add tests for code completion

[ilya-biryukov]

LGTM from my side, with a few NITs.
But let's wait for an LGTM from Haojian too, to make sure his concerns are addressed.

unittests/clangd/SymbolCollectorTests.cpp

1126

That's definitely too much setup for such a simple test.
I thought it's possible to wire up a real index in the completion tests, but it seems that's not the case. So let's not bother to run an actual completion here, ignore my previous comment about adding a test.

1163

a typo NIT: s/does not/do not
also, maybe use "type info" or "signature" instead of "target info"?

[kadircet]

• Revert last change

[hokein]

Thanks for the explanation.

In D58341#1401365, @ilya-biryukov wrote:

In D58341#1401295, @hokein wrote:

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?

Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.

Is it possible for us to extend Sema to support it?

We could, but then we'd loose the signatures of the targets functions, which is sad :-(

If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.

The problem is that using-decls have multiple signatures. They can introduce more than one name into the scope, so the question is which one should we pick and how should we store them.

I think for most cases, the using-decl has only one shadow decl, anyway it is out scope of this patch.

In any case, it feels like any solution we can come up with would require storing using declarations in the index in one form or the other, so this patch definitely makes sense: it gives us hooks we can use to handle usings in clangd.

unittests/clangd/SymbolCollectorTests.cpp

15

These includes are not needed.

58

here as well.

1126

I thought it's possible to wire up a real index in the completion tests, but it seems that's not the case. So let's not bother to run an actual completion here, ignore my previous comment about adding a test.

Adding completions to SymbolCollectorTest is overweight, but I think this is possible (and worthy) to add one to CodeCompleteTest without too much effort. We have TU.index() to build a real index.

I understand the problem better now, we are missing some decls from sema because we avoid deserialization in preamble, I think we should document it somewhere, can't think a better place, maybe at the completion test?

[kadircet]

• Address comments

unittests/clangd/SymbolCollectorTests.cpp

 //===-- SymbolCollectorTests.cpp  -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "ClangdServer.h"
+#include "CodeComplete.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Index.h"
+#include "index/MemIndex.h"
 #include "index/SymbolCollector.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"

 MATCHER_P(Labeled, Label, "") {
   return (arg.Name + arg.Signature).str() == Label;
 }
 MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
 MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
 MATCHER_P(Snippet, S, "") {
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
+MATCHER_P(CompletionQName, Name, "") { return (arg.Scope + arg.Name) == Name; }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 MATCHER_P(DeclURI, P, "") {
   return StringRef(arg.CanonicalDeclaration.FileURI) == P;
 }
 MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
          (arg.IncludeHeaders.begin()->IncludeHeader == P);
const std::string Header = R"(
 
     // Namespace
     namespace bar {
     int v2;
     }
     // Namespace alias
     namespace baz = bar;
 
-    // FIXME: using declaration is not supported as the IndexAction will ignore
-    // implicit declarations (the implicit using shadow declaration) by default,
-    // and there is no way to customize this behavior at the moment.
     using bar::v2;
     } // namespace foo
   )";
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
               UnorderedElementsAreArray(
                   {AllOf(QName("Foo"), ForCodeCompletion(true)),
                    AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
EXPECT_THAT(Symbols,
                    AllOf(QName("KInt"), ForCodeCompletion(true)),
                    AllOf(QName("kStr"), ForCodeCompletion(true)),
                    AllOf(QName("foo"), ForCodeCompletion(true)),
                    AllOf(QName("foo::bar"), ForCodeCompletion(true)),
                    AllOf(QName("foo::int32"), ForCodeCompletion(true)),
                    AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
                    AllOf(QName("foo::v1"), ForCodeCompletion(true)),
                    AllOf(QName("foo::bar::v2"), ForCodeCompletion(true)),
+                   AllOf(QName("foo::v2"), ForCodeCompletion(true)),
                    AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
 }
 
 TEST_F(SymbolCollectorTest, FileLocal) {
   const std::string Header = R"(
     class Foo {};
     namespace {
       class Ignored {};
TEST_F(SymbolCollectorTest, ImplementationDetail) {
   )";
   runSymbolCollector(Header, /**/ "");
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(
                   AllOf(QName("X_Y_Decl"), ImplementationDetail()),
                   AllOf(QName("Public"), Not(ImplementationDetail()))));
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  auto completions = [](const Annotations &Test, SymbolSlab SS) {
+    class IgnoreDiagnostics : public DiagnosticsConsumer {
+      void onDiagnosticsReady(PathRef File,
+                              std::vector<Diag> Diagnostics) override {}
+    };
+
+    MockFSProvider FS;
+    MockCompilationDatabase CDB;
+    IgnoreDiagnostics DiagConsumer;
+    ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+    std::unique_ptr<SymbolIndex> OverrideIndex =
+        MemIndex::build(std::move(SS), {});
+    clangd::CodeCompleteOptions Opts;
+    Opts.Index = OverrideIndex.get();
+
+    auto File = testPath("foo.cpp");
+    runAddDocument(Server, File, Test.code());
+    auto CompletionList =
+        llvm::cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+    return CompletionList;
+  };
+
+  const Annotations Header(R"(
+  void foo();
+  namespace std {
+    using ::foo;
+  }
+  void bar() {
+    (void)std::^foo;
+  })");
+  runSymbolCollector(Header.code(), /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+
+  const auto Results = completions(Header, std::move(Symbols));
+  EXPECT_THAT(
+      Results.Completions,
+      Contains(AllOf(
+          ReturnType(/*Currently using decls does not export target info*/ ""),
+          CompletionQName("std::foo"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/SymbolInfoTests.cpp

std::pair<const char *, std::vector<SymbolDetails>>
           void foo(bool) {}
           void foo(int) {}
           namespace bar {
             using ::fo^o;
           }
         )cpp",
               {CreateExpectedSymbolDetails("foo", "", "c:@F@foo#"),
                CreateExpectedSymbolDetails("foo", "", "c:@F@foo#b#"),
-               CreateExpectedSymbolDetails("foo", "", "c:@F@foo#I#")}},
+               CreateExpectedSymbolDetails("foo", "", "c:@F@foo#I#"),
+               CreateExpectedSymbolDetails("foo", "bar::", "c:@N@bar@UD@foo")}},
           {
               R"cpp( // Multiple symbols returned - implicit conversion
           struct foo {};
           struct bar {
             bar(const foo&) {}
           };
           void func_baz1(bar) {}
           void func_baz2() {