Skip to content

Commit cfdd8bc

Browse files
balazskebalazske
authored
[clang-tidy] Add check 'bugprone-assignment-in-selection-statement' (llvm#180219)
1 parent 8944608 commit cfdd8bc

File tree

11 files changed

+435
-0
lines changed

11 files changed

+435
-0
lines changed

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

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
//===----------------------------------------------------------------------===//
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 "AssignmentInSelectionStatementCheck.h"
10+
#include "clang/AST/IgnoreExpr.h"
11+
#include "clang/AST/StmtVisitor.h"
12+
#include "clang/ASTMatchers/ASTMatchFinder.h"
13+
#include "llvm/ADT/TypeSwitch.h"
14+
15+
using namespace clang;
16+
using namespace clang::ast_matchers;
17+
18+
namespace {
19+
20+
class ConditionValueCanPropagateFrom
21+
: public ConstStmtVisitor<ConditionValueCanPropagateFrom, void> {
22+
public:
23+
llvm::SmallVector<const Expr *, 2> ExprToProcess;
24+
25+
void VisitBinaryOperator(const BinaryOperator *BO) {
26+
if (BO->isCommaOp())
27+
ExprToProcess.push_back(BO->getRHS()->IgnoreParenImpCasts());
28+
}
29+
void VisitAbstractConditionalOperator(const AbstractConditionalOperator *CO) {
30+
ExprToProcess.push_back(CO->getFalseExpr()->IgnoreParenImpCasts());
31+
ExprToProcess.push_back(CO->getTrueExpr()->IgnoreParenImpCasts());
32+
}
33+
};
34+
35+
AST_MATCHER_P(Expr, conditionValueCanPropagateFrom,
36+
ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
37+
bool Found = false;
38+
ConditionValueCanPropagateFrom Visitor;
39+
Visitor.Visit(&Node); // Do not match Node itself.
40+
while (!Visitor.ExprToProcess.empty()) {
41+
const Expr *E = Visitor.ExprToProcess.pop_back_val();
42+
ast_matchers::internal::BoundNodesTreeBuilder Result;
43+
if (InnerMatcher.matches(*E, Finder, &Result)) {
44+
Found = true;
45+
Builder->addMatch(Result);
46+
}
47+
Visitor.Visit(E);
48+
}
49+
return Found;
50+
}
51+
52+
// Ignore implicit casts (including C++ conversion member calls) but not parens.
53+
AST_MATCHER_P(Expr, ignoringImplicitAsWritten,
54+
ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
55+
auto IgnoreImplicitMemberCallSingleStep = [](Expr *E) {
56+
if (auto *C = dyn_cast<CXXMemberCallExpr>(E)) {
57+
Expr *ExprNode = C->getImplicitObjectArgument();
58+
if (ExprNode->getSourceRange() == E->getSourceRange())
59+
return ExprNode;
60+
ExprNode = ExprNode->IgnoreParenImpCasts();
61+
if (ExprNode->getSourceRange() == E->getSourceRange())
62+
return ExprNode;
63+
}
64+
return E;
65+
};
66+
67+
const Expr *IgnoreE = IgnoreExprNodes(&Node, IgnoreImplicitSingleStep,
68+
IgnoreImplicitCastsExtraSingleStep,
69+
IgnoreImplicitMemberCallSingleStep);
70+
71+
return InnerMatcher.matches(*IgnoreE, Finder, Builder);
72+
}
73+
74+
} // namespace
75+
76+
namespace clang::tidy::bugprone {
77+
78+
void AssignmentInSelectionStatementCheck::registerMatchers(
79+
MatchFinder *Finder) {
80+
auto AssignOpNoParens = ignoringImplicitAsWritten(
81+
binaryOperation(hasOperatorName("=")).bind("assignment"));
82+
auto AssignOpMaybeParens = ignoringParenImpCasts(
83+
binaryOperation(hasOperatorName("=")).bind("assignment"));
84+
auto AssignOpFromEmbeddedExpr = expr(ignoringParenImpCasts(
85+
conditionValueCanPropagateFrom(AssignOpMaybeParens)));
86+
87+
auto CondExprWithAssign = anyOf(AssignOpNoParens, AssignOpFromEmbeddedExpr);
88+
auto OpCondExprWithAssign =
89+
anyOf(AssignOpMaybeParens, AssignOpFromEmbeddedExpr);
90+
91+
// In these cases "single primary expression" is possible.
92+
// A single assignment within a 'ParenExpr' is allowed (but not if mixed with
93+
// other operators).
94+
auto FoundControlStmt = mapAnyOf(ifStmt, whileStmt, doStmt, forStmt)
95+
.with(hasCondition(CondExprWithAssign));
96+
// In these cases "single primary expression" is not possible because the
97+
// assignment is already part of a bigger expression.
98+
auto FoundConditionalOperator =
99+
mapAnyOf(conditionalOperator, binaryConditionalOperator)
100+
.with(hasCondition(OpCondExprWithAssign));
101+
auto FoundLogicalOp = binaryOperator(
102+
hasAnyOperatorName("&&", "||"),
103+
eachOf(hasLHS(OpCondExprWithAssign), hasRHS(OpCondExprWithAssign)));
104+
105+
auto FoundSelectionStmt =
106+
stmt(anyOf(FoundControlStmt, FoundConditionalOperator, FoundLogicalOp))
107+
.bind("parent");
108+
109+
Finder->addMatcher(FoundSelectionStmt, this);
110+
}
111+
112+
void AssignmentInSelectionStatementCheck::check(
113+
const MatchFinder::MatchResult &Result) {
114+
const auto *FoundAssignment = Result.Nodes.getNodeAs<Stmt>("assignment");
115+
assert(FoundAssignment);
116+
117+
const auto *ParentStmt = Result.Nodes.getNodeAs<Stmt>("parent");
118+
const StringRef CondStr =
119+
llvm::TypeSwitch<const Stmt *, const char *>(ParentStmt)
120+
.Case([](const IfStmt *) { return "condition of 'if' statement"; })
121+
.Case<WhileStmt, DoStmt, ForStmt>(
122+
[](const Stmt *) { return "condition of a loop"; })
123+
.Case([](const ConditionalOperator *) {
124+
return "condition of a ternary operator";
125+
})
126+
.Case([](const BinaryOperator *) {
127+
return "operand of a logical operator";
128+
})
129+
.DefaultUnreachable();
130+
131+
const SourceLocation OpLoc =
132+
llvm::TypeSwitch<const Stmt *, SourceLocation>(FoundAssignment)
133+
.Case<BinaryOperator, CXXOperatorCallExpr>(
134+
[](const auto *Op) { return Op->getOperatorLoc(); })
135+
.Default(FoundAssignment->getBeginLoc());
136+
diag(OpLoc, "assignment within %0 may indicate programmer error")
137+
<< FoundAssignment->getSourceRange() << CondStr;
138+
diag(OpLoc, "if it should be an assignment, move it out of the condition",
139+
DiagnosticIDs::Note);
140+
diag(OpLoc, "if it is meant to be an equality check, change '=' to '=='",
141+
DiagnosticIDs::Note);
142+
}
143+
144+
} // namespace clang::tidy::bugprone

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
//===----------------------------------------------------------------------===//
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_ASSIGNMENTINSELECTIONSTATEMENTCHECK_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINSELECTIONSTATEMENTCHECK_H
11+
12+
#include "../ClangTidyCheck.h"
13+
14+
namespace clang::tidy::bugprone {
15+
16+
/// Finds assignments within selection statements.
17+
///
18+
/// For the user-facing documentation see:
19+
/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/assignment-in-selection-statement.html
20+
class AssignmentInSelectionStatementCheck : public ClangTidyCheck {
21+
public:
22+
AssignmentInSelectionStatementCheck(StringRef Name, ClangTidyContext *Context)
23+
: ClangTidyCheck(Name, Context) {}
24+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
25+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
26+
};
27+
28+
} // namespace clang::tidy::bugprone
29+
30+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINSELECTIONSTATEMENTCHECK_H

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "ArgumentCommentCheck.h"
1212
#include "AssertSideEffectCheck.h"
1313
#include "AssignmentInIfConditionCheck.h"
14+
#include "AssignmentInSelectionStatementCheck.h"
1415
#include "BadSignalToKillThreadCheck.h"
1516
#include "BitwisePointerCastCheck.h"
1617
#include "BoolPointerImplicitConversionCheck.h"
@@ -127,6 +128,8 @@ class BugproneModule : public ClangTidyModule {
127128
"bugprone-assert-side-effect");
128129
CheckFactories.registerCheck<AssignmentInIfConditionCheck>(
129130
"bugprone-assignment-in-if-condition");
131+
CheckFactories.registerCheck<AssignmentInSelectionStatementCheck>(
132+
"bugprone-assignment-in-selection-statement");
130133
CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
131134
"bugprone-bad-signal-to-kill-thread");
132135
CheckFactories.registerCheck<BitwisePointerCastCheck>(

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ add_clang_library(clangTidyBugproneModule STATIC
77
ArgumentCommentCheck.cpp
88
AssertSideEffectCheck.cpp
99
AssignmentInIfConditionCheck.cpp
10+
AssignmentInSelectionStatementCheck.cpp
1011
BadSignalToKillThreadCheck.cpp
1112
BitwisePointerCastCheck.cpp
1213
BoolPointerImplicitConversionCheck.cpp

‎clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include "../ClangTidy.h"
1010
#include "../ClangTidyModule.h"
11+
#include "../bugprone/AssignmentInSelectionStatementCheck.h"
1112
#include "../bugprone/BadSignalToKillThreadCheck.h"
1213
#include "../bugprone/CommandProcessorCheck.h"
1314
#include "../bugprone/CopyConstructorMutatesArgumentCheck.h"
@@ -308,6 +309,8 @@ class CERTModule : public ClangTidyModule {
308309
// EXP
309310
CheckFactories.registerCheck<bugprone::SuspiciousMemoryComparisonCheck>(
310311
"cert-exp42-c");
312+
CheckFactories.registerCheck<bugprone::AssignmentInSelectionStatementCheck>(
313+
"cert-exp45-c");
311314
// FLP
312315
CheckFactories.registerCheck<bugprone::FloatLoopCounterCheck>(
313316
"cert-flp30-c");

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,11 @@ Improvements to clang-tidy
114114
New checks
115115
^^^^^^^^^^
116116

117+
- New :doc:`bugprone-assignment-in-selection-statement
118+
<clang-tidy/checks/bugprone/assignment-in-selection-statement>` check.
119+
120+
Finds assignments within selection statements.
121+
117122
- New :doc:`bugprone-unsafe-to-allow-exceptions
118123
<clang-tidy/checks/bugprone/unsafe-to-allow-exceptions>` check.
119124

@@ -178,6 +183,10 @@ New checks
178183
New check aliases
179184
^^^^^^^^^^^^^^^^^
180185

186+
- New alias :doc:`cert-exp45-c <clang-tidy/checks/cert/exp45-c>`
187+
to :doc:`bugprone-assignment-in-selection-statement
188+
<clang-tidy/checks/bugprone/assignment-in-selection-statement>`.
189+
181190
- Renamed :doc:`hicpp-exception-baseclass
182191
<clang-tidy/checks/hicpp/exception-baseclass>`
183192
to :doc:`bugprone-std-exception-baseclass

‎clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-selection-statement.rst‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
.. title:: clang-tidy - bugprone-assignment-in-selection-statement
2+
3+
bugprone-assignment-in-selection-statement
4+
==========================================
5+
6+
Finds assignments within selection statements.
7+
Such assignments may indicate programmer error because they may have been
8+
intended as equality tests. The selection statements are conditions of ``if``
9+
and loop (``for``, ``while``, ``do``) statements, condition of conditional
10+
operator (``?:``) and any operand of a binary logical operator (``&&``,
11+
``||``). The check finds assignments within these contexts if the single
12+
expression is an assignment or the assignment is contained (recursively) in
13+
last operand of a comma (``,``) operator or true and false expressions in a
14+
conditional operator. The warning is suppressed if the assignment is placed in
15+
extra parentheses, but only if the assignment is the single expression of a
16+
condition (of ``if`` or a loop statement).
17+
18+
This check corresponds to the CERT rule
19+
`EXP45-C. Do not perform assignments in selection statements
20+
<https://wiki.sei.cmu.edu/confluence/spaces/c/pages/87152228/EXP45-C.+Do+not+perform+assignments+in+selection+statements>`_.
21+
22+
Examples
23+
========
24+
25+
The check emits a warning in the following cases at the indicated locations:
26+
27+
.. code-block:: c++
28+
29+
int x = 3;
30+
31+
if (x = 4) // should it be `x == 4` instead of 'x = 4' ?
32+
x = x + 1;
33+
34+
while ((x <= 11) || (x = 22)) // assignment appears as operand of a logical operator
35+
x += 2;
36+
37+
do {
38+
x += 5;
39+
} while ((x > 10) ? (x = 11) : (x > 5)); // assignment in loop condition (from `x = 11`)
40+
41+
for (int i = 0; i == 2, x = 5; ++i) // assignment in loop condition (from last operand of comma)
42+
foo1(i, x);
43+
44+
for (int i = 0; i == 2, (x = 5); ++i) // assignment is not a single expression, parentheses do not prevent the warning
45+
foo1(i, x);
46+
47+
int a = (x == 2) || (x = 3); // assignment appears in the operand a logical operator
48+
49+
The following cases do not produce a warning:
50+
51+
.. code-block:: c++
52+
53+
if ((x = 1)) { // a single assignment between parentheses
54+
x += 10;
55+
56+
if ((x = 1) != 0) { // assignment appears in a complex expression and without a logical operator
57+
++x;
58+
59+
if (foo(x = 9) && array[x = 8]) { // assignment appears in argument of function call or array index
60+
++x;
61+
62+
for (int i = 0; i = 2, x == 5; ++i) // assignment does not take part in the condition of the loop
63+
foo1(i, x);

‎clang-tools-extra/docs/clang-tidy/checks/cert/exp45-c.rst‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
.. meta::
2+
:http-equiv=refresh: 5;URL=../bugprone/assignment-in-selection-statement.html
3+
4+
cert-exp45-c
5+
============
6+
7+
The `cert-exp45-c` check is an alias, please see
8+
:doc:`bugprone-assignment-in-selection-statement
9+
<../bugprone/assignment-in-selection-statement>` for more information.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ Clang-Tidy Checks
8181
:doc:`bugprone-argument-comment <bugprone/argument-comment>`, "Yes"
8282
:doc:`bugprone-assert-side-effect <bugprone/assert-side-effect>`,
8383
:doc:`bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition>`,
84+
:doc:`bugprone-assignment-in-selection-statement <bugprone/assignment-in-selection-statement>`,
8485
:doc:`bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread>`,
8586
:doc:`bugprone-bitwise-pointer-cast <bugprone/bitwise-pointer-cast>`,
8687
:doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes"

0 commit comments

Comments
 (0)