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] +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits