[clang-tidy] Added bugprone-inc-dec-in-conditions check

Detects when a variable is both incremented/decremented and referenced inside a
complex condition and suggests moving them outside to avoid ambiguity in the
variable's value.

Reviewed By: xgupta

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

Author: PiotrZSL

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

@@ -27,6 +27,7 @@
 #include "ForwardingReferenceOverloadCheck.h"
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
+#include "IncDecInConditionsCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
@@ -122,6 +123,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(
         "bugprone-switch-missing-default-case");
+    CheckFactories.registerCheck<IncDecInConditionsCheck>(
+        "bugprone-inc-dec-in-conditions");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<InfiniteLoopCheck>("bugprone-infinite-loop");

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

@@ -23,6 +23,7 @@ add_clang_library(clangTidyBugproneModule
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
   SwitchMissingDefaultCaseCheck.cpp
+  IncDecInConditionsCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp

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

@@ -0,0 +1,82 @@
+//===--- IncDecInConditionsCheck.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 "IncDecInConditionsCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+AST_MATCHER(BinaryOperator, isLogicalOperator) { return Node.isLogicalOp(); }
+
+AST_MATCHER(UnaryOperator, isUnaryPrePostOperator) {
+  return Node.isPrefix() || Node.isPostfix();
+}
+
+AST_MATCHER(CXXOperatorCallExpr, isPrePostOperator) {
+  return Node.getOperator() == OO_PlusPlus ||
+         Node.getOperator() == OO_MinusMinus;
+}
+
+void IncDecInConditionsCheck::registerMatchers(MatchFinder *Finder) {
+  auto OperatorMatcher = expr(
+      anyOf(binaryOperator(anyOf(isComparisonOperator(), isLogicalOperator())),
+            cxxOperatorCallExpr(isComparisonOperator())));
+
+  Finder->addMatcher(
+      expr(
+          OperatorMatcher, unless(isExpansionInSystemHeader()),
+          unless(hasAncestor(OperatorMatcher)), expr().bind("parent"),
+
+          forEachDescendant(
+              expr(anyOf(unaryOperator(isUnaryPrePostOperator(),
+                                       hasUnaryOperand(expr().bind("operand"))),
+                         cxxOperatorCallExpr(
+                             isPrePostOperator(),
+                             hasUnaryOperand(expr().bind("operand")))),
+                   hasAncestor(
+                       expr(equalsBoundNode("parent"),
+                            hasDescendant(
+                                expr(unless(equalsBoundNode("operand")),
+                                     matchers::isStatementIdenticalToBoundNode(
+                                         "operand"))
+                                    .bind("second")))))
+                  .bind("operator"))),
+      this);
+}
+
+void IncDecInConditionsCheck::check(const MatchFinder::MatchResult &Result) {
+
+  SourceLocation ExprLoc;
+  bool IsIncrementOp = false;
+
+  if (const auto *MatchedDecl =
+          Result.Nodes.getNodeAs<CXXOperatorCallExpr>("operator")) {
+    ExprLoc = MatchedDecl->getExprLoc();
+    IsIncrementOp = (MatchedDecl->getOperator() == OO_PlusPlus);
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<UnaryOperator>("operator")) {
+    ExprLoc = MatchedDecl->getExprLoc();
+    IsIncrementOp = MatchedDecl->isIncrementOp();
+  } else
+    return;
+
+  diag(ExprLoc,
+       "%select{decrementing|incrementing}0 and referencing a variable in a "
+       "complex condition can cause unintended side-effects due to C++'s order "
+       "of evaluation, consider moving the modification outside of the "
+       "condition to avoid misunderstandings")
+      << IsIncrementOp;
+  diag(Result.Nodes.getNodeAs<Expr>("second")->getExprLoc(),
+       "variable is referenced here", DiagnosticIDs::Note);
+}
+
+} // namespace clang::tidy::bugprone

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

@@ -0,0 +1,35 @@
+//===--- IncDecInConditionsCheck.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_INCDECINCONDITIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCDECINCONDITIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detects when a variable is both incremented/decremented and referenced
+/// inside a complex condition and suggests moving them outside to avoid
+/// ambiguity in the variable's value.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/inc-dec-in-conditions.html
+class IncDecInConditionsCheck : public ClangTidyCheck {
+public:
+  IncDecInConditionsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCDECINCONDITIONSCHECK_H

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

@@ -17,6 +17,7 @@ add_clang_library(clangTidyUtils
   IncludeInserter.cpp
   IncludeSorter.cpp
   LexerUtils.cpp
+  Matchers.cpp
   NamespaceAliaser.cpp
   OptionsUtils.cpp
   RenamerClangTidyCheck.cpp

clang-tools-extra/clang-tidy/utils/Matchers.cpp

@@ -0,0 +1,20 @@
+//===---------- Matchers.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 "Matchers.h"
+#include "ASTUtils.h"
+
+namespace clang::tidy::matchers {
+
+bool NotIdenticalStatementsPredicate::operator()(
+    const clang::ast_matchers::internal::BoundNodesMap &Nodes) const {
+  return !utils::areStatementsIdentical(Node.get<Stmt>(),
+                                        Nodes.getNodeAs<Stmt>(ID), *Context);
+}
+
+} // namespace clang::tidy::matchers

clang-tools-extra/clang-tidy/utils/Matchers.h

@@ -140,6 +140,24 @@ matchesAnyListedName(llvm::ArrayRef<StringRef> NameList) {
       new MatchesAnyListedNameMatcher(NameList));
 }
 
+// Predicate that verify if statement is not identical to one bound to ID node.
+struct NotIdenticalStatementsPredicate {
+  bool
+  operator()(const clang::ast_matchers::internal::BoundNodesMap &Nodes) const;
+
+  std::string ID;
+  ::clang::DynTypedNode Node;
+  ASTContext *Context;
+};
+
+// Checks if statement is identical (utils::areStatementsIdentical) to one bound
+// to ID node.
+AST_MATCHER_P(Stmt, isStatementIdenticalToBoundNode, std::string, ID) {
+  NotIdenticalStatementsPredicate Predicate{
+      ID, ::clang::DynTypedNode::create(Node), &(Finder->getASTContext())};
+  return Builder->removeBindings(Predicate);
+}
+
 } // namespace clang::tidy::matchers
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -116,6 +116,13 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-inc-dec-in-conditions
+  <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check.
+
+  Detects when a variable is both incremented/decremented and referenced inside
+  a complex condition and suggests moving them outside to avoid ambiguity in
+  the variable's value.
+
 - New :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst

@@ -0,0 +1,67 @@
+.. title:: clang-tidy - bugprone-inc-dec-in-conditions
+
+bugprone-inc-dec-in-conditions
+==============================
+
+Detects when a variable is both incremented/decremented and referenced inside a
+complex condition and suggests moving them outside to avoid ambiguity in the
+variable's value.
+
+When a variable is modified and also used in a complex condition, it can lead to
+unexpected behavior. The side-effect of changing the variable's value within the
+condition can make the code difficult to reason about. Additionally, the
+developer's intended timing for the modification of the variable may not be
+clear, leading to misunderstandings and errors. This can be particularly
+problematic when the condition involves logical operators like ``&&`` and
+``||``, where the order of evaluation can further complicate the situation.
+
+Consider the following example:
+
+.. code-block:: c++
+
+  int i = 0;
+  // ...
+  if (i++ < 5 && i > 0) {
+    // do something
+  }
+
+In this example, the result of the expression may not be what the developer
+intended. The original intention of the developer could be to increment ``i``
+after the entire condition is evaluated, but in reality, i will be incremented
+before ``i > 0`` is executed. This can lead to unexpected behavior and bugs in
+the code. To fix this issue, the developer should separate the increment
+operation from the condition and perform it separately. For example, they can
+increment ``i`` in a separate statement before or after the condition is
+evaluated. This ensures that the value of ``i`` is predictable and consistent
+throughout the code.
+
+.. code-block:: c++
+
+  int i = 0;
+  // ...
+  i++;
+  if (i <= 5 && i > 0) {
+    // do something
+  }
+
+Another common issue occurs when multiple increments or decrements are performed
+on the same variable inside a complex condition. For example:
+
+.. code-block:: c++
+
+  int i = 4;
+  // ...
+  if (i++ < 5 || --i > 2) {
+    // do something
+  }
+
+There is a potential issue with this code due to the order of evaluation in C++.
+The ``||`` operator used in the condition statement guarantees that if the first
+operand evaluates to ``true``, the second operand will not be evaluated. This
+means that if ``i`` were initially ``4``, the first operand ``i < 5`` would
+evaluate to ``true`` and the second operand ``i > 2`` would not be evaluated.
+As a result, the decrement operation ``--i`` would not be executed and ``i``
+would hold value ``5``, which may not be the intended behavior for the developer.
+
+To avoid this potential issue, the both increment and decrement operation on
+``i`` should be moved outside the condition statement.

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

@@ -93,6 +93,7 @@ Clang-Tidy Checks
    `bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload.html>`_,
    `bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result.html>`_, "Yes"
    `bugprone-inaccurate-erase <bugprone/inaccurate-erase.html>`_, "Yes"
+   `bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions.html>`_,
    `bugprone-incorrect-roundings <bugprone/incorrect-roundings.html>`_,
    `bugprone-infinite-loop <bugprone/infinite-loop.html>`_,
    `bugprone-integer-division <bugprone/integer-division.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp

@@ -0,0 +1,70 @@
+// RUN: %check_clang_tidy %s bugprone-inc-dec-in-conditions %t
+
+template<typename T>
+struct Iterator {
+  Iterator operator++(int);
+  Iterator operator--(int);
+  Iterator& operator++();
+  Iterator& operator--();
+  T operator*();
+  bool operator==(Iterator) const;
+  bool operator!=(Iterator) const;
+};
+
+template<typename T>
+struct Container {
+  Iterator<T> begin();
+  Iterator<T> end();
+};
+
+bool f(int x) {
+  return (++x != 5 or x == 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool f2(int x) {
+  return (x++ != 5 or x == 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool c(Container<int> x) {
+  auto it = x.begin();
+  return (it++ != x.end() and *it);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool c2(Container<int> x) {
+  auto it = x.begin();
+  return (++it != x.end() and *it);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool d(int x) {
+  return (--x != 5 or x == 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool d2(int x) {
+  return (x-- != 5 or x == 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool g(Container<int> x) {
+  auto it = x.begin();
+  return (it-- != x.end() and *it);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool g2(Container<int> x) {
+  auto it = x.begin();
+  return (--it != x.end() and *it);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool doubleCheck(Container<int> x) {
+  auto it = x.begin();
+  auto it2 = x.begin();
+  return (--it != x.end() and ++it2 != x.end()) and (*it == *it2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+  // CHECK-MESSAGES: :[[@LINE-2]]:31: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}