[clang-tidy][readability-container-contains] Extend to any class with contains

nicovank · nicovank · commit 6fae5b43f1e9 · 2024-09-12T01:48:06.000-04:00
This check will now work out of the box with other containers that have a
`contains` method, such as `folly::F14` or Abseil containers.

It will also work with strings, which are basically just weird containers.
`std::string` and `std::string_view` will have a `contains` method starting with
C++23. `llvm::StringRef` and `folly::StringPiece` are examples of existing
implementations with a `contains` method.
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -14,89 +14,156 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::readability {
 
+namespace {
+struct NotMatchingBoundType {
+  NotMatchingBoundType(std::string ArgumentTypeBoundID, DynTypedNode Node)
+      : ArgumentTypeBoundID(std::move(ArgumentTypeBoundID)),
+        Node(std::move(Node)) {}
+  bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+    const Type *ParamType = Node.get<Type>();
+    const Type *ArgType = Nodes.getNodeAs<Type>(ArgumentTypeBoundID);
+    if (!ParamType || !ArgType) {
+      return true;
+    }
+
+    ParamType = ParamType->getUnqualifiedDesugaredType();
+    ArgType = ArgType->getUnqualifiedDesugaredType();
+
+    if (ParamType->isReferenceType()) {
+      ParamType = ParamType->getPointeeType()->getUnqualifiedDesugaredType();
+    }
+
+    while (ParamType->isPointerType()) {
+      if (!ArgType->isPointerType()) {
+        return true;
+      }
+
+      ParamType = ParamType->getPointeeType()->getUnqualifiedDesugaredType();
+      ArgType = ArgType->getPointeeType()->getUnqualifiedDesugaredType();
+    }
+
+    return ParamType != ArgType;
+  }
+
+private:
+  std::string ArgumentTypeBoundID;
+  DynTypedNode Node;
+};
+
+AST_MATCHER_P(Type, matchesBoundType, std::string, ArgumentTypeBoundID) {
+  return Builder->removeBindings(
+      NotMatchingBoundType(ArgumentTypeBoundID, DynTypedNode::create(Node)));
+}
+} // namespace
+
 void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
-  const auto SupportedContainers = hasType(
-      hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
-          hasAnyName("::std::set", "::std::unordered_set", "::std::map",
-                     "::std::unordered_map", "::std::multiset",
-                     "::std::unordered_multiset", "::std::multimap",
-                     "::std::unordered_multimap"))))));
+  const auto HasContainsMethod = hasMethod(cxxMethodDecl(
+      isConst(), parameterCountIs(1), returns(booleanType()),
+      hasName("contains"), unless(isDeleted()), isPublic(),
+      hasParameter(0, hasType(matchesBoundType("argumentType")))));
+  const auto ContainerWithContains = hasType(
+      hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(anyOf(
+          HasContainsMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
+                                 cxxRecordDecl(HasContainsMethod)))))))))));
+
+  // Hack in CountCall and FindCall: hasArgument(0, ...) always ignores implicit
+  // casts. We do not want this behavior. We use hasAnyArgument instead, which
+  // does not ignore implicit casts. Thankfully, we only have one argument in
+  // every case making this possible. Really, hasArgument should also respect
+  // the current traversal mode. GitHub issues #54919 and #75754 track this.
 
   const auto CountCall =
-      cxxMemberCallExpr(on(SupportedContainers),
+      cxxMemberCallExpr(argumentCountIs(1),
                         callee(cxxMethodDecl(hasName("count"))),
-                        argumentCountIs(1))
+                        hasAnyArgument(hasType(type().bind("argumentType"))),
+                        on(ContainerWithContains))
           .bind("call");
 
   const auto FindCall =
-      cxxMemberCallExpr(on(SupportedContainers),
+      cxxMemberCallExpr(argumentCountIs(1),
                         callee(cxxMethodDecl(hasName("find"))),
-                        argumentCountIs(1))
+                        hasAnyArgument(hasType(type().bind("argumentType"))),
+                        on(ContainerWithContains))
           .bind("call");
 
-  const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
+  const auto EndCall = cxxMemberCallExpr(argumentCountIs(0),
                                          callee(cxxMethodDecl(hasName("end"))),
-                                         argumentCountIs(0));
+                                         on(ContainerWithContains));
 
   const auto Literal0 = integerLiteral(equals(0));
   const auto Literal1 = integerLiteral(equals(1));
 
-  auto AddSimpleMatcher = [&](auto Matcher) {
-    Finder->addMatcher(
-        traverse(TK_IgnoreUnlessSpelledInSource, std::move(Matcher)), this);
-  };
-
   // Find membership tests which use `count()`.
   Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
                                       hasSourceExpression(CountCall))
                          .bind("positiveComparison"),
                      this);
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName("!="), hasRHS(Literal0))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal0), hasOperatorName("!="), hasRHS(CountCall))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall))
-          .bind("positiveComparison"));
+  Finder->addMatcher(
+      binaryOperator(hasOperatorName("!="),
+                     hasOperands(ignoringParenImpCasts(CountCall),
+                                 ignoringParenImpCasts(Literal0)))
+          .bind("positiveComparison"),
+      this);
+  Finder->addMatcher(binaryOperator(hasOperatorName(">"),
+                                    hasLHS(ignoringParenImpCasts(CountCall)),
+                                    hasRHS(ignoringParenImpCasts(Literal0)))
+                         .bind("positiveComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName("<"),
+                                    hasLHS(ignoringParenImpCasts(Literal0)),
+                                    hasRHS(ignoringParenImpCasts(CountCall)))
+                         .bind("positiveComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName(">="),
+                                    hasLHS(ignoringParenImpCasts(CountCall)),
+                                    hasRHS(ignoringParenImpCasts(Literal1)))
+                         .bind("positiveComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName("<="),
+                                    hasLHS(ignoringParenImpCasts(Literal1)),
+                                    hasRHS(ignoringParenImpCasts(CountCall)))
+                         .bind("positiveComparison"),
+                     this);
 
   // Find inverted membership tests which use `count()`.
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName("=="), hasRHS(Literal0))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal0), hasOperatorName("=="), hasRHS(CountCall))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
-          .bind("negativeComparison"));
+  Finder->addMatcher(
+      binaryOperator(hasOperatorName("=="),
+                     hasOperands(ignoringParenImpCasts(CountCall),
+                                 ignoringParenImpCasts(Literal0)))
+          .bind("negativeComparison"),
+      this);
+  Finder->addMatcher(binaryOperator(hasOperatorName("<="),
+                                    hasLHS(ignoringParenImpCasts(CountCall)),
+                                    hasRHS(ignoringParenImpCasts(Literal0)))
+                         .bind("negativeComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName(">="),
+                                    hasLHS(ignoringParenImpCasts(Literal0)),
+                                    hasRHS(ignoringParenImpCasts(CountCall)))
+                         .bind("negativeComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName("<"),
+                                    hasLHS(ignoringParenImpCasts(CountCall)),
+                                    hasRHS(ignoringParenImpCasts(Literal1)))
+                         .bind("negativeComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName(">"),
+                                    hasLHS(ignoringParenImpCasts(Literal1)),
+                                    hasRHS(ignoringParenImpCasts(CountCall)))
+                         .bind("negativeComparison"),
+                     this);
 
   // Find membership tests based on `find() == end()`.
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(FindCall), hasOperatorName("!="), hasRHS(EndCall))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(FindCall), hasOperatorName("=="), hasRHS(EndCall))
-          .bind("negativeComparison"));
+  Finder->addMatcher(binaryOperator(hasOperatorName("!="),
+                                    hasOperands(ignoringParenImpCasts(FindCall),
+                                                ignoringParenImpCasts(EndCall)))
+                         .bind("positiveComparison"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName("=="),
+                                    hasOperands(ignoringParenImpCasts(FindCall),
+                                                ignoringParenImpCasts(EndCall)))
+                         .bind("negativeComparison"),
+                     this);
 }
 
 void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -13,8 +13,9 @@
 
 namespace clang::tidy::readability {
 
-/// Finds usages of `container.count()` and `find() == end()` which should be
-/// replaced by a call to the `container.contains()` method introduced in C++20.
+/// Finds usages of ``container.count()`` and
+/// ``container.find() == container.end()`` which should be replaced by a call
+/// to the ``container.contains()`` method.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html
@@ -24,10 +25,11 @@ class ContainerContainsCheck : public ClangTidyCheck {
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) final;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
-
-protected:
   bool isLanguageVersionSupported(const LangOptions &LO) const final {
-    return LO.CPlusPlus20;
+    return LO.CPlusPlus;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_AsIs;
   }
 };
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -123,7 +123,11 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-std-print>` check to support replacing
   member function calls too.
 
-- Improved :doc:`readablility-implicit-bool-conversion
+- Improved :doc:`readability-container-contains
+  <clang-tidy/checks/readability/container-contains>` check
+  to let it work on any class that has a ``contains`` method.
+
+- Improved :doc:`readability-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check
   by adding the option `UseUpperCaseLiteralSuffix` to select the
   case of the literal suffix in fixes.
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
@@ -3,23 +3,31 @@
 readability-container-contains
 ==============================
 
-Finds usages of ``container.count()`` and ``container.find() == container.end()`` which should be replaced by a call to the ``container.contains()`` method introduced in C++20.
+Finds usages of ``container.count()`` and
+``container.find() == container.end()`` which should be replaced by a call to
+the ``container.contains()`` method.
 
-Whether an element is contained inside a container should be checked with ``contains`` instead of ``count``/``find`` because ``contains`` conveys the intent more clearly. Furthermore, for containers which permit multiple entries per key (``multimap``, ``multiset``, ...), ``contains`` is more efficient than ``count`` because ``count`` has to do unnecessary additional work.
+Whether an element is contained inside a container should be checked with
+``contains`` instead of ``count``/``find`` because ``contains`` conveys the
+intent more clearly. Furthermore, for containers which permit multiple entries
+per key (``multimap``, ``multiset``, ...), ``contains`` is more efficient than
+``count`` because ``count`` has to do unnecessary additional work.
 
 Examples:
 
-===========================================  ==============================
-Initial expression                           Result
--------------------------------------------  ------------------------------
-``myMap.find(x) == myMap.end()``             ``!myMap.contains(x)``
-``myMap.find(x) != myMap.end()``             ``myMap.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)``
-``bool exists = myMap.count(x) >= 1``        ``bool exists = myMap.contains(x)``
-``bool missing = myMap.count(x) == 0``       ``bool missing = !myMap.contains(x)``
-===========================================  ==============================
+=========================================  ==============================
+Initial expression                         Result
+-----------------------------------------  ------------------------------
+``myMap.find(x) == myMap.end()``           ``!myMap.contains(x)``
+``myMap.find(x) != myMap.end()``           ``myMap.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)``
+``bool exists = myMap.count(x) >= 1``      ``bool exists = myMap.contains(x)``
+``bool missing = myMap.count(x) == 0``     ``bool missing = !myMap.contains(x)``
+=========================================  ==============================
 
-This check applies to ``std::set``, ``std::unordered_set``, ``std::map``, ``std::unordered_map`` and the corresponding multi-key variants.
-It is only active for C++20 and later, as the ``contains`` method was only added in C++20.
+This check will apply to any class that has a ``contains`` method, notably
+including ``std::set``, ``std::unordered_set``, ``std::map``, and
+``std::unordered_map`` as of C++20, and ``std::string`` and ``std::string_view``
+as of C++23.
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
