[clang-tidy] Add readability-container-contains check

This commit introduces a new check `readability-container-contains` which finds usages of `container.count()` and `container.find() != container.end()` and instead recommends the `container.contains()` method introduced in C++20. For containers which permit multiple entries per key (`multimap`, `multiset`, ...), `contains` is more efficient than `count` because `count` has to do unnecessary additional work. While this this performance difference does not exist for containers with only a single entry per key (`map`, `unordered_map`, ...), `contains` still conveys the intent better. Reviewed By: xazax.hun, whisperity Differential Revision: http://reviews.llvm.org/D112646
llvm · Jan 24, 2022 · 3696c70 · 3696c70
1 parent 3e50593
commit 3696c70
Show file tree

Hide file tree

Showing 8 changed files with 450 additions and 0 deletions.
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -7,6 +7,7 @@ add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
+  ContainerContainsCheck.cpp
   ContainerDataPointerCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp

diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -0,0 +1,144 @@
+//===--- ContainerContainsCheck.cpp - clang-tidy --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ContainerContainsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace 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 CountCall =
+      cxxMemberCallExpr(on(SupportedContainers),
+                        callee(cxxMethodDecl(hasName("count"))),
+                        argumentCountIs(1))
+          .bind("call");
+
+  const auto FindCall =
+      cxxMemberCallExpr(on(SupportedContainers),
+                        callee(cxxMethodDecl(hasName("find"))),
+                        argumentCountIs(1))
+          .bind("call");
+
+  const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
+                                         callee(cxxMethodDecl(hasName("end"))),
+                                         argumentCountIs(0));
+
+  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"));
+
+  // 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"));
+
+  // 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"));
+}
+
+void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
+  // Extract the information about the match
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const auto *PositiveComparison =
+      Result.Nodes.getNodeAs<Expr>("positiveComparison");
+  const auto *NegativeComparison =
+      Result.Nodes.getNodeAs<Expr>("negativeComparison");
+  assert(
+      !PositiveComparison ||
+      !NegativeComparison &&
+          "only one of PositiveComparison or NegativeComparison should be set");
+  bool Negated = NegativeComparison != nullptr;
+  const auto *Comparison = Negated ? NegativeComparison : PositiveComparison;
+
+  // Diagnose the issue.
+  auto Diag =
+      diag(Call->getExprLoc(), "use 'contains' to check for membership");
+
+  // 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();
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange::getCharRange(ComparisonBegin, CallBegin),
+      Negated ? "!" : "");
+  Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+      CallEnd.getLocWithOffset(1), ComparisonEnd.getLocWithOffset(1)));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -0,0 +1,40 @@
+//===--- ContainerContainsCheck.h - clang-tidy ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERCONTAINSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERCONTAINSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace 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.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-container-contains.html
+class ContainerContainsCheck : public ClangTidyCheck {
+public:
+  ContainerContainsCheck(StringRef Name, ClangTidyContext *Context)
+      : 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;
+  }
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERCONTAINSCHECK_H
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -12,6 +12,7 @@
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
+#include "ContainerContainsCheck.h"
 #include "ContainerDataPointerCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
@@ -64,6 +65,8 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ConstReturnTypeCheck>(
         "readability-const-return-type");
+    CheckFactories.registerCheck<ContainerContainsCheck>(
+        "readability-container-contains");
     CheckFactories.registerCheck<ContainerDataPointerCheck>(
         "readability-container-data-pointer");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -118,6 +118,12 @@ New checks
 
   Reports identifier with unicode right-to-left characters.
 
+- New :doc:`readability-container-contains
+  <clang-tidy/checks/readability-container-contains>` check.
+
+  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.
+
 - New :doc:`readability-container-data-pointer
   <clang-tidy/checks/readability-container-data-pointer>` check.
 

diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -290,6 +290,7 @@ Clang-Tidy Checks
    `readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_, "Yes"
    `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"
    `readability-const-return-type <readability-const-return-type.html>`_, "Yes"
+   `readability-container-contains <readability-container-contains.html>`_, "Yes"
    `readability-container-data-pointer <readability-container-data-pointer.html>`_, "Yes"
    `readability-container-size-empty <readability-container-size-empty.html>`_, "Yes"
    `readability-convert-member-functions-to-static <readability-convert-member-functions-to-static.html>`_, "Yes"

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
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - readability-container-contains
+
+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.
+
+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)``
+===========================================  ==============================
+
+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.