[clang-tidy] Support find for string-like classes in readability-container-contains (#157243)

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

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 =
+      // 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<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

@@ -312,6 +312,11 @@ 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``. Internal changes may cause new rare false positives
+  in non-standard containers.
+
 - 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;