[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

2018-03-16 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327717: [clangd] Handle multiple callbacks from Semas 
completion (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D44567

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,43 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -450,8 +450,15 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv(
+  "Multiple code complete callbacks (parser backtracked?). "
+  "Dropping results from context {0}, keeping results from {1}.",
+  getCompletionKindString(this->CCContext.getKind()),
+  getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,43 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -450,8 +450,15 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv(
+  "Multiple code complete callbacks (parser backtracked?). "
+  "Dropping results from context {0}, keeping results from {1}.",
+  getCompletionKindString(this->CCContext.getKind()),
+  getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 
___

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

2018-03-16 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE327717: [clangd] Handle multiple callbacks from 
Semas completion (authored by ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D44567?vs=138706=138713#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44567

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -450,8 +450,15 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv(
+  "Multiple code complete callbacks (parser backtracked?). "
+  "Dropping results from context {0}, keeping results from {1}.",
+  getCompletionKindString(this->CCContext.getKind()),
+  getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,43 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;


Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -450,8 +450,15 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv(
+  "Multiple code complete callbacks (parser backtracked?). "
+  "Dropping results from context {0}, keeping results from {1}.",
+  getCompletionKindString(this->CCContext.getKind()),
+  getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,43 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

2018-03-16 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov updated this revision to Diff 138706.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

- Address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44567

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,43 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -450,8 +450,15 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv(
+  "Multiple code complete callbacks (parser backtracked?). "
+  "Dropping results from context {0}, keeping results from {1}.",
+  getCompletionKindString(this->CCContext.getKind()),
+  getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,43 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -450,8 +450,15 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv(
+  "Multiple code complete callbacks (parser backtracked?). "
+  "Dropping results from context {0}, keeping results from {1}.",
+  getCompletionKindString(this->CCContext.getKind()),
+  getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

2018-03-16 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/CodeComplete.cpp:454
+if (CCSema) {
+  log(llvm::formatv("ProcessCodeCompleteResults called multiple times."
+"Previous ContextKind: '{0}'. New ContextKind: '{1}'",

Suggest some hint in the log that helps readers who don't know the code 
structure. (It's easy to find the code structure by searching.

E.g. "Multiple code complete callbacks (parser backtracked?). Dropping results 
from context {0}, keeping results from {1}."



Comment at: unittests/clangd/CodeCompleteTests.cpp:623
+
+  Results = completions(R"cpp(
+struct FooBarBaz {};

dead code? delete it or add an assertion


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44567



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

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

2018-03-16 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

As a follow-up to an offline discussion: I opted for not excluding results from 
'Recovery' contexts, because they are actually somewhat semantically relevant 
(i.e. contain only local variables from the current function). One example 
where this is useful are skipped PP branches (see CompleteInExcludedPPBranch 
test).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44567



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

[PATCH] D44567: [clangd] Handle multiple callbacks from Sema's completion

2018-03-16 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: ioeric, jkorous-apple, klimek.

When parser backtracks, we might receive multiple code completion
callbacks.
Previously we had a failing assertion there, now we take first results
and hope they are good enough.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44567

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,42 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  Results = completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -450,8 +450,14 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv("ProcessCodeCompleteResults called multiple times."
+"Previous ContextKind: '{0}'. New ContextKind: '{1}'",
+getCompletionKindString(this->CCContext.getKind()),
+getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -608,6 +608,42 @@
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+  namespace ns {
+  struct FooBarBaz {};
+  } // namespace ns
+
+ int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  Results = completions(R"cpp(
+struct FooBarBaz {};
+void test() {
+  if (FooBarBaz * x^) {}
+}
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+int bar(int param_in_bar) {
+}
+
+int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+}
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar";
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -450,8 +450,14 @@
   void ProcessCodeCompleteResults(class Sema , CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+if (CCSema) {
+  log(llvm::formatv("ProcessCodeCompleteResults called multiple times."
+"Previous ContextKind: '{0}'. New ContextKind: '{1}'",
+getCompletionKindString(this->CCContext.getKind()),
+getCompletionKindString(Context.getKind(;
+  return;
+}
 // Record the completion context.
-assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
 CCSema = 
 CCContext = Context;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits