llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) <details> <summary>Changes</summary> This PR introduces a new check `bugprone-missing-end-comparison`. It detects instances where the result of a standard algorithm is used directly in a boolean context without being compared against the corresponding end iterator. Currently the check can't handle algorithms returning `std::pair` and `std::ranges::mismatch_result`, but it should be a good enough starting point for future improvements. Closes https://github.com/llvm/llvm-project/issues/178731 --- Patch is 21.87 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/182543.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp (+175) - (added) clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.h (+34) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/missing-end-comparison.rst (+69) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/missing-end-comparison.cpp (+196) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 310184037afbd..f7723df85e868 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -51,6 +51,7 @@ #include "MisplacedOperatorInStrlenInAllocCheck.h" #include "MisplacedPointerArithmeticInAllocCheck.h" #include "MisplacedWideningCastCheck.h" +#include "MissingEndComparisonCheck.h" #include "MoveForwardingReferenceCheck.h" #include "MultiLevelImplicitPointerConversionCheck.h" #include "MultipleNewInOneExpressionCheck.h" @@ -177,6 +178,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( "bugprone-incorrect-enable-shared-from-this"); + CheckFactories.registerCheck<MissingEndComparisonCheck>( + "bugprone-missing-end-comparison"); CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 96ad671d03b39..275decf647e00 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -37,6 +37,7 @@ add_clang_library(clangTidyBugproneModule STATIC IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp + MissingEndComparisonCheck.cpp UnintendedCharOstreamOutputCheck.cpp ReturnConstRefFromParameterCheck.cpp SuspiciousStringviewDataUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp new file mode 100644 index 0000000000000..42ff99b121f84 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.cpp @@ -0,0 +1,175 @@ +//===----------------------------------------------------------------------===// +// +// 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 "MissingEndComparisonCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "clang/Tooling/FixIt.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +constexpr llvm::StringRef IteratorAlgorithms[] = { + "::std::find", "::std::find_if", + "::std::find_if_not", "::std::search", + "::std::search_n", "::std::find_end", + "::std::find_first_of", "::std::lower_bound", + "::std::upper_bound", "::std::partition_point", + "::std::min_element", "::std::max_element", + "::std::adjacent_find", "::std::is_sorted_until"}; + +constexpr llvm::StringRef RangeAlgorithms[] = { + "::std::ranges::find", "::std::ranges::find_if", + "::std::ranges::find_if_not", "::std::ranges::lower_bound", + "::std::ranges::upper_bound", "::std::ranges::min_element", + "::std::ranges::max_element"}; + +} // namespace + +void MissingEndComparisonCheck::registerMatchers(MatchFinder *Finder) { + const auto StdAlgoCall = callExpr( + callee(functionDecl(hasAnyName(IteratorAlgorithms), isInStdNamespace()))); + + const auto RangesCall = cxxOperatorCallExpr( + hasOverloadedOperatorName("()"), + hasArgument(0, declRefExpr(to( + varDecl(hasAnyName(RangeAlgorithms)).bind("cpo"))))); + + const auto AnyAlgoCall = + getLangOpts().CPlusPlus20 + ? expr(anyOf(StdAlgoCall, RangesCall)).bind("algoCall") + : expr(StdAlgoCall).bind("algoCall"); + + // Captures implicit pointer-to-bool casts and operator bool() calls. + const auto IsBoolUsage = anyOf( + implicitCastExpr(hasCastKind(CK_PointerToBoolean), + hasSourceExpression(ignoringParenImpCasts(AnyAlgoCall))), + cxxMemberCallExpr(callee(cxxConversionDecl(returns(booleanType()))), + on(ignoringParenImpCasts(AnyAlgoCall)))); + + // Captures variable usage: `auto it = std::find(...); if (it)` + // FIXME: This only handles variables initialized directly by the algorithm. + // We may need to introduce more accurate dataflow analysis in the future. + const auto VarWithAlgoInit = + varDecl(hasInitializer(ignoringParenImpCasts(AnyAlgoCall))); + + const auto IsVariableBoolUsage = + anyOf(implicitCastExpr(hasCastKind(CK_PointerToBoolean), + hasSourceExpression(ignoringParenImpCasts( + declRefExpr(to(VarWithAlgoInit))))), + cxxMemberCallExpr( + callee(cxxConversionDecl(returns(booleanType()))), + on(ignoringParenImpCasts(declRefExpr(to(VarWithAlgoInit)))))); + + Finder->addMatcher( + expr(anyOf(IsBoolUsage, IsVariableBoolUsage)).bind("boolOp"), this); +} + +void MissingEndComparisonCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("algoCall"); + const auto *BoolOp = Result.Nodes.getNodeAs<Expr>("boolOp"); + const auto *CPO = Result.Nodes.getNodeAs<VarDecl>("cpo"); + + if (!Call || !BoolOp) + return; + + std::string EndExprText; + + if (!CPO) { + if (Call->getNumArgs() < 2) + return; + + const Expr *EndArg = Call->getArg(1); + // Filters nullptr, we assume the intent might be a valid check against null + if (EndArg->IgnoreParenCasts()->isNullPointerConstant( + *Result.Context, Expr::NPC_ValueDependentIsNull)) + return; + + EndExprText = tooling::fixit::getText(*EndArg, *Result.Context).str(); + } else { + const FunctionDecl *Callee = Call->getDirectCallee(); + if (!Callee || Callee->getNumParams() == 0) + return; + + // Range overloads take a reference (R&&), Iterator overloads pass by value. + const bool IsIterPair = + !Callee->getParamDecl(0)->getType()->isReferenceType(); + + if (IsIterPair) { + if (Call->getNumArgs() < 3) + return; + // find(CPO, Iter, Sent, Val...) -> Sent is Arg 2. + const Expr *EndArg = Call->getArg(2); + EndExprText = tooling::fixit::getText(*EndArg, *Result.Context).str(); + } else { + if (Call->getNumArgs() < 2) + return; + // find(CPO, Range, Val, Proj) -> Range is Arg 1. + const Expr *RangeArg = Call->getArg(1); + // Avoid potential side-effects + const Expr *InnerRange = RangeArg->IgnoreParenImpCasts(); + if (isa<DeclRefExpr>(InnerRange) || isa<MemberExpr>(InnerRange)) { + const StringRef RangeText = + tooling::fixit::getText(*RangeArg, *Result.Context); + if (!RangeText.empty()) + EndExprText = ("std::ranges::end(" + RangeText + ")").str(); + } + } + } + + bool IsNegated = false; + const UnaryOperator *NotOp = nullptr; + const Expr *CurrentExpr = BoolOp; + while (true) { + auto Parents = Result.Context->getParents(*CurrentExpr); + if (Parents.empty()) + break; + if (const auto *P = Parents[0].get<ParenExpr>()) { + CurrentExpr = P; + continue; + } + if (const auto *U = Parents[0].get<UnaryOperator>()) { + if (U->getOpcode() == UO_LNot) { + NotOp = U; + IsNegated = true; + } + } + break; + } + + const auto Diag = + diag(BoolOp->getBeginLoc(), + "result of standard algorithm used in boolean context; did " + "you mean to compare with the end iterator?"); + + if (!EndExprText.empty()) { + if (IsNegated) { + // !it -> (it == end) + Diag << FixItHint::CreateReplacement(NotOp->getOperatorLoc(), "("); + Diag << FixItHint::CreateInsertion( + Lexer::getLocForEndOfToken(BoolOp->getEndLoc(), 0, + *Result.SourceManager, + Result.Context->getLangOpts()), + " == " + EndExprText + ")"); + } else { + // it -> (it != end) + Diag << FixItHint::CreateInsertion(BoolOp->getBeginLoc(), "("); + Diag << FixItHint::CreateInsertion( + Lexer::getLocForEndOfToken(BoolOp->getEndLoc(), 0, + *Result.SourceManager, + Result.Context->getLangOpts()), + " != " + EndExprText + ")"); + } + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.h b/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.h new file mode 100644 index 0000000000000..75a2afab5a3a9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MissingEndComparisonCheck.h @@ -0,0 +1,34 @@ +//===----------------------------------------------------------------------===// +// +// 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_MISSINGENDCOMPARISONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISSINGENDCOMPARISONCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Detects usage of the result of standard algorithms (like std::find) in a +/// boolean context without comparing it with the end iterator. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/missing-end-comparison.html +class MissingEndComparisonCheck : public ClangTidyCheck { +public: + MissingEndComparisonCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISSINGENDCOMPARISONCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 68bab88146241..b89e2c0c60bf3 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -102,6 +102,12 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`bugprone-missing-end-comparison + <clang-tidy/checks/bugprone/missing-end-comparison>` check. + + Finds instances where the result of a standard algorithm is used in a boolean + context without being compared to the end iterator. + - New :doc:`bugprone-unsafe-to-allow-exceptions <clang-tidy/checks/bugprone/unsafe-to-allow-exceptions>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/missing-end-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/missing-end-comparison.rst new file mode 100644 index 0000000000000..4f1ac0d7657ab --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/missing-end-comparison.rst @@ -0,0 +1,69 @@ +.. title:: clang-tidy - bugprone-missing-end-comparison + +bugprone-missing-end-comparison +=============================== + +Finds instances where the result of a standard algorithm is used in a boolean +context without being compared to the end iterator. + +Standard algorithms such as `std::find`, `std::search`, and `std::lower_bound` +return an iterator to the element if found, or the end iterator otherwise. + +Using the result directly in a boolean context (like an `if` statement) is +almost always a bug, as it only checks if the iterator itself evaluates to +`true`, which may always be true for many iterator types (including pointers). + +Examples: + +.. code-block:: c++ + + int arr[] = {1, 2, 3}; + int* begin = std::begin(arr); + int* end = std::end(arr); + + // Problematic: + if (std::find(begin, end, 2)) { + // ... + } + + // Fixed by the check: + if ((std::find(begin, end, 2) != end)) { + // ... + } + + // C++20 ranges: + std::vector<int> v = {1, 2, 3}; + if (std::ranges::find(v, 2)) { // Problematic + // ... + } + + // Fixed by the check: + if ((std::ranges::find(v, 2) != std::ranges::end(v))) { + // ... + } + +The check also handles range-based algorithms introduced in C++20. + +Supported algorithms: + +- `std::find` +- `std::find_if` +- `std::find_if_not` +- `std::search` +- `std::search_n` +- `std::find_end` +- `std::find_first_of` +- `std::lower_bound` +- `std::upper_bound` +- `std::partition_point` +- `std::min_element` +- `std::max_element` +- `std::adjacent_find` +- `std::is_sorted_until` +- `std::ranges::find` +- `std::ranges::find_if` +- `std::ranges::find_if_not` +- `std::ranges::lower_bound` +- `std::ranges::upper_bound` +- `std::ranges::min_element` +- `std::ranges::max_element` diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c475870ed7b31..7ed101ed400db 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -121,6 +121,7 @@ Clang-Tidy Checks :doc:`bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc>`, "Yes" :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc>`, "Yes" :doc:`bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast>`, + :doc:`bugprone-missing-end-comparison <bugprone/missing-end-comparison>`, "Yes" :doc:`bugprone-move-forwarding-reference <bugprone/move-forwarding-reference>`, "Yes" :doc:`bugprone-multi-level-implicit-pointer-conversion <bugprone/multi-level-implicit-pointer-conversion>`, :doc:`bugprone-multiple-new-in-one-expression <bugprone/multiple-new-in-one-expression>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/missing-end-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/missing-end-comparison.cpp new file mode 100644 index 0000000000000..6bbeef5cbde6f --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/missing-end-comparison.cpp @@ -0,0 +1,196 @@ +// RUN: %check_clang_tidy -std=c++20 %s bugprone-missing-end-comparison %t + +namespace std { + template<typename T> struct iterator_traits; + struct forward_iterator_tag {}; + + typedef long int ptrdiff_t; + typedef decltype(nullptr) nullptr_t; + + template<typename T> + struct vector { + typedef T* iterator; + typedef const T* const_iterator; + iterator begin(); + iterator end(); + const_iterator begin() const; + const_iterator end() const; + }; + + template<class InputIt, class T> + InputIt find(InputIt first, InputIt last, const T& value); + + template<class ForwardIt, class T> + ForwardIt lower_bound(ForwardIt first, ForwardIt last, const T& value); + + template<class ForwardIt, class ForwardIt2> + ForwardIt search(ForwardIt first, ForwardIt last, ForwardIt first2, ForwardIt2 last2); + + template<class ForwardIt> + ForwardIt min_element(ForwardIt first, ForwardIt last); + + template<class InputIt1, class InputIt2> + struct pair { + InputIt1 first; + InputIt2 second; + }; + + namespace ranges { + template<typename T> + void* begin(T& t); + template<typename T> + void* end(T& t); + + struct FindFn { + template<typename Range, typename T> + void* operator()(Range&& r, const T& value) const; + + template<typename I, typename S, typename T> + void* operator()(I first, S last, const T& value) const; + }; + inline constexpr FindFn find; + } +} + +struct CustomIterator { + int* ptr; + using iterator_category = std::forward_iterator_tag; + using value_type = int; + using difference_type = long; + using pointer = int*; + using reference = int&; + + int& operator*() const { return *ptr; } + CustomIterator& operator++() { ++ptr; return *this; } + bool operator==(const CustomIterator& other) const { return ptr == other.ptr; } + bool operator!=(const CustomIterator& other) const { return ptr != other.ptr; } + + explicit operator bool() const { return ptr != nullptr; } +}; + +void test_raw_pointers() { + int arr[] = {1, 2, 3}; + int* begin = arr; + int* end = arr + 3; + + if (std::find(begin, end, 2)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((std::find(begin, end, 2) != end)) {} + + while (std::lower_bound(begin, end, 2)) { break; } + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: while ((std::lower_bound(begin, end, 2) != end)) { break; } + + if (!std::find(begin, end, 2)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((std::find(begin, end, 2) == end)) {} +} + +void test_vector() { + std::vector<int> v; + if (std::find(v.begin(), v.end(), 2)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((std::find(v.begin(), v.end(), 2) != v.end())) {} + + if (std::min_element(v.begin(), v.end())) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((std::min_element(v.begin(), v.end()) != v.end())) {} +} + +void test_variable_tracking() { + int arr[] = {1, 2, 3}; + auto it = std::find(arr, arr + 3, 2); + if (it) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((it != arr + 3)) {} + + if (!it) {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((it == arr + 3)) {} +} + +void test_ranges() { + std::vector<int> v; + if (std::ranges::find(v, 2)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((std::ranges::find(v, 2) != std::ranges::end(v))) {} + + auto it = std::ranges::find(v, 2); + if (it) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((it != std::ranges::end(v))) {} +} + +void test_ranges_iterator_pair() { + int arr[] = {1, 2, 3}; + int *begin = arr; + int *end = arr + 3; + if (std::ranges::find(begin, end, 2)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] + // CHECK-FIXES: if ((std::ranges::find(begin, end, 2) != end)) {} +} + +void test_side_effects() { + std::vector<int> get_vec(); + if (std::ranges::find(get_vec(), 2)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of standard algorithm used in boolean context; did you mean to compare with the end iterator? [bugprone-missing-end-comparison] +} + +void test_negative() { + std::vector<int> v; + if (std::find(v.begin(), v.end(), 2) != v.end()) {} + if (std::ranges::find(v, 2) == std::ranges::end(v)) {} + auto it = std::find(v.begin(), v.end(), 2); +} + +void te... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/182543 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
