[clang-tidy] add support to 3-arg find() in StringFindStartswith

[niko]

Summary

string::find() has a 3-argument version, with the third argument
specifying the 'length of sequence of characters to match' (for a const
char* 1st argument, typically used when it is not null-terminated).

This change adds support for this by wrapping the search string and the search string length in absl::string_view.

[njames93]

I'm not hugely familiar with the abseil library, but from what I can see absl::StartsWith takes absl::string_view as args. Therefore surely it makes sense to handle the 3rd arg for str::find by explicitly constructing the absl::string_view from the pointer and size.

[niko]

In D76496#1935127, @njames93 wrote:

I'm not hugely familiar with the abseil library, but from what I can see absl::StartsWith takes absl::string_view as args. Therefore surely it makes sense to handle the 3rd arg for str::find by explicitly constructing the absl::string_view from the pointer and size.

I am mainly worried that the 3-arg find is rarely used, and using absl::string_view requires another option to be specified for the header file. If you think that's still a good tradeoff i'll change it to construct a string_view.

[njames93]

In D76496#1947059, @niko wrote:

In D76496#1935127, @njames93 wrote:

I'm not hugely familiar with the abseil library, but from what I can see absl::StartsWith takes absl::string_view as args. Therefore surely it makes sense to handle the 3rd arg for str::find by explicitly constructing the absl::string_view from the pointer and size.

I am mainly worried that the 3-arg find is rarely used, and using absl::string_view requires another option to be specified for the header file. If you think that's still a good tradeoff i'll change it to construct a string_view.

The absl/strings/match header includes the absl/strings/string_view header so you wouldn't need to include another header file. Well that's as long as the user hasn't done something funky with header files, but arguably the shouldn't be using Fix-Its if they have done something like that.

[niko]

In D76496#1947127, @njames93 wrote:

In D76496#1947059, @niko wrote:

In D76496#1935127, @njames93 wrote:

I'm not hugely familiar with the abseil library, but from what I can see absl::StartsWith takes absl::string_view as args. Therefore surely it makes sense to handle the 3rd arg for str::find by explicitly constructing the absl::string_view from the pointer and size.

I am mainly worried that the 3-arg find is rarely used, and using absl::string_view requires another option to be specified for the header file. If you think that's still a good tradeoff i'll change it to construct a string_view.

The absl/strings/match header includes the absl/strings/string_view header so you wouldn't need to include another header file. Well that's as long as the user hasn't done something funky with header files, but arguably the shouldn't be using Fix-Its if they have done something like that.

Correct me if I'm wrong, but that seems to be in violation of IWYU? Maybe I'm misreading this, or is the idea that higher-lever tooling (e.g. IWYU fixer) is supposed to address that?

[njames93]

In D76496#1949851, @niko wrote:

Correct me if I'm wrong, but that seems to be in violation of IWYU? Maybe I'm misreading this, or is the idea that higher-lever tooling (e.g. IWYU fixer) is supposed to address that?

It is but it sort of gets a pass as the absl/strings/match.h has to include it.
Similar to how if you #include <vector>, you wouldn't then #include <initializer_list> so you could construct a std::vector using a std::initializer_list.

[niko]

[clang-tidy] add 3-arg support to StringFindStartswith via string_view

[niko]

In D76496#1976713, @njames93 wrote:

In D76496#1949851, @niko wrote:

Correct me if I'm wrong, but that seems to be in violation of IWYU? Maybe I'm misreading this, or is the idea that higher-lever tooling (e.g. IWYU fixer) is supposed to address that?

It is but it sort of gets a pass as the absl/strings/match.h has to include it.
Similar to how if you #include <vector>, you wouldn't then #include <initializer_list> so you could construct a std::vector using a std::initializer_list.

Makes sense. Thanks!

[niko]

Rebasing to HEAD

clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp

void StringFindStartswithCheck::registerMatchers(MatchFinder *Finder) {
   auto ZeroLiteral = integerLiteral(equals(0));
   auto StringClassMatcher = cxxRecordDecl(hasAnyName(SmallVector<StringRef, 4>(
       StringLikeClasses.begin(), StringLikeClasses.end())));
   auto StringType = hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(StringClassMatcher)));
 
   auto StringFind = cxxMemberCallExpr(
       // .find()-call on a string...
-      callee(cxxMethodDecl(hasName("find"))),
-      on(hasType(StringType)),
+      callee(cxxMethodDecl(hasName("find"))), on(hasType(StringType)),
       // ... with some search expression ...
       hasArgument(0, expr().bind("needle")),
       // ... and either "0" as second argument or the default argument (also 0).
-      anyOf(hasArgument(1, ZeroLiteral), hasArgument(1, cxxDefaultArgExpr())));
+      anyOf(hasArgument(1, ZeroLiteral), hasArgument(1, cxxDefaultArgExpr())),
+      // ... and optionally a third argument for 'search string length'.
+      optionally(hasArgument(2, expr().bind("needle_length"))));
 
   Finder->addMatcher(
       // Match [=!]= with a zero on one side and a string.find on the other.
       binaryOperator(
           hasAnyOperatorName("==", "!="),
           hasEitherOperand(ignoringParenImpCasts(ZeroLiteral)),
           hasEitherOperand(ignoringParenImpCasts(StringFind.bind("findexpr"))))
           .bind("expr"),
       this);
 }
 
 void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) {
   const ASTContext &Context = *Result.Context;
   const SourceManager &Source = Context.getSourceManager();
 
   // Extract matching (sub)expressions
   const auto *ComparisonExpr = Result.Nodes.getNodeAs<BinaryOperator>("expr");
   assert(ComparisonExpr != nullptr);
   const auto *Needle = Result.Nodes.getNodeAs<Expr>("needle");
   assert(Needle != nullptr);
   const Expr *Haystack = Result.Nodes.getNodeAs<CXXMemberCallExpr>("findexpr")
                              ->getImplicitObjectArgument();
   assert(Haystack != nullptr);
+  const auto *NeedleLength = Result.Nodes.getNodeAs<Expr>("needle_length");
 
   if (ComparisonExpr->getBeginLoc().isMacroID())
     return;
 
   // Get the source code blocks (as characters) for both the string object
   // and the search expression
   const StringRef NeedleExprCode = Lexer::getSourceText(
       CharSourceRange::getTokenRange(Needle->getSourceRange()), Source,
       Context.getLangOpts());
   const StringRef HaystackExprCode = Lexer::getSourceText(
       CharSourceRange::getTokenRange(Haystack->getSourceRange()), Source,
       Context.getLangOpts());
 
+  // If there is a third argument (search string length), create an
+  // absl::string_view(search string, search string length) and use that
+  // as the search string in StartsWith.
+  std::string NewNeedleStr;
+  if (NeedleLength != nullptr) {
+    const StringRef NeedleLengthCode = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(NeedleLength->getSourceRange()), Source,
+        Context.getLangOpts());
+    NewNeedleStr = std::string("absl::string_view(") +
+                        NeedleExprCode.str() + ", " + NeedleLengthCode.str() +
+                        ")";
+  } else {
+    NewNeedleStr = NeedleExprCode.str();
+  }
+
   // Create the StartsWith string, negating if comparison was "!=".
   bool Neg = ComparisonExpr->getOpcodeStr() == "!=";
   StringRef StartswithStr;
   if (Neg) {
     StartswithStr = "!absl::StartsWith";
   } else {
     StartswithStr = "absl::StartsWith";
   }
 
   // Create the warning message and a FixIt hint replacing the original expr.
   auto Diagnostic =
       diag(ComparisonExpr->getBeginLoc(),
            (StringRef("use ") + StartswithStr + " instead of find() " +
             ComparisonExpr->getOpcodeStr() + " 0")
                .str());
 
   Diagnostic << FixItHint::CreateReplacement(
       ComparisonExpr->getSourceRange(),
-      (StartswithStr + "(" + HaystackExprCode + ", " + NeedleExprCode + ")")
+      (StartswithStr + "(" + HaystackExprCode + ", " + NewNeedleStr + ")")
           .str());
 
   // Create a preprocessor #include FixIt hint (CreateIncludeInsertion checks
   // whether this already exists).
   Diagnostic << IncludeInserter->CreateIncludeInsertion(
       Source.getFileID(ComparisonExpr->getBeginLoc()), AbseilStringsMatchHeader,
       false);
 }

clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp

 // RUN: %check_clang_tidy %s abseil-string-find-startswith %t -- \
 // RUN:   -config="{CheckOptions: [{key: 'abseil-string-find-startswith.StringLikeClasses', value: '::std::basic_string;::basic_string'}]}"
 
 namespace std {
 template <typename T> class allocator {};
 template <typename T> class char_traits {};
 template <typename C, typename T = std::char_traits<C>,
           typename A = std::allocator<C>>
 struct basic_string {
   basic_string();
   basic_string(const basic_string &);
   basic_string(const C *, const A &a = A());
   ~basic_string();
   int find(basic_string<C> s, int pos = 0);
   int find(const char *s, int pos = 0);
+  int find(const char *s, int pos, int n);
 };
 typedef basic_string<char> string;
 typedef basic_string<wchar_t> wstring;
 
 struct cxx_string {
   int find(const char *s, int pos = 0);
 };
 } // namespace std
void tests(std::string s, global_string s2) {
   s.find("a") == 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
   // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}}
 
   s.find(s) == 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
 
+  s.find(s, 0) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
   s.find("aaa") != 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
 
   s.find(foo(foo(bar()))) != 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, foo(foo(bar())));{{$}}
 
   if (s.find("....") == 0) { /* do something */ }
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}if (absl::StartsWith(s, "....")) { /* do something */ }{{$}}
 
   0 != s.find("a");
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "a");{{$}}
 
   s2.find("a") == 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s2, "a");{{$}}
 
+  s.find("a", 0, 1) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, absl::string_view("a", 1));{{$}}
+
   // expressions that don't trigger the check are here.
   A_MACRO(s.find("a"), 0);
   s.find("a", 1) == 0;
   s.find("a", 1) == 1;
   s.find("a") == 1;
+  s.find("a", 1, 1) == 0;
 }