From 4a0fd329647bac00ac7f1e0e4ccaeed8a99af84d Mon Sep 17 00:00:00 2001 From: Nicolas van Kempen Date: Sat, 20 Sep 2025 15:14:14 -0400 Subject: [PATCH] [clang-tidy] Support find for string-like classes in readability-container-contains Fix #109327. Small parts taken from #110351. Removed the type checking between `contains` and `count`/`find` arguments for simplicity. Because of overloads, this type-checking is tricky. The same strategy is used in `modernize-use-starts-ends-with`. Ended up copying over the FixIt logic from `modernize-use-starts-ends-with`, I think it's the simplest. Open to other suggestions. Since this may introduce false positives in (bad readability) edge cases, I tested this change on several major C++ projects, and didn't find any new build or test failures introduced by this change (some errors due to C++23 upgrade, unrelated). Tested projects: 1. kitware/cmake 2. dolphin-emu/dolphin 3. blender/blender 4. opencv/opencv Co-authored-by: dl8sd11 --- .../readability/ContainerContainsCheck.cpp | 94 +++++------ clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../checks/readability/container-contains.rst | 1 + .../clang-tidy/checkers/Inputs/Headers/string | 11 ++ .../readability/container-contains.cpp | 146 +++++++++++------- 5 files changed, 158 insertions(+), 99 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp index 04c1aa2fab8e6..850ef86c85b17 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp @@ -9,47 +9,43 @@ #include "ContainerContainsCheck.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; namespace clang::tidy::readability { void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) { - const auto HasContainsMatchingParamType = hasMethod( - cxxMethodDecl(isConst(), parameterCountIs(1), returns(booleanType()), - hasName("contains"), unless(isDeleted()), isPublic(), - hasParameter(0, hasType(hasUnqualifiedDesugaredType( - equalsBoundNode("parameterType")))))); + const auto Literal0 = integerLiteral(equals(0)); + const auto Literal1 = integerLiteral(equals(1)); + + const auto ClassWithContains = cxxRecordDecl( + hasMethod(cxxMethodDecl(isConst(), parameterCountIs(1), isPublic(), + unless(isDeleted()), returns(booleanType()), + hasAnyName("contains", "Contains")) + .bind("contains_fun"))); const auto CountCall = - cxxMemberCallExpr( - argumentCountIs(1), - callee(cxxMethodDecl( - hasName("count"), - hasParameter(0, hasType(hasUnqualifiedDesugaredType( - type().bind("parameterType")))), - ofClass(cxxRecordDecl(HasContainsMatchingParamType))))) + cxxMemberCallExpr(argumentCountIs(1), + callee(cxxMethodDecl(hasAnyName("count", "Count"), + ofClass(ClassWithContains)))) .bind("call"); const auto FindCall = + // Either one argument, or assume the second argument is the position to + // start searching from. cxxMemberCallExpr( - argumentCountIs(1), - callee(cxxMethodDecl( - hasName("find"), - hasParameter(0, hasType(hasUnqualifiedDesugaredType( - type().bind("parameterType")))), - ofClass(cxxRecordDecl(HasContainsMatchingParamType))))) + anyOf(argumentCountIs(1), + allOf(argumentCountIs(2), hasArgument(1, Literal0))), + callee(cxxMethodDecl(hasAnyName("find", "Find"), + ofClass(ClassWithContains)))) .bind("call"); const auto EndCall = cxxMemberCallExpr( - argumentCountIs(0), - callee( - cxxMethodDecl(hasName("end"), - // In the matchers below, FindCall should always appear - // before EndCall so 'parameterType' is properly bound. - ofClass(cxxRecordDecl(HasContainsMatchingParamType))))); + argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("end", "End"), + ofClass(ClassWithContains)))); - const auto Literal0 = integerLiteral(equals(0)); - const auto Literal1 = integerLiteral(equals(1)); + const auto StringNpos = anyOf(declRefExpr(to(varDecl(hasName("npos")))), + memberExpr(member(hasName("npos")))); auto AddSimpleMatcher = [&](auto Matcher) { Finder->addMatcher( @@ -94,12 +90,14 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) { binaryOperation(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall)) .bind("negativeComparison")); - // Find membership tests based on `find() == end()`. + // Find membership tests based on `find() == end()` or `find() == npos`. AddSimpleMatcher( - binaryOperation(hasOperatorName("!="), hasOperands(FindCall, EndCall)) + binaryOperation(hasOperatorName("!="), + hasOperands(FindCall, anyOf(EndCall, StringNpos))) .bind("positiveComparison")); AddSimpleMatcher( - binaryOperation(hasOperatorName("=="), hasOperands(FindCall, EndCall)) + binaryOperation(hasOperatorName("=="), + hasOperands(FindCall, anyOf(EndCall, StringNpos))) .bind("negativeComparison")); } @@ -114,29 +112,39 @@ void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) { "only one of PositiveComparison or NegativeComparison should be set"); bool Negated = NegativeComparison != nullptr; const auto *Comparison = Negated ? NegativeComparison : PositiveComparison; + const StringRef ContainsFunName = + Result.Nodes.getNodeAs("contains_fun")->getName(); + const Expr *SearchExpr = Call->getArg(0)->IgnoreParenImpCasts(); // Diagnose the issue. - auto Diag = - diag(Call->getExprLoc(), "use 'contains' to check for membership"); + auto Diag = diag(Call->getExprLoc(), "use '%0' to check for membership") + << ContainsFunName; // Don't fix it if it's in a macro invocation. Leave fixing it to the user. SourceLocation FuncCallLoc = Comparison->getEndLoc(); if (!FuncCallLoc.isValid() || FuncCallLoc.isMacroID()) return; - // Create the fix it. - const auto *Member = cast(Call->getCallee()); - Diag << FixItHint::CreateReplacement( - Member->getMemberNameInfo().getSourceRange(), "contains"); - SourceLocation ComparisonBegin = Comparison->getSourceRange().getBegin(); - SourceLocation ComparisonEnd = Comparison->getSourceRange().getEnd(); - SourceLocation CallBegin = Call->getSourceRange().getBegin(); - SourceLocation CallEnd = Call->getSourceRange().getEnd(); + const StringRef SearchExprText = Lexer::getSourceText( + CharSourceRange::getTokenRange(SearchExpr->getSourceRange()), + *Result.SourceManager, Result.Context->getLangOpts()); + + // Remove everything before the function call. + Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange( + Comparison->getBeginLoc(), Call->getBeginLoc())); + + // Rename the function to `contains`. + Diag << FixItHint::CreateReplacement(Call->getExprLoc(), ContainsFunName); + + // Replace arguments and everything after the function call. Diag << FixItHint::CreateReplacement( - CharSourceRange::getCharRange(ComparisonBegin, CallBegin), - Negated ? "!" : ""); - Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange( - CallEnd.getLocWithOffset(1), ComparisonEnd)); + CharSourceRange::getTokenRange(Call->getArg(0)->getBeginLoc(), + Comparison->getEndLoc()), + (SearchExprText + ")").str()); + + // Add negation if necessary. + if (Negated) + Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), "!"); } } // namespace clang::tidy::readability diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6184b3bb9c434..c2598ddcbc534 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -312,6 +312,11 @@ Changes in existing checks ` check to avoid false positives on pure virtual member functions. +- Improved :doc:`readability-container-contains + ` to support string + comparisons to ``npos``. Internal changes may cause new rare false positives + in non-standard containers. + - Improved :doc:`readability-container-size-empty ` check by correctly generating fix-it hints when size method is called from implicit ``this``, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst index 1cfbf4c511c58..9f5b263f7f671 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst @@ -20,6 +20,7 @@ Initial expression Result -------------------------------------- ------------------------------------- ``myMap.find(x) == myMap.end()`` ``!myMap.contains(x)`` ``myMap.find(x) != myMap.end()`` ``myMap.contains(x)`` +``myStr.find(x) != std::string::npos`` ``myStr.contains(x)`` ``if (myMap.count(x))`` ``if (myMap.contains(x))`` ``bool exists = myMap.count(x)`` ``bool exists = myMap.contains(x)`` ``bool exists = myMap.count(x) > 0`` ``bool exists = myMap.contains(x)`` diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string index 7e579e5319752..6cedda4202f14 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string @@ -50,12 +50,19 @@ struct basic_string { size_type find(const _Type& str, size_type pos = 0) const; size_type find(const C* s, size_type pos = 0) const; size_type find(const C* s, size_type pos, size_type n) const; + size_type find(C ch, size_type pos = 0) const; + template + size_type find(const StringViewLike& t, size_type pos = 0) const; size_type rfind(const _Type& str, size_type pos = npos) const; size_type rfind(const C* s, size_type pos, size_type count) const; size_type rfind(const C* s, size_type pos = npos) const; size_type rfind(C ch, size_type pos = npos) const; + constexpr bool contains(std::basic_string_view sv) const noexcept; + constexpr bool contains(C ch) const noexcept; + constexpr bool contains(const C* s) const; + _Type& insert(size_type pos, const _Type& str); _Type& insert(size_type pos, const C* s); _Type& insert(size_type pos, const C* s, size_type n); @@ -110,6 +117,10 @@ struct basic_string_view { size_type rfind(const C* s, size_type pos, size_type count) const; size_type rfind(const C* s, size_type pos = npos) const; + constexpr bool contains(basic_string_view sv) const noexcept; + constexpr bool contains(C ch) const noexcept; + constexpr bool contains(const C* s) const; + constexpr bool starts_with(basic_string_view sv) const noexcept; constexpr bool starts_with(C ch) const noexcept; constexpr bool starts_with(const C* s) const; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp index f345b3e7768a8..1d848ab98e3ba 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp @@ -1,4 +1,7 @@ -// RUN: %check_clang_tidy -std=c++11-or-later %s readability-container-contains %t +// RUN: %check_clang_tidy %s readability-container-contains %t -- \ +// RUN: -- -isystem %clang_tidy_headers + +#include // Some *very* simplified versions of `map` etc. namespace std { @@ -255,9 +258,9 @@ struct CustomMap { }; void testDifferentCheckTypes(CustomMap &MyMap) { - if (MyMap.count(0)) {}; + if (MyMap.count(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MyMap.contains(0)) {}; + // CHECK-FIXES: if (MyMap.contains(0)) {} MyMap.find(0) != MyMap.end(); // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] @@ -267,9 +270,9 @@ void testDifferentCheckTypes(CustomMap &MyMap) { struct MySubmap : public CustomMap {}; void testSubclass(MySubmap& MyMap) { - if (MyMap.count(0)) {}; + if (MyMap.count(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MyMap.contains(0)) {}; + // CHECK-FIXES: if (MyMap.contains(0)) {} } using UsingMap = CustomMap; @@ -277,9 +280,9 @@ struct MySubmap2 : public UsingMap {}; using UsingMap2 = MySubmap2; void testUsing(UsingMap2& MyMap) { - if (MyMap.count(0)) {}; + if (MyMap.count(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MyMap.contains(0)) {}; + // CHECK-FIXES: if (MyMap.contains(0)) {} } template @@ -295,8 +298,8 @@ struct SubmapContainsDeleted : public CustomMapContainsDeleted {}; void testContainsDeleted(CustomMapContainsDeleted &MyMap, SubmapContainsDeleted &MyMap2) { // No warning if the `contains` method is deleted. - if (MyMap.count(0)) {}; - if (MyMap2.count(0)) {}; + if (MyMap.count(0)) {} + if (MyMap2.count(0)) {} } template @@ -314,20 +317,22 @@ struct SubmapPrivateContains : public CustomMapPrivateContains {}; void testPrivateContains(CustomMapPrivateContains &MyMap, SubmapPrivateContains &MyMap2) { // No warning if the `contains` method is not public. - if (MyMap.count(0)) {}; - if (MyMap2.count(0)) {}; + if (MyMap.count(0)) {} + if (MyMap2.count(0)) {} } -struct MyString {}; - struct WeirdNonMatchingContains { unsigned count(char) const; - bool contains(const MyString&) const; + bool contains(const std::string&) const; }; -void testWeirdNonMatchingContains(WeirdNonMatchingContains &MyMap) { - // No warning if there is no `contains` method with the right type. - if (MyMap.count('a')) {}; +// False positives: when count/find and contains take different types, +// the check will suggest an invalid code transformation. These cases +// should not exist in real code or be rare enough. +void f(WeirdNonMatchingContains MyMap) { + if (MyMap.count('a')) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (MyMap.contains('a')) {} } template @@ -355,15 +360,15 @@ void testSmallPtrSet(const int ***Ptr, SmallPtrSet &MySet, SmallPtrPtrSet &MySet2, SmallPtrPtrPtrSet &MySet3) { - if (MySet.count(**Ptr)) {}; + if (MySet.count(**Ptr)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MySet.contains(**Ptr)) {}; - if (MySet2.count(*Ptr)) {}; + // CHECK-FIXES: if (MySet.contains(**Ptr)) {} + if (MySet2.count(*Ptr)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MySet2.contains(*Ptr)) {}; - if (MySet3.count(Ptr)) {}; + // CHECK-FIXES: if (MySet2.contains(*Ptr)) {} + if (MySet3.count(Ptr)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MySet3.contains(Ptr)) {}; + // CHECK-FIXES: if (MySet3.contains(Ptr)) {} } struct X {}; @@ -375,15 +380,6 @@ void testSubclassEntry(SmallPtrSet& Set, Y* Entry) { // CHECK-FIXES: if (Set.contains(Entry)) {} } -struct WeirdPointerApi { - unsigned count(int** Ptr) const; - bool contains(int* Ptr) const; -}; - -void testWeirdApi(WeirdPointerApi& Set, int* E) { - if (Set.count(&E)) {} -} - void testIntUnsigned(std::set& S, unsigned U) { if (S.count(U)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] @@ -402,18 +398,18 @@ struct C { operator A() const; }; void testConvertibleTypes() { CustomSetConvertible MyMap; - if (MyMap.count(A())) {}; + if (MyMap.count(A())) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MyMap.contains(A())) {}; + // CHECK-FIXES: if (MyMap.contains(A())) {} CustomSetConvertible MyMap2; - if (MyMap2.count(C())) {}; + if (MyMap2.count(C())) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MyMap2.contains(C())) {}; + // CHECK-FIXES: if (MyMap2.contains(C())) {} - if (MyMap2.count(C()) != 0) {}; + if (MyMap2.count(C()) != 0) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (MyMap2.contains(C())) {}; + // CHECK-FIXES: if (MyMap2.contains(C())) {} } template @@ -427,34 +423,72 @@ struct CustomBoxedSet { void testBox() { CustomBoxedSet Set; - if (Set.count(0)) {}; + if (Set.count(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (Set.contains(0)) {}; + // CHECK-FIXES: if (Set.contains(0)) {} } void testOperandPermutations(std::map& Map) { - if (Map.count(0) != 0) {}; + if (Map.count(0) != 0) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (Map.contains(0)) {}; - if (0 != Map.count(0)) {}; + // CHECK-FIXES: if (Map.contains(0)) {} + if (0 != Map.count(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (Map.contains(0)) {}; - if (Map.count(0) == 0) {}; + // CHECK-FIXES: if (Map.contains(0)) {} + if (Map.count(0) == 0) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (!Map.contains(0)) {}; - if (0 == Map.count(0)) {}; + // CHECK-FIXES: if (!Map.contains(0)) {} + if (0 == Map.count(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (!Map.contains(0)) {}; - if (Map.find(0) != Map.end()) {}; + // CHECK-FIXES: if (!Map.contains(0)) {} + if (Map.find(0) != Map.end()) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (Map.contains(0)) {}; - if (Map.end() != Map.find(0)) {}; + // CHECK-FIXES: if (Map.contains(0)) {} + if (Map.end() != Map.find(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (Map.contains(0)) {}; - if (Map.find(0) == Map.end()) {}; + // CHECK-FIXES: if (Map.contains(0)) {} + if (Map.find(0) == Map.end()) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (!Map.contains(0)) {}; - if (Map.end() == Map.find(0)) {}; + // CHECK-FIXES: if (!Map.contains(0)) {} + if (Map.end() == Map.find(0)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] - // CHECK-FIXES: if (!Map.contains(0)) {}; + // CHECK-FIXES: if (!Map.contains(0)) {} +} + +void testStringNpos(std::string Str1, std::string Str2, std::string_view StrView) { + if (Str1.find(Str2) == std::string::npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (!Str1.contains(Str2)) {} + + if (Str1.find("test") == std::string::npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (!Str1.contains("test")) {} + + if (Str1.find('c') != std::string::npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (Str1.contains('c')) {} + + if (Str1.find(Str2, 0) != std::string::npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (Str1.contains(Str2)) {} + + if (std::string::npos == Str1.find("test", 0)) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (!Str1.contains("test")) {} + + if (StrView.find("test") == std::string_view::npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (!StrView.contains("test")) {} + + if (StrView.find('c') != std::string_view::npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (StrView.contains('c')) {} + + if (StrView.find('c') != StrView.npos) {} + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: if (StrView.contains('c')) {} + + // These should not match. + if (Str1.find('c', 1) != Str1.npos) {} + if (Str1.find(StrView, 1) != Str1.npos) {} }