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

[dodohand]

Summary

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

[gribozavr2]

clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst

9 ↗

(On Diff #434472)

Why not improve -Wparentheses to catch these cases too?

[dodohand]

clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst

9 ↗

(On Diff #434472)

IMHO, -Wparentheses isn't broken. Its "disable warning by an extra set of parentheses" behavior has been around for a long time and is common across gcc and clang... it just isn't what is called for when trying to ensure that accidental assignments aren't occurring in even slightly complicated condition statements, or when trying to comply with BARR-group coding standard.

[gribozavr2]

clang-tools-extra/clang-tidy/misc/AssignmentInIfClauseCheck.cpp

39 ↗

(On Diff #434472)

Please follow Clang's message style. The message should start with a lowercase letter and should not end with a period. Suggested fixes (since there are multiple) should be in notes.

warning: an assignment within an 'if' condition is bug-prone
note: if it should be an assignment, move it out of the 'if' condition
note: if it is meant to be an equality check, change '=' to '=='

Also consider adding a fixit to the second note.

clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst

3 ↗

(On Diff #434472)

WDYT about bugprone-assignment-in-if-condition?

6–9 ↗

(On Diff #434472)

9 ↗

(On Diff #434472)

Oh, I see -- it wasn't clear to me that the silencing through the extra set of parens was specifically not desirable for you.

I suggested some edits to the documentation to directly highlight the difference.

I also suggest changing the example below to explicitly mention what assignments are flagged by -Wparentheses and which are flagged by this check.

10 ↗

(On Diff #434472)

17 ↗

(On Diff #434472)

The "static volatile" part is not relevant to the example, and it might distract the reader, suggesting that something is special about such variables.

clang-tools-extra/test/clang-tidy/checkers/misc-assignment-in-if-clause.cpp

68 ↗

(On Diff #434472)

Please clang-format the test.

[dodohand]

clang-tools-extra/clang-tidy/misc/AssignmentInIfClauseCheck.cpp

39 ↗

(On Diff #434472)

Will adjust the messages.

I'm specifically reluctant to add a fixit to this, as I'm not sure how I can tell an intended assignment (for which the fix would be moving the assignment out of the if clause) apart from an accidental assignment (for which the fix is changing '=' to '=='). Thoughts?

clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst

3 ↗

(On Diff #434472)

I'm in favor of that categorization - it reflects how I feel about having assignments within an 'if' condition. I'll change it accordingly

[dodohand]

renamed from misc-assignment-in-if-clause to bugprone-assignment-in-if-condition per review input

Adjusted documentation per review input.

[dodohand]

@gribozavr2 , have I successfully addressed the points you brought up?

[dodohand]

Hi @alexfh , @aaron.ballman, Could one of you have a look at this? I'd like to get it moving toward resolution and don't know who else ought to do a review...

[gribozavr2]

LGTM with a few minor edits.

Do you have commit access?

clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.cpp

20–22

33–35

No need to repeat the code in comments.

clang-tools-extra/test/clang-tidy/checkers/bugprone-assignment-in-if-condition.cpp

2–5

[gribozavr2]

clang-tools-extra/test/clang-tidy/checkers/bugprone-assignment-in-if-condition.cpp

85

[dodohand]

Addressing comments from @gribozavr2 identifying comments needing update/removal/improvement.

[dodohand]

Addressed comments from gribozavr2

[dodohand]

Hi @gribozavr2, I don't have commit access... I'll address the merge issue ASAP though.

[gribozavr2]

Thanks! I'll commit this for you once you upload a refreshed patch (that applies to the main branch cleanly).

[dodohand]

Updated to merge successfully into latest version of main branch

[dodohand]

somehow, clang-format needed to be re-run.

clang-tools-extra/clang-tidy/bugprone/AssignmentInIfConditionCheck.h

+//===--- 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/AssignmentInIfConditionCheck.cpp

+//===--- 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/BugproneTidyModule.cpp

 //===--- BugproneTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../cppcoreguidelines/NarrowingConversionsCheck.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
+#include "AssignmentInIfConditionCheck.h"
 #include "BadSignalToKillThreadCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
 #include "ExceptionEscapeCheck.h"

 
 class BugproneModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>(
         "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>(
         "bugprone-dangling-handle");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<EasilySwappableParametersCheck>(
         "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>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(
         "bugprone-implicit-widening-of-multiplication-result");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "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>(
         "bugprone-lambda-function-name");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "bugprone-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "bugprone-macro-repeated-side-effects");
CheckFactories.registerCheck<RedundantBranchConditionCheck>(
         "bugprone-redundant-branch-condition");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "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>(
         "bugprone-shared-ptr-array-mismatch");
     CheckFactories.registerCheck<SignalHandlerCheck>("bugprone-signal-handler");
     CheckFactories.registerCheck<SignedCharMisuseCheck>(
         "bugprone-signed-char-misuse");
     CheckFactories.registerCheck<SizeofContainerCheck>(
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
         "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");
   }
 };
 
 } // namespace bugprone
 
 // Register the BugproneTidyModule using this statically initialized variable.

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

 set(LLVM_LINK_COMPONENTS
   support
   FrontendOpenMP
   )
 
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  AssignmentInIfConditionCheck.cpp
   BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-unchecked-optional-access
 
   Warns when the code is unwrapping a `std::optional<T>`, `absl::optional<T>`,
   or `base::Optional<T>` object without assuring that it contains a value.
 
 - New :doc:`misc-confusable-identifiers <clang-tidy/checks/misc/confusable-identifiers>` check.
 
   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.
 
   Replaces groups of adjacent macros with an unscoped anonymous enum.
 
 - New :doc:`portability-std-allocator-const <clang-tidy/checks/portability/std-allocator-const>` check.
 
   Report use of ``std::vector<const T>`` (and similar containers of const

clang-tools-extra/docs/clang-tidy/checks/bugprone/assignment-in-if-condition.rst

+.. 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

.. csv-table::
    `android-cloexec-open <android/cloexec-open.html>`_, "Yes"
    `android-cloexec-pipe <android/cloexec-pipe.html>`_, "Yes"
    `android-cloexec-pipe2 <android/cloexec-pipe2.html>`_, "Yes"
    `android-cloexec-socket <android/cloexec-socket.html>`_, "Yes"
    `android-comparison-in-temp-failure-retry <android/comparison-in-temp-failure-retry.html>`_,
    `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>`_,
    `bugprone-copy-constructor-init <bugprone/copy-constructor-init.html>`_, "Yes"
    `bugprone-dangling-handle <bugprone/dangling-handle.html>`_,
    `bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers.html>`_,
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.html>`_,
    `bugprone-exception-escape <bugprone/exception-escape.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone-assignment-in-if-condition.cpp

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