Add support for attribute "enum_extensibility"

[ahatanak]

Summary

This patch adds support for a new attribute that will be used to distinguish between extensible and inextensible enums. The attribute was discussed on cfe-dev a few weeks ago and I've made a few modifications to the original proposal based on the feedback I received.

http://lists.llvm.org/pipermail/cfe-dev/2017-February/052748.html

In this patch, I didn't include the command line option for setting the default enum style for unannotated enums since I wasn't sure it's something we absolutely need. If it turns the warnings are too noisy, I'll consider adding a command line option for silencing the false positive warnings in a follow-up patch.

[arphaman]

include/clang/Basic/Attr.td

887 ↗

(On Diff #91090)

We might as well have a C++11 spelling in the clang namespace as well.

test/Sema/enum-attr.c

27 ↗

(On Diff #91090)

Should this be an error instead?

[ahatanak]

Address Alex's review comments.

test/Sema/enum-attr.c

27 ↗

(On Diff #91090)

Yes, I agree.

[aaron.ballman]

include/clang/Basic/AttrDocs.td

1969 ↗

(On Diff #91577)

Breaking this up into multiple lines would be appreciated (we aren't strict about the 80 col limit in this file, but it should be readable still).

Also, adding some examples would be appreciated.

include/clang/Basic/DiagnosticSemaKinds.td

2383–2384 ↗

(On Diff #91577)

I'd rather see this defined next to warn_attribute_type_not_supported as def err_attribute_type_not_supported : Error<warn_attribute_type_not_supported.Text>;

test/Sema/enum-attr.c

27 ↗

(On Diff #91090)

I'm not opposed to it, but we do treat it as a warning for every other attribute (and just ignore the attribute), FWIW.

test/SemaCXX/enum-attr.cpp

108 ↗

(On Diff #91577)

Missing tests for the attribute being applied to something other than an enum, or taking too few/many arguments.

[ahatanak]

Address review comments from Aaron.

test/Sema/enum-attr.c

27 ↗

(On Diff #91090)

Do you know the reason we treat wrong attribute arguments as a warning in other cases rather than an error?

I'm guessing a warning is preferred because in most cases dropping an attribute doesn't affect correctness (it doesn't cause miscompiles).

[aaron.ballman]

Aside from the request for a FIXME and a decision from the author on error vs warning, the code LGTM. Feel free to make a decision and commit without further review.

test/Sema/enum-attr.c

31 ↗

(On Diff #91782)

That's a rather unfortunate diagnostic. I would have expected this to mention that it only takes one argument as in the zero arg case. I'm not certain that this is the fault of your patch, however. Can you at least add a FIXME to this test that mentions this diagnostic could be improved?

27 ↗

(On Diff #91090)

You are correct. The general rule is to use warnings when an incorrect attribute still has no impact on the code generated for the user, and use an error otherwise. We're not always perfect about this, which is why I'm not opposed, but since this involves adding an entirely new diagnostic, I figured it might be worth mentioning that there's really no reason for this to be an error that prevents the user's source code from being translated either.

[ahatanak]

test/Sema/enum-attr.c

31 ↗

(On Diff #91782)

It's possible to implement a parser function in ParseDecl.cpp specifically for enum_extensibility to improve the diagnostic, but I think ParseAttributeArgsCommon should be able to check the number of arguments and emit a proper diagnostic. I've added a FIXME to the test for now.

It looks like ParseAttributeArgsCommon currently assumes that the first argument can be an identifier but it tries to parse the remaining arguments as expressions. I think it's possible to make it check the number of arguments provided and parse each argument according to its type.

27 ↗

(On Diff #91090)

OK. I don't think we have a strong reason to error out here so I think it's better to follow convention and issue a warning.

cfe/trunk/include/clang/AST/Decl.h

bool isFixed() const {
     return IsFixed;
   }
 
   /// \brief Returns true if this can be considered a complete type.
   bool isComplete() const {
     return isCompleteDefinition() || isFixed();
   }
 
+  /// Returns true if this enum is either annotated with
+  /// enum_extensibility(closed) or isn't annotated with enum_extensibility.
+  bool isClosed() const;
+
+  /// Returns true if this enum is annotated with flag_enum and isn't annotated
+  /// with enum_extensibility(open).
+  bool isClosedFlag() const;
+
+  /// Returns true if this enum is annotated with neither flag_enum nor
+  /// enum_extensibility(open).
+  bool isClosedNonFlag() const;
+
   /// \brief Retrieve the enum definition from which this enumeration could
   /// be instantiated, if it is an instantiation (rather than a non-template).
   EnumDecl *getTemplateInstantiationPattern() const;
 
   /// \brief Returns the enumeration (declared within the template)
   /// from which this enumeration type was instantiated, or NULL if
   /// this enumeration was not instantiated from any template.
   EnumDecl *getInstantiatedFromMemberEnum() const;

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

def MinSize : InheritableAttr {
   let Subjects = SubjectList<[Function, ObjCMethod], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def FlagEnum : InheritableAttr {
   let Spellings = [GNU<"flag_enum">];
   let Subjects = SubjectList<[Enum]>;
   let Documentation = [FlagEnumDocs];
-  let LangOpts = [COnly];
+}
+
+def EnumExtensibility : InheritableAttr {
+  let Spellings = [GNU<"enum_extensibility">,
+                   CXX11<"clang", "enum_extensibility">];
+  let Subjects = SubjectList<[Enum]>;
+  let Args = [EnumArgument<"Extensibility", "Kind",
+              ["closed", "open"], ["Closed", "Open"]>];
+  let Documentation = [EnumExtensibilityDocs];
 }
 
 def Flatten : InheritableAttr {
   let Spellings = [GCC<"flatten">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [FlattenDocs];
 }
 

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

def FlagEnumDocs : Documentation {
   let Content = [{
 This attribute can be added to an enumerator to signal to the compiler that it
 is intended to be used as a flag type. This will cause the compiler to assume
 that the range of the type includes all of the values that you can get by
 manipulating bits of the enumerator when issuing warnings.
   }];
 }
 
+def EnumExtensibilityDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+Attribute ``enum_extensibility`` is used to distinguish between enum definitions
+that are extensible and those that are not. The attribute can take either
+``closed`` or ``open`` as an argument. ``closed`` indicates a variable of the
+enum type takes a value that corresponds to one of the enumerators listed in the
+enum definition or, when the enum is annotated with ``flag_enum``, a value that
+can be constructed using values corresponding to the enumerators. ``open``
+indicates a variable of the enum type can take any values allowed by the
+standard and instructs clang to be more lenient when issuing warnings.
+
+.. code-block:: c
+
+  enum __attribute__((enum_extensibility(closed))) ClosedEnum {
+    A0, A1
+  };
+
+  enum __attribute__((enum_extensibility(open))) OpenEnum {
+    B0, B1
+  };
+
+  enum __attribute__((enum_extensibility(closed),flag_enum)) ClosedFlagEnum {
+    C0 = 1 << 0, C1 = 1 << 1
+  };
+
+  enum __attribute__((enum_extensibility(open),flag_enum)) OpenFlagEnum {
+    D0 = 1 << 0, D1 = 1 << 1
+  };
+
+  void foo1() {
+    enum ClosedEnum ce;
+    enum OpenEnum oe;
+    enum ClosedFlagEnum cfe;
+    enum OpenFlagEnum ofe;
+
+    ce = A1;           // no warnings
+    ce = 100;          // warning issued
+    oe = B1;           // no warnings
+    oe = 100;          // no warnings
+    cfe = C0 | C1;     // no warnings
+    cfe = C0 | C1 | 4; // warning issued
+    ofe = D0 | D1;     // no warnings
+    ofe = D0 | D1 | 4; // no warnings
+  }
+
+  }];
+}
+
 def EmptyBasesDocs : Documentation {
   let Category = DocCatType;
   let Content = [{
 The empty_bases attribute permits the compiler to utilize the
 empty-base-optimization more frequently.
 This attribute only applies to struct, class, and union types.
 It is only supported when using the Microsoft C++ ABI.
   }];

cfe/trunk/lib/AST/Decl.cpp

void EnumDecl::completeDefinition(QualType NewType,
   if (!IntegerType)
     IntegerType = NewType.getTypePtr();
   PromotionType = NewPromotionType;
   setNumPositiveBits(NumPositiveBits);
   setNumNegativeBits(NumNegativeBits);
   TagDecl::completeDefinition();
 }
 
+bool EnumDecl::isClosed() const {
+  if (const auto *A = getAttr<EnumExtensibilityAttr>())
+    return A->getExtensibility() == EnumExtensibilityAttr::Closed;
+  return true;
+}
+
+bool EnumDecl::isClosedFlag() const {
+  return isClosed() && hasAttr<FlagEnumAttr>();
+}
+
+bool EnumDecl::isClosedNonFlag() const {
+  return isClosed() && !hasAttr<FlagEnumAttr>();
+}
+
 TemplateSpecializationKind EnumDecl::getTemplateSpecializationKind() const {
   if (MemberSpecializationInfo *MSI = getMemberSpecializationInfo())
     return MSI->getTemplateSpecializationKind();
 
   return TSK_Undeclared;
 }
 
 void EnumDecl::setTemplateSpecializationKind(TemplateSpecializationKind TSK,

cfe/trunk/lib/Sema/SemaDecl.cpp

for (ECDVector::iterator E = Vec->end(); I != E; ++I)
         << (*I)->getName() << (*I)->getInitVal().toString(10)
         << (*I)->getSourceRange();
     delete Vec;
   }
 }
 
 bool Sema::IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
                              bool AllowMask) const {
-  assert(ED->hasAttr<FlagEnumAttr>() && "looking for value in non-flag enum");
+  assert(ED->isClosedFlag() && "looking for value in non-flag or open enum");
   assert(ED->isCompleteDefinition() && "expected enum definition");
 
   auto R = FlagBitsCache.insert(std::make_pair(ED, llvm::APInt()));
   llvm::APInt &FlagBits = R.first->second;
 
   if (R.second) {
     for (auto *E : ED->enumerators()) {
       const auto &EVal = E->getInitVal();
else
       ECD->setType(NewTy);
   }
 
   Enum->completeDefinition(BestType, BestPromotionType,
                            NumPositiveBits, NumNegativeBits);
 
   CheckForDuplicateEnumValues(*this, Elements, Enum, EnumType);
 
-  if (Enum->hasAttr<FlagEnumAttr>()) {
+  if (Enum->isClosedFlag()) {
     for (Decl *D : Elements) {
       EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(D);
       if (!ECD) continue;  // Already issued a diagnostic.
 
       llvm::APSInt InitVal = ECD->getInitVal();
       if (InitVal != 0 && !InitVal.isPowerOf2() &&
           !IsValueInFlagEnum(Enum, InitVal, true))
         Diag(ECD->getLocation(), diag::warn_flag_enum_constant_out_of_range)

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

if (S.CheckAssignmentConstraints(FD->getParamDecl(0)->getLocation(),
     return;
   }
 
   D->addAttr(::new (S.Context)
              CleanupAttr(Attr.getRange(), S.Context, FD,
                          Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleEnumExtensibilityAttr(Sema &S, Decl *D,
+                                        const AttributeList &Attr) {
+  if (!Attr.isArgIdent(0)) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
+        << Attr.getName() << 0 << AANT_ArgumentIdentifier;
+    return;
+  }
+
+  EnumExtensibilityAttr::Kind ExtensibilityKind;
+  IdentifierInfo *II = Attr.getArgAsIdent(0)->Ident;
+  if (!EnumExtensibilityAttr::ConvertStrToKind(II->getName(),
+                                               ExtensibilityKind)) {
+    S.Diag(Attr.getLoc(), diag::warn_attribute_type_not_supported)
+        << Attr.getName() << II;
+    return;
+  }
+
+  D->addAttr(::new (S.Context) EnumExtensibilityAttr(
+      Attr.getRange(), S.Context, ExtensibilityKind,
+      Attr.getAttributeSpellingListIndex()));
+}
+
 /// Handle __attribute__((format_arg((idx)))) attribute based on
 /// http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
 static void handleFormatArgAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   Expr *IdxExpr = Attr.getArgAsExpr(0);
   uint64_t Idx;
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, IdxExpr, Idx))
     return;
 
case AttributeList::AT_MinSize:
     handleMinSizeAttr(S, D, Attr);
     break;
   case AttributeList::AT_OptimizeNone:
     handleOptimizeNoneAttr(S, D, Attr);
     break;
   case AttributeList::AT_FlagEnum:
     handleSimpleAttribute<FlagEnumAttr>(S, D, Attr);
     break;
+  case AttributeList::AT_EnumExtensibility:
+    handleEnumExtensibilityAttr(S, D, Attr);
+    break;
   case AttributeList::AT_Flatten:
     handleSimpleAttribute<FlattenAttr>(S, D, Attr);
     break;
   case AttributeList::AT_Format:
     handleFormatAttr(S, D, Attr);
     break;
   case AttributeList::AT_FormatArg:
     handleFormatArgAttr(S, D, Attr);

cfe/trunk/lib/Sema/SemaStmt.cpp

 /// Returns true if we should emit a diagnostic about this case expression not
 /// being a part of the enum used in the switch controlling expression.
 static bool ShouldDiagnoseSwitchCaseNotInEnum(const Sema &S,
                                               const EnumDecl *ED,
                                               const Expr *CaseExpr,
                                               EnumValsTy::iterator &EI,
                                               EnumValsTy::iterator &EIEnd,
                                               const llvm::APSInt &Val) {
+  if (!ED->isClosed())
+    return false;
+
   if (const DeclRefExpr *DRE =
           dyn_cast<DeclRefExpr>(CaseExpr->IgnoreParenImpCasts())) {
     if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
       QualType VarType = VD->getType();
       QualType EnumType = S.Context.getTypeDeclType(ED);
       if (VD->hasGlobalStorage() && VarType.isConstQualified() &&
           S.Context.hasSameUnqualifiedType(EnumType, VarType))
         return false;
     }
   }
 
-  if (ED->hasAttr<FlagEnumAttr>()) {
+  if (ED->hasAttr<FlagEnumAttr>())
     return !S.IsValueInFlagEnum(ED, Val, false);
-  } else {
-    while (EI != EIEnd && EI->first < Val)
-      EI++;
+
 
-    if (EI != EIEnd && EI->first == Val)
-      return false;
-  }
 
   return true;
 }
 
 StmtResult
 Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
                             Stmt *BodyStmt) {
   SwitchStmt *SS = cast<SwitchStmt>(Switch);
if (!CaseListIsErroneous && !HasConstantCond && ET &&
         }
 
         if (RI == CaseRanges.end() || EI->first < RI->first) {
           hasCasesNotInSwitch = true;
           UnhandledNames.push_back(EI->second->getDeclName());
         }
       }
 
-      if (TheDefaultStmt && UnhandledNames.empty())
+      if (TheDefaultStmt && UnhandledNames.empty() && ED->isClosedNonFlag())
         Diag(TheDefaultStmt->getDefaultLoc(), diag::warn_unreachable_default);
 
       // Produce a nice diagnostic if multiple values aren't handled.
       if (!UnhandledNames.empty()) {
         DiagnosticBuilder DB = Diag(CondExpr->getExprLoc(),
                                     TheDefaultStmt ? diag::warn_def_missing_case
                                                    : diag::warn_missing_case)
                                << (int)UnhandledNames.size();
if (!Context.hasSameUnqualifiedType(SrcType, DstType) &&
         // Get the bitwidth of the enum value before promotions.
         unsigned DstWidth = Context.getIntWidth(DstType);
         bool DstIsSigned = DstType->isSignedIntegerOrEnumerationType();
 
         llvm::APSInt RhsVal = SrcExpr->EvaluateKnownConstInt(Context);
         AdjustAPSInt(RhsVal, DstWidth, DstIsSigned);
         const EnumDecl *ED = ET->getDecl();
 
+        if (!ED->isClosed())
+          return;
+
         if (ED->hasAttr<FlagEnumAttr>()) {
           if (!IsValueInFlagEnum(ED, RhsVal, true))
             Diag(SrcExpr->getExprLoc(), diag::warn_not_in_enum_assignment)
               << DstType.getUnqualifiedType();
         } else {
           typedef SmallVector<std::pair<llvm::APSInt, EnumConstantDecl *>, 64>
               EnumValsTy;
           EnumValsTy EnumVals;

cfe/trunk/test/Sema/enum-attr.c

+// RUN: %clang_cc1 -fsyntax-only -verify -Wassign-enum -Wswitch-enum -Wcovered-switch-default %s
+
+enum Enum {
+  A0 = 1, A1 = 10
+};
+
+enum __attribute__((enum_extensibility(closed))) EnumClosed {
+  B0 = 1, B1 = 10
+};
+
+enum __attribute__((enum_extensibility(open))) EnumOpen {
+  C0 = 1, C1 = 10
+};
+
+enum __attribute__((flag_enum)) EnumFlag {
+  D0 = 1, D1 = 8
+};
+
+enum __attribute__((flag_enum,enum_extensibility(closed))) EnumFlagClosed {
+  E0 = 1, E1 = 8
+};
+
+enum __attribute__((flag_enum,enum_extensibility(open))) EnumFlagOpen {
+  F0 = 1, F1 = 8
+};
+
+enum __attribute__((enum_extensibility(arg1))) EnumInvalidArg { // expected-warning{{'enum_extensibility' attribute argument not supported: 'arg1'}}
+  X0
+};
+
+// FIXME: The warning should mention that enum_extensibility takes only one argument.
+enum __attribute__((enum_extensibility(closed,open))) EnumTooManyArgs { // expected-error{{use of undeclared identifier 'open'}}
+  X1
+};
+
+enum __attribute__((enum_extensibility())) EnumTooFewArgs { // expected-error{{'enum_extensibility' attribute takes one argument}}
+  X2
+};
+
+struct __attribute__((enum_extensibility(open))) S { // expected-warning{{'enum_extensibility' attribute only applies to enums}}{
+};
+
+void test() {
+  enum Enum t0 = 100; // expected-warning{{integer constant not in range of enumerated type}}
+  t0 = 1;
+
+  switch (t0) { // expected-warning{{enumeration value 'A1' not handled in switch}}
+  case A0: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t0) {
+  case A0: break;
+  case A1: break;
+  default: break; // expected-warning{{default label in switch which covers all enumeration}}
+  }
+
+  enum EnumClosed t1 = 100; // expected-warning{{integer constant not in range of enumerated type}}
+  t1 = 1;
+
+  switch (t1) { // expected-warning{{enumeration value 'B1' not handled in switch}}
+  case B0: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t1) {
+  case B0: break;
+  case B1: break;
+  default: break; // expected-warning{{default label in switch which covers all enumeration}}
+  }
+
+  enum EnumOpen t2 = 100;
+  t2 = 1;
+
+  switch (t2) { // expected-warning{{enumeration value 'C1' not handled in switch}}
+  case C0: break;
+  case 16: break;
+  }
+
+  switch (t2) {
+  case C0: break;
+  case C1: break;
+  default: break;
+  }
+
+  enum EnumFlag t3 = 5; // expected-warning{{integer constant not in range of enumerated type}}
+  t3 = 9;
+
+  switch (t3) { // expected-warning{{enumeration value 'D1' not handled in switch}}
+  case D0: break;
+  case 9: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t3) {
+  case D0: break;
+  case D1: break;
+  default: break;
+  }
+
+  enum EnumFlagClosed t4 = 5; // expected-warning{{integer constant not in range of enumerated type}}
+  t4 = 9;
+
+  switch (t4) { // expected-warning{{enumeration value 'E1' not handled in switch}}
+  case E0: break;
+  case 9: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t4) {
+  case E0: break;
+  case E1: break;
+  default: break;
+  }
+
+  enum EnumFlagOpen t5 = 5;
+  t5 = 9;
+
+  switch (t5) { // expected-warning{{enumeration value 'F1' not handled in switch}}
+  case F0: break;
+  case 9: break;
+  case 16: break;
+  }
+
+  switch (t5) {
+  case F0: break;
+  case F1: break;
+  default: break;
+  }
+}

cfe/trunk/test/SemaCXX/attr-flag-enum-reject.cpp

-// RUN: %clang_cc1 -verify -fsyntax-only -x c++ -Wassign-enum %s
-
-enum __attribute__((flag_enum)) flag { // expected-warning {{ignored}}
-};

cfe/trunk/test/SemaCXX/enum-attr.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wassign-enum -Wswitch-enum -Wcovered-switch-default -std=c++11 %s
+
+enum Enum {
+  A0 = 1, A1 = 10
+};
+
+enum __attribute__((enum_extensibility(closed))) EnumClosed {
+  B0 = 1, B1 = 10
+};
+
+enum [[clang::enum_extensibility(open)]] EnumOpen {
+  C0 = 1, C1 = 10
+};
+
+enum __attribute__((flag_enum)) EnumFlag {
+  D0 = 1, D1 = 8
+};
+
+enum __attribute__((flag_enum,enum_extensibility(closed))) EnumFlagClosed {
+  E0 = 1, E1 = 8
+};
+
+enum __attribute__((flag_enum,enum_extensibility(open))) EnumFlagOpen {
+  F0 = 1, F1 = 8
+};
+
+void test() {
+  enum Enum t0;
+
+  switch (t0) { // expected-warning{{enumeration value 'A1' not handled in switch}}
+  case A0: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t0) {
+  case A0: break;
+  case A1: break;
+  default: break; // expected-warning{{default label in switch which covers all enumeration}}
+  }
+
+  enum EnumClosed t1;
+
+  switch (t1) { // expected-warning{{enumeration value 'B1' not handled in switch}}
+  case B0: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t1) {
+  case B0: break;
+  case B1: break;
+  default: break; // expected-warning{{default label in switch which covers all enumeration}}
+  }
+
+  enum EnumOpen t2;
+
+  switch (t2) { // expected-warning{{enumeration value 'C1' not handled in switch}}
+  case C0: break;
+  case 16: break;
+  }
+
+  switch (t2) {
+  case C0: break;
+  case C1: break;
+  default: break;
+  }
+
+  enum EnumFlag t3;
+
+  switch (t3) { // expected-warning{{enumeration value 'D1' not handled in switch}}
+  case D0: break;
+  case 9: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t3) {
+  case D0: break;
+  case D1: break;
+  default: break;
+  }
+
+  enum EnumFlagClosed t4;
+
+  switch (t4) { // expected-warning{{enumeration value 'E1' not handled in switch}}
+  case E0: break;
+  case 9: break;
+  case 16: break; // expected-warning{{case value not in enumerated type}}
+  }
+
+  switch (t4) {
+  case E0: break;
+  case E1: break;
+  default: break;
+  }
+
+  enum EnumFlagOpen t5;
+
+  switch (t5) { // expected-warning{{enumeration value 'F1' not handled in switch}}
+  case F0: break;
+  case 9: break;
+  case 16: break;
+  }
+
+  switch (t5) {
+  case F0: break;
+  case F1: break;
+  default: break;
+  }
+}