[Parser] Fix assertion-on-invalid for unexpected typename.

[vsapsai]

Summary

In ParseDeclarationSpecifiers for the code

class A typename A;

we were able to annotate token kw_typename because it refers to
existing type. But later during processing token annot_typename we
failed to SetTypeSpecType and exited switch statement leaving
annotation token unconsumed. The code after the switch statement failed
because it didn't expect a special token.

The fix is not to assume that switch statement consumes all special
tokens and consume any token, not just non-special.

rdar://problem/37099386

[vsapsai]

I had performance considerations regarding this change because ConsumeAnyToken is heavier than ConsumeToken. But I didn't notice any problems. If you know this is a hot path that deserves more attention, please let me know.

clang/lib/Parse/ParseDecl.cpp

3076

Here we potentially can leave annotation token unconsumed. But I wasn't able to create a test case that would trigger a problem at this point.

3148

We didn't consume the annotation token because of break on isInvalid a few lines above.

[vsapsai]

Ping.

[rsmith]

Thanks, seems reasonable to me.

clang/lib/Parse/ParseDecl.cpp

3076

I think you're talking about the break a few lines back?

I think we actually get away with this, but only because SetTypeSpecType can only fail if there was already a type specifier, which we checked for on line 3019, so isInvalid is never true.

3148

I wonder if our error recovery would be improved by changing line 3134 to just

if (DS.hasTypeSpecifier())

(which would likewise render the break here unreachable given the current rules enforced by SetTypeSpecType).

3802

Maybe add a comment here saying we can get here with an annotation token after an error?

[vsapsai]

clang/lib/Parse/ParseDecl.cpp

3076

You are right, I was talking about break on the line 3071.

3148

Change in if condition fixes the assertion too and the diagnostics are

repro.cpp:1:18: error: expected a qualified name after 'typename'
class A typename A;
                 ^
repro.cpp:1:18: error: expected unqualified-id
2 errors generated.

With the currently reviewed change diagnostics are

repro.cpp:1:18: error: expected a qualified name after 'typename'
class A typename A;
                 ^
repro.cpp:1:18: error: cannot combine with previous 'class' declaration specifier
2 errors generated.

This is for the file

$ cat repro.cpp 
class A typename A;

This small test isn't representable but I find showing next to each other

expected a qualified name
expected unqualified-id

confusing and contradictory at the first glance.

If there are no further comments, I'll commit the change as is (after adding a comment for ConsumeAnyToken).

3802

Will do.

[vsapsai]

• Add a comment.

clang/lib/Parse/ParseDecl.cpp

case tok::annot_cxxscope: {
                                          Tok.getAnnotationEndLoc(),
                                          PrevSpec, DiagID, T, Policy);
           if (isInvalid)
             break;
         }
         else
           DS.SetTypeSpecError();
         DS.SetRangeEnd(Tok.getAnnotationEndLoc());
         ConsumeAnnotationToken(); // The typename
       }
 
       if (Next.isNot(tok::identifier))
         goto DoneWithDeclSpec;
 
       // Check whether this is a constructor declaration. If we're in a
       // context where the identifier could be a class name, and it has the
       // shape of a constructor declaration, process it as one.
case tok::annot_typename: {
                                        DiagID, T, Policy);
       } else
         DS.SetTypeSpecError();
 
       if (isInvalid)
         break;
 
       DS.SetRangeEnd(Tok.getAnnotationEndLoc());
       ConsumeAnnotationToken(); // The typename
 
       continue;
     }
 
     case tok::kw___is_signed:
       // GNU libstdc++ 4.4 uses __is_signed as an identifier, but Clang
       // typically treats it as a trait. If we see __is_signed as it appears
       // in libstdc++, e.g.,
if (isInvalid) {
                               llvm::to_string((OpenCLVer % 100) / 10);
         Diag(Tok, DiagID) << VerSpec << PrevSpec << isStorageClass;
       } else
         Diag(Tok, DiagID) << PrevSpec;
     }
 
     DS.SetRangeEnd(Tok.getLocation());
     if (DiagID != diag::err_bool_redeclaration)
-      ConsumeToken();
+      ConsumeAnyToken();
 
     AttrsLastTime = false;
   }
 }
 
 /// ParseStructDeclaration - Parse a struct declaration without the terminating
 /// semicolon.
 ///

clang/test/Parser/cxx-decl.cpp

struct D {
       // No warning here: while this could be written as
       //   friend (::B::C)();
       // we do need parentheses *somewhere* here.
       friend A (::B::C());
     };
   }
 }
 
+namespace rdar37099386 {
+  class A typename A; // expected-error {{expected a qualified name after 'typename'}}
+  // expected-error@-1 {{cannot combine with previous 'class' declaration specifier}}
+}
+
 // PR8380
 extern ""      // expected-error {{unknown linkage language}}
 test6a { ;// expected-error {{C++ requires a type specifier for all declarations}}
 #if __cplusplus <= 199711L
 // expected-error@-2 {{expected ';' after top level declarator}}
 #else
 // expected-error@-4 {{expected expression}}
 // expected-note@-5 {{to match this}}
 #endif
   
   int test6b;
 #if __cplusplus >= 201103L
 // expected-error@+3 {{expected}}
 // expected-error@-3 {{expected ';' after top level declarator}}
 #endif