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

[PiotrZSL]

Summary

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.

[PiotrZSL]

Undo change for cppcoreguidelines-use-default-member-init

[PiotrZSL]

Ping

[PiotrZSL]

Ping

[xgupta]

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

31

OperatorMacher missing t in its spelling.

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

143

A comment can be here for this struct.

152

A comment can be here for this matcher.

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

63

If it is false then the second statement will be evaluated, right?

[PiotrZSL]

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

63

Yes my bad, I will correct this.

[PiotrZSL]

Rebase + Fix review comments

[xgupta]

LGTM!

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

 #include "EasilySwappableParametersCheck.h"
 #include "EmptyCatchCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
+#include "IncDecInConditionsCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedPointerArithmeticInAllocCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(
         "bugprone-implicit-widening-of-multiplication-result");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "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");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "bugprone-lambda-function-name");
     CheckFactories.registerCheck<MacroParenthesesCheck>(

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

add_clang_library(clangTidyBugproneModule
   EmptyCatchCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
   SwitchMissingDefaultCaseCheck.cpp
+  IncDecInConditionsCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedPointerArithmeticInAllocCheck.cpp

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

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

+//===--- 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/utils/CMakeLists.txt

add_clang_library(clangTidyUtils
   ExprSequence.cpp
   FileExtensionsUtils.cpp
   FormatStringConverter.cpp
   FixItHintUtils.cpp
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
   LexerUtils.cpp
+  Matchers.cpp
   NamespaceAliaser.cpp
   OptionsUtils.cpp
   RenamerClangTidyCheck.cpp
   TransformerClangTidyCheck.cpp
   TypeTraits.cpp
   UsingInserter.cpp
 
   LINK_LIBS

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

 // expressions. If a regular expression contains starts ':' the NamedDecl's
 // qualified name will be used for matching, otherwise its name will be used.
 inline ::clang::ast_matchers::internal::Matcher<NamedDecl>
 matchesAnyListedName(llvm::ArrayRef<StringRef> NameList) {
   return ::clang::ast_matchers::internal::makeMatcher(
       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/clang-tidy/utils/Matchers.cpp

+//===---------- 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/docs/ReleaseNotes.rst

 - Remove configuration option `AnalyzeTemporaryDestructors`, which was deprecated since
   :program:`clang-tidy` 16.
 
 - Improved `--dump-config` to print check options in alphabetical order.
 
 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.
 
   Detects implicit conversions between pointers of different levels of
   indirection.
 
 - New :doc:`performance-enum-size
   <clang-tidy/checks/performance/enum-size>` check.

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

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

.. csv-table::
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.html>`_,
    `bugprone-empty-catch <bugprone/empty-catch.html>`_,
    `bugprone-exception-escape <bugprone/exception-escape.html>`_,
    `bugprone-fold-init-type <bugprone/fold-init-type.html>`_,
    `bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace.html>`_,
    `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>`_,
    `bugprone-lambda-function-name <bugprone/lambda-function-name.html>`_,
    `bugprone-macro-parentheses <bugprone/macro-parentheses.html>`_, "Yes"
    `bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects.html>`_,
    `bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc.html>`_, "Yes"

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

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