[clang-tidy] Add bugprone-chained-comparison check

[PiotrZSL]

Summary

Check that flags chained comparison expressions,
such as a < b < c or a == b == c, which may have
unintended behavior due to implicit operator
associativity.

[PiotrZSL]

Note that looks like there is currently in implementation new warning -Wcomparison-op-parentheses under D142800, that warning currently handle only chained comparison by using BinaryOperator, not by using CXXOperatorCallExpr.
Scope of "-Wcomparison-op-parentheses" and whatever that warning will be delivered in LLVM 17 will determinate future of this check.
This check is feature-complete.

[PiotrZSL]

Ping, Rebase

[PiotrZSL]

Rebase due to broken baseline

[PiotrZSL]

Ping, Rebase

[PiotrZSL]

Ping

[PiotrZSL]

Ping

[PiotrZSL]

Rebase + Ping

[PiotrZSL]

Ping

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "../cppcoreguidelines/NarrowingConversionsCheck.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignmentInIfConditionCheck.h"
 #include "BadSignalToKillThreadCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
+#include "ChainedComparisonCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
CheckFactories.registerCheck<AssertSideEffectCheck>(
         "bugprone-assert-side-effect");
     CheckFactories.registerCheck<AssignmentInIfConditionCheck>(
         "bugprone-assignment-in-if-condition");
     CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
         "bugprone-bad-signal-to-kill-thread");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "bugprone-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
+    CheckFactories.registerCheck<ChainedComparisonCheck>(
+        "bugprone-chained-comparison");
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<EasilySwappableParametersCheck>(
         "bugprone-easily-swappable-parameters");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   FrontendOpenMP
   )
 
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignmentInIfConditionCheck.cpp
   BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
+  ChainedComparisonCheck.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp

clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp

+//===--- 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 <algorithm>
+#include <array>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+bool isExprAComparisonOperator(const Expr *E) {
+  if (const auto *Op = dyn_cast_or_null<BinaryOperator>(E->IgnoreImplicit()))
+    return Op->isComparisonOp();
+  if (const auto *Op =
+          dyn_cast_or_null<CXXOperatorCallExpr>(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<llvm::StringRef, 26U> 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<const Expr *, 26U> 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<BinaryOperator>(OpExpr), Output) ||
+      extractData(dyn_cast_or_null<CXXOperatorCallExpr>(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<Expr>("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

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New global configuration file options `HeaderFileExtensions` and
   `ImplementationFileExtensions`, replacing the check-local options of the
   same name.
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-chained-comparison
+  <clang-tidy/checks/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
   <clang-tidy/checks/bugprone/unsafe-functions>` check.
 
   Checks for functions that have safer, more secure replacements available, or
   are considered deprecated due to design flaws.
   This check relies heavily on, but is not exclusive to, the functions from
   the *Annex K. "Bounds-checking interfaces"* of C11.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst

+.. 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
+    }
+

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `android-comparison-in-temp-failure-retry <android/comparison-in-temp-failure-retry.html>`_,
    `boost-use-to-string <boost/use-to-string.html>`_, "Yes"
    `bugprone-argument-comment <bugprone/argument-comment.html>`_, "Yes"
    `bugprone-assert-side-effect <bugprone/assert-side-effect.html>`_,
    `bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition.html>`_,
    `bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread.html>`_,
    `bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion.html>`_, "Yes"
    `bugprone-branch-clone <bugprone/branch-clone.html>`_,
+   `bugprone-chained-comparison <bugprone/chained-comparison.html>`_,
    `bugprone-copy-constructor-init <bugprone/copy-constructor-init.html>`_, "Yes"
    `bugprone-dangling-handle <bugprone/dangling-handle.html>`_,
    `bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers.html>`_,
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.html>`_,
    `bugprone-exception-escape <bugprone/exception-escape.html>`_,
    `bugprone-fold-init-type <bugprone/fold-init-type.html>`_,
    `bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace.html>`_,
    `bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp

+// 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]