[Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements

[rnkovacs]

Summary

-Wenum-compare warns if two values with different enumeration types are compared in expressions with binary operators. This patch extends this diagnostic so that comparisons of mixed enumeration types are recognized in switch statements as well.

Example:

enum MixedA { A1, A2, A3 };
enum MixedB { B1, B2, B3 };

void MixedEnums(MixedA a) {
  switch (a) {
    case A1: break; // OK, same enum types.
    case B1: break; // Warn, different enum types.
  }
}

[aaron.ballman]

Can you generate the updated patch with more context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)?

lib/Sema/SemaStmt.cpp

746 ↗

(On Diff #110019)

Cond and Case can be declared as const pointers.

758 ↗

(On Diff #110019)

You can lower this in to the diagnostic.

[rnkovacs]

Uploaded the full diff and addressed comments. Added const qualifiers to GetTypeBeforeIntegralPromotion() function.

[aaron.ballman]

Aside from a minor naming nit, LGTM!

lib/Sema/SemaStmt.cpp

605–608 ↗

(On Diff #110219)

Since you're touching the code -- can you change the names to E (or some other, more descriptive name), Cleanups and Impcast to meet the coding standards?

cfe/trunk/lib/Sema/SemaStmt.cpp

 static bool EqEnumVals(const std::pair<llvm::APSInt, EnumConstantDecl*>& lhs,
                        const std::pair<llvm::APSInt, EnumConstantDecl*>& rhs)
 {
   return lhs.first == rhs.first;
 }
 
 /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of
 /// potentially integral-promoted expression @p expr.
-static QualType GetTypeBeforeIntegralPromotion(Expr *&expr) {
-  if (ExprWithCleanups *cleanups = dyn_cast<ExprWithCleanups>(expr))
-    expr = cleanups->getSubExpr();
-  while (ImplicitCastExpr *impcast = dyn_cast<ImplicitCastExpr>(expr)) {
-    if (impcast->getCastKind() != CK_IntegralCast) break;
-    expr = impcast->getSubExpr();
+static QualType GetTypeBeforeIntegralPromotion(const Expr *&E) {
+  if (const auto *CleanUps = dyn_cast<ExprWithCleanups>(E))
+    E = CleanUps->getSubExpr();
+  while (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
+    if (ImpCast->getCastKind() != CK_IntegralCast) break;
+    E = ImpCast->getSubExpr();
   }
-  return expr->getType();
+  return E->getType();
 }
 
 ExprResult Sema::CheckSwitchCondition(SourceLocation SwitchLoc, Expr *Cond) {
   class SwitchConvertDiagnoser : public ICEConvertDiagnoser {
     Expr *Cond;
 
   public:
     SwitchConvertDiagnoser(Expr *Cond)
while (EI != EIEnd && EI->first < Val)
     EI++;
 
   if (EI != EIEnd && EI->first == Val)
     return false;
 
   return true;
 }
 
+static void checkEnumTypesInSwitchStmt(Sema &S, const Expr *Cond,
+                                       const Expr *Case) {
+  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
+  QualType CaseType = Case->getType();
+
+  const EnumType *CondEnumType = CondType->getAs<EnumType>();
+  const EnumType *CaseEnumType = CaseType->getAs<EnumType>();
+  if (!CondEnumType || !CaseEnumType)
+    return;
+
+  if (S.Context.hasSameUnqualifiedType(CondType, CaseType))
+    return;
+
+  S.Diag(Case->getExprLoc(), diag::warn_comparison_of_mixed_enum_types)
+      << CondType << CaseType << Cond->getSourceRange()
+      << Case->getSourceRange();
+}
+
 StmtResult
 Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
                             Stmt *BodyStmt) {
   SwitchStmt *SS = cast<SwitchStmt>(Switch);
   assert(SS == getCurFunction()->SwitchStack.back() &&
          "switch stack missing push/pop!");
 
   getCurFunction()->SwitchStack.pop_back();
 
   if (!BodyStmt) return StmtError();
   SS->setBody(BodyStmt, SwitchLoc);
 
   Expr *CondExpr = SS->getCond();
   if (!CondExpr) return StmtError();
 
   QualType CondType = CondExpr->getType();
 
-  Expr *CondExprBeforePromotion = CondExpr;
+  const Expr *CondExprBeforePromotion = CondExpr;
   QualType CondTypeBeforePromotion =
       GetTypeBeforeIntegralPromotion(CondExprBeforePromotion);
 
   // C++ 6.4.2.p2:
   // Integral promotions are performed (on the switch condition).
   //
   // A case value unrepresentable by the original switch condition
   // type (before the promotion) doesn't make sense, even when it can
if (DefaultStmt *DS = dyn_cast<DefaultStmt>(SC)) {
 
       Expr *Lo = CS->getLHS();
 
       if (Lo->isTypeDependent() || Lo->isValueDependent()) {
         HasDependentValue = true;
         break;
       }
 
+      checkEnumTypesInSwitchStmt(*this, CondExpr, Lo);
+
       llvm::APSInt LoVal;
 
       if (getLangOpts().CPlusPlus11) {
         // C++11 [stmt.switch]p2: the constant-expression shall be a converted
         // constant expression of the promoted type of the switch condition.
         ExprResult ConvLo =
           CheckConvertedConstantExpression(Lo, CondType, LoVal, CCEK_CaseValue);
         if (ConvLo.isInvalid()) {

cfe/trunk/test/Sema/switch.c

 static const enum ExtendedEnum1_unrelated EE1_d = 101;
 
 void switch_on_ExtendedEnum1(enum ExtendedEnum1 e) {
   switch(e) {
   case EE1_a: break;
   case EE1_b: break;
   case EE1_c: break; // no-warning
   case EE1_d: break; // expected-warning {{case value not in enumerated type 'enum ExtendedEnum1'}}
+  // expected-warning@-1 {{comparison of two values with different enumeration types ('enum ExtendedEnum1' and 'const enum ExtendedEnum1_unrelated')}}
   }
 }
 
 void PR11778(char c, int n, long long ll) {
   // Do not reject this; we don't have duplicate case values because we
   // check for duplicates in the promoted type.
   switch (c) case 1: case 257: ; // expected-warning {{overflow}}
 
   switch (n) case 0x100000001LL: case 1: ; // expected-warning {{overflow}} expected-error {{duplicate}} expected-note {{previous}}
   switch ((int)ll) case 0x100000001LL: case 1: ; // expected-warning {{overflow}} expected-error {{duplicate}} expected-note {{previous}}
   switch ((long long)n) case 0x100000001LL: case 1: ;
   switch (ll) case 0x100000001LL: case 1: ;
 }

cfe/trunk/test/SemaCXX/warn-enum-compare.cpp

void test () {
 
   while (getBar() == x); // expected-warning  {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
   while (getBar() != x); // expected-warning  {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
   while (getBar() >= x); // expected-warning  {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
   while (getBar() <= x); // expected-warning  {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
   while (getBar() > x); // expected-warning  {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
   while (getBar() < x); // expected-warning  {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
 
+  switch (a) {
+    case name1::F1: break;
+    case name1::F3: break;
+    case name2::B2: break; // expected-warning {{comparison of two values with different enumeration types ('name1::Foo' and 'name2::Baz')}}
+  }
+
+  switch (x) {
+    case FooB: break;
+    case FooC: break;
+    case BarD: break; // expected-warning {{comparison of two values with different enumeration types ('Foo' and 'Bar')}}
+  }
+
+  switch(getBar()) {
+    case BarE: break;
+    case BarF: break;
+    case FooA: break; // expected-warning {{comparison of two values with different enumeration types ('Bar' and 'Foo')}}
+  }
 }