=?utf-8?q?Balázs_Kéri?= <[email protected]> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/[email protected]>
llvmbot wrote: <!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Balázs Kéri (balazske) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/180219.diff 9 Files Affected: - (added) clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.cpp (+119) - (added) clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.h (+30) - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp (+3) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-selection-statement.rst (+55) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-selection-statement.c (+124) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.cpp new file mode 100644 index 0000000000000..981a48a734a50 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.cpp @@ -0,0 +1,119 @@ +//===----------------------------------------------------------------------===// +// +// 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 "AssignmentInSelectionStatementCheck.h" +#include "clang/AST/StmtVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang; +using namespace clang::ast_matchers; + +namespace { + +class ConditionValueCanPropagateFrom + : public ConstStmtVisitor<ConditionValueCanPropagateFrom, void> { +public: + llvm::SmallVector<const Expr *, 2> ExprToProcess; + + void VisitBinaryOperator(const BinaryOperator *BO) { + if (BO->isCommaOp()) + ExprToProcess.push_back(BO->getRHS()->IgnoreParenImpCasts()); + } + void VisitConditionalOperator(const ConditionalOperator *CO) { + ExprToProcess.push_back(CO->getFalseExpr()->IgnoreParenImpCasts()); + ExprToProcess.push_back(CO->getTrueExpr()->IgnoreParenImpCasts()); + } +}; + +AST_MATCHER_P(Expr, conditionValueCanPropagateFrom, + ast_matchers::internal::Matcher<Expr>, InnerMatcher) { + bool Found = false; + ConditionValueCanPropagateFrom Visitor; + Visitor.Visit(&Node); // Do not match Node itself. + while (!Visitor.ExprToProcess.empty()) { + const Expr *E = Visitor.ExprToProcess.pop_back_val(); + ast_matchers::internal::BoundNodesTreeBuilder Result; + if (InnerMatcher.matches(*E, Finder, &Result)) { + Found = true; + Builder->addMatch(Result); + } + Visitor.Visit(E); + } + return Found; +} + +} // namespace + +namespace clang::tidy::bugprone { + +void AssignmentInSelectionStatementCheck::registerMatchers( + MatchFinder *Finder) { + auto AssignOp = binaryOperation(hasOperatorName("=")).bind("assignment"); + + auto CondExprWithAssign = expr( + anyOf(ignoringImpCasts(AssignOp), + ignoringParenImpCasts(conditionValueCanPropagateFrom(AssignOp)))); + auto OpCondExprWithAssign = expr(ignoringParenImpCasts( + anyOf(AssignOp, conditionValueCanPropagateFrom(AssignOp)))); + + // In these cases "single primary expression" is possible. + // A single assignment within a 'ParenExpr' is allowed (but not if mixed with + // other operators). + auto FoundControlStmt = mapAnyOf(ifStmt, whileStmt, doStmt, forStmt) + .with(hasCondition(CondExprWithAssign)); + // In these cases "single primary expression" is not possible because the + // assignment is already part of a bigger expression. + auto FoundConditionalOperator = + conditionalOperator(hasCondition(OpCondExprWithAssign)); + auto FoundLogicalOp = binaryOperator( + hasAnyOperatorName("&&", "||"), + eachOf(hasLHS(OpCondExprWithAssign), hasRHS(OpCondExprWithAssign))); + + auto FoundSelectionStmt = + stmt(anyOf(FoundControlStmt, FoundConditionalOperator, FoundLogicalOp)) + .bind("parent"); + + Finder->addMatcher(FoundSelectionStmt, this); +} + +void AssignmentInSelectionStatementCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *FoundAssignment = + Result.Nodes.getNodeAs<BinaryOperator>("assignment"); + if (!FoundAssignment) + return; + const auto *ParentStmt = Result.Nodes.getNodeAs<Stmt>("parent"); + const char *CondStr = nullptr; + switch (ParentStmt->getStmtClass()) { + case Stmt::IfStmtClass: + CondStr = "condition of 'if' statement"; + break; + case Stmt::WhileStmtClass: + CondStr = "condition of 'while' statement"; + break; + case Stmt::DoStmtClass: + CondStr = "condition of 'do' statement"; + break; + case Stmt::ForStmtClass: + CondStr = "condition of 'for' statement"; + break; + case Stmt::ConditionalOperatorClass: + CondStr = "condition of conditional operator"; + break; + case Stmt::BinaryOperatorClass: + CondStr = "operand of a logical operator"; + break; + default: + llvm_unreachable("unexpected statement class, should not match"); + }; + diag(FoundAssignment->getOperatorLoc(), + "Assignment within %0 may indicate programmer error") + << FoundAssignment->getSourceRange() << CondStr; +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.h b/clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.h new file mode 100644 index 0000000000000..4de1f87b3b3ce --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInSelectionStatementCheck.h @@ -0,0 +1,30 @@ +//===----------------------------------------------------------------------===// +// +// 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_ASSIGNMENTINSELECTIONSTATEMENTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINSELECTIONSTATEMENTCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Finds assignments within selection statements. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/assignment-in-selection-statement.html +class AssignmentInSelectionStatementCheck : public ClangTidyCheck { +public: + AssignmentInSelectionStatementCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINSELECTIONSTATEMENTCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 4150442c25d61..c02c1c4d8b657 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -11,6 +11,7 @@ #include "ArgumentCommentCheck.h" #include "AssertSideEffectCheck.h" #include "AssignmentInIfConditionCheck.h" +#include "AssignmentInSelectionStatementCheck.h" #include "BadSignalToKillThreadCheck.h" #include "BitwisePointerCastCheck.h" #include "BoolPointerImplicitConversionCheck.h" @@ -125,6 +126,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-assert-side-effect"); CheckFactories.registerCheck<AssignmentInIfConditionCheck>( "bugprone-assignment-in-if-condition"); + CheckFactories.registerCheck<AssignmentInSelectionStatementCheck>( + "bugprone-assignment-in-selection-statement"); CheckFactories.registerCheck<BadSignalToKillThreadCheck>( "bugprone-bad-signal-to-kill-thread"); CheckFactories.registerCheck<BitwisePointerCastCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index db1256d91d311..4ce1bab8881c5 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -7,6 +7,7 @@ add_clang_library(clangTidyBugproneModule STATIC ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp AssignmentInIfConditionCheck.cpp + AssignmentInSelectionStatementCheck.cpp BadSignalToKillThreadCheck.cpp BitwisePointerCastCheck.cpp BoolPointerImplicitConversionCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp index f64cb47d18b4e..d18a8253b66a9 100644 --- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -8,6 +8,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" +#include "../bugprone/AssignmentInSelectionStatementCheck.h" #include "../bugprone/BadSignalToKillThreadCheck.h" #include "../bugprone/CommandProcessorCheck.h" #include "../bugprone/CopyConstructorMutatesArgumentCheck.h" @@ -311,6 +312,8 @@ class CERTModule : public ClangTidyModule { // EXP CheckFactories.registerCheck<bugprone::SuspiciousMemoryComparisonCheck>( "cert-exp42-c"); + CheckFactories.registerCheck<bugprone::AssignmentInSelectionStatementCheck>( + "cert-exp45-c"); // FLP CheckFactories.registerCheck<bugprone::FloatLoopCounterCheck>( "cert-flp30-c"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 0ad69f5fdc5aa..555caa3b18f9e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -97,6 +97,11 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`bugprone-assignment-in-selection-statement + <clang-tidy/checks/bugprone/assignment-in-selection-statement>` check. + + Finds assignments within selection statements. + - New :doc:`llvm-type-switch-case-types <clang-tidy/checks/llvm/type-switch-case-types>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-selection-statement.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-selection-statement.rst new file mode 100644 index 0000000000000..a56ce75f8a2bf --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-selection-statement.rst @@ -0,0 +1,55 @@ +.. title:: clang-tidy - bugprone-assignment-in-selection-statement + +bugprone-assignment-in-selection-statement +========================================== + +Finds assignments within selection statements. +Such assignments may indicate programmer error because they may have been +intended as equality tests. The selection statements are conditions of ``if`` +and loop (``for``, ``while``, ``do``) statements, condition of conditional +operator (``?:``) and any operand of a binary logical operator (``&&``, ``||``). +The check finds assignments within these contexts if the single expression is an +assignment or the assignment is contained (recursively) in last operand of a +comma (``,``) operator or true and false expressions in a conditional operator. +There is no warning if a single-standing assignment is enclosed in parentheses. + +This check corresponds to the CERT rule +`EXP45-C. Do not perform assignments in selection statements +<https://wiki.sei.cmu.edu/confluence/spaces/c/pages/87152228/EXP45-C.+Do+not+perform+assignments+in+selection+statements>`_. + +Examples +======== + +.. code-block:: c++ + + int x = 3; + + if (x = 4) // warning: should it be `x == 4`? + x = x + 1; + + if ((x = 1)) { // no warning: single assignment in parentheses + x += 10; + + if ((x = 1) != 0) { // no warning: assignment appears in a complex expression and not with a logical operator + ++x; + + if (foo(x = 9) && array[x = 8]) { // no warning: assignment appears in argument of function call or array index + ++x; + + while ((x <= 11) || (x = 22)) // warning: the assignment is found as operand of a logical operator + x += 2; + + do { + x += 5; + } while ((x > 10) ? (x = 11) : (x > 5)); // warning: assignment in loop condition (from `x = 11`) + + for (int i = 0; i == 2, x = 5; ++i) // warning: assignment in loop condition (from last operand of comma) + foo1(i, x); + + for (int i = 0; i == 2, (x = 5); ++i) // warning: assignment is not a single expression, parentheses do not prevent the warning + foo1(i, x); + + for (int i = 0; i = 2, x == 5; ++i) // no warning: assignment does not take part in the condition of the loop + foo1(i, x); + + int a = (x == 2) || (x = 3); // warning: the assignment appears in the operand a logical operator diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 0eabd9929dc39..a16768fd546b1 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -81,6 +81,7 @@ Clang-Tidy Checks :doc:`bugprone-argument-comment <bugprone/argument-comment>`, "Yes" :doc:`bugprone-assert-side-effect <bugprone/assert-side-effect>`, :doc:`bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition>`, + :doc:`bugprone-assignment-in-selection-statement <bugprone/assignment-in-selection-statement>`, :doc:`bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread>`, :doc:`bugprone-bitwise-pointer-cast <bugprone/bitwise-pointer-cast>`, :doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-selection-statement.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-selection-statement.c new file mode 100644 index 0000000000000..594dcc0feac4f --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assignment-in-selection-statement.c @@ -0,0 +1,124 @@ +// RUN: %check_clang_tidy %s bugprone-assignment-in-selection-statement %t + +void test_if(int a, int b, int c, int d) { + if (a = b) {} + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Assignment within condition of 'if' statement may indicate programmer error + if ((a = b)) {} + if (a == b) {} + + if ((b > 0) ? (a = b) : c) {} + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: Assignment within condition of 'if' statement may indicate programmer error + if ((b > 0) ? c : (a = b)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: Assignment within condition of 'if' statement may indicate programmer error + if (a = c, b = c) {} + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Assignment within condition of 'if' statement may indicate programmer error +} + +void test_while(int a, int b, int c) { + while (a = b) {} + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Assignment within condition of 'while' statement may indicate programmer error + while ((a = b)) {} + while (a == b) {} + + while ((b > 0) ? (a = b) : c) {} + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: Assignment within condition of 'while' statement may indicate programmer error + while ((b > 0) ? c : (a = b)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: Assignment within condition of 'while' statement may indicate programmer error + while (a = b, b = c) {} + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: Assignment within condition of 'while' statement may indicate programmer error +} + +void test_do(int a, int b, int c) { + do {} while (a = b); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Assignment within condition of 'do' statement may indicate programmer error + do {} while ((a = b)); + do {} while (a == b); + + do {} while ((b > 0) ? (a = b) : c); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: Assignment within condition of 'do' statement may indicate programmer error + do {} while ((b > 0) ? c : (a = b)); + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: Assignment within condition of 'do' statement may indicate programmer error +} + +void test_for(int a, int b, int c) { + for (int i = 0; a = b; i++) {} + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: Assignment within condition of 'for' statement may indicate programmer error + for (int i = 0; (a = b); i++) {} + for (int i = 0; a == b; i++) {} + + for (int i = 0; (b > 0) ? (a = b) : c; ++i) {} + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: Assignment within condition of 'for' statement may indicate programmer error + for (int i = 0; (b > 0) ? c : (a = b); ++i) {} + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: Assignment within condition of 'for' statement may indicate programmer error +} + +void test_conditional(int a, int b, int c, int d) { + int c1 = (a = b) ? 1 : 2; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Assignment within condition of conditional operator may indicate programmer error + int c2 = ((a = b)) ? 1 : 2; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Assignment within condition of conditional operator may indicate programmer error + int c3 = (a == b) ? 1 : 2; + + if ((c ? (a = b) : d) ? 1 : -1) {} + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Assignment within condition of conditional operator may indicate programmer error + while ((c ? d : (a = b)) ? 1 : -1) {} + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Assignment within condition of conditional operator may indicate programmer error + + int c4 = (c ? (a = b) : 2); + int c5 = (c ? 2 : (a = b)); +} + +void test_bin_op(int a, int b, int c, int d) { + int c1 = (a = b) && c; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Assignment within operand of a logical operator may indicate programmer error + int c2 = c || (a = b); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: Assignment within operand of a logical operator may indicate programmer error + int c3 = ((a = b) && c) || (c == b - a); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Assignment within operand of a logical operator may indicate programmer error + int c4 = c || ((a = b)); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: Assignment within operand of a logical operator may indicate programmer error + int c5 = (a = b) + 2; + int c6 = ((a = b) + 2) && c; + +} + +int f(int); + +void test_misc(int a, int b, int c, int d) { + if ((a = c, b = c)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: Assignment within condition of 'if' statement may indicate programmer error + if (a = c, (b = c)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: Assignment within condition of 'if' statement may indicate programmer error + if ((a > 0) ? ((b < 0) ? (a = b) : (a = c)) : (a = d)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: Assignment within condition of 'if' statement may indicate programmer error + // CHECK-MESSAGES: :[[@LINE-2]]:41: warning: Assignment within condition of 'if' statement may indicate programmer error + // CHECK-MESSAGES: :[[@LINE-3]]:52: warning: Assignment within condition of 'if' statement may indicate programmer error + while (a = c, (b = c, c = d)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: Assignment within condition of 'while' statement may indicate programmer error + for (d = 0; a = c, b = c, ((a > 0) ? d == a : (d = b)); ++d) {} + // CHECK-MESSAGES: :[[@LINE-1]]:52: warning: Assignment within condition of 'for' statement may indicate programmer error + do {} while ((a > 0) ? (a = c, b = c) : d); + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: Assignment within condition of 'do' statement may indicate programmer error + if ((a = b) && (c = d)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Assignment within operand of a logical operator may indicate programmer error + // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: Assignment within operand of a logical operator may indicate programmer error + if ((a ? (b = c) : d) && (d ? c : (b = a))) {} + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Assignment within operand of a logical operator may indicate programmer error + // CHECK-MESSAGES: :[[@LINE-2]]:40: warning: Assignment within operand of a logical operator may indicate programmer error + if (f((a = b) ? c : d)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Assignment within condition of conditional operator may indicate programmer error +} + +void test_no_warning(int a, int b, int c) { + if ((a = b) != 0) {} + if (!(a = b)) {} + if ((int)(a = b)) {} + if ((a = b) + c > 0) {} + if ((b > 0) ? (a == b) : c) {} + if ((b > 0) ? c : (a == b)) {} + if (a = c, b == c) {} + + int arr[10] = {0}; + if (f(a = b)) {} + if (arr[c = a]) {}; +} `````````` </details> https://github.com/llvm/llvm-project/pull/180219 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
