Remove special error recovery for ::(id)

[rnk]

Summary

The code pattern used to implement the token rewriting hack doesn't
interact well with token caching in the pre-processor. As a result,
clang would crash on 'int f(::(id));' while doing a tenative parse of
the contents of the outer parentheses. The original code from PR11852
still doesn't crash the compiler.

This error recovery also often does the wrong thing with member function
pointers. The test case from the original PR doesn't recover the right
way either:

void S::(*pf)() = S::f; // should be 'void (S::*pf)()'

Instead we were recovering as 'void S::*pf()', which is still wrong.

If we still think that users mistakenly parenthesize identifiers in
nested name specifiers, we should change clang to intentionally parse
that form with an error, rather than doing a token rewrite.

Fixes PR26623, but I think there will be many more bugs of this nature
due to other token rewriting attempts.

[rsmith]

Can you also remove the corresponding diagnostic?

lib/Parse/ParseExprCXX.cpp

73–75 ↗

(On Diff #75490)

This seems to mess up the cached token buffer in the same way; it's a little surprising we don't see problems here too.

[rnk]

lib/Parse/ParseExprCXX.cpp

73–75 ↗

(On Diff #75490)

Fuzzing shows that there are more ways to trigger this assertion, so whether we keep this recovery mode or not, we might want to implement that token rewrite functionality.

[rnk]

• Remove unused diagnostic

[rnk]

I think the last time we discussed this, we decided that to truly fix this we need to audit all calls to PP.EnterToken(Stream) from the parser. They are probably broken in token caching mode. I haven't been able to find the time to do that. Should we commit this in the meantime, since we think this error recovery is questionable and adds unnecessary complexity?

cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td

def err_unexpected_token_in_nested_name_spec : Error<
   "'%0' cannot be a part of nested name specifier; did you mean ':'?">;
 def err_bool_redeclaration : Error<
   "redeclaration of C++ built-in type 'bool'">;
 def ext_c11_static_assert : Extension<
   "_Static_assert is a C11-specific feature">, InGroup<C11>;
 def warn_cxx98_compat_static_assert : Warning<
   "static_assert declarations are incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
-def err_paren_after_colon_colon : Error<
-  "unexpected parenthesis after '::'">;
 def err_function_definition_not_allowed : Error<
   "function definition is not allowed here">;
 def err_expected_end_of_enumerator : Error<
   "expected '= constant-expression' or end of enumerator definition">;
 def err_expected_coloncolon_after_super : Error<
   "expected '::' after '__super'">;
 
 def ext_decomp_decl_empty : ExtWarn<

cfe/trunk/include/clang/Parse/Parser.h

private:
 
   bool ParseOptionalCXXScopeSpecifier(CXXScopeSpec &SS,
                                       ParsedType ObjectType,
                                       bool EnteringContext,
                                       bool *MayBePseudoDestructor = nullptr,
                                       bool IsTypename = false,
                                       IdentifierInfo **LastII = nullptr);
 
-  void CheckForLParenAfterColonColon();
-
   //===--------------------------------------------------------------------===//
   // C++0x 5.1.2: Lambda expressions
 
   // [...] () -> type {...}
   ExprResult ParseLambdaExpression();
   ExprResult TryParseLambdaExpression();
   Optional<unsigned> ParseLambdaIntroducer(LambdaIntroducer &Intro,
                                            bool *SkippedInits = nullptr);

cfe/trunk/lib/Parse/ParseExprCXX.cpp

if (!Actions.isTemplateName(getCurScope(), SS, /*hasTemplateKeyword=*/false,
                               TemplateName, ObjectType, EnteringContext,
                               Template, MemberOfUnknownSpecialization))
     return;
 
   FixDigraph(*this, PP, Next, SecondToken, tok::unknown,
              /*AtDigraph*/false);
 }
 
-/// \brief Emits an error for a left parentheses after a double colon.
-///
-/// When a '(' is found after a '::', emit an error.  Attempt to fix the token
-/// stream by removing the '(', and the matching ')' if found.
-void Parser::CheckForLParenAfterColonColon() {
-  if (!Tok.is(tok::l_paren))
-    return;
-
-  Token LParen = Tok;
-  Token NextTok = GetLookAheadToken(1);
-  Token StarTok = NextTok;
-  // Check for (identifier or (*identifier
-  Token IdentifierTok = StarTok.is(tok::star) ? GetLookAheadToken(2) : StarTok;
-  if (IdentifierTok.isNot(tok::identifier))
-    return;
-  // Eat the '('.
-  ConsumeParen();
-  Token RParen;
-  RParen.setLocation(SourceLocation());
-  // Do we have a ')' ?
-  NextTok = StarTok.is(tok::star) ? GetLookAheadToken(2) : GetLookAheadToken(1);
-  if (NextTok.is(tok::r_paren)) {
-    RParen = NextTok;
-    // Eat the '*' if it is present.
-    if (StarTok.is(tok::star))
-      ConsumeToken();
-    // Eat the identifier.
-    ConsumeToken();
-    // Add the identifier token back.
-    PP.EnterToken(IdentifierTok);
-    // Add the '*' back if it was present.
-    if (StarTok.is(tok::star))
-      PP.EnterToken(StarTok);
-    // Eat the ')'.
-    ConsumeParen();
-  }
-
-  Diag(LParen.getLocation(), diag::err_paren_after_colon_colon)
-      << FixItHint::CreateRemoval(LParen.getLocation())
-      << FixItHint::CreateRemoval(RParen.getLocation());
-}
-
 /// \brief Parse global scope or nested-name-specifier if present.
 ///
 /// Parses a C++ global scope specifier ('::') or nested-name-specifier (which
 /// may be preceded by '::'). Note that this routine will not parse ::new or
 /// ::delete; it will just leave them in the token stream.
 ///
 ///       '::'[opt] nested-name-specifier
 ///       '::'
if (NextKind == tok::l_brace) {
       // It is invalid to have :: {, consume the scope qualifier and pretend
       // like we never saw it.
       Diag(ConsumeToken(), diag::err_expected) << tok::identifier;
     } else {
       // '::' - Global scope qualifier.
       if (Actions.ActOnCXXGlobalScopeSpecifier(ConsumeToken(), SS))
         return true;
 
-      CheckForLParenAfterColonColon();
-
       HasScopeSpecifier = true;
     }
   }
 
   if (Tok.is(tok::kw___super)) {
     SourceLocation SuperLoc = ConsumeToken();
     if (!Tok.is(tok::coloncolon)) {
       Diag(Tok.getLocation(), diag::err_expected_coloncolon_after_super);
if (Next.is(tok::coloncolon)) {
       // as the name in a nested-name-specifier.
       Token Identifier = Tok;
       SourceLocation IdLoc = ConsumeToken();
       assert(Tok.isOneOf(tok::coloncolon, tok::colon) &&
              "NextToken() not working properly!");
       Token ColonColon = Tok;
       SourceLocation CCLoc = ConsumeToken();
 
-      CheckForLParenAfterColonColon();
-
       bool IsCorrectedToColon = false;
       bool *CorrectionFlagPtr = ColonIsSacred ? &IsCorrectedToColon : nullptr;
       if (Actions.ActOnCXXNestedNameSpecifier(getCurScope(), IdInfo,
                                               EnteringContext, SS,
                                               false, CorrectionFlagPtr)) {
         // Identifier is not recognized as a nested name, but we can have
         // mistyped '::' instead of ':'.
         if (CorrectionFlagPtr && IsCorrectedToColon) {

cfe/trunk/test/Parser/colon-colon-parentheses.cpp

-// RUN: %clang_cc1 %s -fsyntax-only -verify -DPR21815
-// RUN: cp %s %t
-// RUN: not %clang_cc1 -x c++ -fixit %t
-// RUN: %clang_cc1 -x c++ %t
+// RUN: %clang_cc1 %s -verify -fno-spell-checking
 
 struct S { static int a,b,c;};
-int S::(a);  // expected-error{{unexpected parenthesis after '::'}}
-int S::(b;  // expected-error{{unexpected parenthesis after '::'}}
+int S::(a);  // expected-error{{expected unqualified-id}}
+int S::(b;  // expected-error{{expected unqualified-id}}
+        );
 int S::c;
-int S::(*d);  // expected-error{{unexpected parenthesis after '::'}}
-int S::(*e;  // expected-error{{unexpected parenthesis after '::'}}
+int S::(*d);  // expected-error{{expected unqualified-id}}
+int S::(*e;  // expected-error{{expected unqualified-id}}
+        );
 int S::*f;
-int g = S::(a);  // expected-error{{unexpected parenthesis after '::'}}
-int h = S::(b;  // expected-error{{unexpected parenthesis after '::'}}
+int g = S::(a);  // expected-error {{expected unqualified-id}} expected-error {{use of undeclared identifier 'a'}}
+int h = S::(b;  // expected-error {{expected unqualified-id}} expected-error {{use of undeclared identifier 'b'}}
+            );
 int i = S::c;
 
 void foo() {
   int a;
-  a = ::(g);  // expected-error{{unexpected parenthesis after '::'}}
-  a = ::(h;  // expected-error{{unexpected parenthesis after '::'}}
+  a = ::(g);  // expected-error{{expected unqualified-id}}
+  a = ::(h;  // expected-error{{expected unqualified-id}}
   a = ::i;
 }
 
-#ifdef PR21815
+// The following tests used to be crash bugs.
+
+// PR21815
 // expected-error@+2{{C++ requires a type specifier for all declarations}}
 // expected-error@+1{{expected unqualified-id}}
 a (::( ));
 
 ::((c )); // expected-error{{expected unqualified-id}}
-#endif
+
+// PR26623
+int f1(::(B) p); // expected-error {{expected unqualified-id}} expected-error {{use of undeclared identifier 'B'}}
+
+int f2(::S::(C) p); // expected-error {{expected unqualified-id}} expected-error {{use of undeclared identifier 'C'}}