[clang-tools-extra] f27f22b - [clang-tidy] Added bugprone-inc-dec-in-conditions check
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 30 06:20:37 PDT 2023
Author: Piotr Zegar
Date: 2023-07-30T13:19:51Z
New Revision: f27f22b34516dcb744077710c19d475367e3ef03
URL: https://github.com/llvm/llvm-project/commit/f27f22b34516dcb744077710c19d475367e3ef03
DIFF: https://github.com/llvm/llvm-project/commit/f27f22b34516dcb744077710c19d475367e3ef03.diff
LOG: [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
Added:
clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp
clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h
clang-tools-extra/clang-tidy/utils/Matchers.cpp
clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp
Modified:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/utils/CMakeLists.txt
clang-tools-extra/clang-tidy/utils/Matchers.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index ceb966c655d3a8..853c6d51458793 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/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");
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 4fa32aa61a4b09..5669d6bf4205f6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp
new file mode 100644
index 00000000000000..16f43128d55e87
--- /dev/null
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h b/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h
new file mode 100644
index 00000000000000..1f2f1690041fd9
--- /dev/null
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/utils/CMakeLists.txt b/clang-tools-extra/clang-tidy/utils/CMakeLists.txt
index 6c7d52a4b1873f..36d04ef6d97183 100644
--- a/clang-tools-extra/clang-tidy/utils/CMakeLists.txt
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.cpp b/clang-tools-extra/clang-tidy/utils/Matchers.cpp
new file mode 100644
index 00000000000000..440038e36d7643
--- /dev/null
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h
index b2faf788d687ad..cbb187899a4b79 100644
--- a/clang-tools-extra/clang-tidy/utils/Matchers.h
+++ b/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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 000b80aadc0237..260a369fa2fc58 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst
new file mode 100644
index 00000000000000..380033ff09089e
--- /dev/null
+++ b/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
diff icult 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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 8918899bbf418f..78913e2dcf108b 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/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>`_,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp
new file mode 100644
index 00000000000000..82af039973c3bb
--- /dev/null
+++ b/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]
+}
More information about the cfe-commits
mailing list