llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Nicolas van Kempen (nicovank) <details> <summary>Changes</summary> 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 <gcchen@<!-- -->google.com> --- Full diff: https://github.com/llvm/llvm-project/pull/157243.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp (+52-44) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst (+1) - (modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string (+11) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp (+47-17) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp index fb68c7d334b7f..6157a7121f1e5 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 = - cxxMemberCallExpr( - argumentCountIs(1), - callee(cxxMethodDecl( - hasName("find"), - hasParameter(0, hasType(hasUnqualifiedDesugaredType( - type().bind("parameterType")))), - ofClass(cxxRecordDecl(HasContainsMatchingParamType))))) + // Either one argument, or assume the second argument is the position to + // start searching from. + cxxMemberCallExpr(anyOf(argumentCountIs(1), + cxxMemberCallExpr(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<CXXMethodDecl>("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<MemberExpr>(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 auto 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 32e4dfb8aa329..82f9ed101b64e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -230,6 +230,10 @@ Changes in existing checks <clang-tidy/checks/portability/template-virtual-member-function>` check to avoid false positives on pure virtual member functions. +- Fixed false negatives on string and string-like types in + :doc:`readability-container-contains + <clang-tidy/checks/readability/container-contains>`. + - Improved :doc:`readability-container-size-empty <clang-tidy/checks/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<class StringViewLike> + 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<C, T> 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..dbd1553d22cf2 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 -std=c++11-or-later %s readability-container-contains %t -- \ +// RUN: -- -isystem %clang_tidy_headers + +#include <string> // Some *very* simplified versions of `map` etc. namespace std { @@ -318,17 +321,15 @@ void testPrivateContains(CustomMapPrivateContains<int, int> &MyMap, 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) { MyMap.count('a'); } template <class T> struct SmallPtrSet { @@ -375,15 +376,6 @@ void testSubclassEntry(SmallPtrSet<X>& 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<int>& S, unsigned U) { if (S.count(U)) {} // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] @@ -458,3 +450,41 @@ void testOperandPermutations(std::map<int, int>& Map) { // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains] // 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) {}; +} `````````` </details> https://github.com/llvm/llvm-project/pull/157243 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
