[analyzer] Extend IdenticalExprChecker to check ternary operator results.

Warn if both result expressions of a ternary operator (? :) are the same.
Because only one of them will be executed, this warning will fire even if
the expressions have side effects.

Patch by Anders Rönnholm and Per Viberg!

llvm-svn: 196937

Author: jrose-apple

clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

@@ -27,7 +27,7 @@
 #include <vector>
 
 namespace clang {
-
+class ConditionalOperator;
 class AnalysisDeclContext;
 class BinaryOperator;
 class CompoundStmt;
@@ -211,6 +211,9 @@ class PathDiagnosticLocation {
   /// Assumes the statement has a valid location.
   static PathDiagnosticLocation createOperatorLoc(const BinaryOperator *BO,
                                                   const SourceManager &SM);
+  static PathDiagnosticLocation createConditionalColonLoc(
+                                                  const ConditionalOperator *CO,
+                                                  const SourceManager &SM);
 
   /// For member expressions, return the location of the '.' or '->'.
   /// Assumes the statement has a valid location.

clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

@@ -11,7 +11,8 @@
 /// \brief This defines IdenticalExprChecker, a check that warns about
 /// unintended use of identical expressions.
 ///
-/// It checks for use of identical expressions with comparison operators.
+/// It checks for use of identical expressions with comparison operators and
+/// inside conditional expressions.
 ///
 //===----------------------------------------------------------------------===//
 
@@ -26,7 +27,7 @@ using namespace clang;
 using namespace ento;
 
 static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
-                            const Expr *Expr2);
+                            const Expr *Expr2, bool IgnoreSideEffects = false);
 //===----------------------------------------------------------------------===//
 // FindIdenticalExprVisitor - Identify nodes using identical expressions.
 //===----------------------------------------------------------------------===//
@@ -38,8 +39,9 @@ class FindIdenticalExprVisitor
   explicit FindIdenticalExprVisitor(BugReporter &B, AnalysisDeclContext *A)
       : BR(B), AC(A) {}
   // FindIdenticalExprVisitor only visits nodes
-  // that are binary operators.
+  // that are binary operators or conditional operators.
   bool VisitBinaryOperator(const BinaryOperator *B);
+  bool VisitConditionalOperator(const ConditionalOperator *C);
 
 private:
   BugReporter &BR;
@@ -118,6 +120,34 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
   // True is always returned to traverse ALL nodes.
   return true;
 }
+
+bool FindIdenticalExprVisitor::VisitConditionalOperator(
+    const ConditionalOperator *C) {
+
+  // Check if expressions in conditional expression are identical
+  // from a symbolic point of view.
+
+  if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
+                      C->getFalseExpr(), true)) {
+    PathDiagnosticLocation ELoc =
+        PathDiagnosticLocation::createConditionalColonLoc(
+            C, BR.getSourceManager());
+
+    SourceRange Sr[2];
+    Sr[0] = C->getTrueExpr()->getSourceRange();
+    Sr[1] = C->getFalseExpr()->getSourceRange();
+    BR.EmitBasicReport(
+        AC->getDecl(), "Identical expressions in conditional expression",
+        categories::LogicError,
+        "identical expressions on both sides of ':' in conditional expression",
+        ELoc, Sr);
+  }
+  // We want to visit ALL nodes (expressions in conditional
+  // expressions too) that contains conditional operators,
+  // thus always return true to traverse ALL nodes.
+  return true;
+}
+
 /// \brief Determines whether two expression trees are identical regarding
 /// operators and symbols.
 ///
@@ -127,14 +157,14 @@ bool FindIdenticalExprVisitor::VisitBinaryOperator(const BinaryOperator *B) {
 /// t*(u + t) and t*u + t*t are not considered identical.
 ///
 static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
-                            const Expr *Expr2) {
+                            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 (Expr1->HasSideEffects(Ctx))
+  if (!IgnoreSideEffects && Expr1->HasSideEffects(Ctx))
     return false;
   // Is expression is based on macro then don't warn even if
   // the expressions are identical.
@@ -146,7 +176,8 @@ static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
   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))
+    if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
+                                               IgnoreSideEffects))
       return false;
     ++I1;
     ++I2;
@@ -161,6 +192,7 @@ static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
   switch (Expr1->getStmtClass()) {
   default:
     return false;
+  case Stmt::CallExprClass:
   case Stmt::ArraySubscriptExprClass:
   case Stmt::CStyleCastExprClass:
   case Stmt::ImplicitCastExprClass:
@@ -201,7 +233,7 @@ static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
     const UnaryOperator *UnaryOp2 = dyn_cast<UnaryOperator>(Expr2);
     if (UnaryOp1->getOpcode() != UnaryOp2->getOpcode())
       return false;
-    return !UnaryOp1->isIncrementDecrementOp();
+    return IgnoreSideEffects || !UnaryOp1->isIncrementDecrementOp();
   }
   }
 }

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

@@ -609,6 +609,14 @@ PathDiagnosticLocation
   return PathDiagnosticLocation(BO->getOperatorLoc(), SM, SingleLocK);
 }
 
+PathDiagnosticLocation
+  PathDiagnosticLocation::createConditionalColonLoc(
+                                            const ConditionalOperator *CO,
+                                            const SourceManager &SM) {
+  return PathDiagnosticLocation(CO->getColonLoc(), SM, SingleLocK);
+}
+
+
 PathDiagnosticLocation
   PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
                                           const SourceManager &SM) {

clang/test/Analysis/identical-expressions.cpp

@@ -2,6 +2,21 @@
 
 /* Only one expected warning per function allowed at the very end. */
 
+int func(void)
+{
+  return 0;
+}
+
+int func2(void)
+{
+  return 0;
+}
+
+int funcParam(int a)
+{
+  return 0;
+}
+
 /* '!=' operator*/
 
 /* '!=' with float */
@@ -295,6 +310,38 @@ int checkNotEqualNestedBinaryOpIntPointerCompare2(void) {
 }
 /*   end '!=' int*          */
 
+/* '!=' with function*/
+
+int checkNotEqualSameFunction() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+func() != a+func());  // no warning
+  return (0);
+}
+
+int checkNotEqualDifferentFunction() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+func() != a+func2());  // no warning
+  return (0);
+}
+
+int checkNotEqualSameFunctionSameParam() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+funcParam(a) != a+funcParam(a));  // no warning
+  return (0);
+}
+
+int checkNotEqualSameFunctionDifferentParam() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+funcParam(a) != a+funcParam(b));  // no warning
+  return (0);
+}
+
+/*   end '!=' with function*/
+
 /*   end '!=' */
 
 
@@ -526,6 +573,37 @@ int checkEqualNestedBinaryOpIntCompare3(void) {
   return (0);
 }
 
+/* '==' with function*/
+
+int checkEqualSameFunction() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+func() == a+func());  // no warning
+  return (0);
+}
+
+int checkEqualDifferentFunction() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+func() == a+func2());  // no warning
+  return (0);
+}
+
+int checkEqualSameFunctionSameParam() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+funcParam(a) == a+funcParam(a));  // no warning
+  return (0);
+}
+
+int checkEqualSameFunctionDifferentParam() {
+  unsigned a = 0;
+  unsigned b = 1;
+  int res = (a+funcParam(a) == a+funcParam(b));  // no warning
+  return (0);
+}
+
+/*   end '==' with function*/
 
 /*   end EQ int          */
 
@@ -940,3 +1018,143 @@ int checkGreaterThanNestedBinaryOpIntCompare3(void) {
 /* end GT with int */
 
 /* end GT */
+
+
+/* Checking use of identical expressions in conditional operator*/
+
+unsigned test_unsigned(unsigned a) {
+  unsigned b = 1;
+  a = a > 5 ? b : b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+  return a;
+}
+
+void test_signed() {
+  int a = 0;
+  a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_bool(bool a) {
+  a = a > 0 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_float() {
+  float a = 0;
+  float b = 0;
+  a = a > 5 ? a : a; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_unsigned_expr() {
+  unsigned a = 0;
+  unsigned b = 0;
+  a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_signed_expr() {
+  int a = 0;
+  int b = 1;
+  a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_bool_expr(bool a) {
+  bool b = 0;
+  a = a > 0 ? a&&b : a&&b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_unsigned_expr_negative() {
+  unsigned a = 0;
+  unsigned b = 0;
+  a = a > 5 ? a+b : b+a; // no warning
+}
+
+void test_signed_expr_negative() {
+  int a = 0;
+  int b = 1;
+  a = a > 5 ? b+a : a+b; // no warning
+}
+
+void test_bool_expr_negative(bool a) {
+  bool b = 0;
+  a = a > 0 ? a&&b : b&&a; // no warning
+}
+
+void test_float_expr_positive() {
+  float a = 0;
+  float b = 0;
+  a = a > 5 ? a+b : a+b; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_positive_func() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a+func() : a+func(); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_func() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a+func() : a+func2(); // no warning
+}
+
+void test_expr_positive_funcParam() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a+funcParam(b) : a+funcParam(b); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_funcParam() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a+funcParam(a) : a+funcParam(b); // no warning
+}
+
+void test_expr_positive_inc() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a++ : a++; // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_inc() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a++ : b++; // no warning
+}
+
+void test_expr_positive_assign() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a=1 : a=1;  // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_expr_negative_assign() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a=1 : a=2; // no warning
+}
+
+void test_signed_nested_expr() {
+  int a = 0;
+  int b = 1;
+  int c = 3;
+  a = a > 5 ? a+b+(c+a)*(a + b*(c+a)) : a+b+(c+a)*(a + b*(c+a)); // expected-warning {{identical expressions on both sides of ':' in conditional expression}}
+}
+
+void test_signed_nested_expr_negative() {
+  int a = 0;
+  int b = 1;
+  int c = 3;
+  a = a > 5 ? a+b+(c+a)*(a + b*(c+a)) : a+b+(c+a)*(a + b*(a+c)); // no warning
+}
+
+void test_signed_nested_cond_expr_negative() {
+  int a = 0;
+  int b = 1;
+  int c = 3;
+  a = a > 5 ? (b > 5 ? 1 : 4) : (b > 5 ? 2 : 4); // no warning
+}
+
+void test_signed_nested_cond_expr() {
+  int a = 0;
+  int b = 1;
+  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}}
+}