[CodeCompletion] (mostly) fix completion in incomplete C++ ctor initializers.

[sammccall]

Summary

C++ member function bodies (including ctor initializers) are first captured
into a buffer and then parsed after the class is complete. (This allows
members to be referenced even if declared later).

When the boundary of the function body cannot be established, its buffer is
discarded and late-parsing never happens (it would surely fail).
For code completion this is the wrong tradeoff: the point of the parse is to
generate completions as a side-effect.
Today, when the ctor body wasn't typed yet there are no init list completions.
With this patch we parse such an init-list if it contains the completion point.

There's one caveat: the parser has to decide where to resume parsing members
after a broken init list. Often the first clear recovery point is *after* the
next member, so that member is missing from completion/signature help etc. e.g.

struct S {
  S() m  //<- completion here
  int maaa;
  int mbbb;
}

Here "int maaa;" is treated as part of the init list, so "maaa" is not available
as a completion. Maybe in future indentation can be used to recognize that
this is a separate member, not part of the init list.

[kadircet]

clang/lib/Parse/ParseCXXInlineMethods.cpp

153

i don't follow the logic here. maybe i am reading the comment wrong, but we are actually going to eat more tokens by calling SkipMalformedDecl, possibly the following one, right? for example in a scenario like:

struct Foo {
  Foo : ^b
  int bar;
}

ConsumeAndStoreFunctionPrologue will actually put b following the code completion token (^) into Toks as well, hence when we skip, we actually skip until the next semicolon and throw away bar. But when the code completion token is after b, ConsumeAndStoreFunctionPrologue we'll have code completion token at the end of the Toks and won't skip anything

Do we have cases that break miserably when we don't perform an extra skip here for the (possible) reminder of current initalizer?

[sammccall]

clang/lib/Parse/ParseCXXInlineMethods.cpp

153

i don't follow the logic here. maybe i am reading the comment wrong,

Neither the code nor the comment are very good, but I think they are consistent.

Baseline behavior: we're going to recover by letting SkipMalformedDecl() eat tokens.
Exception: if we already ate the code completion token *and* stopped right afterwards.
Reason: CC token followed by heuristic stop are consistent with the function being truncated at the code completion point.

This exception allows some motivating testcases to pass. I thought maybe further improvements were possible but didn't want to get into them in this patch. However....

Do we have cases that break miserably when we don't perform an extra skip here for the (possible) reminder of current initalizer?

Um, apparently not. I thought I did!
Never skipping is simple and intuitive and makes more testcases pass.
Let's try it, the risk seems low.

[sammccall]

Never eat malformed decl while code completing

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

auto Results = completions(
           class X { public: X(); int x_; };
           X::X() : x_^(0) {}
         }
       )cpp");
   EXPECT_THAT(Results.Completions,
               UnorderedElementsAre(AllOf(Scope("ns::X::"), Named("x_"))));
 }
 
+// Like other class members, constructor init lists have to parse what's below,
+// after the completion point.
+// But recovering from an incomplete constructor init list is particularly
+// tricky because the bulk of the list is not surrounded by brackets.
+TEST(CompletionTest, ConstructorInitListIncomplete) {
+  auto Results = completions(
+      R"cpp(
+        namespace ns {
+          struct X {
+            X() : x^
+            int xyz_;
+          };
+        }
+      )cpp");
+  EXPECT_THAT(Results.Completions, ElementsAre(Named("xyz_")));
+
+  Results = completions(
+      R"cpp(
+        int foo();
+
+        namespace ns {
+          struct X {
+            X() : xyz_(fo^
+            int xyz_;
+          };
+        }
+      )cpp");
+  EXPECT_THAT(Results.Completions, ElementsAre(Named("foo")));
+}
+
 TEST(CompletionTest, CodeCompletionContext) {
   auto Results = completions(
       R"cpp(
         namespace ns {
           class X { public: X(); int x_; };
           void f() {
             X x;
             x.^;
EXPECT_THAT(Results.signatures,
                 ElementsAre(Sig("foo([[int x]], [[int y]]) -> void")));
     EXPECT_EQ(1, Results.activeParameter);
   }
 }
 
 TEST(SignatureHelpTest, ConstructorInitializeFields) {
   {
     const auto Results = signatures(R"cpp(
-      struct A {
-        A(int);
-      };
+      struct A { A(int); };
       struct B {
         B() : a_elem(^) {}
         A a_elem;
       };
     )cpp");
     EXPECT_THAT(Results.signatures,
                 UnorderedElementsAre(Sig("A([[int]])"), Sig("A([[A &&]])"),
                                      Sig("A([[const A &]])")));
   }
   {
     const auto Results = signatures(R"cpp(
+      struct A { A(int); };
+      struct B {
+        B() : a_elem(^
+        A a_elem;
+      };
+    )cpp");
+    // FIXME: currently the parser skips over the decl of a_elem as part of the
+    // (broken) init list, so we don't get signatures for the first member.
+    EXPECT_THAT(Results.signatures, IsEmpty());
+  }
+  {
+    const auto Results = signatures(R"cpp(
+      struct A { A(int); };
+      struct B {
+        B() : a_elem(^
+        int dummy_elem;
+        A a_elem;
+      };
+    )cpp");
+    EXPECT_THAT(Results.signatures,
+                UnorderedElementsAre(Sig("A([[int]])"), Sig("A([[A &&]])"),
+                                     Sig("A([[const A &]])")));
+  }
+  {
+    const auto Results = signatures(R"cpp(
       struct A {
         A(int);
       };
       struct C {
         C(int);
         C(A);
       };
       struct B {

clang/lib/Parse/ParseCXXInlineMethods.cpp

NamedDecl *Parser::ParseCXXInlineMethodDef(
   getCurrentClass().LateParsedDeclarations.push_back(LM);
   CachedTokens &Toks = LM->Toks;
 
   tok::TokenKind kind = Tok.getKind();
   // Consume everything up to (and including) the left brace of the
   // function body.
   if (ConsumeAndStoreFunctionPrologue(Toks)) {
     // We didn't find the left-brace we expected after the
-    // constructor initializer; we already printed an error, and it's likely
-    // impossible to recover, so don't try to parse this method later.
+    // constructor initializer.
+
+    // If we're code-completing and the completion point was in the broken
+    // initializer, we want to parse it even though that will fail.
+    if (PP.isCodeCompletionEnabled() &&
+        llvm::any_of(Toks, [](const Token &Tok) {
+          return Tok.is(tok::code_completion);
+        })) {
+      // If we gave up at the completion point, the initializer list was
+      // likely truncated, so don't eat more tokens. We'll hit some extra
+      // errors, but they should be ignored in code completion.
+      return FnD;
+    }
+
+    // We already printed an error, and it's likely impossible to recover,
+    // so don't try to parse this method later.
     // Skip over the rest of the decl and back to somewhere that looks
     // reasonable.
     SkipMalformedDecl();
     delete getCurrentClass().LateParsedDeclarations.back();
     getCurrentClass().LateParsedDeclarations.pop_back();
     return FnD;
   } else {
     // Consume everything up to (and including) the matching right brace.

clang/test/CodeCompletion/ctor-initializer.cpp

 template <class T>
 struct X : Y<T> {
   X() : Y<T>() {};
 };
 
 // RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:100:9 %s -o - | FileCheck -check-prefix=CHECK-CC11 %s
 // RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:100:9 %s -o - | FileCheck -check-prefix=CHECK-CC11 %s
 // CHECK-CC11: Pattern : Y<T>(<#Y<T>#>)
+
+// Test with incomplete init lists. (Relevant as parsing is *not* cut off).
+struct Incomplete1 {
+  Incomplete1() : mem
+
+  int member1;
+  int member2;
+};
+// RUN: not %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:109:19 %s -o - | FileCheck -check-prefix=CHECK-CC12 %s
+// CHECK-CC12: COMPLETION: Pattern : member1(<#int#>)
+// CHECK-CC12: COMPLETION: Pattern : member2(<#int#>)
+
+struct Incomplete2 {
+  Incomplete2() : member2(
+
+  int member1;
+  int member2;
+};
+// RUN: not %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:119:27 %s -o - | FileCheck -check-prefix=CHECK-CC13 %s
+// CHECK-CC13: PREFERRED-TYPE: int