Teach -Wtautological-overlap-compare about enums

[george.burgess.iv]

Summary

Currently, -Wtautological-overlap-compare only emits warnings if the comparisons are between integer literals and variables. This patch adds support for comparison between variables and enums if the user's intent seems moderately obvious.

Richard -- I chose you for review because it looks like you touched the code last. If you're busy, I'm happy to petition others :)

[aaron.ballman]

Thank you for working on this -- I think it's a good cleanup and feature-add! I have a few minor comments, but generally LGTM. You should wait for an okay from Richard, however.

lib/Analysis/CFG.cpp

54

Please don't compare a pointer against nullptr with an equality operator. This can be simplified into:

if (const auto *DR = dyn_cast<>)
  return isa<> ? "
return nullptr;

97

Are you sure that A and B will only ever be DeclRefExprs? You dyn_cast elsewhere.

[george.burgess.iv]

Addressed all feedback

[george.burgess.iv]

lib/Analysis/CFG.cpp

54

Thanks for catching that!

97

I'm 100% sure if my code matches my intent exactly. ;)

This helper exists solely to make checkIncorrectLogicOperator a bit cleaner -- by the time we call it there, we can assume tryNormalizeBinaryOperator succeeded on both BinOps, and tryNormalizeBinaryOperator only returns DeclRefExprs (that contain EnumConstantDecls) or IntegerLiterals.

I see that this is unclear though, so I added a line to the function comment to make this constraint more explicit. If you think it would be better, I can also just manually inline this code in checkIncorrectLogicOperator.

[aaron.ballman]

Thanks, I think this looks good (pending confirmation from Richard).

~Aaron

[rtrieu]

Fix the comments, then it should be ready to commit.

lib/Analysis/CFG.cpp

49

Why is this a lambda instead of a helper function?

88

Use E1 and E2 instead of A and B to match the naming of decls below.

98

Change the comment to note that IntegerLiterals are handled above and only EnumConstantDecls are expected beyond this point.

99–104

There's a few extra casts in here, plus some blind conversions between types. Add your assumptions for the types in asserts. Also, DeclContext should use cast<> to get to Decl types. I recommend the following:

assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2));
auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();
auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();

assert(isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2));
const DeclContext *DC1 = Decl1->getDeclContext();
const DeclContext *DC2 = Decl2->getDeclContext();

assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(DC2));
return DC1 == DC2;

[george.burgess.iv]

Changed code to address all feedback + committed as r249053. Thanks for the reviews!

lib/Analysis/CFG.cpp

49

Because it's small + super specific to tryNormalizeBinaryOperator, and making it a lambda lets me scope it it only to where it's useful. It's now a helper function. :)

99–104

I was under the impression that the cast<Foo>(Bar) asserts isa<Foo>(Bar) for me, so I thought that asserts like those would just be redundant. Changed to your version anyway :)

[rtrieu]

Next time, add

Differential Revision: <URL>

to your commit and Phabricator will close the diff automatically.

http://llvm.org/docs/Phabricator.html

lib/Analysis/CFG.cpp

99–104

You are correct, 'cast<Foo>(Bar)' does assert 'isa<Foo>(Bar)'. However, when Bar is not Foo, using the assert here means the crash will produce a backtrace will point straight to this function instead of an assert that points deep into the casting functions.

lib/Analysis/CFG.cpp

 
 static SourceLocation GetEndLoc(Decl *D) {
   if (VarDecl *VD = dyn_cast<VarDecl>(D))
     if (Expr *Ex = VD->getInit())
       return Ex->getSourceRange().getEnd();
   return D->getLocation();
 }
 
+/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
+/// an integer literal or an enum constant.
+///
+/// If this fails, at least one of the returned DeclRefExpr or Expr will be
+/// null.
+static std::tuple<const DeclRefExpr *, BinaryOperatorKind, const Expr *>
+tryNormalizeBinaryOperator(const BinaryOperator *B) {
+  auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+    E = E->IgnoreParens();
+    if (isa<IntegerLiteral>(E))
+      return E;
+    if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
+      return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
+    return nullptr;
+  };
+
+  BinaryOperatorKind Op = B->getOpcode();
+
+  const Expr *MaybeDecl = B->getLHS();
+  const Expr *Constant = TryTransformToIntOrEnumConstant(B->getRHS());
+  // Expr looked like `0 == Foo` instead of `Foo == 0`
+  if (Constant == nullptr) {
+    // Flip the operator
+    if (Op == BO_GT)
+      Op = BO_LT;
+    else if (Op == BO_GE)
+      Op = BO_LE;
+    else if (Op == BO_LT)
+      Op = BO_GT;
+    else if (Op == BO_LE)
+      Op = BO_GE;
+
+    MaybeDecl = B->getRHS();
+    Constant = TryTransformToIntOrEnumConstant(B->getLHS());
+  }
+
+  auto *D = dyn_cast<DeclRefExpr>(MaybeDecl->IgnoreParenImpCasts());
+  return std::make_tuple(D, Op, Constant);
+}
+
+/// For an expression `x == Foo && x == Bar`, this determines whether the
+/// `Foo` and `Bar` are either of the same enumeration type, or both integer
+/// literals.
+///
+/// It's an error to pass this arguments that are not either IntegerLiterals
+/// or DeclRefExprs (that have decls of type EnumConstantDecl)
+static bool areExprTypesCompatible(const Expr *A, const Expr *B) {
+  // User intent isn't clear if they're mixing int literals with enum
+  // constants.
+  if (isa<IntegerLiteral>(A) != isa<IntegerLiteral>(B))
+    return false;
+
+  // Integer literal comparisons, regardless of literal type, are acceptable.
+  if (isa<IntegerLiteral>(A))
+    return true;
+
+  // Currently we're only given EnumConstantDecls or IntegerLiterals
+  auto *C1 = cast<EnumConstantDecl>(cast<DeclRefExpr>(A)->getDecl());
+  auto *C2 = cast<EnumConstantDecl>(cast<DeclRefExpr>(B)->getDecl());
+
+  const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext());
+  const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext());
+  return E1 == E2;
+}
+
 class CFGBuilder;
   
 /// The CFG builder uses a recursive algorithm to build the CFG.  When
 ///  we process an expression, sometimes we know that we must add the
 ///  subexpressions as block-level expressions.  For example:
 ///
 ///    exp1 || exp2
 ///
TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
     const BinaryOperator *RHS =
         dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
     if (!LHS || !RHS)
       return TryResult();
 
     if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
       return TryResult();
 
-    BinaryOperatorKind BO1 = LHS->getOpcode();
-    const DeclRefExpr *Decl1 =
-        dyn_cast<DeclRefExpr>(LHS->getLHS()->IgnoreParenImpCasts());
-    const IntegerLiteral *Literal1 =
-        dyn_cast<IntegerLiteral>(LHS->getRHS()->IgnoreParens());
-    if (!Decl1 && !Literal1) {
-      if (BO1 == BO_GT)
-        BO1 = BO_LT;
-      else if (BO1 == BO_GE)
-        BO1 = BO_LE;
-      else if (BO1 == BO_LT)
-        BO1 = BO_GT;
-      else if (BO1 == BO_LE)
-        BO1 = BO_GE;
-      Decl1 = dyn_cast<DeclRefExpr>(LHS->getRHS()->IgnoreParenImpCasts());
-      Literal1 = dyn_cast<IntegerLiteral>(LHS->getLHS()->IgnoreParens());
-    }
+    const DeclRefExpr *Decl1;
+    const Expr *Expr1;
+    BinaryOperatorKind BO1;
+    std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS);
 
-    if (!Decl1 || !Literal1)
+    if (!Decl1 || !Expr1)
       return TryResult();
 
-    BinaryOperatorKind BO2 = RHS->getOpcode();
-    const DeclRefExpr *Decl2 =
-        dyn_cast<DeclRefExpr>(RHS->getLHS()->IgnoreParenImpCasts());
-    const IntegerLiteral *Literal2 =
-        dyn_cast<IntegerLiteral>(RHS->getRHS()->IgnoreParens());
-    if (!Decl2 && !Literal2) {
-      if (BO2 == BO_GT)
-        BO2 = BO_LT;
-      else if (BO2 == BO_GE)
-        BO2 = BO_LE;
-      else if (BO2 == BO_LT)
-        BO2 = BO_GT;
-      else if (BO2 == BO_LE)
-        BO2 = BO_GE;
-      Decl2 = dyn_cast<DeclRefExpr>(RHS->getRHS()->IgnoreParenImpCasts());
-      Literal2 = dyn_cast<IntegerLiteral>(RHS->getLHS()->IgnoreParens());
-    }
+    const DeclRefExpr *Decl2;
+    const Expr *Expr2;
+    BinaryOperatorKind BO2;
+    std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS);
 
-    if (!Decl2 || !Literal2)
+    if (!Decl2 || !Expr2)
       return TryResult();
 
     // Check that it is the same variable on both sides.
     if (Decl1->getDecl() != Decl2->getDecl())
       return TryResult();
 
+    // Make sure the user's intent is clear (e.g. they're comparing against two
+    // int literals, or two things from the same enum)
+    if (!areExprTypesCompatible(Expr1, Expr2))
+      return TryResult();
+
     llvm::APSInt L1, L2;
 
-    if (!Literal1->EvaluateAsInt(L1, *Context) ||
-        !Literal2->EvaluateAsInt(L2, *Context))
+    if (!Expr1->EvaluateAsInt(L1, *Context) ||
+        !Expr2->EvaluateAsInt(L2, *Context))
       return TryResult();
 
     // Can't compare signed with unsigned or with different bit width.
     if (L1.isSigned() != L2.isSigned() || L1.getBitWidth() != L2.getBitWidth())
       return TryResult();
 
     // Values that will be used to determine if result of logical
     // operator is always true/false

test/Sema/warn-overlap.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-overlap-compare %s
 
 #define mydefine 2
 
+enum Choices {
+  CHOICE_0 = 0,
+  CHOICE_1 = 1
+};
+
+enum Unchoices {
+  UNCHOICE_0 = 0,
+  UNCHOICE_1 = 1
+};
+
 void f(int x) {
   int y = 0;
 
   // > || <
   if (x > 2 || x < 1) { }
   if (x > 2 || x < 2) { }
   if (x != 2 || x != 3) { } // expected-warning {{overlapping comparisons always evaluate to true}}
   if (x > 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}}
void f(int x) {
   if (x != 2 || x < 3) { }   // expected-warning {{overlapping comparisons always evaluate to true}}
   if (x == 2 && x == 3) { }  // expected-warning {{overlapping comparisons always evaluate to false}}
   if (x == 2 && x > 3) { }   // expected-warning {{overlapping comparisons always evaluate to false}}
   if (x == 3 && x < 0) { }  // expected-warning {{overlapping comparisons always evaluate to false}}
   if (3 == x && x < 0) { }  // expected-warning {{overlapping comparisons always evaluate to false}}
 
   if (x == mydefine && x > 3) { }
   if (x == (mydefine + 1) && x > 3) { }
+
+  if (x != CHOICE_0 || x != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}}
+  if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+
+  // Don't warn if comparing x to different types
+  if (x == CHOICE_0 && x == 1) { }
+  if (x != CHOICE_0 || x != 1) { }
+
+  // "Different types" includes different enums
+  if (x == CHOICE_0 && x == UNCHOICE_1) { }
+  if (x != CHOICE_0 || x != UNCHOICE_1) { }
+}
+
+void enums(enum Choices c) {
+  if (c != CHOICE_0 || c != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}}
+  if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+
+  // Don't warn if comparing x to different types
+  if (c == CHOICE_0 && c == 1) { }
+  if (c != CHOICE_0 || c != 1) { }
+
+  // "Different types" includes different enums
+  if (c == CHOICE_0 && c == UNCHOICE_1) { }
+  if (c != CHOICE_0 || c != UNCHOICE_1) { }
 }
 
 // Don't generate a warning here.
 void array_out_of_bounds() {
   int x;
   int buffer[4];
   x = (-7 > 0) ? (buffer[-7]) : 0;
 }