[Parser][ObjC] Improve diagnostics and recovery when C++ keywords are used as identifiers in Objective-C++

[arphaman]

Summary

This patch improves the 'expected identifier' errors that are presented when a C++ keyword is used as an identifier in Objective-C++ by mentioning that this is a C++ keyword in the diagnostic message. It also improves the error recovery: the parser will treat the C++ keywords as identifiers to prevent unrelated parsing errors.

[manmanren]

Cheers,
Manman

include/clang/Basic/DiagnosticParseKinds.td

442 ↗

(On Diff #77482)

This name is a little misleading. It sounds like we are expecting a keyword :]

include/clang/Parse/Parser.h

798 ↗

(On Diff #77482)

--> if the next token is a C++ (if to is)

801 ↗

(On Diff #77482)

Same here. The function name is kind of misleading.

lib/Parse/ParseDecl.cpp

5405 ↗

(On Diff #77482)

Does C++ have the same issue? Or is this only needed for Objective-C++?

lib/Parse/ParseObjc.cpp

153 ↗

(On Diff #77482)

Are we doing this for all occurrences of "isNot(token::identifier)"?
Is it better to wrap this "&&" in the function and rename the function to something like diagnoseNonIdentifier?

[arphaman]

Addressed review comments by renaming the diagnostic and simplifying the name and the use of the expectIdentifier method.

[arphaman]

lib/Parse/ParseDecl.cpp

5405 ↗

(On Diff #77482)

I suppose it kinda does have it as well, but Objective-C++ has worse error recovery for Objective-C declarations in this case, so it's more important for it. I wouldn't mind adding a similar diagnostic for C++ though, but I think that should be done in a separate patch.

[manmanren]

LGTM

Manman

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

 
 def err_expected_field_designator : Error<
   "expected a field designator, such as '.field = 4'">;
 
 def err_declaration_does_not_declare_param : Error<
   "declaration does not declare a parameter">;
 def err_no_matching_param : Error<"parameter named %0 is missing">;
 
+/// Objective-C++ parser diagnostics
+def err_expected_token_instead_of_objcxx_keyword : Error<
+  "expected %0; %1 is a keyword in Objective-C++">;
+def err_expected_member_name_or_semi_objcxx_keyword : Error<
+  "expected member name or ';' after declaration specifiers; "
+  "%0 is a keyword in Objective-C++">;
+
 /// C++ parser diagnostics
 def err_invalid_operator_on_type : Error<
   "cannot use %select{dot|arrow}0 operator on a type">;
 def err_expected_unqualified_id : Error<
   "expected %select{identifier|unqualified-id}0">;
 def err_brackets_go_after_unqualified_id : Error<
   "brackets are not allowed here; to declare an array, "
   "place the brackets after the %select{identifier|name}0">;

cfe/trunk/include/clang/Basic/IdentifierTable.h

void setIsCPlusPlusOperatorKeyword(bool Val = true) {
     if (Val)
       NeedsHandleIdentifier = true;
     else
       RecomputeNeedsHandleIdentifier();
   }
   bool isCPlusPlusOperatorKeyword() const { return IsCPPOperatorKeyword; }
 
   /// \brief Return true if this token is a keyword in the specified language.
-  bool isKeyword(const LangOptions &LangOpts);
+  bool isKeyword(const LangOptions &LangOpts) const;
+
+  /// \brief Return true if this token is a C++ keyword in the specified
+  /// language.
+  bool isCPlusPlusKeyword(const LangOptions &LangOpts) const;
 
   /// getFETokenInfo/setFETokenInfo - The language front-end is allowed to
   /// associate arbitrary metadata with this token.
   template<typename T>
   T *getFETokenInfo() const { return static_cast<T*>(FETokenInfo); }
   void setFETokenInfo(void *T) { FETokenInfo = T; }
 
   /// \brief Return true if the Preprocessor::HandleIdentifier must be called

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

enum ExtraSemiKind {
     InsideStruct = 1,
     InstanceVariableList = 2,
     AfterMemberFunctionDefinition = 3
   };
 
   /// \brief Consume any extra semi-colons until the end of the line.
   void ConsumeExtraSemi(ExtraSemiKind Kind, unsigned TST = TST_unspecified);
 
+  /// Return false if the next token is an identifier. An 'expected identifier'
+  /// error is emitted otherwise.
+  ///
+  /// The parser tries to recover from the error by checking if the next token
+  /// is a C++ keyword when parsing Objective-C++. Return false if the recovery
+  /// was successful.
+  bool expectIdentifier();
+
 public:
   //===--------------------------------------------------------------------===//
   // Scope manipulation
 
   /// ParseScope - Introduces a new scope for parsing. The kind of
   /// scope is determined by ScopeFlags. Objects of this type should
   /// be created on the stack to coincide with the position where the
   /// parser enters the new scope, and this object's constructor will

cfe/trunk/lib/Basic/IdentifierTable.cpp

#define KEYWORD(NAME, FLAGS) \
   case tok::kw_##NAME: return getKeywordStatus(LangOpts, FLAGS);
 #include "clang/Basic/TokenKinds.def"
   default: return KS_Disabled;
   }
 }
 
 /// \brief Returns true if the identifier represents a keyword in the
 /// specified language.
-bool IdentifierInfo::isKeyword(const LangOptions &LangOpts) {
+bool IdentifierInfo::isKeyword(const LangOptions &LangOpts) const {
   switch (getTokenKwStatus(LangOpts, getTokenID())) {
   case KS_Enabled:
   case KS_Extension:
     return true;
   default:
     return false;
   }
 }
 
+/// \brief Returns true if the identifier represents a C++ keyword in the
+/// specified language.
+bool IdentifierInfo::isCPlusPlusKeyword(const LangOptions &LangOpts) const {
+  if (!LangOpts.CPlusPlus || !isKeyword(LangOpts))
+    return false;
+  // This is a C++ keyword if this identifier is not a keyword when checked
+  // using LangOptions without C++ support.
+  LangOptions LangOptsNoCPP = LangOpts;
+  LangOptsNoCPP.CPlusPlus = false;
+  LangOptsNoCPP.CPlusPlus11 = false;
+  return !isKeyword(LangOptsNoCPP);
+}
+
 tok::PPKeywordKind IdentifierInfo::getPPKeywordID() const {
   // We use a perfect hash function here involving the length of the keyword,
   // the first and third character.  For preprocessor ID's there are no
   // collisions (if there were, the switch below would complain about duplicate
   // case values).  Note that this depends on 'if' being null terminated.
 
 #define HASH(LEN, FIRST, THIRD) \
   (LEN << 5) + (((FIRST-'a') + (THIRD-'a')) & 31)

cfe/trunk/lib/Parse/ParseDecl.cpp

if (D.hasEllipsis() && D.hasGroupingParens())
       Diag(PP.getLocForEndOfToken(D.getEllipsisLoc()),
            diag::ext_abstract_pack_declarator_parens);
   } else {
     if (Tok.getKind() == tok::annot_pragma_parser_crash)
       LLVM_BUILTIN_TRAP;
     if (Tok.is(tok::l_square))
       return ParseMisplacedBracketDeclarator(D);
     if (D.getContext() == Declarator::MemberContext) {
+      // Objective-C++: Detect C++ keywords and try to prevent further errors by
+      // treating these keyword as valid member names.
+      if (getLangOpts().ObjC1 && getLangOpts().CPlusPlus &&
+          Tok.getIdentifierInfo() &&
+          Tok.getIdentifierInfo()->isCPlusPlusKeyword(getLangOpts())) {
+        Diag(getMissingDeclaratorIdLoc(D, Tok.getLocation()),
+             diag::err_expected_member_name_or_semi_objcxx_keyword)
+            << Tok.getIdentifierInfo()
+            << (D.getDeclSpec().isEmpty() ? SourceRange()
+                                          : D.getDeclSpec().getSourceRange());
+        D.SetIdentifier(Tok.getIdentifierInfo(), Tok.getLocation());
+        D.SetRangeEnd(Tok.getLocation());
+        ConsumeToken();
+        goto PastIdentifier;
+      }
       Diag(getMissingDeclaratorIdLoc(D, Tok.getLocation()),
            diag::err_expected_member_name_or_semi)
           << (D.getDeclSpec().isEmpty() ? SourceRange()
                                         : D.getDeclSpec().getSourceRange());
     } else if (getLangOpts().CPlusPlus) {
       if (Tok.isOneOf(tok::period, tok::arrow))
         Diag(Tok, diag::err_invalid_operator_on_type) << Tok.is(tok::arrow);
       else {

cfe/trunk/lib/Parse/ParseObjc.cpp

 Parser::ParseObjCAtClassDeclaration(SourceLocation atLoc) {
   ConsumeToken(); // the identifier "class"
   SmallVector<IdentifierInfo *, 8> ClassNames;
   SmallVector<SourceLocation, 8> ClassLocs;
   SmallVector<ObjCTypeParamList *, 8> ClassTypeParams;
 
   while (1) {
     MaybeSkipAttributes(tok::objc_class);
-    if (Tok.isNot(tok::identifier)) {
-      Diag(Tok, diag::err_expected) << tok::identifier;
+    if (expectIdentifier()) {
       SkipUntil(tok::semi);
       return Actions.ConvertDeclToDeclGroup(nullptr);
     }
     ClassNames.push_back(Tok.getIdentifierInfo());
     ClassLocs.push_back(Tok.getLocation());
     ConsumeToken();
 
     // Parse the optional objc-type-parameter-list.
Decl *Parser::ParseObjCAtInterfaceDeclaration(SourceLocation AtLoc,
   if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteObjCInterfaceDecl(getCurScope());
     cutOffParsing();
     return nullptr;
   }
 
   MaybeSkipAttributes(tok::objc_interface);
 
-  if (Tok.isNot(tok::identifier)) {
-    Diag(Tok, diag::err_expected)
-        << tok::identifier; // missing class or category name.
-    return nullptr;
-  }
+  if (expectIdentifier())
+    return nullptr; // missing class or category name.
 
   // We have a class or category name - consume it.
   IdentifierInfo *nameId = Tok.getIdentifierInfo();
   SourceLocation nameLoc = ConsumeToken();
 
   // Parse the objc-type-parameter-list or objc-protocol-refs. For the latter
   // case, LAngleLoc will be valid and ProtocolIdents will capture the
   // protocol references (that have not yet been resolved).
if (Tok.is(tok::colon)) { // a super class is specified.
 
     // Code completion of superclass names.
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteObjCSuperclass(getCurScope(), nameId, nameLoc);
       cutOffParsing();
       return nullptr;
     }
 
-    if (Tok.isNot(tok::identifier)) {
-      Diag(Tok, diag::err_expected)
-          << tok::identifier; // missing super class name.
-      return nullptr;
-    }
+    if (expectIdentifier())
+      return nullptr; // missing super class name.
     superClassId = Tok.getIdentifierInfo();
     superClassLoc = ConsumeToken();
 
     // Type arguments for the superclass or protocol conformances.
     if (Tok.is(tok::less)) {
       parseObjCTypeArgsOrProtocolQualifiers(
           nullptr, typeArgsLAngleLoc, typeArgs, typeArgsRAngleLoc, LAngleLoc,
           protocols, protocolLocs, EndProtoLoc,
if (Tok.is(tok::code_completion)) {
       KeyIdents.push_back(SelIdent);
       Actions.CodeCompleteObjCMethodDeclSelector(getCurScope(), 
                                                  mType == tok::minus,
                                                  /*AtParameterName=*/true,
                                                  ReturnType, KeyIdents);
       cutOffParsing();
       return nullptr;
     }
-    
-    if (Tok.isNot(tok::identifier)) {
-      Diag(Tok, diag::err_expected)
-          << tok::identifier; // missing argument name.
-      break;
-    }
+    if (expectIdentifier())
+      break; // missing argument name.
 
     ArgInfo.Name = Tok.getIdentifierInfo();
     ArgInfo.NameLoc = Tok.getLocation();
     ConsumeToken(); // Eat the identifier.
 
     ArgInfos.push_back(ArgInfo);
     KeyIdents.push_back(SelIdent);
     KeyLocs.push_back(selLoc);
ParseObjCProtocolReferences(SmallVectorImpl<Decl *> &Protocols,
 
   while (1) {
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteObjCProtocolReferences(ProtocolIdents);
       cutOffParsing();
       return true;
     }
 
-    if (Tok.isNot(tok::identifier)) {
-      Diag(Tok, diag::err_expected) << tok::identifier;
+    if (expectIdentifier()) {
       SkipUntil(tok::greater, StopAtSemi);
       return true;
     }
     ProtocolIdents.push_back(std::make_pair(Tok.getIdentifierInfo(),
                                        Tok.getLocation()));
     ProtocolLocs.push_back(Tok.getLocation());
     ConsumeToken();
 
Parser::ParseObjCAtProtocolDeclaration(SourceLocation AtLoc,
   if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteObjCProtocolDecl(getCurScope());
     cutOffParsing();
     return nullptr;
   }
 
   MaybeSkipAttributes(tok::objc_protocol);
 
-  if (Tok.isNot(tok::identifier)) {
-    Diag(Tok, diag::err_expected) << tok::identifier; // missing protocol name.
-    return nullptr;
-  }
+  if (expectIdentifier())
+    return nullptr; // missing protocol name.
   // Save the protocol name, then consume it.
   IdentifierInfo *protocolName = Tok.getIdentifierInfo();
   SourceLocation nameLoc = ConsumeToken();
 
   if (TryConsumeToken(tok::semi)) { // forward declaration of one protocol.
     IdentifierLocPair ProtoInfo(protocolName, nameLoc);
     return Actions.ActOnForwardProtocolDeclaration(AtLoc, ProtoInfo,
                                                    attrs.getList());
   }
 
   CheckNestedObjCContexts(AtLoc);
 
   if (Tok.is(tok::comma)) { // list of forward declarations.
     SmallVector<IdentifierLocPair, 8> ProtocolRefs;
     ProtocolRefs.push_back(std::make_pair(protocolName, nameLoc));
 
     // Parse the list of forward declarations.
     while (1) {
       ConsumeToken(); // the ','
-      if (Tok.isNot(tok::identifier)) {
-        Diag(Tok, diag::err_expected) << tok::identifier;
+      if (expectIdentifier()) {
         SkipUntil(tok::semi);
         return nullptr;
       }
       ProtocolRefs.push_back(IdentifierLocPair(Tok.getIdentifierInfo(),
                                                Tok.getLocation()));
       ConsumeToken(); // the identifier
 
       if (Tok.isNot(tok::comma))
Parser::ParseObjCAtImplementationDeclaration(SourceLocation AtLoc) {
   if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteObjCImplementationDecl(getCurScope());
     cutOffParsing();
     return nullptr;
   }
 
   MaybeSkipAttributes(tok::objc_implementation);
 
-  if (Tok.isNot(tok::identifier)) {
-    Diag(Tok, diag::err_expected)
-        << tok::identifier; // missing class or category name.
-    return nullptr;
-  }
+  if (expectIdentifier())
+    return nullptr; // missing class or category name.
   // We have a class or category name - consume it.
   IdentifierInfo *nameId = Tok.getIdentifierInfo();
   SourceLocation nameLoc = ConsumeToken(); // consume class or category name
   Decl *ObjCImpDecl = nullptr;
 
   // Neither a type parameter list nor a list of protocol references is
   // permitted here. Parse and diagnose them.
   if (Tok.is(tok::less)) {
ObjCImpDecl = Actions.ActOnStartCategoryImplementation(
                                     categoryLoc);
 
   } else {
     // We have a class implementation
     SourceLocation superClassLoc;
     IdentifierInfo *superClassId = nullptr;
     if (TryConsumeToken(tok::colon)) {
       // We have a super class
-      if (Tok.isNot(tok::identifier)) {
-        Diag(Tok, diag::err_expected)
-            << tok::identifier; // missing super class name.
-        return nullptr;
-      }
+      if (expectIdentifier())
+        return nullptr; // missing super class name.
       superClassId = Tok.getIdentifierInfo();
       superClassLoc = ConsumeToken(); // Consume super class name
     }
     ObjCImpDecl = Actions.ActOnStartClassImplementation(
                                     AtLoc, nameId, nameLoc,
                                     superClassId, superClassLoc);
   
     if (Tok.is(tok::l_brace)) // we have ivars

 
 ///   compatibility-alias-decl:
 ///     @compatibility_alias alias-name  class-name ';'
 ///
 Decl *Parser::ParseObjCAtAliasDeclaration(SourceLocation atLoc) {
   assert(Tok.isObjCAtKeyword(tok::objc_compatibility_alias) &&
          "ParseObjCAtAliasDeclaration(): Expected @compatibility_alias");
   ConsumeToken(); // consume compatibility_alias
-  if (Tok.isNot(tok::identifier)) {
-    Diag(Tok, diag::err_expected) << tok::identifier;
+  if (expectIdentifier())
     return nullptr;
-  }
   IdentifierInfo *aliasId = Tok.getIdentifierInfo();
   SourceLocation aliasLoc = ConsumeToken(); // consume alias-name
-  if (Tok.isNot(tok::identifier)) {
-    Diag(Tok, diag::err_expected) << tok::identifier;
+  if (expectIdentifier())
     return nullptr;
-  }
   IdentifierInfo *classId = Tok.getIdentifierInfo();
   SourceLocation classLoc = ConsumeToken(); // consume class-name;
   ExpectAndConsume(tok::semi, diag::err_expected_after, "@compatibility_alias");
   return Actions.ActOnCompatibilityAlias(atLoc, aliasId, aliasLoc,
                                          classId, classLoc);
 }
 
 ///   property-synthesis:
while (true) {
     SourceLocation propertyIvarLoc;
     if (TryConsumeToken(tok::equal)) {
       // property '=' ivar-name
       if (Tok.is(tok::code_completion)) {
         Actions.CodeCompleteObjCPropertySynthesizeIvar(getCurScope(), propertyId);
         cutOffParsing();
         return nullptr;
       }
-      
-      if (Tok.isNot(tok::identifier)) {
-        Diag(Tok, diag::err_expected) << tok::identifier;
+      if (expectIdentifier())
         break;
-      }
       propertyIvar = Tok.getIdentifierInfo();
       propertyIvarLoc = ConsumeToken(); // consume ivar-name
     }
     Actions.ActOnPropertyImplDecl(
         getCurScope(), atLoc, propertyLoc, true,
         propertyId, propertyIvar, propertyIvarLoc,
         ObjCPropertyQueryKind::OBJC_PR_query_unknown);
     if (Tok.isNot(tok::comma))
Decl *Parser::ParseObjCPropertyDynamic(SourceLocation atLoc) {
   }
 
   while (true) {
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteObjCPropertyDefinition(getCurScope());
       cutOffParsing();
       return nullptr;
     }
-    
-    if (Tok.isNot(tok::identifier)) {
-      Diag(Tok, diag::err_expected) << tok::identifier;
+    if (expectIdentifier()) {
       SkipUntil(tok::semi);
       return nullptr;
     }
     
     IdentifierInfo *propertyId = Tok.getIdentifierInfo();
     SourceLocation propertyLoc = ConsumeToken(); // consume property name
     Actions.ActOnPropertyImplDecl(
         getCurScope(), atLoc, propertyLoc, false,
Parser::ParseObjCProtocolExpression(SourceLocation AtLoc) {
   SourceLocation ProtoLoc = ConsumeToken();
 
   if (Tok.isNot(tok::l_paren))
     return ExprError(Diag(Tok, diag::err_expected_lparen_after) << "@protocol");
 
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
-  if (Tok.isNot(tok::identifier))
-    return ExprError(Diag(Tok, diag::err_expected) << tok::identifier);
+  if (expectIdentifier())
+    return ExprError();
 
   IdentifierInfo *protocolId = Tok.getIdentifierInfo();
   SourceLocation ProtoIdLoc = ConsumeToken();
 
   T.consumeClose();
 
   return Actions.ParseObjCProtocolExpression(protocolId, AtLoc, ProtoLoc,
                                              T.getOpenLocation(), ProtoIdLoc,

cfe/trunk/lib/Parse/Parser.cpp

Diag(StartLoc, diag::ext_extra_semi)
                                     Actions.getASTContext().getPrintingPolicy())
         << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
   else
     // A single semicolon is valid after a member function definition.
     Diag(StartLoc, diag::warn_extra_semi_after_mem_fn_def)
       << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
 }
 
+bool Parser::expectIdentifier() {
+  if (Tok.is(tok::identifier))
+    return false;
+  if (const auto *II = Tok.getIdentifierInfo()) {
+    if (II->isCPlusPlusKeyword(getLangOpts())) {
+      Diag(Tok, diag::err_expected_token_instead_of_objcxx_keyword)
+          << tok::identifier << Tok.getIdentifierInfo();
+      // Objective-C++: Recover by treating this keyword as a valid identifier.
+      return false;
+    }
+  }
+  Diag(Tok, diag::err_expected) << tok::identifier;
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // Error recovery.
 //===----------------------------------------------------------------------===//
 
 static bool HasFlagsSet(Parser::SkipUntilFlags L, Parser::SkipUntilFlags R) {
   return (static_cast<unsigned>(L) & static_cast<unsigned>(R)) != 0;
 }
 

cfe/trunk/test/Parser/objc-cxx-keyword-identifiers.mm

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wno-objc-root-class -Wno-incomplete-implementation -triple x86_64-apple-macosx10.10.0 -verify %s
+
+// rdar://20626062
+
+struct S {
+  int throw; // expected-error {{expected member name or ';' after declaration specifiers; 'throw' is a keyword in Objective-C++}}
+};
+
+@interface class // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
+@end
+
+@interface Bar: class // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
+@end
+
+@protocol P // ok
+@end
+
+@protocol new // expected-error {{expected identifier; 'new' is a keyword in Objective-C++}}
+@end
+
+@protocol P2, delete; // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+
+@class Foo, try; // expected-error {{expected identifier; 'try' is a keyword in Objective-C++}}
+
+@interface Foo
+
+@property (readwrite, nonatomic) int a, b, throw; // expected-error {{expected member name or ';' after declaration specifiers; 'throw' is a keyword in Objective-C++}}
+
+-foo:(int)class; // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
++foo:(int)constexpr; // expected-error {{expected identifier; 'constexpr' is a keyword in Objective-C++}}
+
+@end
+
+@interface Foo () <P, new> // expected-error {{expected identifier; 'new' is a keyword in Objective-C++}}
+@end
+
+@implementation Foo
+
+@synthesize a = _a; // ok
+@synthesize b = virtual; // expected-error {{expected identifier; 'virtual' is a keyword in Objective-C++}}
+
+@dynamic throw; // expected-error {{expected identifier; 'throw' is a keyword in Objective-C++}}
+
+-foo:(int)class { // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
+}
+
+@end
+
+@implementation class // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
+@end
+
+@implementation Bar: class // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
+@end
+
+@compatibility_alias C Foo; // ok
+@compatibility_alias const_cast Bar; // expected-error {{expected identifier; 'const_cast' is a keyword in Objective-C++}}
+@compatibility_alias C2 class; // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}
+
+void func() {
+  (void)@protocol(P); // ok
+  (void)@protocol(delete); // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+}