Merge and improve code that detects same value in comparisons.

-Wtautological-overlap-compare and self-comparison from -Wtautological-compare
relay on detecting the same operand in different locations.  Previously, each
warning had it's own operand checker.  Now, both are merged together into
one function that each can call.  The function also now looks through member
access and array accesses.

Differential Revision: https://reviews.llvm.org/D66045


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

9 files changed, 249 insertions, 35 deletions

diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index e04e568ddb..bb93fb5477 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -53,6 +53,9 @@ Improvements to Clang's diagnostics
 
 - -Wtautological-overlap-compare will warn on negative numbers and non-int
   types.
+- -Wtautological-compare for self comparisons and
+  -Wtautological-overlap-compare will now look through member and array
+  access to determine if two operand expressions are the same.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index 30fa84315f..ffa7d4db96 100644
--- a/include/clang/AST/Expr.h
+++ b/include/clang/AST/Expr.h
@@ -906,6 +906,11 @@ public:
     return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
   }
 
+  /// Checks that the two Expr's will refer to the same value as a comparison
+  /// operand.  The caller must ensure that the values referenced by the Expr's
+  /// are not modified between E1 and E2 or the result my be invalid.
+  static bool isSameComparisonOperand(const Expr* E1, const Expr* E2);
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() >= firstExprConstant &&
            T->getStmtClass() <= lastExprConstant;
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index 00c6a81704..b5fb7fafb0 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -3921,6 +3921,111 @@ bool Expr::refersToGlobalRegisterVar() const {
   return false;
 }
 
+bool Expr::isSameComparisonOperand(const Expr* E1, const Expr* E2) {
+  E1 = E1->IgnoreParens();
+  E2 = E2->IgnoreParens();
+
+  if (E1->getStmtClass() != E2->getStmtClass())
+    return false;
+
+  switch (E1->getStmtClass()) {
+    default:
+      return false;
+    case CXXThisExprClass:
+      return true;
+    case DeclRefExprClass: {
+      // DeclRefExpr without an ImplicitCastExpr can happen for integral
+      // template parameters.
+      const auto *DRE1 = cast<DeclRefExpr>(E1);
+      const auto *DRE2 = cast<DeclRefExpr>(E2);
+      return DRE1->isRValue() && DRE2->isRValue() &&
+             DRE1->getDecl() == DRE2->getDecl();
+    }
+    case ImplicitCastExprClass: {
+      // Peel off implicit casts.
+      while (true) {
+        const auto *ICE1 = dyn_cast<ImplicitCastExpr>(E1);
+        const auto *ICE2 = dyn_cast<ImplicitCastExpr>(E2);
+        if (!ICE1 || !ICE2)
+          return false;
+        if (ICE1->getCastKind() != ICE2->getCastKind())
+          return false;
+        E1 = ICE1->getSubExpr()->IgnoreParens();
+        E2 = ICE2->getSubExpr()->IgnoreParens();
+        // The final cast must be one of these types.
+        if (ICE1->getCastKind() == CK_LValueToRValue ||
+            ICE1->getCastKind() == CK_ArrayToPointerDecay ||
+            ICE1->getCastKind() == CK_FunctionToPointerDecay) {
+          break;
+        }
+      }
+
+      const auto *DRE1 = dyn_cast<DeclRefExpr>(E1);
+      const auto *DRE2 = dyn_cast<DeclRefExpr>(E2);
+      if (DRE1 && DRE2)
+        return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl());
+
+      const auto *Ivar1 = dyn_cast<ObjCIvarRefExpr>(E1);
+      const auto *Ivar2 = dyn_cast<ObjCIvarRefExpr>(E2);
+      if (Ivar1 && Ivar2) {
+        return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() &&
+               declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl());
+      }
+
+      const auto *Array1 = dyn_cast<ArraySubscriptExpr>(E1);
+      const auto *Array2 = dyn_cast<ArraySubscriptExpr>(E2);
+      if (Array1 && Array2) {
+        if (!isSameComparisonOperand(Array1->getBase(), Array2->getBase()))
+          return false;
+
+        auto Idx1 = Array1->getIdx();
+        auto Idx2 = Array2->getIdx();
+        const auto Integer1 = dyn_cast<IntegerLiteral>(Idx1);
+        const auto Integer2 = dyn_cast<IntegerLiteral>(Idx2);
+        if (Integer1 && Integer2) {
+          if (Integer1->getValue() != Integer2->getValue())
+            return false;
+        } else {
+          if (!isSameComparisonOperand(Idx1, Idx2))
+            return false;
+        }
+
+        return true;
+      }
+
+      // Walk the MemberExpr chain.
+      while (isa<MemberExpr>(E1) && isa<MemberExpr>(E2)) {
+        const auto *ME1 = cast<MemberExpr>(E1);
+        const auto *ME2 = cast<MemberExpr>(E2);
+        if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl()))
+          return false;
+        if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
+          if (D->isStaticDataMember())
+            return true;
+        E1 = ME1->getBase()->IgnoreParenImpCasts();
+        E2 = ME2->getBase()->IgnoreParenImpCasts();
+      }
+
+      if (isa<CXXThisExpr>(E1) && isa<CXXThisExpr>(E2))
+        return true;
+
+      // A static member variable can end the MemberExpr chain with either
+      // a MemberExpr or a DeclRefExpr.
+      auto getAnyDecl = [](const Expr *E) -> const ValueDecl * {
+        if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
+          return DRE->getDecl();
+        if (const auto *ME = dyn_cast<MemberExpr>(E))
+          return ME->getMemberDecl();
+        return nullptr;
+      };
+
+      const ValueDecl *VD1 = getAnyDecl(E1);
+      const ValueDecl *VD2 = getAnyDecl(E2);
+      return declaresSameEntity(VD1, VD2);
+    }
+  }
+}
+
 /// isArrow - Return true if the base expression is a pointer to vector,
 /// return false if the base expression is a vector.
 bool ExtVectorElementExpr::isArrow() const {
diff --git a/lib/Analysis/CFG.cpp b/lib/Analysis/CFG.cpp
index 86816997f0..687cf57059 100644
--- a/lib/Analysis/CFG.cpp
+++ b/lib/Analysis/CFG.cpp
@@ -105,12 +105,12 @@ static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
   return nullptr;
 }
 
-/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
-/// an integer literal or an enum constant.
+/// Tries to interpret a binary operator into `Expr Op NumExpr` form, if
+/// NumExpr 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 *>
+static std::tuple<const Expr *, BinaryOperatorKind, const Expr *>
 tryNormalizeBinaryOperator(const BinaryOperator *B) {
   BinaryOperatorKind Op = B->getOpcode();
 
@@ -132,8 +132,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
     Constant = tryTransformToIntOrEnumConstant(B->getLHS());
   }
 
-  auto *D = dyn_cast<DeclRefExpr>(MaybeDecl->IgnoreParenImpCasts());
-  return std::make_tuple(D, Op, Constant);
+  return std::make_tuple(MaybeDecl, Op, Constant);
 }
 
 /// For an expression `x == Foo && x == Bar`, this determines whether the
@@ -1045,34 +1044,34 @@ private:
     if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
       return {};
 
-    const DeclRefExpr *Decl1;
-    const Expr *Expr1;
+    const Expr *DeclExpr1;
+    const Expr *NumExpr1;
     BinaryOperatorKind BO1;
-    std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS);
+    std::tie(DeclExpr1, BO1, NumExpr1) = tryNormalizeBinaryOperator(LHS);
 
-    if (!Decl1 || !Expr1)
+    if (!DeclExpr1 || !NumExpr1)
       return {};
 
-    const DeclRefExpr *Decl2;
-    const Expr *Expr2;
+    const Expr *DeclExpr2;
+    const Expr *NumExpr2;
     BinaryOperatorKind BO2;
-    std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS);
+    std::tie(DeclExpr2, BO2, NumExpr2) = tryNormalizeBinaryOperator(RHS);
 
-    if (!Decl2 || !Expr2)
+    if (!DeclExpr2 || !NumExpr2)
       return {};
 
     // Check that it is the same variable on both sides.
-    if (Decl1->getDecl() != Decl2->getDecl())
+    if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
       return {};
 
     // 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))
+    if (!areExprTypesCompatible(NumExpr1, NumExpr2))
       return {};
 
     Expr::EvalResult L1Result, L2Result;
-    if (!Expr1->EvaluateAsInt(L1Result, *Context) ||
-        !Expr2->EvaluateAsInt(L2Result, *Context))
+    if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
+        !NumExpr2->EvaluateAsInt(L2Result, *Context))
       return {};
 
     llvm::APSInt L1 = L1Result.Val.getInt();
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 808c0e450a..6dfbc4cda5 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -10245,20 +10245,18 @@ static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
       << FixItHint::CreateInsertion(SecondClose, ")");
 }
 
-// Get the decl for a simple expression: a reference to a variable,
-// an implicit C++ field reference, or an implicit ObjC ivar reference.
-static ValueDecl *getCompareDecl(Expr *E) {
-  if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
-    return DR->getDecl();
-  if (ObjCIvarRefExpr *Ivar = dyn_cast<ObjCIvarRefExpr>(E)) {
-    if (Ivar->isFreeIvar())
-      return Ivar->getDecl();
-  }
-  if (MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
+// Returns true if E refers to a non-weak array.
+static bool checkForArray(const Expr *E) {
+  const ValueDecl *D = nullptr;
+  if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E)) {
+    D = DR->getDecl();
+  } else if (const MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
     if (Mem->isImplicitAccess())
-      return Mem->getMemberDecl();
+      D = Mem->getMemberDecl();
   }
-  return nullptr;
+  if (!D)
+    return false;
+  return D->getType()->isArrayType() && !D->isWeak();
 }
 
 /// Diagnose some forms of syntactically-obvious tautological comparison.
@@ -10291,8 +10289,6 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
   // obvious cases in the definition of the template anyways. The idea is to
   // warn when the typed comparison operator will always evaluate to the same
   // result.
-  ValueDecl *DL = getCompareDecl(LHSStripped);
-  ValueDecl *DR = getCompareDecl(RHSStripped);
 
   // Used for indexing into %select in warn_comparison_always
   enum {
@@ -10301,7 +10297,8 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
     AlwaysFalse,
     AlwaysEqual, // std::strong_ordering::equal from operator<=>
   };
-  if (DL && DR && declaresSameEntity(DL, DR)) {
+
+  if (Expr::isSameComparisonOperand(LHS, RHS)) {
     unsigned Result;
     switch (Opc) {
     case BO_EQ: case BO_LE: case BO_GE:
@@ -10321,9 +10318,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                           S.PDiag(diag::warn_comparison_always)
                               << 0 /*self-comparison*/
                               << Result);
-  } else if (DL && DR &&
-             DL->getType()->isArrayType() && DR->getType()->isArrayType() &&
-             !DL->isWeak() && !DR->isWeak()) {
+  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
     // What is it always going to evaluate to?
     unsigned Result;
     switch(Opc) {
diff --git a/test/Analysis/array-struct-region.cpp b/test/Analysis/array-struct-region.cpp
index cfb57d3924..1b9fa3e8db 100644
--- a/test/Analysis/array-struct-region.cpp
+++ b/test/Analysis/array-struct-region.cpp
@@ -1,20 +1,26 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -Wno-tautological-compare\
 // RUN:                    -x c %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -Wno-tautological-compare\
 // RUN:                    -x c++ -std=c++14 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -Wno-tautological-compare\
 // RUN:                    -x c++ -std=c++17 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -Wno-tautological-compare\
 // RUN:                    -DINLINE -x c %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -Wno-tautological-compare\
 // RUN:                    -DINLINE -x c++ -std=c++14 %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -Wno-tautological-compare\
 // RUN:                    -DINLINE -x c++ -std=c++17 %s
 
 void clang_analyzer_eval(int);
diff --git a/test/Sema/warn-overlap.c b/test/Sema/warn-overlap.c
index d72e60755d..066312591c 100644
--- a/test/Sema/warn-overlap.c
+++ b/test/Sema/warn-overlap.c
@@ -158,3 +158,17 @@ int no_warning(unsigned x) {
   return x >= 0 || x == 1;
   // no warning since "x >= 0" is caught by a different tautological warning.
 }
+
+struct A {
+  int x;
+  int y;
+};
+
+int struct_test(struct A a) {
+  return a.x > 5 && a.y < 1;  // no warning, different variables
+
+  return a.x > 5 && a.x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+  return a.y == 1 || a.y != 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
diff --git a/test/SemaCXX/compare-cxx2a.cpp b/test/SemaCXX/compare-cxx2a.cpp
index c68a0ae913..b6e7fc8061 100644
--- a/test/SemaCXX/compare-cxx2a.cpp
+++ b/test/SemaCXX/compare-cxx2a.cpp
@@ -8,12 +8,18 @@
 #define ASSERT_TYPE(...) static_assert(__is_same(__VA_ARGS__))
 #define ASSERT_EXPR_TYPE(Expr, Expect) static_assert(__is_same(decltype(Expr), Expect));
 
+struct S {
+  static int x[5];
+};
+
 void self_compare() {
   int a;
   int *b = nullptr;
+  S s;
 
   (void)(a <=> a); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
   (void)(b <=> b); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
+  (void)(s.x[a] <=> S::x[a]); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
 }
 
 void test0(long a, unsigned long b) {
diff --git a/test/SemaCXX/self-comparison.cpp b/test/SemaCXX/self-comparison.cpp
index ac129b68a6..4ea16665f1 100644
--- a/test/SemaCXX/self-comparison.cpp
+++ b/test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,84 @@ bool g() {
     Y<T>::n == Y<U>::n;
 }
 template bool g<int, int>(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+  int field;
+  static int static_field;
+  int test(B b) {
+    return field == field;  // expected-warning {{self-comparison always evaluates to true}}
+    return static_field == static_field;  // expected-warning {{self-comparison always evaluates to true}}
+    return static_field == b.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+    return B::static_field == this->static_field;  // expected-warning {{self-comparison always evaluates to true}}
+    return this == this;  // expected-warning {{self-comparison always evaluates to true}}
+
+    return field == b.field;
+    return this->field == b.field;
+  }
+};
+
+enum {
+  I0,
+  I1,
+  I2,
+};
+
+struct S {
+  int field;
+  static int static_field;
+  int array[4];
+};
+
+struct T {
+  int field;
+  static int static_field;
+  int array[4];
+  S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+  return s1.field == s1.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s2.field == s2.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.static_field == s2.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return S::static_field == s1.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->field == s3->field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[0] == s1.array[0];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[I1] == s1.array[I1];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[s2.array[0]] == s1.array[s2.array[0]];  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->array[t.field] == s3->array[t.field];  // expected-warning {{self-comparison always evaluates to true}}
+
+  // Try all operators
+  return t.field == t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field <= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field >= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+
+  return t.field != t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field < t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field > t.field;  // expected-warning {{self-comparison always evaluates to false}}
+
+  // no warning
+  return s1.field == s2.field;
+  return s2.array == s1.array;
+  return s2.array[0] == s1.array[0];
+  return s1.array[I1] == s1.array[I2];
+
+  return s1.static_field == t.static_field;
+};
+
+struct U {
+  bool operator!=(const U&);
+};
+
+bool operator==(const U&, const U&);
+
+// May want to warn on this in the future.
+int user_defined(U u) {
+  return u == u;
+  return u != u;
+}
+
+} // namespace member_tests