diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -16,6 +16,7 @@ #include "BadSignalToKillThreadCheck.h" #include "BoolPointerImplicitConversionCheck.h" #include "BranchCloneCheck.h" +#include "ChainedComparisonCheck.h" #include "CopyConstructorInitCheck.h" #include "DanglingHandleCheck.h" #include "DynamicStaticInitializersCheck.h" @@ -94,6 +95,8 @@ CheckFactories.registerCheck( "bugprone-bool-pointer-implicit-conversion"); CheckFactories.registerCheck("bugprone-branch-clone"); + CheckFactories.registerCheck( + "bugprone-chained-comparison"); CheckFactories.registerCheck( "bugprone-copy-constructor-init"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -11,6 +11,7 @@ BoolPointerImplicitConversionCheck.cpp BranchCloneCheck.cpp BugproneTidyModule.cpp + ChainedComparisonCheck.cpp CopyConstructorInitCheck.cpp DanglingHandleCheck.cpp DynamicStaticInitializersCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h @@ -0,0 +1,33 @@ +//===--- ChainedComparisonCheck.h - clang-tidy ------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// FIXME: Write a short description. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/chained-comparison.html +class ChainedComparisonCheck : public ClangTidyCheck { +public: + ChainedComparisonCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp @@ -0,0 +1,164 @@ +//===--- ChainedComparisonCheck.cpp - clang-tidy --------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ChainedComparisonCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/ADT/SmallVector.h" +#include +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +bool isExprAComparisonOperator(const Expr *E) { + if (const auto *Op = dyn_cast_or_null(E->IgnoreImplicit())) + return Op->isComparisonOp(); + if (const auto *Op = + dyn_cast_or_null(E->IgnoreImplicit())) + return Op->isComparisonOp(); + return false; +} + +AST_MATCHER(BinaryOperator, + hasBinaryOperatorAChildComparisonOperatorWithoutParen) { + return isExprAComparisonOperator(Node.getLHS()) || + isExprAComparisonOperator(Node.getRHS()); +} + +AST_MATCHER(CXXOperatorCallExpr, + hasCppOperatorAChildComparisonOperatorWithoutParen) { + return std::any_of(Node.arg_begin(), Node.arg_end(), + isExprAComparisonOperator); +} + +constexpr std::array Letters = { + "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", + "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"}; + +struct ChainedComparisonData { + llvm::SmallString<256U> Name; + llvm::SmallVector Operands; + bool Full = false; + + void Add(const Expr *Operand) { + if (Full) + return; + if (!Name.empty()) + Name += ' '; + Name += Letters[Operands.size()]; + Operands.push_back(Operand); + + if (Operands.size() == Letters.size()) { + Name += " ..."; + Full = true; + } + } + + void Add(llvm::StringRef Opcode) { + if (Full) + return; + + Name += ' '; + Name += Opcode.str(); + } +}; + +} // namespace + +static void extractData(const Expr *Op, ChainedComparisonData &Output); + +inline bool extractData(const BinaryOperator *Op, + ChainedComparisonData &Output) { + if (!Op) + return false; + + if (isExprAComparisonOperator(Op->getLHS())) + extractData(Op->getLHS()->IgnoreImplicit(), Output); + else + Output.Add(Op->getLHS()->IgnoreUnlessSpelledInSource()); + + Output.Add(Op->getOpcodeStr()); + + if (isExprAComparisonOperator(Op->getRHS())) + extractData(Op->getRHS()->IgnoreImplicit(), Output); + else + Output.Add(Op->getRHS()->IgnoreUnlessSpelledInSource()); + return true; +} + +inline bool extractData(const CXXOperatorCallExpr *Op, + ChainedComparisonData &Output) { + if (!Op || Op->getNumArgs() != 2U) + return false; + + const Expr *FirstArg = Op->getArg(0U)->IgnoreImplicit(); + if (isExprAComparisonOperator(FirstArg)) + extractData(FirstArg, Output); + else + Output.Add(FirstArg->IgnoreUnlessSpelledInSource()); + + Output.Add(getOperatorSpelling(Op->getOperator())); + + const Expr *SecondArg = Op->getArg(1U)->IgnoreImplicit(); + if (isExprAComparisonOperator(SecondArg)) + extractData(SecondArg, Output); + else + Output.Add(SecondArg->IgnoreUnlessSpelledInSource()); + return true; +} + +static void extractData(const Expr *OpExpr, ChainedComparisonData &Output) { + OpExpr->dump(); + extractData(dyn_cast_or_null(OpExpr), Output) || + extractData(dyn_cast_or_null(OpExpr), Output); +} + +void ChainedComparisonCheck::registerMatchers(MatchFinder *Finder) { + const auto OperatorMatcher = expr(anyOf( + binaryOperator(isComparisonOperator(), + hasBinaryOperatorAChildComparisonOperatorWithoutParen()), + cxxOperatorCallExpr( + isComparisonOperator(), + hasCppOperatorAChildComparisonOperatorWithoutParen()))); + + Finder->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, + expr(unless(isExpansionInSystemHeader()), OperatorMatcher, + unless(hasParent(OperatorMatcher))) + .bind("op")), + this); +} + +void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedOperator = Result.Nodes.getNodeAs("op"); + + ChainedComparisonData Data; + extractData(MatchedOperator, Data); + + if (Data.Operands.empty()) + return; + + diag(MatchedOperator->getBeginLoc(), + "chained comparison '%0' may generate unintended results, use " + "parentheses to specify order of evaluation or a logical operator to " + "separate comparison expressions") + << llvm::StringRef(Data.Name).trim(); + + for (std::size_t Index = 0U; Index < Data.Operands.size(); ++Index) { + diag(Data.Operands[Index]->getBeginLoc(), "operand '%0' is here", + DiagnosticIDs::Note) + << Letters[Index]; + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,6 +101,12 @@ New checks ^^^^^^^^^^ +- New :doc:`bugprone-chained-comparison + ` check. + + Check detects chained comparison operators that can lead to unintended + behavior or logical errors in C++ code. + - New :doc:`bugprone-unsafe-functions ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst @@ -0,0 +1,73 @@ +.. title:: clang-tidy - bugprone-chained-comparison + +bugprone-chained-comparison +=========================== + +Check detects chained comparison operators that can lead to unintended +behavior or logical errors in C++ code. + +Chained comparisons are expressions that use multiple comparison operators +to compare three or more values. For example, the expression ``a < b < c`` +compares the values of ``a``, ``b``, and ``c``. However, this expression does +not evaluate as ``(a < b) && (b < c)``, which is probably what the developer +intended. Instead, it evaluates as ``(a < b) < c``, which may produce +unintended results, especially when the types of ``a``, ``b``, and ``c`` are +different. + +To avoid such errors, the check will issue a warning when a chained +comparison operator is detected, suggesting to use parentheses to specify +the order of evaluation or to use a logical operator to separate comparison +expressions. + +Consider the following examples: + +.. code-block:: c++ + + int a = 2, b = 6, c = 4; + if (a < b < c) { + // This block will be executed + } + + +In this example, the developer intended to check if ``a`` is less than ``b`` +and ``b`` is less than ``c``. However, the expression ``a < b < c`` is +equivalent to ``(a < b) < c``. Since ``a < b`` is ``true``, the expression +``(a < b) < c`` is evaluated as ``1 < c``, which is equivalent to ``true < c`` +and is invalid in this case as ``b < c`` is ``false``. + +Even that above issue could be detected as comparison of ``int`` to ``bool``, +there is more dangerous example: + +.. code-block:: c++ + + bool a = false, b = false, c = true; + if (a == b == c) { + // This block will be executed + } + +In this example, the developer intended to check if ``a``, ``b``, and ``c`` are +all equal. However, the expression ``a == b == c`` is evaluated as +``(a == b) == c``. Since ``a == b`` is true, the expression ``(a == b) == c`` +is evaluated as ``true == c``, which is equivalent to ``true == true``. +This comparison yields ``true``, even though ``a`` and ``b`` are ``false``, and +are not equal to ``c``. + +To avoid this issue, the developer can use a logical operator to separate the +comparison expressions, like this: + +.. code-block:: c++ + + if (a == b && b == c) { + // This block will not be executed + } + + +Alternatively, use of parentheses in the comparison expressions can make the +developer's intention more explicit and help avoid misunderstanding. + +.. code-block:: c++ + + if ((a == b) == c) { + // This block will be executed + } + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -82,6 +82,7 @@ `bugprone-bad-signal-to-kill-thread `_, `bugprone-bool-pointer-implicit-conversion `_, "Yes" `bugprone-branch-clone `_, + `bugprone-chained-comparison `_, `bugprone-copy-constructor-init `_, "Yes" `bugprone-dangling-handle `_, `bugprone-dynamic-static-initializers `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp @@ -0,0 +1,91 @@ +// RUN: %check_clang_tidy -std=c++98-or-later %s bugprone-chained-comparison %t + +void badly_chained_1(int x, int y, int z) +{ + bool result = x < y < z; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_2(int x, int y, int z) +{ + bool result = x <= y <= z; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a <= b <= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_3(int x, int y, int z) +{ + bool result = x > y > z; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a > b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_4(int x, int y, int z) +{ + bool result = x >= y >= z; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a >= b >= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_5(int x, int y, int z) +{ + bool result = x == y != z; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b != c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_6(bool x, bool y, bool z) +{ + bool result = x != y == z; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a != b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_multiple(bool a, bool b, bool c, bool d, bool e, bool f, bool g, bool h) +{ + bool result = a == b == c == d == e == f == g == h; +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_limit(bool v[29]) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h == i == j == k == l == m == n == o == p == q == r == s == t == u == v == w == x == y == z ...' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + bool result = v[0] == v[1] == v[2] == v[3] == v[4] == v[5] == v[6] == v[7] == + v[8] == v[9] == v[10] == v[11] == v[12] == v[13] == v[14] == + v[15] == v[16] == v[17] == v[18] == v[19] == v[20] == v[21] == + v[22] == v[23] == v[24] == v[25] == v[26] == v[27] == v[28]; + +} + +void badly_chained_parens2(int x, int y, int z, int t, int a, int b) +{ + bool result = (x < y) < (z && t) > (a == b); +} +// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void badly_chained_inner(int x, int y, int z, int t, int u) +{ + bool result = x && y < z < t && u; +} +// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +void properly_chained_1(int x, int y, int z) +{ + bool result = x < y && y < z; +} + +void properly_chained_2(int x, int y, bool z) +{ + bool result = (x < y) == z; +} + +struct Value { + bool operator<(const Value&) const; +}; + +bool operator==(bool, const Value&); + +bool badWithCppOperator(Value a, Value b, Value c) { + return a < b == c; +} +// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'a < b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison] + +bool mixedBinaryAndCpp(Value a, Value b, bool c) { + return a < b == c; +} +// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'a < b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]