[clang-tools-extra] [clang-tidy] Support find for string-like classes in readability-container-contains (PR #157243)
Nicolas van Kempen via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 8 12:06:43 PDT 2025
https://github.com/nicovank updated https://github.com/llvm/llvm-project/pull/157243
>From 8ae7dd75aaa7d62bb9926e822fda01d2173f3799 Mon Sep 17 00:00:00 2001
From: Nicolas van Kempen <nvankemp at gmail.com>
Date: Mon, 8 Sep 2025 15:06:29 -0400
Subject: [PATCH] [clang-tidy] Support find for string-like classes in
readability-container-contains
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`.
Ended up copying over the FixIt logic from `modernize-use-starts-ends-with`, I think it's the simplest. Open to other suggestions.
Since this may introduce false positives in (bad readability) edge cases, I tested this change on several major C++ projects, and didn't find any new build or test failures introduced by this change (some errors due to C++23 upgrade, unrelated). Tested projects:
1. kitware/cmake
2. dolphin-emu/dolphin
3. blender/blender
4. opencv/opencv
Co-authored-by: dl8sd11 <gcchen at google.com>
---
.../readability/ContainerContainsCheck.cpp | 96 ++++++++++---------
clang-tools-extra/docs/ReleaseNotes.rst | 4 +
.../checks/readability/container-contains.rst | 1 +
.../clang-tidy/checkers/Inputs/Headers/string | 11 +++
.../readability/container-contains.cpp | 66 +++++++++----
5 files changed, 118 insertions(+), 60 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index fb68c7d334b7f..628cf895a3612 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/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 =
- cxxMemberCallExpr(
- argumentCountIs(1),
- callee(cxxMethodDecl(
- hasName("find"),
- hasParameter(0, hasType(hasUnqualifiedDesugaredType(
- type().bind("parameterType")))),
- ofClass(cxxRecordDecl(HasContainsMatchingParamType)))))
+ // Either one argument, or assume the second argument is the position to
+ // start searching from.
+ cxxMemberCallExpr(anyOf(argumentCountIs(1),
+ cxxMemberCallExpr(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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index bd757851abe02..32b5bd02d2023 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -263,6 +263,10 @@ 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``.
+
- 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``,
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 1cfbf4c511c58..9f5b263f7f671 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
@@ -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)``
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
index 7e579e5319752..6cedda4202f14 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/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;
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 f345b3e7768a8..d7c1e15c29d40 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
@@ -1,4 +1,7 @@
-// RUN: %check_clang_tidy -std=c++11-or-later %s readability-container-contains %t
+// RUN: %check_clang_tidy %s readability-container-contains %t -- \
+// RUN: -- -isystem %clang_tidy_headers
+
+#include <string>
// Some *very* simplified versions of `map` etc.
namespace std {
@@ -318,16 +321,18 @@ void testPrivateContains(CustomMapPrivateContains<int, int> &MyMap,
if (MyMap2.count(0)) {};
}
-struct MyString {};
-
struct WeirdNonMatchingContains {
unsigned count(char) const;
- bool contains(const MyString&) const;
+ bool contains(const std::string&) const;
};
-void testWeirdNonMatchingContains(WeirdNonMatchingContains &MyMap) {
- // No warning if there is no `contains` method with the right type.
- if (MyMap.count('a')) {};
+// False positives: when count/find and contains take different types,
+// the check will suggest an invalid code transformation. These cases
+// should not exist in real code or be rare enough.
+void f(WeirdNonMatchingContains 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')) {}
}
template <class T>
@@ -375,15 +380,6 @@ void testSubclassEntry(SmallPtrSet<X>& Set, Y* Entry) {
// 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]
@@ -458,3 +454,41 @@ void testOperandPermutations(std::map<int, int>& Map) {
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
// CHECK-FIXES: if (!Map.contains(0)) {};
}
+
+void testStringNpos(std::string Str1, std::string Str2, std::string_view StrView) {
+ if (Str1.find(Str2) == std::string::npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (!Str1.contains(Str2)) {};
+
+ if (Str1.find("test") == std::string::npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (!Str1.contains("test")) {};
+
+ if (Str1.find('c') != std::string::npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (Str1.contains('c')) {};
+
+ if (Str1.find(Str2, 0) != std::string::npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (Str1.contains(Str2)) {};
+
+ if (std::string::npos == Str1.find("test", 0)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (!Str1.contains("test")) {};
+
+ if (StrView.find("test") == std::string_view::npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (!StrView.contains("test")) {};
+
+ if (StrView.find('c') != std::string_view::npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (StrView.contains('c')) {};
+
+ if (StrView.find('c') != StrView.npos) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (StrView.contains('c')) {};
+
+ // These should not match.
+ if (Str1.find('c', 1) != Str1.npos) {};
+ if (Str1.find(StrView, 1) != Str1.npos) {};
+}
More information about the cfe-commits
mailing list