diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -132,6 +132,10 @@ 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: `_). Bug Fixes in This Version ------------------------- diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h --- a/clang/include/clang/Analysis/CFG.h +++ b/clang/include/clang/Analysis/CFG.h @@ -1162,6 +1162,7 @@ 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 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -678,13 +678,15 @@ 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 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -9796,6 +9796,12 @@ def warn_comparison_bitwise_or : Warning< "bitwise or with non-zero value always evaluates to true">, InGroup, DefaultIgnore; +def warn_tautological_negation_and_compare: Warning< + "'&&' of a value and its negation always evaluates to false">, + InGroup, DefaultIgnore; +def warn_tautological_negation_or_compare: Warning< + "'||' of a value and its negation always evaluates to true">, + InGroup, DefaultIgnore; def warn_tautological_overlap_comparison : Warning< "overlapping comparisons always evaluate to %select{false|true}0">, InGroup, DefaultIgnore; diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -1070,16 +1070,41 @@ } } - /// 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(B->getLHS()->IgnoreParens()); - const BinaryOperator *RHS = - dyn_cast(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(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(LHSExpr); + const auto *RHS = dyn_cast(RHSExpr); if (!LHS || !RHS) return {}; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -158,6 +158,17 @@ 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 @@ 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 --- a/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp +++ b/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp @@ -1393,13 +1393,13 @@ // 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 --- a/clang/test/Misc/warning-wall.c +++ b/clang/test/Misc/warning-wall.c @@ -55,6 +55,7 @@ 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 new file mode 100644 --- /dev/null +++ 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 --- a/clang/test/SemaCXX/warn-infinite-recursion.cpp +++ b/clang/test/SemaCXX/warn-infinite-recursion.cpp @@ -203,3 +203,13 @@ (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); +}