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

[nicovank]

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.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Nicolas van Kempen (nicovank)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/107521.diff

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp (+8-9)
• (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h (+1-3)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5-1)
• (modified) clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst (+23-15)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp (+27-10)

diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index dbb50a060e5960..9fe5fbae32173a 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -15,26 +15,25 @@ using namespace clang::ast_matchers;
 namespace clang::tidy::readability {
 
 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(hasName("contains")));
+  const auto ContainerWithContains = hasType(
+      hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(anyOf(
+          HasContainsMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
+                                 cxxRecordDecl(HasContainsMethod)))))))))));
 
   const auto CountCall =
-      cxxMemberCallExpr(on(SupportedContainers),
+      cxxMemberCallExpr(on(ContainerWithContains),
                         callee(cxxMethodDecl(hasName("count"))),
                         argumentCountIs(1))
           .bind("call");
 
   const auto FindCall =
-      cxxMemberCallExpr(on(SupportedContainers),
+      cxxMemberCallExpr(on(ContainerWithContains),
                         callee(cxxMethodDecl(hasName("find"))),
                         argumentCountIs(1))
           .bind("call");
 
-  const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
+  const auto EndCall = cxxMemberCallExpr(on(ContainerWithContains),
                                          callee(cxxMethodDecl(hasName("end"))),
                                          argumentCountIs(0));
 
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
index 2e8276d684cd79..de9fb862f49c7a 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -24,10 +24,8 @@ 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;
   }
 };
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6999c1ef2ea4b0..ccf7ace1811dc3 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,7 +116,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
index b28daecf7a2cf3..86aac019359ff9 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
@@ -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
index 0ecb61b2e7df06..2342b8b38eb883 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
@@ -240,7 +240,7 @@ int testMacroExpansion(std::unordered_set<int> &MySet) {
   return 0;
 }
 
-// The following map has the same interface like `std::map`.
+// The following map has the same interface as `std::map`.
 template <class Key, class T>
 struct CustomMap {
   unsigned count(const Key &K) const;
@@ -249,13 +249,30 @@ struct CustomMap {
   void *end();
 };
 
-// The clang-tidy check is currently hard-coded against the `std::` containers
-// and hence won't annotate the following instance. We might change this in the
-// future and also detect the following case.
-void *testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
-  if (MyMap.count(0))
-    // NO-WARNING.
-    // CHECK-FIXES: if (MyMap.count(0))
-    return nullptr;
-  return MyMap.find(2);
+void testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
+  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)) {};
+
+  MyMap.find(0) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: MyMap.contains(0);
+}
+
+struct MySubmap : public CustomMap<int, int> {};
+
+void testSubclass(MySubmap& MyMap) {
+  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)) {};
+}
+
+using UsingMap = CustomMap<int, int>;
+struct MySubmap2 : public UsingMap {};
+using UsingMap2 = MySubmap2;
+
+void testUsing(UsingMap2& MyMap) {
+  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)) {};
 }

[nicovank]

Pinging @vogelsgesang, @PiotrZSL, @5chmidti, @HerrCai0907, @carlosgalvezp for review.

I applied this modified version to llvm-project root, 554 files changed, check-all passes. Almost all cases are moving from count to contains. How do you suggest putting up this PR for easier review? Split it up by top-level directory?

[5chmidti]

I applied this modified version to llvm-project root

One PR per subproject seems to be the preferred way, judging by the responses to some of those all of llvm PRs. IMO, this is a readability cleanup and would probably be accepted.

[nicovank]

... and is sufficiently visible.

I added isPublic, I think the false negatives where we are in another private method would be exceedingly rare so I don't think handling that case is necessary.

check if the type of the parameter from the called method and that of the contains function are equal.

Checking types turns out is very tricky because of casting, conversions, references, pointers, amplified by template types which these classes typically have. Is there any other checks doing this? I wrote a version that works but adds some complexity. On llvm-project, there are 5 cases not matched by this version that previously were without checking types (546/551), still investigating what's going on with those.

FWIW in modernize-use-starts-ends-with I didn't add type-checking: UseStartsEndsWithCheck.cpp#L26-L30.

[nicovank]

Update, now getting the same hits on llvm-project with/without type checking (553/553), a good indication no true positives are missed.

[nicovank]

I was over-complicating the task. Instead of trying to match the type of the parameter with the type of the argument, I can just match the types of both parameters. Since on "normal" containers methods will have the same signature minus name, this shouldn't remove any true positives due to conversions.

See updated simplified PR. Number of matches on llvm-project is the same with or without hasParameter(0, ...) clauses.

[HerrCai0907]

LGTM