[clang-tidy] Support find for string-like classes in readability-container-contains #157243
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-clang-tidy Author: Nicolas van Kempen (nicovank) ChangesFix #109327. Small parts taken from #110351. Removed the type checking between Ended up copying over the FixIt logic from 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:
Co-authored-by: dl8sd11 <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/157243.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index fb68c7d334b7f..6157a7121f1e5 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 auto 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 32e4dfb8aa329..82f9ed101b64e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -230,6 +230,10 @@ Changes in existing checks
<clang-tidy/checks/portability/template-virtual-member-function>` check to
avoid false positives on pure virtual member functions.
+- Fixed false negatives on string and string-like types in
+ :doc:`readability-container-contains
+ <clang-tidy/checks/readability/container-contains>`.
+
- 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..dbd1553d22cf2 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 -std=c++11-or-later %s readability-container-contains %t -- \
+// RUN: -- -isystem %clang_tidy_headers
+
+#include <string>
// Some *very* simplified versions of `map` etc.
namespace std {
@@ -318,17 +321,15 @@ 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) { MyMap.count('a'); }
template <class T>
struct SmallPtrSet {
@@ -375,15 +376,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 +450,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) {};
+}
|
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: Nicolas van Kempen (nicovank) ChangesFix #109327. Small parts taken from #110351. Removed the type checking between Ended up copying over the FixIt logic from 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:
Co-authored-by: dl8sd11 <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/157243.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index fb68c7d334b7f..6157a7121f1e5 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 auto 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 32e4dfb8aa329..82f9ed101b64e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -230,6 +230,10 @@ Changes in existing checks
<clang-tidy/checks/portability/template-virtual-member-function>` check to
avoid false positives on pure virtual member functions.
+- Fixed false negatives on string and string-like types in
+ :doc:`readability-container-contains
+ <clang-tidy/checks/readability/container-contains>`.
+
- 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..dbd1553d22cf2 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 -std=c++11-or-later %s readability-container-contains %t -- \
+// RUN: -- -isystem %clang_tidy_headers
+
+#include <string>
// Some *very* simplified versions of `map` etc.
namespace std {
@@ -318,17 +321,15 @@ 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) { MyMap.count('a'); }
template <class T>
struct SmallPtrSet {
@@ -375,15 +376,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 +450,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) {};
+}
|
nicovank
requested review from
5chmidti,
HerrCai0907,
PiotrZSL,
carlosgalvezp and
vbvictor
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
Outdated
Show resolved
Hide resolved
| <clang-tidy/checks/portability/template-virtual-member-function>` check to | ||
| avoid false positives on pure virtual member functions. | ||
|
|
||
| - Fixed false negatives on string and string-like types in |
Contributor
There was a problem hiding this comment.
Please follow format of other similar statements.
Contributor
Author
There was a problem hiding this comment.
Sure. Was there official guidelines for this? It wasn't the case up to the 17 release.
Since then this became the default (everyone copy-pasting other entries?). I feel like "Improved" is not always the best verb to express the change.
localspook
reviewed
Sep 8, 2025
clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
Outdated
Show resolved
Hide resolved
Contributor
|
It looks like the check could be extended quite easily to support |
localspook
reviewed
Sep 8, 2025
clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
Outdated
Show resolved
Hide resolved
Contributor
Author
Good idea, I'll make a small follow-up diff. Should just be a matter of adding the string to the matcher + a small test. |
vbvictor
reviewed
Sep 9, 2025
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
Outdated
Show resolved
Hide resolved
vbvictor
approved these changes
Sep 9, 2025
Contributor
Author
|
Test failures seem unrelated after rebase on main. I'll try again a little bit later. |
…ainer-contains Fix llvm#109327. Small parts taken from llvm#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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #109327. Small parts taken from #110351.
Removed the type checking between
containsandcount/findarguments for simplicity. Because of overloads, this type-checking is tricky. The same strategy is used inmodernize-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:
Co-authored-by: dl8sd11 [email protected]