[clang-tidy][readability-container-contains] Also match when comparing find() to string::npos.

[nicovank]

Opening this issue for tracking.

For std::string and std::string_view, an element is (not) found when comparing find() against std::string::npos or std::string_view::npos, not end(). This is not causing false positives since comparing a find() result to end() doesn't make sense to begin with in those classes, so in fact the check would probably be fixing bugs. (Is there a bugprone-string-find-end check?)

As of C++23 the following transformations and derivatives can be added to the check:

string.find(...) != std::string::npos; // -> string.contains(...)
string.find(...) != string.npos; // -> string.contains(...)

// Ideally, make this work for any npos, e.g. llvm::StringRef::npos.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Nicolas van Kempen (nicovank)

Opening this issue for tracking.

For std::string and std::string_view, an element is (not) found when comparing find() against std::string::npos or std::string_view::npos, not end(). This is not causing false positives since comparing a find() result to end() doesn't make sense to begin with in those classes, so in fact the check would probably be fixing bugs. (Is there a bugprone-string-find-end check?)

As of C++23 the following transformations and derivatives can be added to the check:

string.find(...) != std::string::npos; // -> string.contains(...)
string.find(...) != string.npos; // -> string.contains(...)

// Ideally, make this work for any npos, e.g. llvm::StringRef::npos.

[dl8sd11]

grep -r "std::string::npos" under test/clang-tidy only gets abseil-string-find-str-contains.

I would like to try adding it to readability-container-contains

[dl8sd11]

The interfaces of std::string::find (2 arguments) and std::map::find are quite different.

I think abseil-string-find-str-contains already implements the matcher of this case only the fixit hint is different. (absl::StrContains v.s. std::string::contain)

I'm not sure if duplicating the code is fine, or should I reuse the code between abseil-string-find-str-contains and readability-container-contains.

[nicovank]

In this case I think it's okay to duplicate code, but I don't think it's necessary. It makes no sense to compare other (non-string) containers to npos so a lot of logic can be saved.

In readability-container-contains, I would copy EndCall and make a simple StringNpos matcher to compare against npos, and in the matchers below just use anyOf(EndCall, StringNpos). You could probably copy abseil-string-find-str-contains's StringNpos matcher for this.

Assuming this feature gets in at some point, since it essentially replaces abseil-string-find-str-contains, the abseil check could probably be deprecated in ~7 years and eventually deleted.

[dl8sd11]

Thanks for your suggestion, I created a PoC for this.

The current matcher requires the container to have a method called contains with parameter type that equals the type of the first parameter of find. However, find accepts const std::string& while it only accepts basic_string_view.

Is it possible to check whether the parameter of contains is constructible from the find parameter?

[firewave]

FYI #79437 details some additional shortcomings.

[nicovank]

Is it possible to check whether the parameter of contains is constructible from the find parameter?

In my experience, working with types in Clang-Tidy is annoying. There is no easy way to find if types are equivalent (especially if pointer/references are involved) and no easy way to check if things are convertible. I'm personally not against removing the type check entirely for readability-container-contains since I think false-positives would be low to non-existent, but if it can keep working with it then that's of course better.