Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jan 22, 2024

Conversation

PiotrZSL
Copy link
Member

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.

Moved from Phabricator (D144429).

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.
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 25, 2023

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

Changes

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.

Moved from Phabricator (D144429).


Full diff: https://github.com/llvm/llvm-project/pull/76365.diff

8 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3)
  • (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1)
  • (added) clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp (+161)
  • (added) clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h (+37)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
  • (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst (+73)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
  • (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp (+91)
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 435cb1e3fbcff3..a8a23b045f80bb 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -17,6 +17,7 @@
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CastingThroughVoidCheck.h"
+#include "ChainedComparisonCheck.h"
 #include "ComparePointerToMemberVirtualFunctionCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
@@ -108,6 +109,8 @@ class BugproneModule : public ClangTidyModule {
     CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
     CheckFactories.registerCheck<CastingThroughVoidCheck>(
         "bugprone-casting-through-void");
+    CheckFactories.registerCheck<ChainedComparisonCheck>(
+        "bugprone-chained-comparison");
     CheckFactories.registerCheck<ComparePointerToMemberVirtualFunctionCheck>(
         "bugprone-compare-pointer-to-member-virtual-function");
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 70e7fbc7ec0c14..1cd6fb207d7625 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -12,6 +12,7 @@ add_clang_library(clangTidyBugproneModule
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
   CastingThroughVoidCheck.cpp
+  ChainedComparisonCheck.cpp
   ComparePointerToMemberVirtualFunctionCheck.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
new file mode 100644
index 00000000000000..07f65d062e51cf
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
@@ -0,0 +1,161 @@
+//===--- 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(
+      expr(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
diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
new file mode 100644
index 00000000000000..953ab63298df7a
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
@@ -0,0 +1,37 @@
+//===--- 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 {
+
+/// Check detects chained comparison operators that can lead to unintended
+/// behavior or logical errors in C++ code.
+///
+/// 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;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6e7554e0433c25..8410ea7e14cda6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,12 @@ New checks
 
   Detects unsafe or redundant two-step casting operations involving ``void*``.
 
+- 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-compare-pointer-to-member-virtual-function
   <clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function>` 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
index 00000000000000..fb704c1eec68b4
--- /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
index 39d8b490d927cc..bcfc68a04562f2 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -83,6 +83,7 @@ Clang-Tidy Checks
    :doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes"
    :doc:`bugprone-branch-clone <bugprone/branch-clone>`,
    :doc:`bugprone-casting-through-void <bugprone/casting-through-void>`,
+   :doc:`bugprone-chained-comparison <bugprone/chained-comparison>`,
    :doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`,
    :doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
    :doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,
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
index 00000000000000..dea1ee732aa01e
--- /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]

@PiotrZSL PiotrZSL self-assigned this Jan 7, 2024
Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Except I think this check is available for C also.

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some small suggestions.

Copy link

github-actions bot commented Jan 21, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (-formatting)

@PiotrZSL PiotrZSL merged commit 06c3c3b into llvm:main Jan 22, 2024
5 checks passed
@PiotrZSL PiotrZSL deleted the bugprone-chained-comparison branch January 22, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants