[clang-tidy] Support find for string-like classes in readability-container-contains

Fix llvm#109327. Small parts taken from llvm#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 <[email protected]>

Author: nicovank

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 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

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.
 
+- Improved :doc:`readability-container-contains
+  <clang-tidy/checks/readability/container-contains>` to support string
+  comparisons to ``npos``.
+
 - 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``,

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)``

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;

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) {};
+}