Skip to content

Commit 06c3c3b

Browse files
PiotrZSLPiotrZSL
authored
[clang-tidy] Add bugprone-chained-comparison check (#76365)
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).
1 parent 27eb8d5 commit 06c3c3b

File tree

9 files changed

+435
-0
lines changed

9 files changed

+435
-0
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "BoolPointerImplicitConversionCheck.h"
1818
#include "BranchCloneCheck.h"
1919
#include "CastingThroughVoidCheck.h"
20+
#include "ChainedComparisonCheck.h"
2021
#include "ComparePointerToMemberVirtualFunctionCheck.h"
2122
#include "CopyConstructorInitCheck.h"
2223
#include "DanglingHandleCheck.h"
@@ -108,6 +109,8 @@ class BugproneModule : public ClangTidyModule {
108109
CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
109110
CheckFactories.registerCheck<CastingThroughVoidCheck>(
110111
"bugprone-casting-through-void");
112+
CheckFactories.registerCheck<ChainedComparisonCheck>(
113+
"bugprone-chained-comparison");
111114
CheckFactories.registerCheck<ComparePointerToMemberVirtualFunctionCheck>(
112115
"bugprone-compare-pointer-to-member-virtual-function");
113116
CheckFactories.registerCheck<CopyConstructorInitCheck>(

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ add_clang_library(clangTidyBugproneModule
1212
BranchCloneCheck.cpp
1313
BugproneTidyModule.cpp
1414
CastingThroughVoidCheck.cpp
15+
ChainedComparisonCheck.cpp
1516
ComparePointerToMemberVirtualFunctionCheck.cpp
1617
CopyConstructorInitCheck.cpp
1718
DanglingHandleCheck.cpp

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

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
//===--- ChainedComparisonCheck.cpp - clang-tidy --------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "ChainedComparisonCheck.h"
10+
#include "clang/AST/ASTContext.h"
11+
#include "clang/ASTMatchers/ASTMatchFinder.h"
12+
#include "llvm/ADT/SmallString.h"
13+
#include "llvm/ADT/SmallVector.h"
14+
#include <algorithm>
15+
16+
using namespace clang::ast_matchers;
17+
18+
namespace clang::tidy::bugprone {
19+
static bool isExprAComparisonOperator(const Expr *E) {
20+
if (const auto *Op = dyn_cast_or_null<BinaryOperator>(E->IgnoreImplicit()))
21+
return Op->isComparisonOp();
22+
if (const auto *Op =
23+
dyn_cast_or_null<CXXOperatorCallExpr>(E->IgnoreImplicit()))
24+
return Op->isComparisonOp();
25+
return false;
26+
}
27+
28+
namespace {
29+
AST_MATCHER(BinaryOperator,
30+
hasBinaryOperatorAChildComparisonOperatorWithoutParen) {
31+
return isExprAComparisonOperator(Node.getLHS()) ||
32+
isExprAComparisonOperator(Node.getRHS());
33+
}
34+
35+
AST_MATCHER(CXXOperatorCallExpr,
36+
hasCppOperatorAChildComparisonOperatorWithoutParen) {
37+
return std::any_of(Node.arg_begin(), Node.arg_end(),
38+
isExprAComparisonOperator);
39+
}
40+
41+
struct ChainedComparisonData {
42+
llvm::SmallString<256U> Name;
43+
llvm::SmallVector<const Expr *, 32U> Operands;
44+
45+
explicit ChainedComparisonData(const Expr *Op) { extract(Op); }
46+
47+
private:
48+
void add(const Expr *Operand);
49+
void add(llvm::StringRef Opcode);
50+
void extract(const Expr *Op);
51+
void extract(const BinaryOperator *Op);
52+
void extract(const CXXOperatorCallExpr *Op);
53+
};
54+
55+
void ChainedComparisonData::add(const Expr *Operand) {
56+
if (!Name.empty())
57+
Name += ' ';
58+
Name += 'v';
59+
Name += std::to_string(Operands.size());
60+
Operands.push_back(Operand);
61+
}
62+
63+
void ChainedComparisonData::add(llvm::StringRef Opcode) {
64+
Name += ' ';
65+
Name += Opcode;
66+
}
67+
68+
void ChainedComparisonData::extract(const BinaryOperator *Op) {
69+
const Expr *LHS = Op->getLHS()->IgnoreImplicit();
70+
if (isExprAComparisonOperator(LHS))
71+
extract(LHS);
72+
else
73+
add(LHS);
74+
75+
add(Op->getOpcodeStr());
76+
77+
const Expr *RHS = Op->getRHS()->IgnoreImplicit();
78+
if (isExprAComparisonOperator(RHS))
79+
extract(RHS);
80+
else
81+
add(RHS);
82+
}
83+
84+
void ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) {
85+
const Expr *FirstArg = Op->getArg(0U)->IgnoreImplicit();
86+
if (isExprAComparisonOperator(FirstArg))
87+
extract(FirstArg);
88+
else
89+
add(FirstArg);
90+
91+
add(getOperatorSpelling(Op->getOperator()));
92+
93+
const Expr *SecondArg = Op->getArg(1U)->IgnoreImplicit();
94+
if (isExprAComparisonOperator(SecondArg))
95+
extract(SecondArg);
96+
else
97+
add(SecondArg);
98+
}
99+
100+
void ChainedComparisonData::extract(const Expr *Op) {
101+
if (!Op)
102+
return;
103+
104+
if (const auto *BinaryOp = dyn_cast<BinaryOperator>(Op)) {
105+
extract(BinaryOp);
106+
return;
107+
}
108+
109+
if (const auto *OverloadedOp = dyn_cast<CXXOperatorCallExpr>(Op)) {
110+
if (OverloadedOp->getNumArgs() == 2U)
111+
extract(OverloadedOp);
112+
}
113+
}
114+
115+
} // namespace
116+
117+
void ChainedComparisonCheck::registerMatchers(MatchFinder *Finder) {
118+
const auto OperatorMatcher = expr(anyOf(
119+
binaryOperator(isComparisonOperator(),
120+
hasBinaryOperatorAChildComparisonOperatorWithoutParen()),
121+
cxxOperatorCallExpr(
122+
isComparisonOperator(),
123+
hasCppOperatorAChildComparisonOperatorWithoutParen())));
124+
125+
Finder->addMatcher(
126+
expr(OperatorMatcher, unless(hasParent(OperatorMatcher))).bind("op"),
127+
this);
128+
}
129+
130+
void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) {
131+
const auto *MatchedOperator = Result.Nodes.getNodeAs<Expr>("op");
132+
133+
ChainedComparisonData Data(MatchedOperator);
134+
if (Data.Operands.empty())
135+
return;
136+
137+
diag(MatchedOperator->getBeginLoc(),
138+
"chained comparison '%0' may generate unintended results, use "
139+
"parentheses to specify order of evaluation or a logical operator to "
140+
"separate comparison expressions")
141+
<< llvm::StringRef(Data.Name).trim() << MatchedOperator->getSourceRange();
142+
143+
for (std::size_t Index = 0U; Index < Data.Operands.size(); ++Index) {
144+
diag(Data.Operands[Index]->getBeginLoc(), "operand 'v%0' is here",
145+
DiagnosticIDs::Note)
146+
<< Index << Data.Operands[Index]->getSourceRange();
147+
}
148+
}
149+
150+
} // namespace clang::tidy::bugprone

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
//===--- ChainedComparisonCheck.h - clang-tidy ------------------*- C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H
11+
12+
#include "../ClangTidyCheck.h"
13+
14+
namespace clang::tidy::bugprone {
15+
16+
/// Check detects chained comparison operators that can lead to unintended
17+
/// behavior or logical errors.
18+
///
19+
/// For the user-facing documentation see:
20+
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/chained-comparison.html
21+
class ChainedComparisonCheck : public ClangTidyCheck {
22+
public:
23+
ChainedComparisonCheck(StringRef Name, ClangTidyContext *Context)
24+
: ClangTidyCheck(Name, Context) {}
25+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
26+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
27+
std::optional<TraversalKind> getCheckTraversalKind() const override {
28+
return TK_IgnoreUnlessSpelledInSource;
29+
}
30+
};
31+
32+
} // namespace clang::tidy::bugprone
33+
34+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,12 @@ New checks
143143

144144
Detects unsafe or redundant two-step casting operations involving ``void*``.
145145

146+
- New :doc:`bugprone-chained-comparison
147+
<clang-tidy/checks/bugprone/chained-comparison>` check.
148+
149+
Check detects chained comparison operators that can lead to unintended
150+
behavior or logical errors.
151+
146152
- New :doc:`bugprone-compare-pointer-to-member-virtual-function
147153
<clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function>` check.
148154

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
.. title:: clang-tidy - bugprone-chained-comparison
2+
3+
bugprone-chained-comparison
4+
===========================
5+
6+
Check detects chained comparison operators that can lead to unintended
7+
behavior or logical errors.
8+
9+
Chained comparisons are expressions that use multiple comparison operators
10+
to compare three or more values. For example, the expression ``a < b < c``
11+
compares the values of ``a``, ``b``, and ``c``. However, this expression does
12+
not evaluate as ``(a < b) && (b < c)``, which is probably what the developer
13+
intended. Instead, it evaluates as ``(a < b) < c``, which may produce
14+
unintended results, especially when the types of ``a``, ``b``, and ``c`` are
15+
different.
16+
17+
To avoid such errors, the check will issue a warning when a chained
18+
comparison operator is detected, suggesting to use parentheses to specify
19+
the order of evaluation or to use a logical operator to separate comparison
20+
expressions.
21+
22+
Consider the following examples:
23+
24+
.. code-block:: c++
25+
26+
int a = 2, b = 6, c = 4;
27+
if (a < b < c) {
28+
// This block will be executed
29+
}
30+
31+
32+
In this example, the developer intended to check if ``a`` is less than ``b``
33+
and ``b`` is less than ``c``. However, the expression ``a < b < c`` is
34+
equivalent to ``(a < b) < c``. Since ``a < b`` is ``true``, the expression
35+
``(a < b) < c`` is evaluated as ``1 < c``, which is equivalent to ``true < c``
36+
and is invalid in this case as ``b < c`` is ``false``.
37+
38+
Even that above issue could be detected as comparison of ``int`` to ``bool``,
39+
there is more dangerous example:
40+
41+
.. code-block:: c++
42+
43+
bool a = false, b = false, c = true;
44+
if (a == b == c) {
45+
// This block will be executed
46+
}
47+
48+
In this example, the developer intended to check if ``a``, ``b``, and ``c`` are
49+
all equal. However, the expression ``a == b == c`` is evaluated as
50+
``(a == b) == c``. Since ``a == b`` is true, the expression ``(a == b) == c``
51+
is evaluated as ``true == c``, which is equivalent to ``true == true``.
52+
This comparison yields ``true``, even though ``a`` and ``b`` are ``false``, and
53+
are not equal to ``c``.
54+
55+
To avoid this issue, the developer can use a logical operator to separate the
56+
comparison expressions, like this:
57+
58+
.. code-block:: c++
59+
60+
if (a == b && b == c) {
61+
// This block will not be executed
62+
}
63+
64+
65+
Alternatively, use of parentheses in the comparison expressions can make the
66+
developer's intention more explicit and help avoid misunderstanding.
67+
68+
.. code-block:: c++
69+
70+
if ((a == b) == c) {
71+
// This block will be executed
72+
}
73+

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ Clang-Tidy Checks
8383
:doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes"
8484
:doc:`bugprone-branch-clone <bugprone/branch-clone>`,
8585
:doc:`bugprone-casting-through-void <bugprone/casting-through-void>`,
86+
:doc:`bugprone-chained-comparison <bugprone/chained-comparison>`,
8687
:doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`,
8788
:doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
8889
:doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// RUN: %check_clang_tidy %s bugprone-chained-comparison %t
2+
3+
void badly_chained_1(int x, int y, int z)
4+
{
5+
int result = x < y < z;
6+
}
7+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 < v1 < v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
8+
9+
void badly_chained_2(int x, int y, int z)
10+
{
11+
int result = x <= y <= z;
12+
}
13+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 <= v1 <= v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
14+
15+
void badly_chained_3(int x, int y, int z)
16+
{
17+
int result = x > y > z;
18+
}
19+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 > v1 > v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
20+
21+
void badly_chained_4(int x, int y, int z)
22+
{
23+
int result = x >= y >= z;
24+
}
25+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 >= v1 >= v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
26+
27+
void badly_chained_5(int x, int y, int z)
28+
{
29+
int result = x == y != z;
30+
}
31+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 == v1 != v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
32+
33+
void badly_chained_6(int x, int y, int z)
34+
{
35+
int result = x != y == z;
36+
}
37+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 != v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
38+
39+
void badly_chained_multiple(int a, int b, int c, int d, int e, int f, int g, int h)
40+
{
41+
int result = a == b == c == d == e == f == g == h;
42+
}
43+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 == v1 == v2 == v3 == v4 == v5 == v6 == v7' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
44+
45+
void badly_chained_limit(int v[29])
46+
{
47+
// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: chained comparison 'v0 == v1 == v2 == v3 == v4 == v5 == v6 == v7 == v8 == v9 == v10 == v11 == v12 == v13 == v14 == v15 == v16 == v17 == v18 == v19 == v20 == v21 == v22 == v23 == v24 == v25 == v26 == v27 == v28' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
48+
int result = v[0] == v[1] == v[2] == v[3] == v[4] == v[5] == v[6] == v[7] ==
49+
v[8] == v[9] == v[10] == v[11] == v[12] == v[13] == v[14] ==
50+
v[15] == v[16] == v[17] == v[18] == v[19] == v[20] == v[21] ==
51+
v[22] == v[23] == v[24] == v[25] == v[26] == v[27] == v[28];
52+
53+
}
54+
55+
void badly_chained_parens2(int x, int y, int z, int t, int a, int b)
56+
{
57+
int result = (x < y) < (z && t) > (a == b);
58+
}
59+
// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 < v1 > v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
60+
61+
void badly_chained_inner(int x, int y, int z, int t, int u)
62+
{
63+
int result = x && y < z < t && u;
64+
}
65+
// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: chained comparison 'v0 < v1 < v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
66+
67+
void properly_chained_1(int x, int y, int z)
68+
{
69+
int result = x < y && y < z;
70+
}
71+
72+
void properly_chained_2(int x, int y, int z)
73+
{
74+
int result = (x < y) == z;
75+
}
76+

0 commit comments

Comments
 (0)