Fix heuristics skipping invalid ctor-initializers with C++11

[ogoffart]

Summary

Use better heuristics to detect if a '{' might be the start of the
constructor body or not. Especially when there is a completion token.

Fix the test 'test/CodeCompletion/ctor-initializer.cpp ' when clang defaults to c++11

Replaces http://reviews.llvm.org/D21497

[ogoffart]

Ping.

The problem i'm fixing here is how we recover invalid code in the ctor-init part as we skip the function body.
In particular, we want to know if the '{' is the begining of the body or not. In C++03, we always consider it as the beginng of the body. The problem was that in C++11 we don't, making the code skip too much, causing worse parse error later.

So what this patch is doing is finding heuristics to know if the '{' is starting a function body or not.
The rules are the following: If we are not in a template argument, anf that the previous tokens are not an identifier, or a > , then it is much more likely to be the function body. We verify that further by checking that the token after the matching '}'

I also changed the code to just ignore the code_completion token at this point. The previous code was making it thinking that it would then be a template argument. But this is not likely.

[ogoffart]

Ping!

[rsmith]

Please produce patches with more lines of context in future; phabricator only lets us comment on lines that are included in the patch, and in this case some of the relevant parts of the function are not in the context. (The equivalent of diff -U1000 is a common approach for this.)

Taking a step back, I wonder whether we have the right strategy overall for code completion within in-class mem-initializer-lists. The code after the code completion token is quite plausibly not even brace-balanced, in a case where you're writing a new constructor. Consider this completion:

struct Foo {
  Foo() : some_long_x(0), some_|
  int some_long_x, some_long_y;
};

Here, we ought to be able to complete "some_long_y", but we need to recognize that the next token is not part of the function definition in order to do that (and then we need to not try to consume a function body once we're done with the initializer). And conversely when completing here:

struct Foo {
  Foo() : some_long_x(0), some_| {}
  int some_long_x, some_long_y;
};

... we need to recognize that we do have a function body so that we can parse the members to find out what names we should complete.

However, this patch is an incremental improvement over what we already have, so I'm happy to go in this direction for now.

lib/Parse/ParseCXXInlineMethods.cpp

840

Can you delete the check for kw_template here? We handle the template keyword above, and any time we actually hit this case we would expect template to be followed by one of ::, (, or {, which makes no sense.

847–851

You should probably also handle the case of a comma immediately after the code completion token, for completions like this:

struct A {
  A() : new_mem|, existing_member() {}
  int new_member, existing_member;
};

... and likewise the case where the completion token is followed by an identifier, a ::, or a decltype, for completions like this:

struct A {
  A() : new_mem| existing_member() {}
  int new_member, existing_member;
};

In all those cases, I think you can just continue to pick up the rest of the initializers after the code completion token. (And if you continue from here in those cases, I think you can remove the handling of the code_completion token up on line 835, since this codepath will do the right thing in all those cases.)

894–897

Please add a comment here indicating that if the previous token is not one of these kinds, we've encountered an error (because this mem-initializer is missing its initializer). This is not really obvious, and it's important because that's what makes it correct to use a heuristic to guess whether we've got a function body next.

[ogoffart]

Made the requested changes

[ogoffart]

Regarding this:

struct Foo {
  Foo() : some_long_x(0), some_| {}
  int some_long_x, some_long_y;
};

That should work fine because the token before the { is the code completion token, not an identifier. This is basicaly tested by the test with CHECK-CC2.

However, this does not work because if the completion is within an identifier, the Lexer will abort by calling cutOffLexing from Lexer::LexIdentifier

[ogoffart]

ping

[ogoffart]

Ping

[ogoffart]

Ping?
I guess i coud just commit it now.

[Prazek]

Richard?

lib/Parse/ParseCXXInlineMethods.cpp

         ConsumeToken();
 
         if (Tok.is(tok::kw_template)) {
           Toks.push_back(Tok);
           ConsumeToken();
         }
       }
 
-      if (Tok.isOneOf(tok::identifier, tok::kw_template)) {
+      if (Tok.is(tok::identifier)) {
         Toks.push_back(Tok);
         ConsumeToken();
-      } else if (Tok.is(tok::code_completion)) {
-        Toks.push_back(Tok);
-        ConsumeCodeCompletionToken();
-        // Consume the rest of the initializers permissively.
-        // FIXME: We should be able to perform code-completion here even if
-        //        there isn't a subsequent '{' token.
-        MightBeTemplateArgument = true;
-        break;
       } else {
         break;
       }
     } while (Tok.is(tok::coloncolon));
 
+    if (Tok.is(tok::code_completion)) {
+      Toks.push_back(Tok);
+      ConsumeCodeCompletionToken();
+      if (Tok.isOneOf(tok::identifier, tok::coloncolon, tok::kw_decltype)) {
+        // Could be the start of another member initializer (the ',' has not
+        // been written yet)
+        continue;
+      }
+    }
+
+    if (Tok.is(tok::comma)) {
+      // The initialization is missing, we'll diagnose it later.
+      Toks.push_back(Tok);
+      ConsumeToken();
+      continue;
+    }
     if (Tok.is(tok::less))
       MightBeTemplateArgument = true;
 
     if (MightBeTemplateArgument) {
       // We may be inside a template argument list. Grab up to the start of the
       // next parenthesized initializer or braced-init-list. This *might* be the
       // initializer, or it might be a subexpression in the template argument
       // list.

     Toks.push_back(Tok);
     bool IsLParen = (kind == tok::l_paren);
     SourceLocation OpenLoc = Tok.getLocation();
 
     if (IsLParen) {
       ConsumeParen();
     } else {
       assert(kind == tok::l_brace && "Must be left paren or brace here.");
       ConsumeBrace();
       // In C++03, this has to be the start of the function body, which
       // means the initializer is malformed; we'll diagnose it later.
       if (!getLangOpts().CPlusPlus11)
         return false;
+
+      const Token &PreviousToken = Toks[Toks.size() - 2];
+      if (!MightBeTemplateArgument &&
+          !PreviousToken.isOneOf(tok::identifier, tok::greater,
+                                 tok::greatergreater)) {
+        // If the opening brace is not preceded by one of these tokens, we are
+        // missing the mem-initializer-id. In order to recover better, we need
+        // to use heuristics to determine if this '{' is most likely the
+        // begining of a brace-init-list or the function body.
+        // Check the token after the corresponding '}'.
+        TentativeParsingAction PA(*this);
+        if (SkipUntil(tok::r_brace) &&
+            !Tok.isOneOf(tok::comma, tok::ellipsis, tok::l_brace)) {
+          // Consider there was a malformed initializer and this is the start
+          // of the function body. We'll diagnose it later.
+          PA.Revert();
+          return false;
+        }
+        PA.Revert();
+      }
     }
 
     // Grab the initializer (or the subexpression of the template argument).
     // FIXME: If we support lambdas here, we'll need to set StopAtSemi to false
     //        if we might be inside the braces of a lambda-expression.
     tok::TokenKind CloseKind = IsLParen ? tok::r_paren : tok::r_brace;
     if (!ConsumeAndStoreUntil(CloseKind, Toks, /*StopAtSemi=*/true)) {
       Diag(Tok, diag::err_expected) << CloseKind;

test/CodeCompletion/ctor-initializer.cpp

 struct Base1 {
   Base1() : {}
-  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:2:12 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
+  // RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:2:12 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
+  // RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:2:12 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
   // CHECK-CC1: COMPLETION: Pattern : member1(<#args#>)
   // CHECK-CC1: COMPLETION: Pattern : member2(<#args#>
 
   Base1(int) : member1(123), {}
-  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:7:30 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
+  // RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:8:30 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
+  // RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:8:30 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
   // CHECK-CC2-NOT: COMPLETION: Pattern : member1(<#args#>)
   // CHECK-CC2: COMPLETION: Pattern : member2(<#args#>
 
   int member1;
   float member2;
 };
 
 struct Derived : public Base1 {
   Derived();
   Derived(int);
   Derived(float);
   int deriv1;
 };
 
 Derived::Derived() : {}
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:23:22 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:25:22 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:25:22 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
 // CHECK-CC3: COMPLETION: Pattern : Base1(<#args#>)
 // CHECK-CC3: COMPLETION: Pattern : deriv1(<#args#>)
 
 Derived::Derived(int) try : {
 } catch (...) {
 }
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:28:29 %s -o - | FileCheck -check-prefix=CHECK-CC4 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:31:29 %s -o - | FileCheck -check-prefix=CHECK-CC4 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:31:29 %s -o - | FileCheck -check-prefix=CHECK-CC4 %s
 // CHECK-CC4: COMPLETION: Pattern : Base1(<#args#>)
 // CHECK-CC4: COMPLETION: Pattern : deriv1(<#args#>)
 
 Derived::Derived(float) try : Base1(),
 {
 } catch (...) {
 }
-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:35:39 %s -o - | FileCheck -check-prefix=CHECK-CC5 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:39:39 %s -o - | FileCheck -check-prefix=CHECK-CC5 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:39:39 %s -o - | FileCheck -check-prefix=CHECK-CC5 %s
 // CHECK-CC5-NOT: COMPLETION: Pattern : Base1(<#args#>)
 // CHECK-CC5: COMPLETION: Pattern : deriv1(<#args#>)
+
+struct A {
+  A() : , member2() {}
+  // RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:49:9 %s -o - | FileCheck -check-prefix=CHECK-CC6 %s
+  // RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:49:9 %s -o - | FileCheck -check-prefix=CHECK-CC6 %s
+  // CHECK-CC6: COMPLETION: Pattern : member1(<#args#>
+  int member1, member2;
+};
+
+struct B {
+  B() : member2() {}
+  // RUN: %clang_cc1 -fsyntax-only -std=c++98 -code-completion-at=%s:57:9 %s -o - | FileCheck -check-prefix=CHECK-CC7 %s
+  // RUN: %clang_cc1 -fsyntax-only -std=c++14 -code-completion-at=%s:57:9 %s -o - | FileCheck -check-prefix=CHECK-CC7 %s
+  // CHECK-CC7: COMPLETION: Pattern : member1(<#args#>
+  int member1, member2;
+};