[analyzer] Extend IdenticalExprChecker to check the two branches of an if.

This extends the checks for identical expressions to handle identical
statements, and compares the consequent and alternative ("then" and "else")
branches of an if-statement to see if they are identical, treating a single
statement surrounded by braces as equivalent to one without braces.

This does /not/ check subsequent branches in an if/else chain, let alone
branches that are not consecutive. This may improve in a future patch, but
it would certainly take more work.

Patch by Daniel Fahlgren!

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@201701 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
index aeefc4260b11abc0c5881957563b4198753473ab..6c6f472f3f70ae4d92943486c4c9c20156af18aa 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -26,8 +26,8 @@
 using namespace clang;
 using namespace ento;
 
-static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
-                            const Expr *Expr2, bool IgnoreSideEffects = false);
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+                            const Stmt *Stmt2, bool IgnoreSideEffects = false);
 //===----------------------------------------------------------------------===//
 // FindIdenticalExprVisitor - Identify nodes using identical expressions.
 //===----------------------------------------------------------------------===//
@@ -41,8 +41,10 @@ public:
                                     AnalysisDeclContext *A)
       : BR(B), Checker(Checker), AC(A) {}
   // FindIdenticalExprVisitor only visits nodes
-  // that are binary operators or conditional operators.
+  // that are binary operators, if statements or
+  // conditional operators.
   bool VisitBinaryOperator(const BinaryOperator *B);
+  bool VisitIfStmt(const IfStmt *I);
   bool VisitConditionalOperator(const ConditionalOperator *C);
 
 private:
@@ -52,6 +54,39 @@ private:
 };
 } // end anonymous namespace
 
+bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
+  const Stmt *Stmt1 = I->getThen();
+  const Stmt *Stmt2 = I->getElse();
+
+  if (!Stmt1 || !Stmt2)
+    return true;
+
+  // Special handling for code like:
+  //
+  // if (b) {
+  //   i = 1;
+  // } else
+  //   i = 1;
+  if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt1)) {
+    if (CompStmt->size() == 1)
+      Stmt1 = CompStmt->body_back();
+  }
+  if (const CompoundStmt *CompStmt = dyn_cast<CompoundStmt>(Stmt2)) {
+    if (CompStmt->size() == 1)
+      Stmt2 = CompStmt->body_back();
+  }
+
+  if (isIdenticalStmt(AC->getASTContext(), Stmt1, Stmt2, true)) {
+      PathDiagnosticLocation ELoc =
+          PathDiagnosticLocation::createBegin(I, BR.getSourceManager(), AC);
+      BR.EmitBasicReport(AC->getDecl(), Checker,
+                         "Identical branches",
+                         categories::LogicError,
+                         "true and false branches are identical", ELoc);
+  }
+  return true;
+}
+
 bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
   BinaryOperator::Opcode Op = B->getOpcode();
   if (!BinaryOperator::isComparisonOp(Op))
@@ -107,7 +142,7 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
     // No special case with floating-point representation, report as usual.
   }
 
-  if (isIdenticalExpr(AC->getASTContext(), B->getLHS(), B->getRHS())) {
+  if (isIdenticalStmt(AC->getASTContext(), B->getLHS(), B->getRHS())) {
     PathDiagnosticLocation ELoc =
         PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager());
     StringRef Message;
@@ -131,7 +166,7 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
   // Check if expressions in conditional expression are identical
   // from a symbolic point of view.
 
-  if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
+  if (isIdenticalStmt(AC->getASTContext(), C->getTrueExpr(),
                       C->getFalseExpr(), true)) {
     PathDiagnosticLocation ELoc =
         PathDiagnosticLocation::createConditionalColonLoc(
@@ -153,7 +188,7 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
   return true;
 }
 
-/// \brief Determines whether two expression trees are identical regarding
+/// \brief Determines whether two statement trees are identical regarding
 /// operators and symbols.
 ///
 /// Exceptions: expressions containing macros or functions with possible side
@@ -161,81 +196,183 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator(
 /// Limitations: (t + u) and (u + t) are not considered identical.
 /// t*(u + t) and t*u + t*t are not considered identical.
 ///
-static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
-                            const Expr *Expr2, bool IgnoreSideEffects) {
-  // If Expr1 & Expr2 are of different class then they are not
-  // identical expression.
-  if (Expr1->getStmtClass() != Expr2->getStmtClass())
-    return false;
-  // If Expr1 has side effects then don't warn even if expressions
-  // are identical.
-  if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
+                            const Stmt *Stmt2, bool IgnoreSideEffects) {
+
+  if (!Stmt1 || !Stmt2) {
+    if (!Stmt1 && !Stmt2)
+      return true;
     return false;
-  // If either expression comes from a macro then don't warn even if
-  // the expressions are identical.
-  if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+  }
+
+  // If Stmt1 & Stmt2 are of different class then they are not
+  // identical statements.
+  if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
     return false;
-  // If all children of two expressions are identical, return true.
-  Expr::const_child_iterator I1 = Expr1->child_begin();
-  Expr::const_child_iterator I2 = Expr2->child_begin();
-  while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
-    const Expr *Child1 = dyn_cast<Expr>(*I1);
-    const Expr *Child2 = dyn_cast<Expr>(*I2);
-    if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
-                                               IgnoreSideEffects))
+
+  const Expr *Expr1 = dyn_cast<Expr>(Stmt1);
+  const Expr *Expr2 = dyn_cast<Expr>(Stmt2);
+
+  if (Expr1 && Expr2) {
+    // If Stmt1 has side effects then don't warn even if expressions
+    // are identical.
+    if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
+      return false;
+    // If either expression comes from a macro then don't warn even if
+    // the expressions are identical.
+    if ((Expr1->getExprLoc().isMacroID()) || (Expr2->getExprLoc().isMacroID()))
+      return false;
+
+    // If all children of two expressions are identical, return true.
+    Expr::const_child_iterator I1 = Expr1->child_begin();
+    Expr::const_child_iterator I2 = Expr2->child_begin();
+    while (I1 != Expr1->child_end() && I2 != Expr2->child_end()) {
+      if (!*I1 || !*I2 || !isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+        return false;
+      ++I1;
+      ++I2;
+    }
+    // If there are different number of children in the statements, return
+    // false.
+    if (I1 != Expr1->child_end())
+      return false;
+    if (I2 != Expr2->child_end())
       return false;
-    ++I1;
-    ++I2;
   }
-  // If there are different number of children in the expressions, return false.
-  // (TODO: check if this is a redundant condition.)
-  if (I1 != Expr1->child_end())
-    return false;
-  if (I2 != Expr2->child_end())
-    return false;
 
-  switch (Expr1->getStmtClass()) {
+  switch (Stmt1->getStmtClass()) {
   default:
     return false;
   case Stmt::CallExprClass:
   case Stmt::ArraySubscriptExprClass:
-  case Stmt::CStyleCastExprClass:
   case Stmt::ImplicitCastExprClass:
   case Stmt::ParenExprClass:
+  case Stmt::BreakStmtClass:
+  case Stmt::ContinueStmtClass:
+  case Stmt::NullStmtClass:
+    return true;
+  case Stmt::CStyleCastExprClass: {
+    const CStyleCastExpr* CastExpr1 = cast<CStyleCastExpr>(Stmt1);
+    const CStyleCastExpr* CastExpr2 = cast<CStyleCastExpr>(Stmt2);
+
+    return CastExpr1->getTypeAsWritten() == CastExpr2->getTypeAsWritten();
+  }
+  case Stmt::ReturnStmtClass: {
+    const ReturnStmt *ReturnStmt1 = cast<ReturnStmt>(Stmt1);
+    const ReturnStmt *ReturnStmt2 = cast<ReturnStmt>(Stmt2);
+
+    return isIdenticalStmt(Ctx, ReturnStmt1->getRetValue(),
+                           ReturnStmt2->getRetValue(), IgnoreSideEffects);
+  }
+  case Stmt::ForStmtClass: {
+    const ForStmt *ForStmt1 = cast<ForStmt>(Stmt1);
+    const ForStmt *ForStmt2 = cast<ForStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::DoStmtClass: {
+    const DoStmt *DStmt1 = cast<DoStmt>(Stmt1);
+    const DoStmt *DStmt2 = cast<DoStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::WhileStmtClass: {
+    const WhileStmt *WStmt1 = cast<WhileStmt>(Stmt1);
+    const WhileStmt *WStmt2 = cast<WhileStmt>(Stmt2);
+
+    return isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
+                           IgnoreSideEffects);
+  }
+  case Stmt::IfStmtClass: {
+    const IfStmt *IStmt1 = cast<IfStmt>(Stmt1);
+    const IfStmt *IStmt2 = cast<IfStmt>(Stmt2);
+
+    if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
+                         IgnoreSideEffects))
+      return false;
+    if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
+                         IgnoreSideEffects))
+      return false;
+    return true;
+  }
+  case Stmt::CompoundStmtClass: {
+    const CompoundStmt *CompStmt1 = cast<CompoundStmt>(Stmt1);
+    const CompoundStmt *CompStmt2 = cast<CompoundStmt>(Stmt2);
+
+    if (CompStmt1->size() != CompStmt2->size())
+      return false;
+
+    CompoundStmt::const_body_iterator I1 = CompStmt1->body_begin();
+    CompoundStmt::const_body_iterator I2 = CompStmt2->body_begin();
+    while (I1 != CompStmt1->body_end() && I2 != CompStmt2->body_end()) {
+      if (!isIdenticalStmt(Ctx, *I1, *I2, IgnoreSideEffects))
+        return false;
+      ++I1;
+      ++I2;
+    }
+
     return true;
+  }
+  case Stmt::CompoundAssignOperatorClass:
   case Stmt::BinaryOperatorClass: {
-    const BinaryOperator *BinOp1 = cast<BinaryOperator>(Expr1);
-    const BinaryOperator *BinOp2 = cast<BinaryOperator>(Expr2);
+    const BinaryOperator *BinOp1 = cast<BinaryOperator>(Stmt1);
+    const BinaryOperator *BinOp2 = cast<BinaryOperator>(Stmt2);
     return BinOp1->getOpcode() == BinOp2->getOpcode();
   }
   case Stmt::CharacterLiteralClass: {
-    const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Expr1);
-    const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Expr2);
+    const CharacterLiteral *CharLit1 = cast<CharacterLiteral>(Stmt1);
+    const CharacterLiteral *CharLit2 = cast<CharacterLiteral>(Stmt2);
     return CharLit1->getValue() == CharLit2->getValue();
   }
   case Stmt::DeclRefExprClass: {
-    const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Expr1);
-    const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Expr2);
+    const DeclRefExpr *DeclRef1 = cast<DeclRefExpr>(Stmt1);
+    const DeclRefExpr *DeclRef2 = cast<DeclRefExpr>(Stmt2);
     return DeclRef1->getDecl() == DeclRef2->getDecl();
   }
   case Stmt::IntegerLiteralClass: {
-    const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Expr1);
-    const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Expr2);
+    const IntegerLiteral *IntLit1 = cast<IntegerLiteral>(Stmt1);
+    const IntegerLiteral *IntLit2 = cast<IntegerLiteral>(Stmt2);
     return IntLit1->getValue() == IntLit2->getValue();
   }
   case Stmt::FloatingLiteralClass: {
-    const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Expr1);
-    const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Expr2);
+    const FloatingLiteral *FloatLit1 = cast<FloatingLiteral>(Stmt1);
+    const FloatingLiteral *FloatLit2 = cast<FloatingLiteral>(Stmt2);
     return FloatLit1->getValue().bitwiseIsEqual(FloatLit2->getValue());
   }
+  case Stmt::StringLiteralClass: {
+    const StringLiteral *StringLit1 = cast<StringLiteral>(Stmt1);
+    const StringLiteral *StringLit2 = cast<StringLiteral>(Stmt2);
+    return StringLit1->getString() == StringLit2->getString();
+  }
   case Stmt::MemberExprClass: {
-    const MemberExpr *MemberExpr1 = cast<MemberExpr>(Expr1);
-    const MemberExpr *MemberExpr2 = cast<MemberExpr>(Expr2);
-    return MemberExpr1->getMemberDecl() == MemberExpr2->getMemberDecl();
+    const MemberExpr *MemberStmt1 = cast<MemberExpr>(Stmt1);
+    const MemberExpr *MemberStmt2 = cast<MemberExpr>(Stmt2);
+    return MemberStmt1->getMemberDecl() == MemberStmt2->getMemberDecl();
   }
   case Stmt::UnaryOperatorClass: {
-    const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Expr1);
-    const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Expr2);
+    const UnaryOperator *UnaryOp1 = cast<UnaryOperator>(Stmt1);
+    const UnaryOperator *UnaryOp2 = cast<UnaryOperator>(Stmt2);
     return UnaryOp1->getOpcode() == UnaryOp2->getOpcode();
   }
   }

diff --git a/test/Analysis/identical-expressions.cpp b/test/Analysis/identical-expressions.cpp
index 3a331fb8f2ff636d1b78e6e1e3749da15ccdbea9..acb7c8c57b0648a36d53bee39b69536be424ea8f 100644 (file)
--- a/test/Analysis/identical-expressions.cpp
+++ b/test/Analysis/identical-expressions.cpp
@@ -1043,6 +1043,11 @@ void test_float() {
   a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
 }
 
+const char *test_string() {
+  float a = 0;
+  return a > 5 ? "abc" : "abc"; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
 void test_unsigned_expr() {
   unsigned a = 0;
   unsigned b = 0;
@@ -1158,3 +1163,149 @@ void test_signed_nested_cond_expr() {
   int c = 3;
   a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 4 : 4); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
 }
+
+void test_identical_branches1(bool b) {
+  int i = 0;
+  if (b) { // expected-warning {{true and false branches are identical}}
+    ++i;
+  } else {
+    ++i;
+  }
+}
+
+void test_identical_branches2(bool b) {
+  int i = 0;
+  if (b) { // expected-warning {{true and false branches are identical}}
+    ++i;
+  } else
+    ++i;
+}
+
+void test_identical_branches3(bool b) {
+  int i = 0;
+  if (b) { // no warning
+    ++i;
+  } else {
+    i++;
+  }
+}
+
+void test_identical_branches4(bool b) {
+  int i = 0;
+  if (b) { // expected-warning {{true and false branches are identical}}
+  } else {
+  }
+}
+
+void test_identical_branches_break(bool b) {
+  while (true) {
+    if (b) // expected-warning {{true and false branches are identical}}
+      break;
+    else
+      break;
+  }
+}
+
+void test_identical_branches_continue(bool b) {
+  while (true) {
+    if (b) // expected-warning {{true and false branches are identical}}
+      continue;
+    else
+      continue;
+  }
+}
+
+void test_identical_branches_func(bool b) {
+  if (b) // expected-warning {{true and false branches are identical}}
+    func();
+  else
+    func();
+}
+
+void test_identical_branches_func_arguments(bool b) {
+  if (b) // no-warning
+    funcParam(1);
+  else
+    funcParam(2);
+}
+
+void test_identical_branches_cast1(bool b) {
+  long v = -7;
+  if (b) // no-warning
+    v = (signed int) v;
+  else
+    v = (unsigned int) v;
+}
+
+void test_identical_branches_cast2(bool b) {
+  long v = -7;
+  if (b) // expected-warning {{true and false branches are identical}}
+    v = (signed int) v;
+  else
+    v = (signed int) v;
+}
+
+int test_identical_branches_return_int(bool b) {
+  int i = 0;
+  if (b) { // expected-warning {{true and false branches are identical}}
+    i++;
+    return i;
+  } else {
+    i++;
+    return i;
+  }
+}
+
+int test_identical_branches_return_func(bool b) {
+  if (b) { // expected-warning {{true and false branches are identical}}
+    return func();
+  } else {
+    return func();
+  }
+}
+
+void test_identical_branches_for(bool b) {
+  int i;
+  int j;
+  if (b) { // expected-warning {{true and false branches are identical}}
+    for (i = 0, j = 0; i < 10; i++)
+      j += 4;
+  } else {
+    for (i = 0, j = 0; i < 10; i++)
+      j += 4;
+  }
+}
+
+void test_identical_branches_while(bool b) {
+  int i = 10;
+  if (b) { // expected-warning {{true and false branches are identical}}
+    while (func())
+      i--;
+  } else {
+    while (func())
+      i--;
+  }
+}
+
+void test_identical_branches_do_while(bool b) {
+  int i = 10;
+  if (b) { // expected-warning {{true and false branches are identical}}
+    do {
+      i--;
+    } while (func());
+  } else {
+    do {
+      i--;
+    } while (func());
+  }
+}
+
+void test_identical_branches_if(bool b, int i) {
+  if (b) { // expected-warning {{true and false branches are identical}}
+    if (i < 5)
+      i += 10;
+  } else {
+    if (i < 5)
+      i += 10;
+  }
+}

diff --git a/test/Analysis/misc-ps-region-store.cpp b/test/Analysis/misc-ps-region-store.cpp
index 6a873f01b3d314328c824ab9b677c5a12bea666d..cb510421791c200f2755846b668ad86e80ecb411 100644 (file)
--- a/test/Analysis/misc-ps-region-store.cpp
+++ b/test/Analysis/misc-ps-region-store.cpp
@@ -588,6 +588,7 @@ void rdar11401827() {
   int x = int();
   if (!x) {
     clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+    ; // Suppress warning that both branches are identical
   }
   else {
     clang_analyzer_warnIfReached();  // no-warning