new clang-tidy checker for assignments within condition clause of if statement

new clang-tidy checker for assignments within the condition clause of an 'if' statement.

Reviewed By: gribozavr2

Differential Revision: https://reviews.llvm.org/D127114

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<clang::Stmt>("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

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

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 @@ public:
         "bugprone-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "bugprone-assert-side-effect");
+    CheckFactories.registerCheck<AssignmentInIfConditionCheck>(
+        "bugprone-assignment-in-if-condition");
     CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
         "bugprone-bad-signal-to-kill-thread");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "bugprone-bool-pointer-implicit-conversion");
-    CheckFactories.registerCheck<BranchCloneCheck>(
-        "bugprone-branch-clone");
+    CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
@@ -100,8 +102,7 @@ public:
         "bugprone-easily-swappable-parameters");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
-    CheckFactories.registerCheck<FoldInitTypeCheck>(
-        "bugprone-fold-init-type");
+    CheckFactories.registerCheck<FoldInitTypeCheck>("bugprone-fold-init-type");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
@@ -112,8 +113,7 @@ public:
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
-    CheckFactories.registerCheck<InfiniteLoopCheck>(
-        "bugprone-infinite-loop");
+    CheckFactories.registerCheck<InfiniteLoopCheck>("bugprone-infinite-loop");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
@@ -141,8 +141,7 @@ public:
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
-    CheckFactories.registerCheck<PosixReturnCheck>(
-        "bugprone-posix-return");
+    CheckFactories.registerCheck<PosixReturnCheck>("bugprone-posix-return");
     CheckFactories.registerCheck<ReservedIdentifierCheck>(
         "bugprone-reserved-identifier");
     CheckFactories.registerCheck<SharedPtrArrayMismatchCheck>(
@@ -196,12 +195,10 @@ public:
         "bugprone-unhandled-self-assignment");
     CheckFactories.registerCheck<UnhandledExceptionAtNewCheck>(
         "bugprone-unhandled-exception-at-new");
-    CheckFactories.registerCheck<UnusedRaiiCheck>(
-        "bugprone-unused-raii");
+    CheckFactories.registerCheck<UnusedRaiiCheck>("bugprone-unused-raii");
     CheckFactories.registerCheck<UnusedReturnValueCheck>(
         "bugprone-unused-return-value");
-    CheckFactories.registerCheck<UseAfterMoveCheck>(
-        "bugprone-use-after-move");
+    CheckFactories.registerCheck<UseAfterMoveCheck>("bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");
   }

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

@@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  AssignmentInIfConditionCheck.cpp
   BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp

clang-tools-extra/docs/ReleaseNotes.rst

@@ -132,6 +132,11 @@ New checks
 
   Detects confusable Unicode identifiers.
 
+- New :doc:`bugprone-assignment-in-if-condition
+  <clang-tidy/checks/bugprone-assignment-in-if-condition>` check.
+
+  Warns when there is an assignment within an if statement condition expression.
+
 - New :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize/macro-to-enum>` check.
 

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" <https://barrgroup.com/embedded-systems/books/embedded-c-coding-standard/statement-rules/if-else-statements>`_.
+
+.. 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;
+  }

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

@@ -78,6 +78,7 @@ Clang-Tidy Checks
    `boost-use-to-string <boost/use-to-string.html>`_, "Yes"
    `bugprone-argument-comment <bugprone/argument-comment.html>`_, "Yes"
    `bugprone-assert-side-effect <bugprone/assert-side-effect.html>`_,
+   `bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition.html>`_,
    `bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread.html>`_,
    `bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion.html>`_, "Yes"
    `bugprone-branch-clone <bugprone/branch-clone.html>`_,

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;
+  }
+}