Introduce -Wreserved-identifier

Warn when a declaration uses an identifier that doesn't obey the reserved
identifier rule from C and/or C++.

Differential Revision: https://reviews.llvm.org/D93095

Author: serge-sans-paille

clang/docs/ReleaseNotes.rst

@@ -67,7 +67,8 @@ Non-comprehensive list of changes in this release
 New Compiler Flags
 ------------------
 
-- ...
+- ``-Wreserved-identifier`` emits warning when user code uses reserved
+  identifiers.
 
 Deprecated Compiler Flags
 -------------------------

clang/include/clang/AST/Decl.h

@@ -356,6 +356,10 @@ class NamedDecl : public Decl {
   /// a C++ class.
   bool isCXXInstanceMember() const;
 
+  /// Determine if the declaration obeys the reserved identifier rules of the
+  /// given language.
+  ReservedIdentifierStatus isReserved(const LangOptions &LangOpts) const;
+
   /// Determine what kind of linkage this entity has.
   ///
   /// This is not the linkage as defined by the standard or the codegen notion

clang/include/clang/Basic/DiagnosticGroups.td

@@ -638,7 +638,8 @@ def : DiagGroup<"sequence-point", [Unsequenced]>;
 // Preprocessor warnings.
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
 def KeywordAsMacro : DiagGroup<"keyword-macro">;
-def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
+def ReservedIdAsMacro : DiagGroup<"reserved-macro-identifier">;
+def ReservedIdAsMacroAlias : DiagGroup<"reserved-id-macro", [ReservedIdAsMacro]>;
 
 // Just silence warnings about -Wstrict-aliasing for now.
 def : DiagGroup<"strict-aliasing=0">;
@@ -801,6 +802,9 @@ def LargeByValueCopy : DiagGroup<"large-by-value-copy">;
 def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">;
 def SignedEnumBitfield : DiagGroup<"signed-enum-bitfield">;
 
+def ReservedIdentifier : DiagGroup<"reserved-identifier",
+    [ReservedIdAsMacro]>;
+
 // Unreachable code warning groups.
 //
 //  The goal is make -Wunreachable-code on by default, in -Wall, or at

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -379,6 +379,15 @@ def warn_unused_lambda_capture: Warning<"lambda capture %0 is not "
   "%select{used|required to be captured for this use}1">,
   InGroup<UnusedLambdaCapture>, DefaultIgnore;
 
+def warn_reserved_extern_symbol: Warning<
+  "identifier %0 is reserved because %select{"
+  "<ERROR>|" // ReservedIdentifierStatus::NotReserved
+  "it starts with '_' at global scope|"
+  "it starts with '__'|"
+  "it starts with '_' followed by a capital letter|"
+  "it contains '__'}1">,
+  InGroup<ReservedIdentifier>, DefaultIgnore;
+
 def warn_parameter_size: Warning<
   "%0 is a large (%1 bytes) pass-by-value argument; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;

clang/include/clang/Basic/IdentifierTable.h

@@ -40,6 +40,14 @@ class LangOptions;
 class MultiKeywordSelector;
 class SourceLocation;
 
+enum class ReservedIdentifierStatus {
+  NotReserved = 0,
+  StartsWithUnderscoreAtGlobalScope,
+  StartsWithDoubleUnderscore,
+  StartsWithUnderscoreFollowedByCapitalLetter,
+  ContainsDoubleUnderscore,
+};
+
 /// A simple pair of identifier info and location.
 using IdentifierLocPair = std::pair<IdentifierInfo *, SourceLocation>;
 
@@ -385,14 +393,7 @@ class alignas(IdentifierInfoAlignment) IdentifierInfo {
 
   /// Determine whether \p this is a name reserved for the implementation (C99
   /// 7.1.3, C++ [lib.global.names]).
-  bool isReservedName(bool doubleUnderscoreOnly = false) const {
-    if (getLength() < 2)
-      return false;
-    const char *Name = getNameStart();
-    return Name[0] == '_' &&
-           (Name[1] == '_' ||
-            (Name[1] >= 'A' && Name[1] <= 'Z' && !doubleUnderscoreOnly));
-  }
+  ReservedIdentifierStatus isReserved(const LangOptions &LangOpts) const;
 
   /// Provide less than operator for lexicographical sorting.
   bool operator<(const IdentifierInfo &RHS) const {

clang/include/clang/Sema/Sema.h

@@ -2590,6 +2590,8 @@ class Sema final {
                                           SourceLocation Less,
                                           SourceLocation Greater);
 
+  void warnOnReservedIdentifier(const NamedDecl *D);
+
   Decl *ActOnDeclarator(Scope *S, Declarator &D);
 
   NamedDecl *HandleDeclarator(Scope *S, Declarator &D,

clang/lib/AST/Decl.cpp

@@ -1078,6 +1078,29 @@ bool NamedDecl::isLinkageValid() const {
   return L == getCachedLinkage();
 }
 
+ReservedIdentifierStatus
+NamedDecl::isReserved(const LangOptions &LangOpts) const {
+  const IdentifierInfo *II = getIdentifier();
+  if (!II)
+    if (const auto *FD = dyn_cast<FunctionDecl>(this))
+      II = FD->getLiteralIdentifier();
+
+  if (!II)
+    return ReservedIdentifierStatus::NotReserved;
+
+  ReservedIdentifierStatus Status = II->isReserved(LangOpts);
+  if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope) {
+    // Check if we're at TU level or not.
+    if (isa<ParmVarDecl>(this) || isTemplateParameter())
+      return ReservedIdentifierStatus::NotReserved;
+    const DeclContext *DC = getDeclContext()->getRedeclContext();
+    if (!DC->isTranslationUnit())
+      return ReservedIdentifierStatus::NotReserved;
+  }
+
+  return Status;
+}
+
 ObjCStringFormatFamily NamedDecl::getObjCFStringFormattingFamily() const {
   StringRef name = getName();
   if (name.empty()) return SFF_None;

clang/lib/Basic/IdentifierTable.cpp

@@ -273,6 +273,39 @@ bool IdentifierInfo::isCPlusPlusKeyword(const LangOptions &LangOpts) const {
   return !isKeyword(LangOptsNoCPP);
 }
 
+ReservedIdentifierStatus
+IdentifierInfo::isReserved(const LangOptions &LangOpts) const {
+  StringRef Name = getName();
+
+  // '_' is a reserved identifier, but its use is so common (e.g. to store
+  // ignored values) that we don't warn on it.
+  if (Name.size() <= 1)
+    return ReservedIdentifierStatus::NotReserved;
+
+  // [lex.name] p3
+  if (Name[0] == '_') {
+
+    // Each name that begins with an underscore followed by an uppercase letter
+    // or another underscore is reserved.
+    if (Name[1] == '_')
+      return ReservedIdentifierStatus::StartsWithDoubleUnderscore;
+
+    if ('A' <= Name[1] && Name[1] <= 'Z')
+      return ReservedIdentifierStatus::
+          StartsWithUnderscoreFollowedByCapitalLetter;
+
+    // This is a bit misleading: it actually means it's only reserved if we're
+    // at global scope because it starts with an underscore.
+    return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+  }
+
+  // Each name that contains a double underscore (__) is reserved.
+  if (LangOpts.CPlusPlus && Name.contains("__"))
+    return ReservedIdentifierStatus::ContainsDoubleUnderscore;
+
+  return ReservedIdentifierStatus::NotReserved;
+}
+
 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

clang/lib/CodeGen/CGDebugInfo.cpp

@@ -4058,9 +4058,9 @@ void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
   if (CalleeDecl->getBuiltinID() != 0 || CalleeDecl->hasAttr<NoDebugAttr>() ||
       getCallSiteRelatedAttrs() == llvm::DINode::FlagZero)
     return;
-  if (const auto *Id = CalleeDecl->getIdentifier())
-    if (Id->isReservedName())
-      return;
+  if (CalleeDecl->isReserved(CGM.getLangOpts()) !=
+      ReservedIdentifierStatus::NotReserved)
+    return;
 
   // If there is no DISubprogram attached to the function being called,
   // create the one describing the function in order to have complete

clang/lib/Sema/SemaCodeComplete.cpp

@@ -739,18 +739,17 @@ getRequiredQualification(ASTContext &Context, const DeclContext *CurContext,
 // Filter out names reserved for the implementation if they come from a
 // system header.
 static bool shouldIgnoreDueToReservedName(const NamedDecl *ND, Sema &SemaRef) {
-  const IdentifierInfo *Id = ND->getIdentifier();
-  if (!Id)
-    return false;
-
+  ReservedIdentifierStatus Status = ND->isReserved(SemaRef.getLangOpts());
   // Ignore reserved names for compiler provided decls.
-  if (Id->isReservedName() && ND->getLocation().isInvalid())
+  if ((Status != ReservedIdentifierStatus::NotReserved) &&
+      (Status != ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope) &&
+      ND->getLocation().isInvalid())
     return true;
 
   // For system headers ignore only double-underscore names.
   // This allows for system headers providing private symbols with a single
   // underscore.
-  if (Id->isReservedName(/*doubleUnderscoreOnly=*/true) &&
+  if (Status == ReservedIdentifierStatus::StartsWithDoubleUnderscore &&
       SemaRef.SourceMgr.isInSystemHeader(
           SemaRef.SourceMgr.getSpellingLoc(ND->getLocation())))
     return true;

clang/lib/Sema/SemaDecl.cpp

@@ -1516,6 +1516,7 @@ void Sema::PushOnScopeChains(NamedDecl *D, Scope *S, bool AddToContext) {
   } else {
     IdResolver.AddDecl(D);
   }
+  warnOnReservedIdentifier(D);
 }
 
 bool Sema::isDeclInScope(NamedDecl *D, DeclContext *Ctx, Scope *S,
@@ -5558,6 +5559,18 @@ static bool RebuildDeclaratorInCurrentInstantiation(Sema &S, Declarator &D,
   return false;
 }
 
+void Sema::warnOnReservedIdentifier(const NamedDecl *D) {
+  // Avoid warning twice on the same identifier, and don't warn on redeclaration
+  // of system decl.
+  if (D->getPreviousDecl() || D->isImplicit())
+    return;
+  ReservedIdentifierStatus Status = D->isReserved(getLangOpts());
+  if (Status != ReservedIdentifierStatus::NotReserved &&
+      !Context.getSourceManager().isInSystemHeader(D->getLocation()))
+    Diag(D->getLocation(), diag::warn_reserved_extern_symbol)
+        << D << static_cast<int>(Status);
+}
+
 Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Declaration);
   Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg());

clang/lib/Sema/SemaDeclCXX.cpp

@@ -16778,6 +16778,8 @@ NamedDecl *Sema::ActOnFriendFunctionDecl(Scope *S, Declarator &D,
     }
   }
 
+  warnOnReservedIdentifier(ND);
+
   return ND;
 }
 

clang/lib/Sema/SemaStmt.cpp

@@ -542,6 +542,12 @@ Sema::ActOnLabelStmt(SourceLocation IdentLoc, LabelDecl *TheDecl,
     return SubStmt;
   }
 
+  ReservedIdentifierStatus Status = TheDecl->isReserved(getLangOpts());
+  if (Status != ReservedIdentifierStatus::NotReserved &&
+      !Context.getSourceManager().isInSystemHeader(IdentLoc))
+    Diag(IdentLoc, diag::warn_reserved_extern_symbol)
+        << TheDecl << static_cast<int>(Status);
+
   // Otherwise, things are good.  Fill in the declaration and return it.
   LabelStmt *LS = new (Context) LabelStmt(IdentLoc, TheDecl, SubStmt);
   TheDecl->setStmt(LS);

clang/lib/Sema/SemaTemplate.cpp

@@ -1677,6 +1677,9 @@ Sema::ActOnTemplateParameterList(unsigned Depth,
   if (ExportLoc.isValid())
     Diag(ExportLoc, diag::warn_template_export_unsupported);
 
+  for (NamedDecl *P : Params)
+    warnOnReservedIdentifier(P);
+
   return TemplateParameterList::Create(
       Context, TemplateLoc, LAngleLoc,
       llvm::makeArrayRef(Params.data(), Params.size()),

clang/test/Preprocessor/macro-reserved.c

@@ -46,7 +46,7 @@
 #define volatile   // expected-warning {{keyword is hidden by macro definition}}
 #undef  volatile
 
-#pragma clang diagnostic warning "-Wreserved-id-macro"
+#pragma clang diagnostic warning "-Wreserved-macro-identifier"
 
 #define switch if  // expected-warning {{keyword is hidden by macro definition}}
 #define final 1

clang/test/Preprocessor/macro-reserved.cpp

@@ -47,8 +47,7 @@
 #define volatile   // expected-warning {{keyword is hidden by macro definition}}
 #undef  volatile
 
-
-#pragma clang diagnostic warning "-Wreserved-id-macro"
+#pragma clang diagnostic warning "-Wreserved-macro-identifier"
 
 #define switch if  // expected-warning {{keyword is hidden by macro definition}}
 #define final 1

clang/test/Sema/reserved-identifier.c

@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }    // no-warning
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _foo() { return 0; }        // expected-warning {{identifier '_foo' is reserved because it starts with '_' at global scope}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =               // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+      _Reserved;                   // no-warning
+  goto __reserved;                 // expected-warning {{identifier '__reserved' is reserved because it starts with '__'}}
+__reserved: // expected-warning {{identifier '__reserved' is reserved because it starts with '__'}}
+            ;
+}
+
+void foot(unsigned int _not_reserved) {} // no-warning
+
+enum __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,     // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,     // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other      // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+struct __babar { // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+};
+
+struct _Zebulon;   // expected-warning {{identifier '_Zebulon' is reserved because it starts with '_' followed by a capital letter}}
+struct _Zebulon2 { // expected-warning {{identifier '_Zebulon2' is reserved because it starts with '_' followed by a capital letter}}
+} * p;
+struct _Zebulon3 *pp; // expected-warning {{identifier '_Zebulon3' is reserved because it starts with '_' followed by a capital letter}}
+
+typedef struct {
+  int _Field; // expected-warning {{identifier '_Field' is reserved because it starts with '_' followed by a capital letter}}
+  int _field; // no-warning
+} _Typedef;   // expected-warning {{identifier '_Typedef' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+struct _reserved { // expected-warning {{identifier '_reserved' is reserved because it starts with '_' at global scope}}
+  int a;
+} cunf(void) {
+  return (struct _reserved){1};
+}
+
+// FIXME: According to clang declaration context layering, _preserved belongs to
+// the translation unit, so we emit a warning. It's unclear that's what the
+// standard mandate, but it's such a corner case we can live with it.
+void func(struct _preserved { int a; } r) {} // expected-warning {{identifier '_preserved' is reserved because it starts with '_' at global scope}}
+
+extern char *_strdup(const char *); // expected-warning {{identifier '_strdup' is reserved because it starts with '_' at global scope}}
+
+// Don't warn on redecleration
+extern char *_strdup(const char *); // no-warning
+
+void ok() {
+  void _ko();           // expected-warning {{identifier '_ko' is reserved because it starts with '_' at global scope}}
+  extern int _ko_again; // expected-warning {{identifier '_ko_again' is reserved because it starts with '_' at global scope}}
+}