diff --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h @@ -0,0 +1,34 @@ +//===--- AssignmentInIfConditionCheck.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_ASSIGNMENTINIFCONDITIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINIFCONDITIONCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Catches assignments within the condition clause of an if statement. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-assignment-in-if-condition.html +class AssignmentInIfConditionCheck : public ClangTidyCheck { +public: + AssignmentInIfConditionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSIGNMENTINIFCONDITIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp @@ -0,0 +1,49 @@ +//===--- AssignmentInIfConditionCheck.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 "AssignmentInIfConditionCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void AssignmentInIfConditionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(ifStmt(hasCondition(forEachDescendant( + binaryOperator(isAssignmentOperator()) + .bind("assignment_in_if_statement")))), + this); + Finder->addMatcher(ifStmt(hasCondition(forEachDescendant( + cxxOperatorCallExpr(isAssignmentOperator()) + .bind("assignment_in_if_statement")))), + this); +} + +void AssignmentInIfConditionCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = + Result.Nodes.getNodeAs("assignment_in_if_statement"); + if (!MatchedDecl) { + return; + } + diag(MatchedDecl->getBeginLoc(), + "an assignment within an 'if' condition is bug-prone"); + diag(MatchedDecl->getBeginLoc(), + "if it should be an assignment, move it out of the 'if' condition", + DiagnosticIDs::Note); + diag(MatchedDecl->getBeginLoc(), + "if it is meant to be an equality check, change '=' to '=='", + DiagnosticIDs::Note); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -12,6 +12,7 @@ #include "../cppcoreguidelines/NarrowingConversionsCheck.h" #include "ArgumentCommentCheck.h" #include "AssertSideEffectCheck.h" +#include "AssignmentInIfConditionCheck.h" #include "BadSignalToKillThreadCheck.h" #include "BoolPointerImplicitConversionCheck.h" #include "BranchCloneCheck.h" @@ -84,12 +85,13 @@ "bugprone-argument-comment"); CheckFactories.registerCheck( "bugprone-assert-side-effect"); + CheckFactories.registerCheck( + "bugprone-assignment-in-if-condition"); CheckFactories.registerCheck( "bugprone-bad-signal-to-kill-thread"); CheckFactories.registerCheck( "bugprone-bool-pointer-implicit-conversion"); - CheckFactories.registerCheck( - "bugprone-branch-clone"); + CheckFactories.registerCheck("bugprone-branch-clone"); CheckFactories.registerCheck( "bugprone-copy-constructor-init"); CheckFactories.registerCheck( @@ -100,8 +102,7 @@ "bugprone-easily-swappable-parameters"); CheckFactories.registerCheck( "bugprone-exception-escape"); - CheckFactories.registerCheck( - "bugprone-fold-init-type"); + CheckFactories.registerCheck("bugprone-fold-init-type"); CheckFactories.registerCheck( "bugprone-forward-declaration-namespace"); CheckFactories.registerCheck( @@ -112,8 +113,7 @@ "bugprone-inaccurate-erase"); CheckFactories.registerCheck( "bugprone-incorrect-roundings"); - CheckFactories.registerCheck( - "bugprone-infinite-loop"); + CheckFactories.registerCheck("bugprone-infinite-loop"); CheckFactories.registerCheck( "bugprone-integer-division"); CheckFactories.registerCheck( @@ -141,8 +141,7 @@ "bugprone-not-null-terminated-result"); CheckFactories.registerCheck( "bugprone-parent-virtual-call"); - CheckFactories.registerCheck( - "bugprone-posix-return"); + CheckFactories.registerCheck("bugprone-posix-return"); CheckFactories.registerCheck( "bugprone-reserved-identifier"); CheckFactories.registerCheck( @@ -196,12 +195,10 @@ "bugprone-unhandled-self-assignment"); CheckFactories.registerCheck( "bugprone-unhandled-exception-at-new"); - CheckFactories.registerCheck( - "bugprone-unused-raii"); + CheckFactories.registerCheck("bugprone-unused-raii"); CheckFactories.registerCheck( "bugprone-unused-return-value"); - CheckFactories.registerCheck( - "bugprone-use-after-move"); + CheckFactories.registerCheck("bugprone-use-after-move"); CheckFactories.registerCheck( "bugprone-virtual-near-miss"); } diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -6,6 +6,7 @@ add_clang_library(clangTidyBugproneModule ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp + AssignmentInIfConditionCheck.cpp BadSignalToKillThreadCheck.cpp BoolPointerImplicitConversionCheck.cpp BranchCloneCheck.cpp diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -132,6 +132,11 @@ Detects confusable Unicode identifiers. +- New :doc:`bugprone-assignment-in-if-condition + ` check. + + Warns when there is an assignment within an if statement condition expression. + - New :doc:`modernize-macro-to-enum ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-if-condition.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-if-condition.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-if-condition.rst @@ -0,0 +1,23 @@ +.. title:: clang-tidy - bugprone-assignment-in-if-condition + +bugprone-assignment-in-if-condition +=================================== + +Finds assignments within conditions of `if` statements. +Such assignments are bug-prone because they may have been intended as equality tests. + +This check finds all assignments within `if` conditions, including ones that are not flagged +by `-Wparentheses` due to an extra set of parentheses, and including assignments that call +an overloaded `operator=()`. The identified assignments violate +`BARR group "Rule 8.2.c" `_. + +.. code-block:: c++ + + int f = 3; + if(f = 4) { // This is identified by both `Wparentheses` and this check - should it have been: `if (f == 4)` ? + f = f + 1; + } + + if((f == 5) || (f = 6)) { // the assignment here `(f = 6)` is identified by this check, but not by `-Wparentheses`. Should it have been `(f == 6)` ? + f = f + 2; + } diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -78,6 +78,7 @@ `boost-use-to-string `_, "Yes" `bugprone-argument-comment `_, "Yes" `bugprone-assert-side-effect `_, + `bugprone-assignment-in-if-condition `_, `bugprone-bad-signal-to-kill-thread `_, `bugprone-bool-pointer-implicit-conversion `_, "Yes" `bugprone-branch-clone `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-assignment-in-if-condition.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-assignment-in-if-condition.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-assignment-in-if-condition.cpp @@ -0,0 +1,103 @@ +// RUN: %check_clang_tidy %s bugprone-assignment-in-if-condition %t + +void f(int arg) { + int f = 3; + if ((f = arg) || (f == (arg + 1))) + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 5; + } +} + +void f1(int arg) { + int f = 3; + if ((f == arg) || (f = (arg + 1))) + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 5; + } +} + +void f2(int arg) { + int f = 3; + if (f = arg) + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 5; + } +} + +volatile int v = 32; + +void f3(int arg) { + int f = 3; + if ((f == arg) || ((arg + 6 < f) && (f = v))) + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 5; + } +} + +void f4(int arg) { + int f = 3; + if ((f == arg) || ((arg + 6 < f) && ((f = v) || (f < 8)))) + // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 5; + } else if ((arg + 8 < f) && ((f = v) || (f < 8))) + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 6; + } +} + +class BrokenOperator { +public: + int d = 0; + int operator=(const int &val) { + d = val + 1; + return d; + } +}; + +void f5(int arg) { + BrokenOperator bo; + int f = 3; + bo = f; + if (bo.d == 3) { + f = 6; + } + if (bo = 3) + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 7; + } + if ((arg == 3) || (bo = 6)) + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition] + { + f = 8; + } + v = f; +} + +// Tests that shouldn't trigger warnings. +void awesome_f2(int arg) { + int f = 3; + if ((f == arg) || (f == (arg + 1))) { + f = 5; + } +} + +void awesome_f3(int arg) { + int f = 3; + if (f == arg) { + f = 5; + } +} + +void awesome_f4(int arg) { + int f = 3; + if ((f == arg) || ((arg + 6 < f) && ((f == v) || (f < 8)))) { + f = 5; + } +}