Emit warning if define or undef reserved identifier or keyword.

[sepavloff]

Summary

This change implements warnings if macro name is identical to a keyword or
reserved identifier. The warnings are different depending on the "danger"
of the operation. Defining macro that replaces a keyword is on by default.
Other cases produce warning that is off by default but can be turned on
using option -Wreserved-id-macro.

This change fixes PR11488.

[rnk]

include/clang/Basic/DiagnosticLexKinds.td

299

Personally, I think it's confusing to have a warning group where some instances are on by default and others are off. We end up with a tri-state of default, on, off. Many users do things like clang -w -Werror -Wthing1 -Wthing2 -Wthing3 ... to explicitly use a curated set of warnings. It would be too bad if they couldn't use the on-by-default part of this warning due to lumping them into the same group.

Maybe we can split the group into -Wkeyword-macro and -Wreserved-id-macro? Does gcc have a flag name we should be using for compatibility?

lib/Basic/IdentifierTable.cpp

216

indentation is off

lib/Lex/PPDirectives.cpp

111

The LLVM naming conventions call for leading lower camel case function names, so this should be shouldWarnOnMacroDef.

117

indentation

117

We have a table lookup based isUppercase() for this. Factoring this check out as a helper gives us a good place to put this quote from the C++ standard.

133

s/Should/should/

143

Reuse this code

197

There's got to be a better way to identify builtin macros...

[sepavloff]

Addressed reviewer's notes.

[sepavloff]

Thank you for review!

include/clang/Basic/DiagnosticLexKinds.td

299

Implemented two groups, -Wkeyword-macro and -Wreserved-id-macro, the first is on by default.
Bug tracker of gcc has issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51437, which is a clone of clang PR11488. It is still in unconfirmed state. Experiments show that gcc does not emit warnings for described use.

lib/Basic/IdentifierTable.cpp

216

Fixed.

lib/Lex/PPDirectives.cpp

111

Ah, old naming. Fixed.

117

Introduced function isReservedId, which makes this check.

133

Fixed.

143

Done: calls isReservedId.

197

It seems they are identified by similar way throughout clang sources. SourceManager considers all sources as files and the only way to check if SourceLocation is in some predefined source is to check "file name".

[rnk]

According to phabricator, lib/Lex/PPDirectives.cpp got chmoded to executable. Looks good if you can revert that change.

[sepavloff]

Closed by commit rL223114 (authored by @sepavloff).

include/clang/Basic/DiagnosticGroups.td

 def Varargs : DiagGroup<"varargs">;
 
 def Unsequenced : DiagGroup<"unsequenced">;
 // GCC name for -Wunsequenced
 def : DiagGroup<"sequence-point", [Unsequenced]>;
 
 // Preprocessor warnings.
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
+def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
 
 // Just silence warnings about -Wstrict-aliasing for now.
 def : DiagGroup<"strict-aliasing=0">;
 def : DiagGroup<"strict-aliasing=1">;
 def : DiagGroup<"strict-aliasing=2">;
 def : DiagGroup<"strict-aliasing">;
 
 // Just silence warnings about -Wstrict-overflow for now.

include/clang/Basic/DiagnosticLexKinds.td

def warn_pp_undef_identifier : Warning<
   "%0 is not defined, evaluates to 0">,
   InGroup<DiagGroup<"undef">>, DefaultIgnore;
 def warn_pp_ambiguous_macro : Warning<
   "ambiguous expansion of macro %0">, InGroup<AmbiguousMacro>;
 def note_pp_ambiguous_macro_chosen : Note<
   "expanding this definition of %0">;
 def note_pp_ambiguous_macro_other : Note<
   "other definition of %0">;
+def warn_pp_macro_hides_keyword : Warning<
+  "keyword or reserved identifier is hidden by macro definition">,
+  InGroup<ReservedIdAsMacro>;
+def warn_pp_macro_is_reserved_id : Warning<
+  "macro name is a keyword or reserved identifier">, InGroup<ReservedIdAsMacro>;
+def warn_pp_defundef_reserved_ident : Warning<
+  "reserved identifier is used as macro name">, DefaultIgnore,
+  InGroup<ReservedIdAsMacro>;
 
 def pp_invalid_string_literal : Warning<
   "invalid string literal, ignoring final '\\'">;
 def warn_pp_expr_overflow : Warning<
   "integer overflow in preprocessor expression">;
 def warn_pp_convert_lhs_to_positive : Warning<
   "left side of operator converted from negative value to unsigned: %0">;
 def warn_pp_convert_rhs_to_positive : Warning<

include/clang/Basic/IdentifierTable.h

void setIsCPlusPlusOperatorKeyword(bool Val = true) {
     IsCPPOperatorKeyword = Val;
     if (Val)
       NeedsHandleIdentifier = 1;
     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);
+
   /// 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
   /// on a token of this identifier.

lib/Basic/IdentifierTable.cpp

 #define TESTING_KEYWORD(NAME, FLAGS)
 #include "clang/Basic/TokenKinds.def"
 
   if (LangOpts.ParseUnknownAnytype)
     AddKeyword("__unknown_anytype", tok::kw___unknown_anytype, KEYALL,
                LangOpts, *this);
 }
 
+/// \brief Checks if the specified token kind represents a keyword in the
+/// specified language.
+/// \returns Status of the keyword in the language.
+static KeywordStatus GetKeywordStatus(const LangOptions &LangOpts,
+  tok::TokenKind K) {
+  switch (K) {
+#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) {
+  switch (GetKeywordStatus(LangOpts, getTokenID())) {
+  case KS_Enabled:
+  case KS_Extension:
+    return true;
+  default:
+    return false;
+  }
+}
+
 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)

lib/Lex/PPDirectives.cpp

 void Preprocessor::DiscardUntilEndOfDirective() {
   Token Tmp;
   do {
     LexUnexpandedToken(Tmp);
     assert(Tmp.isNot(tok::eof) && "EOF seen while discarding directive tokens");
   } while (Tmp.isNot(tok::eod));
 }
 
+/// \brief Enumerates possible cases of #define/#undef a reserved identifier.
+enum MacroDiag {
+  MD_NoWarn,      //> Not a reserved identifier
+  MD_Keyword,     //> Macro hides keyword
+  MD_Warn,        //> Warn, on by default
+  MD_WarnStrict   //> Warn, off by default
+};
+
+static MacroDiag ShouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
+  StringRef Text = II->getName();
+  // C++ [macro.globals], C11 7.1.3: All identifiers that begin with an
+  // underscore and either an uppercase letter or another underscore are always
+  // reserved for any use.
+  if (Text.size() >= 2 && Text[0] == '_' &&
+    ((Text[1] >= 'A' && Text[1] <= 'Z') || Text[1] == '_'))
+    return MD_WarnStrict;
+  if (PP.getLangOpts().CPlusPlus) {
+    if (Text.find("__") != StringRef::npos)
+      return MD_WarnStrict;
+  }
+  if (II->isKeyword(PP.getLangOpts()))
+    return MD_Keyword;
+  // See C++11 [macro.names].
+  if (PP.getLangOpts().CPlusPlus) {
+    if (Text.equals("override") || Text.equals("final"))
+      return MD_Keyword;
+  }
+  return MD_NoWarn;
+}
+
+static MacroDiag ShouldWarnOnMacroUndef(Preprocessor &PP, IdentifierInfo *II) {
+  if (II->isKeyword(PP.getLangOpts()))
+    return MD_Warn;
+  StringRef Text = II->getName();
+  // C++11 [macro.names]
+  if (PP.getLangOpts().CPlusPlus) {
+    if (Text.equals("override") || Text.equals("final"))
+      return MD_Warn;
+  }
+  if (Text.size() >= 2 && Text[0] == '_' &&
+    ((Text[1] >= 'A' && Text[1] <= 'Z') || Text[1] == '_'))
+    return MD_WarnStrict;
+  if (PP.getLangOpts().CPlusPlus) {
+    if (Text.find("__") != StringRef::npos)
+      return MD_WarnStrict;
+  }
+  return MD_NoWarn;
+}
+
 bool Preprocessor::CheckMacroName(Token &MacroNameTok, MacroUse isDefineUndef) {
   // Missing macro name?
   if (MacroNameTok.is(tok::eod))
     return Diag(MacroNameTok, diag::err_pp_missing_macro_name);
 
   IdentifierInfo *II = MacroNameTok.getIdentifierInfo();
   if (!II) {
     bool Invalid = false;
bool Preprocessor::CheckMacroName(Token &MacroNameTok, MacroUse isDefineUndef) {
 
   if (isDefineUndef == MU_Undef && II->hasMacroDefinition() &&
       getMacroInfo(II)->isBuiltinMacro()) {
     // Warn if undefining "__LINE__" and other builtins, per C99 6.10.8/4
     // and C++ [cpp.predefined]p4], but allow it as an extension.
     Diag(MacroNameTok, diag::ext_pp_undef_builtin_macro);
   }
 
+  // Warn if defining/undefining reserved identifier including keywords.
+  SourceLocation MacroNameLoc = MacroNameTok.getLocation();
+  bool WarnOnReserved = !SourceMgr.isInSystemHeader(MacroNameLoc);
+  if (WarnOnReserved) {
+    PresumedLoc PLoc = getSourceManager().getPresumedLoc(MacroNameLoc, false);
+    WarnOnReserved = (strcmp(PLoc.getFilename(), "<built-in>") != 0);
+  }
+  if (WarnOnReserved) {
+    MacroDiag D = MD_NoWarn;
+    if (isDefineUndef == MU_Define)
+      D = ShouldWarnOnMacroDef(*this, II);
+    else if (isDefineUndef == MU_Undef)
+      D = ShouldWarnOnMacroUndef(*this, II);
+    if (D == MD_Keyword)
+      Diag(MacroNameTok, diag::warn_pp_macro_hides_keyword);
+    if (D == MD_Warn)
+      Diag(MacroNameTok, diag::warn_pp_macro_is_reserved_id);
+    else if (D == MD_WarnStrict)
+      Diag(MacroNameTok, diag::warn_pp_defundef_reserved_ident);
+  }
+
   // Okay, we got a good identifier.
   return false;
 }
 
 /// \brief Lex and validate a macro name, which occurs after a
 /// \#define or \#undef.
 ///
 /// This sets the token kind to eod and discards the rest of the macro line if

test/PCH/single-token-macro.c

 // rdar://10588825
 
 // Test this without pch.
 // RUN: %clang_cc1 %s -include %s -verify -fsyntax-only
 
 // Test with pch.
 // RUN: %clang_cc1 %s -emit-pch -o %t
 // RUN: %clang_cc1 %s -include-pch %t -verify -fsyntax-only
 
 // expected-no-diagnostics
 
 #ifndef HEADER
 #define HEADER
 
+#pragma clang diagnostic ignored "-Wreserved-id-macro"
+
 #ifdef __stdcall
 // __stdcall is defined as __attribute__((__stdcall__)) for targeting mingw32.
 #undef __stdcall
 #endif
 
 #define __stdcall
 #define STDCALL __stdcall
 

test/Preprocessor/cxx_oper_keyword_ms_compat.cpp

 // RUN: %clang_cc1 %s -E -verify -fms-extensions
 // expected-no-diagnostics
 
+#pragma clang diagnostic ignored "-Wreserved-id-macro"
+
 bool f() {
   // Check that operators still work before redefining them.
 #if compl 0 bitand 1
   return true and false;
 #endif
 }
 
 #ifdef and

test/Preprocessor/macro-reserved.c

+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+#define for 0    // expected-warning {{keyword or reserved identifier is hidden by macro definition}}
+#define final 1
+#define __HAVE_X 0
+#define _HAVE_X 0
+#define X__Y
+
+#undef inline    // expected-warning {{macro name is a keyword or reserved identifier}}
+#undef override
+#undef __cplusplus
+#undef _HAVE_X
+#undef X__Y
+
+#pragma clang diagnostic warning "-Wreserved-id-macro"
+
+#define switch if  // expected-warning {{keyword or reserved identifier is hidden by macro definition}}
+#define final 1
+#define __HAVE_X 0 // expected-warning {{reserved identifier is used as macro name}}
+#define _HAVE_X 0  // expected-warning {{reserved identifier is used as macro name}}
+#define X__Y
+
+#undef inline      // expected-warning {{macro name is a keyword or reserved identifier}}
+#undef override
+#undef __cplusplus // expected-warning {{reserved identifier is used as macro name}}
+#undef _HAVE_X     // expected-warning {{reserved identifier is used as macro name}}
+#undef X__Y
+
+int x;

test/Preprocessor/macro-reserved.cpp

+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+#define for 0    // expected-warning {{keyword or reserved identifier is hidden by macro definition}}
+#define final 1  // expected-warning {{keyword or reserved identifier is hidden by macro definition}}
+#define __HAVE_X 0
+#define _HAVE_X 0
+#define X__Y
+
+#undef inline    // expected-warning {{macro name is a keyword or reserved identifier}}
+#undef override  // expected-warning {{macro name is a keyword or reserved identifier}}
+#undef __cplusplus
+#undef _HAVE_X
+#undef X__Y
+
+#pragma clang diagnostic warning "-Wreserved-id-macro"
+
+#define switch if  // expected-warning {{keyword or reserved identifier is hidden by macro definition}}
+#define final 1    // expected-warning {{keyword or reserved identifier is hidden by macro definition}}
+#define __HAVE_X 0 // expected-warning {{reserved identifier is used as macro name}}
+#define _HAVE_X 0  // expected-warning {{reserved identifier is used as macro name}}
+#define X__Y       // expected-warning {{reserved identifier is used as macro name}}
+
+#undef inline      // expected-warning {{macro name is a keyword or reserved identifier}}
+#undef override    // expected-warning {{macro name is a keyword or reserved identifier}}
+#undef __cplusplus // expected-warning {{reserved identifier is used as macro name}}
+#undef _HAVE_X     // expected-warning {{reserved identifier is used as macro name}}
+#undef X__Y        // expected-warning {{reserved identifier is used as macro name}}
+
+int x;

test/Sema/thread-specifier.c

 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -DGNU
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DGNU
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -DC11 -D__thread=_Thread_local
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DCXX11 -D__thread=thread_local -std=c++11 -Wno-deprecated
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local -std=c++11 -Wno-deprecated
 
+#pragma clang diagnostic ignored "-Wreserved-id-macro"
+
 #ifdef __cplusplus
 // In C++, we define __private_extern__ to extern.
 #undef __private_extern__
 #endif
 
 __thread int t1;
 __thread extern int t2;
 __thread static int t3;