Improve error recovery around colon.

[sepavloff]

Summary

Recognize additional cases, when '::' is mistyped as ':' and provide
descriptive diagnostics for them.
This is a fix to RP18587 - colons have too much protection in member-declarations

[sepavloff]

Added tests for unnamed fields.

[sepavloff]

Changed check as Richard recommended.

Sorry, overlooked it.

[sepavloff]

Fixed parsing unnamed bit fields

Thank you for example, it represents a case missed from the patch.
Adding colon protector indeed helps for the code:

const int m = 4;
struct A {
  struct n {
    int m;
  };
  int A::n : m;
};

I found another example, that is not MS-specific and is currently
misparsed by clang:

struct A {
  enum class B {
    C
  };
};
const int C = 4;
struct D {
  A::B : C;
};

Fixing this case is a bit trickier because declspec is parsed with colon
protection turned off, otherwise we cannot recover typos like:

A:B c;

To recogneze that A::B:C is not a typo for A::B::C we must know that the
parsed scope spec is a part of type specifier.

In the updated patch TryAnnotateCXXScopeToken has additional parameter,
that provide information whether a type is expected, with this change the
above example is parsed correctly.

[rsmith]

Fixing this case is a bit trickier because declspec is parsed with colon
protection turned off, otherwise we cannot recover typos like:

A:B c;

With your patch, it's parsed with colon protection turned on, right? Or do we also need to add colon protection to tentative parsing of member-declarations?

If you want to detect and recover from the above case, you need more lookahead than you're currently using: you need to check if the colon is followed by *two* identifiers. If it is, you can temporarily turn off colon protection.

lib/Parse/Parser.cpp

1692–1694 ↗

(On Diff #10083)

This doesn't seem correct. Even if A::B::C is a type, we still should not do the recovery here, because this may still be an unnamed bitfield.

Also, I'm concerned about whether this NeedType thing is correct, since we use TryAnnotateCXXScopeSpec from tentative parsing when we don't know whether we expect a type or not.

[sepavloff]

In D3653#10, @rsmith wrote:

Fixing this case is a bit trickier because declspec is parsed with colon
protection turned off, otherwise we cannot recover typos like:

A:B c;

With your patch, it's parsed with colon protection turned on, right? Or do we also need to add colon protection to tentative parsing of member-declarations?

With this patch declspec is parsed with colon protection turned off, this allows make recover of errors like:

struct S2b {
  C1:C2 f(C1::C2);
};

or

struct S6b {
  C1:C2 m1 : B1::B2; 
};

Colon protection is turned on when parsing declarator, - when parser expects nested-name-specifier of pointer to a member and when parsing direct-declarator.

If you want to detect and recover from the above case, you need more lookahead than you're currently using: you need to check if the colon is followed by *two* identifiers. If it is, you can temporarily turn off colon protection.

The patch uses a bit different way. If parser parses declspec and sees

A:B

it may be one of two things:

• It is unnamed bit field. In this case A must be a type suitable for bit field and B must be a value.
• It is typo for A::B. In this case A::B must be a type.

In both cases parser must read type first. As there is no case when both A and A::B are types suitable for declaring bit fields, these cases can be distinguished unambiguously, just by trying to build longest nested name specifier, which is still used to refer to a type. If A::B is a type or namespace, A cannot be used as a type of a bit field and recovery A:B -> A::B is valid. Without scoped enums it would be enough to check if A::B exists, that is why we need to pass NeedType when analyzing C++ scope.

lib/Parse/Parser.cpp

1692–1694 ↗

(On Diff #10083)

Whether the recovery is attempted is determined by ColonIsSacred field. NeedType only assists the recovering when ColonIsSacred is false. By default NeedType is false and behavior is identical to the current one. In the patch it is set only when parsing declspec, to correctly handle case when A is scoped enum and A::B is valid.

[sepavloff]

Parse declspec with colon protection turned on.

It reduces number of cases where colon recovery is possible, but looks more reliable.

[rsmith]

Looks great, thanks!

lib/Parse/ParseDecl.cpp

4511–4515

Please reindent this to make the precedence more obvious (or clang-format it). [Last 3 lines should be one space further to the right.]

[sepavloff]

Closed by commit rL212957 (authored by @sepavloff).

lib/Parse/ParseDecl.cpp

case tok::kw___is_signed:
       // We're done with the declaration-specifiers.
       goto DoneWithDeclSpec;
 
       // typedef-name
     case tok::kw_decltype:
     case tok::identifier: {
       // In C++, check to see if this is a scope specifier like foo::bar::, if
       // so handle it as such.  This is important for ctor parsing.
-      if (getLangOpts().CPlusPlus) {
+      if (getLangOpts().CPlusPlus && !DS.hasTypeSpecifier()) {
         if (TryAnnotateCXXScopeToken(EnteringContext)) {
           if (!DS.hasTypeSpecifier())
             DS.SetTypeSpecError();
           goto DoneWithDeclSpec;
         }
         if (!Tok.is(tok::identifier))
           continue;
       }
void Parser::ParseDeclaratorInternal(Declarator &D,
                                      DirectDeclParseFunction DirectDeclParser) {
   if (Diags.hasAllExtensionsSilenced())
     D.setExtension();
 
   // C++ member pointers start with a '::' or a nested-name.
   // Member pointers get special handling, since there's no place for the
   // scope spec in the generic path below.
   if (getLangOpts().CPlusPlus &&
-      (Tok.is(tok::coloncolon) || Tok.is(tok::identifier) ||
-       Tok.is(tok::annot_cxxscope))) {
+      (Tok.is(tok::coloncolon) ||
+      (Tok.is(tok::identifier) && (NextToken().is(tok::coloncolon) ||
+                                   NextToken().is(tok::less))) ||
     bool EnteringContext = D.getContext() == Declarator::FileContext ||
                            D.getContext() == Declarator::MemberContext;
     CXXScopeSpec SS;
     ParseOptionalCXXScopeSpecifier(SS, ParsedType(), EnteringContext);
 
     if (SS.isNotEmpty()) {
       if (Tok.isNot(tok::star)) {
         // The scope spec really belongs to the direct-declarator.
         if (D.mayHaveIdentifier())

 ///         template-id
 ///
 /// Note, any additional constructs added here may need corresponding changes
 /// in isConstructorDeclarator.
 void Parser::ParseDirectDeclarator(Declarator &D) {
   DeclaratorScopeObj DeclScopeObj(*this, D.getCXXScopeSpec());
 
   if (getLangOpts().CPlusPlus && D.mayHaveIdentifier()) {
+    // Don't parse FOO:BAR as if it were a typo for FOO::BAR, in this context it
+    // is a bitfield.
+    ColonProtectionRAIIObject X(*this,
+                                D.getContext() == Declarator::MemberContext);
+
     // ParseDeclaratorInternal might already have parsed the scope.
     if (D.getCXXScopeSpec().isEmpty()) {
       bool EnteringContext = D.getContext() == Declarator::FileContext ||
                              D.getContext() == Declarator::MemberContext;
       ParseOptionalCXXScopeSpecifier(D.getCXXScopeSpec(), ParsedType(),
                                      EnteringContext);
     }
 

lib/Parse/ParseDeclCXX.cpp

if (TagType == DeclSpec::TST_struct && !Tok.is(tok::identifier) &&
     // We rarely end up here so the following check is efficient.
     if (II && II->getName().startswith("__is_"))
       TryKeywordIdentFallback(true);
   }
 
   // Parse the (optional) nested-name-specifier.
   CXXScopeSpec &SS = DS.getTypeSpecScope();
   if (getLangOpts().CPlusPlus) {
-    // "FOO : BAR" is not a potential typo for "FOO::BAR".
+    // "FOO : BAR" is not a potential typo for "FOO::BAR".  In this context it
+    // is base-specifier-list.
     ColonProtectionRAIIObject X(*this);
 
     if (ParseOptionalCXXScopeSpecifier(SS, ParsedType(), EnteringContext))
       DS.SetTypeSpecError();
     if (SS.isSet())
       if (Tok.isNot(tok::identifier) && Tok.isNot(tok::annot_template_id))
         Diag(Tok, diag::err_expected) << tok::identifier;
   }

 /// brace-or-equal-initializer or pure-specifier.
 void Parser::ParseCXXMemberDeclaratorBeforeInitializer(
     Declarator &DeclaratorInfo, VirtSpecifiers &VS, ExprResult &BitfieldSize,
     LateParsedAttrList &LateParsedAttrs) {
   // member-declarator:
   //   declarator pure-specifier[opt]
   //   declarator brace-or-equal-initializer[opt]
   //   identifier[opt] ':' constant-expression
-  if (Tok.isNot(tok::colon)) {
-    // Don't parse FOO:BAR as if it were a typo for FOO::BAR, in this context it
-    // is a bitfield.
-    // FIXME: This should only apply when parsing the id-expression (see
-    // PR18587).
-    ColonProtectionRAIIObject X(*this);
+  if (Tok.isNot(tok::colon))
     ParseDeclarator(DeclaratorInfo);
-  }
 
   if (!DeclaratorInfo.isFunctionDeclarator() && TryConsumeToken(tok::colon)) {
     BitfieldSize = ParseConstantExpression();
     if (BitfieldSize.isInvalid())
       SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
   } else
     ParseOptionalCXX11VirtSpecifierSeq(VS, getCurrentClass().IsInterface);
 
void Parser::ParseCXXClassMemberDeclaration(AccessSpecifier AS,
 
   // decl-specifier-seq:
   // Parse the common declaration-specifiers piece.
   ParsingDeclSpec DS(*this, TemplateDiags);
   DS.takeAttributesFrom(attrs);
   if (MalformedTypeSpec)
     DS.SetTypeSpecError();
 
-  {
-    // Don't parse FOO:BAR as if it were a typo for FOO::BAR, in this context it
-    // is a bitfield.
-    ColonProtectionRAIIObject X(*this);
-    ParseDeclarationSpecifiers(DS, TemplateInfo, AS, DSC_class,
-                               &CommonLateParsedAttrs);
-  }
 
   // If we had a free-standing type definition with a missing semicolon, we
   // may get this far before the problem becomes obvious.
   if (DS.hasTagDefinition() &&
       TemplateInfo.Kind == ParsedTemplateInfo::NonTemplate &&
       DiagnoseMissingSemiAfterTagDefinition(DS, AS, DSC_class,
                                             &CommonLateParsedAttrs))
     return;

test/SemaCXX/nested-name-spec.cpp

namespace N {
     struct Foo {
       struct N::X *foo(); // expected-error{{no struct named 'X' in namespace 'N::N'}}
     };
   }
 }
 
 namespace TypedefNamespace { typedef int F; };
 TypedefNamespace::F::NonexistentName BadNNSWithCXXScopeSpec; // expected-error {{'F' (aka 'int') is not a class, namespace, or scoped enumeration}}
+
+namespace PR18587 {
+
+struct C1 {
+  int a, b, c;
+  typedef int C2;
+  struct B1 {
+    struct B2 {
+      int a, b, c;
+    };
+  };
+};
+struct C2 { static const unsigned N1 = 1; };
+struct B1 {
+  enum { B2 = 2 };
+  static const int B3 = 3;
+};
+const int N1 = 2;
+
+// Function declarators
+struct S1a { int f(C1::C2); };
+struct S1b { int f(C1:C2); };  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+
+struct S2a {
+  C1::C2 f(C1::C2);
+};
+struct S2b {
+  C1:C2 f(C1::C2);  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+struct S2c {
+  C1::C2 f(C1:C2);  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+struct S3a {
+  int f(C1::C2), C2 : N1;
+  int g : B1::B2;
+};
+struct S3b {
+  int g : B1:B2;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+// Inside square brackets
+struct S4a {
+  int f[C2::N1];
+};
+struct S4b {
+  int f[C2:N1];  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+struct S5a {
+  int f(int xx[B1::B3 ? C2::N1 : B1::B2]);
+};
+struct S5b {
+  int f(int xx[B1::B3 ? C2::N1 : B1:B2]);  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+struct S5c {
+  int f(int xx[B1:B3 ? C2::N1 : B1::B2]);  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+// Bit fields
+struct S6a {
+  C1::C2 m1 : B1::B2;
+};
+struct S6b {
+  C1:C2 m1 : B1::B2;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+struct S6c {
+  C1::C2 m1 : B1:B2;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+struct S6d {
+  int C2:N1;
+};
+
+// Template parameters
+template <int N> struct T1 {
+  int a,b,c;
+  static const unsigned N1 = N;
+  typedef unsigned C1;
+};
+T1<C2::N1> var_1a;
+T1<C2:N1> var_1b;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+template<int N> int F() {}
+int (*X1)() = (B1::B2 ? F<1> : F<2>);
+int (*X2)() = (B1:B2 ? F<1> : F<2>);  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+
+// Bit fields + templates
+struct S7a {
+  T1<B1::B2>::C1 m1 : T1<B1::B2>::N1;
+};
+struct S7b {
+  T1<B1:B2>::C1 m1 : T1<B1::B2>::N1;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+struct S7c {
+  T1<B1::B2>::C1 m1 : T1<B1:B2>::N1;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+// Member pointers
+struct S8a {
+  C1::C2 C1::*m1;
+};
+struct S8b {
+  C1:C2 C1::*m1;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+// nested types
+struct S9a {
+  typedef C1::C2 t1;
+};
+struct S9b {
+  typedef C1:C2 t1;  // expected-error{{unexpected ':' in nested name specifier; did you mean '::'?}}
+};
+
+}