diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- 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 @@ "bugprone-inaccurate-erase"); CheckFactories.registerCheck( "bugprone-switch-missing-default-case"); + CheckFactories.registerCheck( + "bugprone-inc-dec-in-conditions"); CheckFactories.registerCheck( "bugprone-incorrect-roundings"); CheckFactories.registerCheck("bugprone-infinite-loop"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -23,6 +23,7 @@ ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp SwitchMissingDefaultCaseCheck.cpp + IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp InfiniteLoopCheck.cpp IntegerDivisionCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h b/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h new file mode 100644 --- /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 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/bugprone/IncDecInConditionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp new file mode 100644 --- /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("operator")) { + ExprLoc = MatchedDecl->getExprLoc(); + IsIncrementOp = (MatchedDecl->getOperator() == OO_PlusPlus); + } else if (const auto *MatchedDecl = + Result.Nodes.getNodeAs("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("second")->getExprLoc(), + "variable is referenced here", DiagnosticIDs::Note); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/utils/CMakeLists.txt b/clang-tools-extra/clang-tidy/utils/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/utils/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/utils/CMakeLists.txt @@ -17,6 +17,7 @@ IncludeInserter.cpp IncludeSorter.cpp LexerUtils.cpp + Matchers.cpp NamespaceAliaser.cpp OptionsUtils.cpp RenamerClangTidyCheck.cpp diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h --- a/clang-tools-extra/clang-tidy/utils/Matchers.h +++ b/clang-tools-extra/clang-tidy/utils/Matchers.h @@ -140,6 +140,24 @@ 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/clang-tidy/utils/Matchers.cpp b/clang-tools-extra/clang-tidy/utils/Matchers.cpp new file mode 100644 --- /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(), + Nodes.getNodeAs(ID), *Context); +} + +} // namespace clang::tidy::matchers diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -116,6 +116,13 @@ New checks ^^^^^^^^^^ +- New :doc:`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 ` 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 --- /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 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. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -93,6 +93,7 @@ `bugprone-forwarding-reference-overload `_, `bugprone-implicit-widening-of-multiplication-result `_, "Yes" `bugprone-inaccurate-erase `_, "Yes" + `bugprone-inc-dec-in-conditions `_, `bugprone-incorrect-roundings `_, `bugprone-infinite-loop `_, `bugprone-integer-division `_, 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 --- /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 +struct Iterator { + Iterator operator++(int); + Iterator operator--(int); + Iterator& operator++(); + Iterator& operator--(); + T operator*(); + bool operator==(Iterator) const; + bool operator!=(Iterator) const; +}; + +template +struct Container { + Iterator begin(); + Iterator 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 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 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 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 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 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] +}