[clang-tidy] Support find for string-like classes in readability-container-contains (#157243)
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`.
Co-authored-by: dl8sd11 <gcchen@google.com>
GitOrigin-RevId: 5c21b1b6dcc808795d1596ea2e29f94314488026
diff --git a/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tidy/readability/ContainerContainsCheck.cpp
index 04c1aa2..850ef86 100644
--- a/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/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 =
+ // Either one argument, or assume the second argument is the position to
+ // start searching from.
cxxMemberCallExpr(
- argumentCountIs(1),
- callee(cxxMethodDecl(
- hasName("find"),
- hasParameter(0, hasType(hasUnqualifiedDesugaredType(
- type().bind("parameterType")))),
- ofClass(cxxRecordDecl(HasContainsMatchingParamType)))))
+ anyOf(argumentCountIs(1),
+ allOf(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 @@
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 @@
"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());
+ 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(
- 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::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/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index 6184b3b..c2598dd 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -312,6 +312,11 @@
<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``. Internal changes may cause new rare false positives
+ in non-standard containers.
+
- 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/docs/clang-tidy/checks/readability/container-contains.rst b/docs/clang-tidy/checks/readability/container-contains.rst
index 1cfbf4c..9f5b263 100644
--- a/docs/clang-tidy/checks/readability/container-contains.rst
+++ b/docs/clang-tidy/checks/readability/container-contains.rst
@@ -20,6 +20,7 @@
-------------------------------------- -------------------------------------
``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/test/clang-tidy/checkers/Inputs/Headers/string b/test/clang-tidy/checkers/Inputs/Headers/string
index 7e579e5..6cedda4 100644
--- a/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/test/clang-tidy/checkers/Inputs/Headers/string
@@ -50,12 +50,19 @@
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 @@
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/test/clang-tidy/checkers/readability/container-contains.cpp b/test/clang-tidy/checkers/readability/container-contains.cpp
index f345b3e..1d848ab 100644
--- a/test/clang-tidy/checkers/readability/container-contains.cpp
+++ b/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 {
@@ -255,9 +258,9 @@
};
void testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
- if (MyMap.count(0)) {};
+ 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)) {};
+ // 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]
@@ -267,9 +270,9 @@
struct MySubmap : public CustomMap<int, int> {};
void testSubclass(MySubmap& MyMap) {
- if (MyMap.count(0)) {};
+ 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)) {};
+ // CHECK-FIXES: if (MyMap.contains(0)) {}
}
using UsingMap = CustomMap<int, int>;
@@ -277,9 +280,9 @@
using UsingMap2 = MySubmap2;
void testUsing(UsingMap2& MyMap) {
- if (MyMap.count(0)) {};
+ 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)) {};
+ // CHECK-FIXES: if (MyMap.contains(0)) {}
}
template <class Key, class T>
@@ -295,8 +298,8 @@
void testContainsDeleted(CustomMapContainsDeleted<int, int> &MyMap,
SubmapContainsDeleted &MyMap2) {
// No warning if the `contains` method is deleted.
- if (MyMap.count(0)) {};
- if (MyMap2.count(0)) {};
+ if (MyMap.count(0)) {}
+ if (MyMap2.count(0)) {}
}
template <class Key, class T>
@@ -314,20 +317,22 @@
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)) {};
+ if (MyMap.count(0)) {}
+ 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>
@@ -355,15 +360,15 @@
SmallPtrSet<int> &MySet,
SmallPtrPtrSet<int> &MySet2,
SmallPtrPtrPtrSet<int> &MySet3) {
- if (MySet.count(**Ptr)) {};
+ 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-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-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)) {};
+ // CHECK-FIXES: if (MySet3.contains(Ptr)) {}
}
struct X {};
@@ -375,15 +380,6 @@
// 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]
@@ -402,18 +398,18 @@
void testConvertibleTypes() {
CustomSetConvertible<B> MyMap;
- if (MyMap.count(A())) {};
+ 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())) {};
+ // CHECK-FIXES: if (MyMap.contains(A())) {}
CustomSetConvertible<A> MyMap2;
- if (MyMap2.count(C())) {};
+ 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())) {};
+ // CHECK-FIXES: if (MyMap2.contains(C())) {}
- if (MyMap2.count(C()) != 0) {};
+ 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())) {};
+ // CHECK-FIXES: if (MyMap2.contains(C())) {}
}
template<class U>
@@ -427,34 +423,72 @@
void testBox() {
CustomBoxedSet<int> Set;
- if (Set.count(0)) {};
+ 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)) {};
+ // CHECK-FIXES: if (Set.contains(0)) {}
}
void testOperandPermutations(std::map<int, int>& Map) {
- if (Map.count(0) != 0) {};
+ if (Map.count(0) != 0) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (Map.contains(0)) {};
- if (0 != Map.count(0)) {};
+ // CHECK-FIXES: if (Map.contains(0)) {}
+ if (0 != Map.count(0)) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (Map.contains(0)) {};
- if (Map.count(0) == 0) {};
+ // CHECK-FIXES: if (Map.contains(0)) {}
+ if (Map.count(0) == 0) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (!Map.contains(0)) {};
- if (0 == Map.count(0)) {};
+ // CHECK-FIXES: if (!Map.contains(0)) {}
+ if (0 == Map.count(0)) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (!Map.contains(0)) {};
- if (Map.find(0) != Map.end()) {};
+ // CHECK-FIXES: if (!Map.contains(0)) {}
+ if (Map.find(0) != Map.end()) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (Map.contains(0)) {};
- if (Map.end() != Map.find(0)) {};
+ // CHECK-FIXES: if (Map.contains(0)) {}
+ if (Map.end() != Map.find(0)) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (Map.contains(0)) {};
- if (Map.find(0) == Map.end()) {};
+ // CHECK-FIXES: if (Map.contains(0)) {}
+ if (Map.find(0) == Map.end()) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (!Map.contains(0)) {};
- if (Map.end() == Map.find(0)) {};
+ // CHECK-FIXES: if (!Map.contains(0)) {}
+ if (Map.end() == Map.find(0)) {}
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
- // CHECK-FIXES: if (!Map.contains(0)) {};
+ // 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) {}
}
