[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

30

OperatorMacher missing t in its spelling.

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

141

A comment can be here for this struct.

150

A comment can be here for this matcher.

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

62

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

62

Yes my bad, I will correct this.

[PiotrZSL]

Rebase + Fix review comments

[xgupta]

LGTM!

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

 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.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<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<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
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.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 OperatorMacher = expr(
+      anyOf(binaryOperator(anyOf(isComparisonOperator(), isLogicalOperator())),
+            cxxOperatorCallExpr(isComparisonOperator())));
+
+  Finder->addMatcher(
+      expr(
+          OperatorMacher, unless(isExpansionInSystemHeader()),
+          unless(hasAncestor(OperatorMacher)), 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
   ExceptionSpecAnalyzer.cpp
   ExprSequence.cpp
   FileExtensionsUtils.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));
 }
 
+struct NotIdenticalStatementsPredicate {
+  bool
+  operator()(const clang::ast_matchers::internal::BoundNodesMap &Nodes) const;
+
+  std::string ID;
+  ::clang::DynTypedNode Node;
+  ASTContext *Context;
+};
+
+AST_MATCHER_P(Stmt, isStatementIdenticalToBoundNode, std::string, ID) {
+  NotIdenticalStatementsPredicate Predicate;
+  Predicate.ID = ID;
+  Predicate.Node = ::clang::DynTypedNode::create(Node);
+  Predicate.Context = &(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.getNode(ID).get<Stmt>(), *Context);
+}
+
+} // namespace clang::tidy::matchers

clang-tools-extra/docs/ReleaseNotes.rst

 - Support specifying `Checks` as a YAML list in the `.clang-tidy` configuration
   file.
 
 - Fix a potential crash when using the `--dump-config` option.
 
 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-non-zero-enum-to-bool-conversion
   <clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check.
 
   Detect implicit and explicit casts of ``enum`` type into ``bool`` where ``enum`` type
   doesn't have a zero-value enumerator.
 
 - New :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` 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 = 5;
+  // ...
+  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 ``5``, the first operand ``i < 5`` would
+evaluate to ``false`` 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 ``6``, 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-dynamic-static-initializers <bugprone/dynamic-static-initializers.html>`_,
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.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]
+}