[PATCH] D58341: [clangd] Index UsingDecls

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE354879: [clangd] Index UsingDecls (authored by kadircet, 
committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58341?vs=188170&id=188363#toc

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341

Files:
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/SymbolInfoTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -331,9 +331,6 @@
 // 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
   )";
@@ -360,6 +357,7 @@
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))}));
 }
 
@@ -1149,6 +1147,16 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const char *Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  })";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -17,6 +17,7 @@
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestIndex.h"
+#include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Error.h"
@@ -2314,6 +2315,26 @@
   EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
 }
 
+TEST(CompletionTest, UsingDecl) {
+  const char *Header(R"cpp(
+void foo(int);
+namespace std {
+  using ::foo;
+})cpp");
+  const char *Source(R"cpp(
+void bar() {
+  std::^;
+})cpp");
+  auto Index = TestTU::withHeaderCode(Header).index();
+  clangd::CodeCompleteOptions Opts;
+  Opts.Index = Index.get();
+  Opts.AllScopes = true;
+  auto R = completions(Source, {}, Opts);
+  EXPECT_THAT(R.Completions,
+  ElementsAre(AllOf(Scope("std::"), Named("foo"),
+Kind(CompletionItemKind::Reference;
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/FindSymbolsTests.cpp
===
--- unittests/clangd/FindSymbolsTests.cpp
+++ unittests/clangd/FindSymbolsTests.cpp
@@ -368,9 +368,6 @@
   // 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
 )");
@@ -415,7 +412,7 @@
   Children(,
  AllOf(WithName("baz"), WithKind(SymbolKind::Namespace),
Children()),
- AllOf(WithName("v2"), WithKind(SymbolKind::Variable}));
+ AllOf(WithName("v2"), WithKind(SymbolKind::Namespace}));
 }
 
 TEST_F(DocumentSymbolsTest, DeclarationDefinition) {
Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 188170.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341

Files:
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/SymbolInfoTests.cpp


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -325,9 +325,6 @@
 // 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
   )";
@@ -354,6 +351,7 @@
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))}));
 }
 
@@ -1118,6 +1116,16 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const char *Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  })";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -17,6 +17,7 @@
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestIndex.h"
+#include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Error.h"
@@ -2314,6 +2315,26 @@
   EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
 }
 
+TEST(CompletionTest, UsingDecl) {
+  const char *Header(R"cpp(
+void foo(int);
+namespace std {
+  using ::foo;
+})cpp");
+  const char *Source(R"cpp(
+void bar() {
+  std::^;
+})cpp");
+  auto Index = TestTU::withHeaderCode(Header).index();
+  clangd::CodeCompleteOptions Opts;
+  Opts.Index = Index.get();
+  Opts.AllScopes = true;
+  auto R = completions(Source, {}, Opts);
+  EXPECT_THAT(R.Completions,
+  ElementsAre(AllOf(Scope("std::"), Named("foo"),
+Kind(CompletionItemKind::Reference;
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -325,9 +325,6 @@
 // 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
   )";
@@ -354,6 +351,7 @@
AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
All

[PATCH] D58341: [clangd] Index UsingDecls

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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.




Comment at: unittests/clangd/SymbolCollectorTests.cpp:1126
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  auto completions = [](const Annotations &Test, SymbolSlab SS) {
+class IgnoreDiagnostics : public DiagnosticsConsumer {

ilya-biryukov wrote:
> 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.
> 
> 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?



Comment at: unittests/clangd/SymbolCollectorTests.cpp:14
 #include "TestTU.h"
+#include "index/Index.h"
+#include "index/MemIndex.h"

These includes are not needed.



Comment at: unittests/clangd/SymbolCollectorTests.cpp:57
 }
+MATCHER_P(CompletionQName, Name, "") { return (arg.Scope + arg.Name) == Name; }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }

here as well.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 187376.
kadircet marked an inline comment as done.
kadircet added a comment.

- Revert last change


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341

Files:
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/SymbolInfoTests.cpp


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -7,8 +7,12 @@
 
//===--===//
 
 #include "Annotations.h"
+#include "ClangdServer.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"
@@ -50,6 +54,7 @@
 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;
@@ -325,9 +330,6 @@
 // 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
   )";
@@ -354,6 +356,7 @@
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))}));
 }
 
@@ -1118,6 +1121,16 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const char *Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  })";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -7,8 +7,12 @@
 //===--===//
 
 #include "Annotations.h"
+#include "ClangdServer.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"
@@ -50,6 +54,7 @@
 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;
@@ -325,9 +330,6 @@
 // Namespace alias
 namespace baz = bar;
 
-// FIXME: using declaration is not supported as the IndexAction will ignore

[PATCH] D58341: [clangd] Index UsingDecls

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

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.




Comment at: unittests/clangd/SymbolCollectorTests.cpp:1126
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  auto completions = [](const Annotations &Test, SymbolSlab SS) {
+class IgnoreDiagnostics : public DiagnosticsConsumer {

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.




Comment at: unittests/clangd/SymbolCollectorTests.cpp:1163
+  Contains(AllOf(
+  ReturnType(/*Currently using decls does not export target info*/ ""),
+  CompletionQName("std::foo";

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


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 187367.
kadircet added a comment.

- Add tests for code completion


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341

Files:
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/SymbolInfoTests.cpp

Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -7,8 +7,13 @@
 //===--===//
 
 #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"
@@ -50,6 +55,7 @@
 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;
@@ -325,9 +331,6 @@
 // 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
   )";
@@ -354,6 +357,7 @@
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))}));
 }
 
@@ -1118,6 +1122,48 @@
   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 Diagnostics) override {}
+};
+
+MockFSProvider FS;
+MockCompilationDatabase CDB;
+IgnoreDiagnostics DiagConsumer;
+ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+std::unique_ptr 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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::^
  }


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 187209.
kadircet added a comment.

- Update USR to include "UD"


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58341/new/

https://reviews.llvm.org/D58341

Files:
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/SymbolInfoTests.cpp


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -325,9 +325,6 @@
 // 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
   )";
@@ -354,6 +351,7 @@
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))}));
 }
 
@@ -1118,6 +1116,17 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const std::string Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  }
+  )";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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 {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -325,9 +325,6 @@
 // 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
   )";
@@ -354,6 +351,7 @@
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))}));
 }
 
@@ -1118,6 +1116,17 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const std::string Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  }
+  )";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58341: [clangd] Index UsingDecls

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added subscribers: cfe-commits, jdoerfert, arphaman, jkorous, MaskRay, 
ioeric, ilya-biryukov.
Herald added a project: clang.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D58341

Files:
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/SymbolInfoTests.cpp


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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@foo")}},
   {
   R"cpp( // Multiple symbols returned - implicit conversion
   struct foo {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -325,9 +325,6 @@
 // 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
   )";
@@ -354,6 +351,7 @@
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))}));
 }
 
@@ -1118,6 +1116,17 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const std::string Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  }
+  )";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: unittests/clangd/SymbolInfoTests.cpp
===
--- unittests/clangd/SymbolInfoTests.cpp
+++ unittests/clangd/SymbolInfoTests.cpp
@@ -167,7 +167,8 @@
 )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@foo")}},
   {
   R"cpp( // Multiple symbols returned - implicit conversion
   struct foo {};
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -325,9 +325,6 @@
 // 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
   )";
@@ -354,6 +351,7 @@
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))}));
 }
 
@@ -1118,6 +1116,17 @@
   AllOf(QName("Public"), Not(ImplementationDetail();
 }
 
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  const std::string Header = R"(
+  void foo();
+  namespace std {
+using ::foo;
+  }
+  )";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols, Contains(QName("std::foo")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits