[clang-tidy][readability-container-contains] Extend to any class with contains (#107521)

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.
2024-09-18 14:57:31 -04:00
parent 51df8a3327
commit 1be4c9710b
5 changed files with 237 additions and 46 deletions
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -13,30 +13,40 @@
 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 HasContainsMatchingParamType = hasMethod(
+      cxxMethodDecl(isConst(), parameterCountIs(1), returns(booleanType()),
+                    hasName("contains"), unless(isDeleted()), isPublic(),
+                    hasParameter(0, hasType(hasUnqualifiedDesugaredType(
+                                        equalsBoundNode("parameterType"))))));

  const auto CountCall =
-      cxxMemberCallExpr(on(SupportedContainers),
-                        callee(cxxMethodDecl(hasName("count"))),
-                        argumentCountIs(1))
+      cxxMemberCallExpr(
+          argumentCountIs(1),
+          callee(cxxMethodDecl(
+              hasName("count"),
+              hasParameter(0, hasType(hasUnqualifiedDesugaredType(
+                                  type().bind("parameterType")))),
+              ofClass(cxxRecordDecl(HasContainsMatchingParamType)))))
          .bind("call");

  const auto FindCall =
-      cxxMemberCallExpr(on(SupportedContainers),
-                        callee(cxxMethodDecl(hasName("find"))),
-                        argumentCountIs(1))
+      cxxMemberCallExpr(
+          argumentCountIs(1),
+          callee(cxxMethodDecl(
+              hasName("find"),
+              hasParameter(0, hasType(hasUnqualifiedDesugaredType(
+                                  type().bind("parameterType")))),
+              ofClass(cxxRecordDecl(HasContainsMatchingParamType)))))
          .bind("call");

-  const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
-                                         callee(cxxMethodDecl(hasName("end"))),
-                                         argumentCountIs(0));
+  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)))));

  const auto Literal0 = integerLiteral(equals(0));
  const auto Literal1 = integerLiteral(equals(1));
--- 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 @@ public:
      : 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;
  }
 };

--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -165,6 +165,10 @@ Changes in existing checks
  <clang-tidy/checks/performance/avoid-endl>` check to use ``std::endl`` as
  placeholder when lexer cannot get source text.

+- 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
--- 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.
--- 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,180 @@ 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)) {};
+}
+
+template <class Key, class T>
+struct CustomMapContainsDeleted {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const = delete;
+  void *find(const Key &K);
+  void *end();
+};
+
+struct SubmapContainsDeleted : public CustomMapContainsDeleted<int, int> {};
+
+void testContainsDeleted(CustomMapContainsDeleted<int, int> &MyMap,
+                         SubmapContainsDeleted &MyMap2) {
+  // No warning if the `contains` method is deleted.
+  if (MyMap.count(0)) {};
+  if (MyMap2.count(0)) {};
+}
+
+template <class Key, class T>
+struct CustomMapPrivateContains {
+  unsigned count(const Key &K) const;
+  void *find(const Key &K);
+  void *end();
+
+private:
+  bool contains(const Key &K) const;
+};
+
+struct SubmapPrivateContains : public CustomMapPrivateContains<int, int> {};
+
+void testPrivateContains(CustomMapPrivateContains<int, int> &MyMap,
+                         SubmapPrivateContains &MyMap2) {
+  // No warning if the `contains` method is not public.
+  if (MyMap.count(0)) {};
+  if (MyMap2.count(0)) {};
+}
+
+struct MyString {};
+
+struct WeirdNonMatchingContains {
+  unsigned count(char) const;
+  bool contains(const MyString&) const;
+};
+
+void testWeirdNonMatchingContains(WeirdNonMatchingContains &MyMap) {
+  // No warning if there is no `contains` method with the right type.
+  if (MyMap.count('a')) {};
+}
+
+template <class T>
+struct SmallPtrSet {
+  using ConstPtrType = const T*;
+  unsigned count(ConstPtrType Ptr) const;
+  bool contains(ConstPtrType Ptr) const;
+};
+
+template <class T>
+struct SmallPtrPtrSet {
+  using ConstPtrType = const T**;
+  unsigned count(ConstPtrType Ptr) const;
+  bool contains(ConstPtrType Ptr) const;
+};
+
+template <class T>
+struct SmallPtrPtrPtrSet {
+  using ConstPtrType = const T***;
+  unsigned count(ConstPtrType Ptr) const;
+  bool contains(ConstPtrType Ptr) const;
+};
+
+void testSmallPtrSet(const int ***Ptr,
+                     SmallPtrSet<int> &MySet,
+                     SmallPtrPtrSet<int> &MySet2,
+                     SmallPtrPtrPtrSet<int> &MySet3) {
+  if (MySet.count(**Ptr)) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (MySet.contains(**Ptr)) {};
+  if (MySet2.count(*Ptr)) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (MySet2.contains(*Ptr)) {};
+  if (MySet3.count(Ptr)) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (MySet3.contains(Ptr)) {};
+}
+
+struct X {};
+struct Y : public X {};
+
+void testSubclassEntry(SmallPtrSet<X>& Set, Y* Entry) {
+  if (Set.count(Entry)) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // 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]
+  // CHECK-FIXES: if (S.contains(U)) {}
+}
+
+template <class T>
+struct CustomSetConvertible {
+  unsigned count(const T &K) const;
+  bool contains(const T &K) const;
+};
+
+struct A {};
+struct B { B() = default; B(const A&) {} };
+struct C { operator A() const; };
+
+void testConvertibleTypes() {
+  CustomSetConvertible<B> MyMap;
+  if (MyMap.count(A())) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (MyMap.contains(A())) {};
+
+  CustomSetConvertible<A> MyMap2;
+  if (MyMap2.count(C())) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (MyMap2.contains(C())) {};
+
+  if (MyMap2.count(C()) != 0) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (MyMap2.contains(C())) {};
+}
+
+template<class U>
+using Box = const U& ;
+
+template <class T>
+struct CustomBoxedSet {
+  unsigned count(Box<T> K) const;
+  bool contains(Box<T> K) const;
+};
+
+void testBox() {
+  CustomBoxedSet<int> Set;
+  if (Set.count(0)) {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: if (Set.contains(0)) {};
 }