[clang][Analysis] Handle && and || against variable and its negation …

…as tautology This patch introduces a new warning flag -Wtautological-negation-compare grouped in -Wtautological-compare that warns on the use of && or || operators against a variable and its negation. e.g. x || !x and !x && x This also makes the -Winfinite-recursion diagnose more cases. Fixes #56035 Differential Revision: https://reviews.llvm.org/D152093
llvm · Aug 17, 2023 · b3469ce · b3469ce · Endilll · Dec 2, 2023
1 parent b719e41
commit b3469ce
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 9 deletions.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -132,6 +132,10 @@ Improvements to Clang's diagnostics
   of a base class is not called in the constructor of its derived class.
 - Clang no longer emits ``-Wmissing-variable-declarations`` for variables declared
   with the ``register`` storage class.
+- Clang's ``-Wtautological-negation-compare`` flag now diagnoses logical
+  tautologies like ``x && !x`` and ``!x || x`` in expressions. This also
+  makes ``-Winfinite-recursion`` diagnose more cases.
+  (`#56035: <https://github.com/llvm/llvm-project/issues/56035>`_).
 
 Bug Fixes in This Version
 -------------------------

diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h
@@ -1162,6 +1162,7 @@ class CFGCallback {
   CFGCallback() = default;
   virtual ~CFGCallback() = default;
 
+  virtual void logicAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
   virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
   virtual void compareBitwiseEquality(const BinaryOperator *B,
                                       bool isAlwaysTrue) {}

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -678,13 +678,15 @@ def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
 def TautologicalBitwiseCompare : DiagGroup<"tautological-bitwise-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
 def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;
+def TautologicalNegationCompare : DiagGroup<"tautological-negation-compare">;
 def TautologicalCompare : DiagGroup<"tautological-compare",
                                     [TautologicalConstantCompare,
                                      TautologicalPointerCompare,
                                      TautologicalOverlapCompare,
                                      TautologicalBitwiseCompare,
                                      TautologicalUndefinedCompare,
-                                     TautologicalObjCBoolCompare]>;
+                                     TautologicalObjCBoolCompare,
+                                     TautologicalNegationCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9796,6 +9796,12 @@ def warn_comparison_bitwise_always : Warning<
 def warn_comparison_bitwise_or : Warning<
   "bitwise or with non-zero value always evaluates to true">,
   InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
+def warn_tautological_negation_and_compare: Warning<
+  "'&&' of a value and its negation always evaluates to false">,
+  InGroup<TautologicalNegationCompare>, DefaultIgnore;
+def warn_tautological_negation_or_compare: Warning<
+  "'||' of a value and its negation always evaluates to true">,
+  InGroup<TautologicalNegationCompare>, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<
   "overlapping comparisons always evaluate to %select{false|true}0">,
   InGroup<TautologicalOverlapCompare>, DefaultIgnore;

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
@@ -1070,16 +1070,41 @@ class CFGBuilder {
     }
   }
 
-  /// Find a pair of comparison expressions with or without parentheses
+  /// There are two checks handled by this function:
+  /// 1. Find a law-of-excluded-middle or law-of-noncontradiction expression
+  /// e.g. if (x || !x), if (x && !x)
+  /// 2. Find a pair of comparison expressions with or without parentheses
   /// with a shared variable and constants and a logical operator between them
   /// that always evaluates to either true or false.
   /// e.g. if (x != 3 || x != 4)
   TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
     assert(B->isLogicalOp());
-    const BinaryOperator *LHS =
-        dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
-    const BinaryOperator *RHS =
-        dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
+    const Expr *LHSExpr = B->getLHS()->IgnoreParens();
+    const Expr *RHSExpr = B->getRHS()->IgnoreParens();
+
+    auto CheckLogicalOpWithNegatedVariable = [this, B](const Expr *E1,
+                                                       const Expr *E2) {
+      if (const auto *Negate = dyn_cast<UnaryOperator>(E1)) {
+        if (Negate->getOpcode() == UO_LNot &&
+            Expr::isSameComparisonOperand(Negate->getSubExpr(), E2)) {
+          bool AlwaysTrue = B->getOpcode() == BO_LOr;
+          if (BuildOpts.Observer)
+            BuildOpts.Observer->logicAlwaysTrue(B, AlwaysTrue);
+          return TryResult(AlwaysTrue);
+        }
+      }
+      return TryResult();
+    };
+
+    TryResult Result = CheckLogicalOpWithNegatedVariable(LHSExpr, RHSExpr);
+    if (Result.isKnown())
+        return Result;
+    Result = CheckLogicalOpWithNegatedVariable(RHSExpr, LHSExpr);
+    if (Result.isKnown())
+        return Result;
+
+    const auto *LHS = dyn_cast<BinaryOperator>(LHSExpr);
+    const auto *RHS = dyn_cast<BinaryOperator>(RHSExpr);
     if (!LHS || !RHS)
       return {};
 

diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -158,6 +158,17 @@ class LogicalErrorHandler : public CFGCallback {
     return false;
   }
 
+  void logicAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
+    if (HasMacroID(B))
+      return;
+
+    unsigned DiagID = isAlwaysTrue
+                          ? diag::warn_tautological_negation_or_compare
+                          : diag::warn_tautological_negation_and_compare;
+    SourceRange DiagRange = B->getSourceRange();
+    S.Diag(B->getExprLoc(), DiagID) << DiagRange;
+  }
+
   void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
     if (HasMacroID(B))
       return;
@@ -188,7 +199,8 @@ class LogicalErrorHandler : public CFGCallback {
   static bool hasActiveDiagnostics(DiagnosticsEngine &Diags,
                                    SourceLocation Loc) {
     return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
-           !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+           !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc) ||
+           !Diags.isIgnored(diag::warn_tautological_negation_and_compare, Loc);
   }
 };
 } // anonymous namespace

diff --git a/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp b/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -1393,13 +1393,13 @@ const C &bar3(bool coin) {
 // CHECK:     Succs (2): B2 B1
 // CHECK:   [B4 (NORETURN)]
 // CHECK:     1: ~NoReturn() (Temporary object destructor)
-// CHECK:     Preds (1): B5
+// CHECK:     Preds (1): B5(Unreachable)
 // CHECK:     Succs (1): B0
 // CHECK:   [B5]
 // CHECK:     1: [B8.3] || [B7.2] || [B6.7]
 // CHECK:     T: (Temp Dtor) [B6.4]
 // CHECK:     Preds (3): B6 B7 B8
-// CHECK:     Succs (2): B4 B3
+// CHECK:     Succs (2): B4(Unreachable) B3
 // CHECK:   [B6]
 // CHECK:     1: check
 // CHECK:     2: [B6.1] (ImplicitCastExpr, FunctionToPointerDecay, _Bool (*)(const NoReturn &))

diff --git a/clang/test/Misc/warning-wall.c b/clang/test/Misc/warning-wall.c
@@ -55,6 +55,7 @@ CHECK-NEXT:      -Wtautological-overlap-compare
 CHECK-NEXT:      -Wtautological-bitwise-compare
 CHECK-NEXT:      -Wtautological-undefined-compare
 CHECK-NEXT:      -Wtautological-objc-bool-compare
+CHECK-NEXT:      -Wtautological-negation-compare
 CHECK-NEXT:    -Wtrigraphs
 CHECK-NEXT:    -Wuninitialized
 CHECK-NEXT:      -Wsometimes-uninitialized

diff --git a/clang/test/SemaCXX/tautological-negation-compare.cpp b/clang/test/SemaCXX/tautological-negation-compare.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-negation-compare -Wno-constant-logical-operand %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare -Wno-constant-logical-operand %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis -Wno-constant-logical-operand %s
+
+#define COPY(x) x
+
+void test_int(int x) {
+  if (x || !x) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+  if (!x || x) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+  if (x && !x) {} // expected-warning {{'&&' of a value and its negation always evaluates to false}}
+  if (!x && x) {} // expected-warning {{'&&' of a value and its negation always evaluates to false}}
+
+  // parentheses are ignored
+  if (x || (!x)) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+  if (!(x) || x) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+
+  // don't warn on macros
+  if (COPY(x) || !x) {}
+  if (!x || COPY(x)) {}
+  if (x && COPY(!x)) {}
+  if (COPY(!x && x)) {}
+
+  // dont' warn on literals
+  if (1 || !1) {}
+  if (!42 && 42) {}
+
+
+  // don't warn on overloads
+  struct Foo{
+    int val;
+    Foo operator!() const { return Foo{!val}; }
+    bool operator||(const Foo other) const { return val || other.val; }
+    bool operator&&(const Foo other) const { return val && other.val; }
+  };
+
+  Foo f{3};
+  if (f || !f) {}
+  if (!f || f) {}
+  if (f.val || !f.val) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+  if (!f.val && f.val) {} // expected-warning {{'&&' of a value and its negation always evaluates to false}}
+}
diff --git a/clang/test/SemaCXX/warn-infinite-recursion.cpp b/clang/test/SemaCXX/warn-infinite-recursion.cpp
@@ -203,3 +203,13 @@ int unevaluated_recursive_function() {
   (void)typeid(unevaluated_recursive_function());
   return 0;
 }
+
+void func1(int i) { // expected-warning {{call itself}}
+  if (i || !i)
+    func1(i);
+}
+void func2(int i) { // expected-warning {{call itself}}
+  if (!i && i) {}
+  else
+    func2(i);
+}