[clang-tidy][modernize-use-starts-ends-with] Add support for two ends_with patterns (#110448)

nicovank · web-flow · commit f1367a473d96 · 2024-10-11T21:00:38.000-04:00
Add support for the following two patterns:
```
haystack.compare(haystack.length() - needle.length(), needle.length(), needle) == 0;
haystack.rfind(needle) == (haystack.size() - needle.size());
```
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "UseStartsEndsWithCheck.h"
 
+#include "../utils/ASTUtils.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/Lex/Lexer.h"
 
@@ -16,13 +17,71 @@
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::modernize {
+struct NotLengthExprForStringNode {
+  NotLengthExprForStringNode(std::string ID, DynTypedNode Node,
+                             ASTContext *Context)
+      : ID(std::move(ID)), Node(std::move(Node)), Context(Context) {}
+  bool operator()(const internal::BoundNodesMap &Nodes) const {
+    // Match a string literal and an integer size or strlen() call.
+    if (const auto *StringLiteralNode = Nodes.getNodeAs<StringLiteral>(ID)) {
+      if (const auto *IntegerLiteralSizeNode = Node.get<IntegerLiteral>()) {
+        return StringLiteralNode->getLength() !=
+               IntegerLiteralSizeNode->getValue().getZExtValue();
+      }
+
+      if (const auto *StrlenNode = Node.get<CallExpr>()) {
+        if (StrlenNode->getDirectCallee()->getName() != "strlen" ||
+            StrlenNode->getNumArgs() != 1) {
+          return true;
+        }
+
+        if (const auto *StrlenArgNode = dyn_cast<StringLiteral>(
+                StrlenNode->getArg(0)->IgnoreParenImpCasts())) {
+          return StrlenArgNode->getLength() != StringLiteralNode->getLength();
+        }
+      }
+    }
+
+    // Match a string variable and a call to length() or size().
+    if (const auto *ExprNode = Nodes.getNodeAs<Expr>(ID)) {
+      if (const auto *MemberCallNode = Node.get<CXXMemberCallExpr>()) {
+        const CXXMethodDecl *MethodDeclNode = MemberCallNode->getMethodDecl();
+        const StringRef Name = MethodDeclNode->getName();
+        if (!MethodDeclNode->isConst() || MethodDeclNode->getNumParams() != 0 ||
+            (Name != "size" && Name != "length")) {
+          return true;
+        }
+
+        if (const auto *OnNode =
+                dyn_cast<Expr>(MemberCallNode->getImplicitObjectArgument())) {
+          return !utils::areStatementsIdentical(OnNode->IgnoreParenImpCasts(),
+                                                ExprNode->IgnoreParenImpCasts(),
+                                                *Context);
+        }
+      }
+    }
+
+    return true;
+  }
+
+private:
+  std::string ID;
+  DynTypedNode Node;
+  ASTContext *Context;
+};
+
+AST_MATCHER_P(Expr, lengthExprForStringNode, std::string, ID) {
+  return Builder->removeBindings(NotLengthExprForStringNode(
+      ID, DynTypedNode::create(Node), &(Finder->getASTContext())));
+}
 
 UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context) {}
 
 void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
   const auto ZeroLiteral = integerLiteral(equals(0));
+
   const auto HasStartsWithMethodWithName = [](const std::string &Name) {
     return hasMethod(
         cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
@@ -32,119 +91,104 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
       anyOf(HasStartsWithMethodWithName("starts_with"),
             HasStartsWithMethodWithName("startsWith"),
             HasStartsWithMethodWithName("startswith"));
-  const auto ClassWithStartsWithFunction = cxxRecordDecl(anyOf(
-      HasStartsWithMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
-                               cxxRecordDecl(HasStartsWithMethod)))))));
+  const auto OnClassWithStartsWithFunction =
+      on(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+          anyOf(HasStartsWithMethod,
+                hasAnyBase(hasType(hasCanonicalType(
+                    hasDeclaration(cxxRecordDecl(HasStartsWithMethod)))))))))));
 
+  const auto HasEndsWithMethodWithName = [](const std::string &Name) {
+    return hasMethod(
+        cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
+            .bind("ends_with_fun"));
+  };
+  const auto HasEndsWithMethod = anyOf(HasEndsWithMethodWithName("ends_with"),
+                                       HasEndsWithMethodWithName("endsWith"),
+                                       HasEndsWithMethodWithName("endswith"));
+  const auto OnClassWithEndsWithFunction =
+      on(expr(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+                  anyOf(HasEndsWithMethod,
+                        hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
+                            cxxRecordDecl(HasEndsWithMethod)))))))))))
+             .bind("haystack"));
+
+  // Case 1: X.find(Y) [!=]= 0 -> starts_with.
   const auto FindExpr = cxxMemberCallExpr(
-      // A method call with no second argument or the second argument is zero...
       anyOf(argumentCountIs(1), hasArgument(1, ZeroLiteral)),
-      // ... named find...
       callee(cxxMethodDecl(hasName("find")).bind("find_fun")),
-      // ... on a class with a starts_with function.
-      on(hasType(
-          hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
-      // Bind search expression.
-      hasArgument(0, expr().bind("search_expr")));
+      OnClassWithStartsWithFunction, hasArgument(0, expr().bind("needle")));
 
+  // Case 2: X.rfind(Y, 0) [!=]= 0 -> starts_with.
   const auto RFindExpr = cxxMemberCallExpr(
-      // A method call with a second argument of zero...
       hasArgument(1, ZeroLiteral),
-      // ... named rfind...
       callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
-      // ... on a class with a starts_with function.
-      on(hasType(
-          hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
-      // Bind search expression.
-      hasArgument(0, expr().bind("search_expr")));
-
-  // Match a string literal and an integer or strlen() call matching the length.
-  const auto HasStringLiteralAndLengthArgs = [](const auto StringArgIndex,
-                                                const auto LengthArgIndex) {
-    return allOf(
-        hasArgument(StringArgIndex, stringLiteral().bind("string_literal_arg")),
-        hasArgument(LengthArgIndex,
-                    anyOf(integerLiteral().bind("integer_literal_size_arg"),
-                          callExpr(callee(functionDecl(parameterCountIs(1),
-                                                       hasName("strlen"))),
-                                   hasArgument(0, stringLiteral().bind(
-                                                      "strlen_arg"))))));
-  };
-
-  // Match a string variable and a call to length() or size().
-  const auto HasStringVariableAndSizeCallArgs = [](const auto StringArgIndex,
-                                                   const auto LengthArgIndex) {
-    return allOf(
-        hasArgument(StringArgIndex, declRefExpr(hasDeclaration(
-                                        decl().bind("string_var_decl")))),
-        hasArgument(LengthArgIndex,
-                    cxxMemberCallExpr(
-                        callee(cxxMethodDecl(isConst(), parameterCountIs(0),
-                                             hasAnyName("size", "length"))),
-                        on(declRefExpr(
-                            to(decl(equalsBoundNode("string_var_decl"))))))));
-  };
-
-  // Match either one of the two cases above.
-  const auto HasStringAndLengthArgs =
-      [HasStringLiteralAndLengthArgs, HasStringVariableAndSizeCallArgs](
-          const auto StringArgIndex, const auto LengthArgIndex) {
-        return anyOf(
-            HasStringLiteralAndLengthArgs(StringArgIndex, LengthArgIndex),
-            HasStringVariableAndSizeCallArgs(StringArgIndex, LengthArgIndex));
-      };
+      OnClassWithStartsWithFunction, hasArgument(0, expr().bind("needle")));
 
+  // Case 3: X.compare(0, LEN(Y), Y) [!=]= 0 -> starts_with.
   const auto CompareExpr = cxxMemberCallExpr(
-      // A method call with three arguments...
-      argumentCountIs(3),
-      // ... where the first argument is zero...
-      hasArgument(0, ZeroLiteral),
-      // ... named compare...
+      argumentCountIs(3), hasArgument(0, ZeroLiteral),
       callee(cxxMethodDecl(hasName("compare")).bind("find_fun")),
-      // ... on a class with a starts_with function...
-      on(hasType(
-          hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
-      // ... where the third argument is some string and the second a length.
-      HasStringAndLengthArgs(2, 1),
-      // Bind search expression.
-      hasArgument(2, expr().bind("search_expr")));
+      OnClassWithStartsWithFunction, hasArgument(2, expr().bind("needle")),
+      hasArgument(1, lengthExprForStringNode("needle")));
 
+  // Case 4: X.compare(LEN(X) - LEN(Y), LEN(Y), Y) [!=]= 0 -> ends_with.
+  const auto CompareEndsWithExpr = cxxMemberCallExpr(
+      argumentCountIs(3),
+      callee(cxxMethodDecl(hasName("compare")).bind("find_fun")),
+      OnClassWithEndsWithFunction, hasArgument(2, expr().bind("needle")),
+      hasArgument(1, lengthExprForStringNode("needle")),
+      hasArgument(0,
+                  binaryOperator(hasOperatorName("-"),
+                                 hasLHS(lengthExprForStringNode("haystack")),
+                                 hasRHS(lengthExprForStringNode("needle")))));
+
+  // All cases comparing to 0.
   Finder->addMatcher(
-      // Match [=!]= with a zero on one side and (r?)find|compare on the other.
       binaryOperator(
           hasAnyOperatorName("==", "!="),
-          hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr))
+          hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr,
+                                              CompareEndsWithExpr))
                           .bind("find_expr"),
                       ZeroLiteral))
           .bind("expr"),
       this);
+
+  // Case 5: X.rfind(Y) [!=]= LEN(X) - LEN(Y) -> ends_with.
+  Finder->addMatcher(
+      binaryOperator(
+          hasAnyOperatorName("==", "!="),
+          hasOperands(
+              cxxMemberCallExpr(
+                  anyOf(
+                      argumentCountIs(1),
+                      allOf(argumentCountIs(2),
+                            hasArgument(
+                                1,
+                                anyOf(declRefExpr(to(varDecl(hasName("npos")))),
+                                      memberExpr(member(hasName("npos"))))))),
+                  callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
+                  OnClassWithEndsWithFunction,
+                  hasArgument(0, expr().bind("needle")))
+                  .bind("find_expr"),
+              binaryOperator(hasOperatorName("-"),
+                             hasLHS(lengthExprForStringNode("haystack")),
+                             hasRHS(lengthExprForStringNode("needle")))))
+          .bind("expr"),
+      this);
 }
 
 void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *ComparisonExpr = Result.Nodes.getNodeAs<BinaryOperator>("expr");
   const auto *FindExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("find_expr");
   const auto *FindFun = Result.Nodes.getNodeAs<CXXMethodDecl>("find_fun");
-  const auto *SearchExpr = Result.Nodes.getNodeAs<Expr>("search_expr");
+  const auto *SearchExpr = Result.Nodes.getNodeAs<Expr>("needle");
   const auto *StartsWithFunction =
       Result.Nodes.getNodeAs<CXXMethodDecl>("starts_with_fun");
-
-  const auto *StringLiteralArg =
-      Result.Nodes.getNodeAs<StringLiteral>("string_literal_arg");
-  const auto *IntegerLiteralSizeArg =
-      Result.Nodes.getNodeAs<IntegerLiteral>("integer_literal_size_arg");
-  const auto *StrlenArg = Result.Nodes.getNodeAs<StringLiteral>("strlen_arg");
-
-  // Filter out compare cases where the length does not match string literal.
-  if (StringLiteralArg && IntegerLiteralSizeArg &&
-      StringLiteralArg->getLength() !=
-          IntegerLiteralSizeArg->getValue().getZExtValue()) {
-    return;
-  }
-
-  if (StringLiteralArg && StrlenArg &&
-      StringLiteralArg->getLength() != StrlenArg->getLength()) {
-    return;
-  }
+  const auto *EndsWithFunction =
+      Result.Nodes.getNodeAs<CXXMethodDecl>("ends_with_fun");
+  assert(bool(StartsWithFunction) != bool(EndsWithFunction));
+  const CXXMethodDecl *ReplacementFunction =
+      StartsWithFunction ? StartsWithFunction : EndsWithFunction;
 
   if (ComparisonExpr->getBeginLoc().isMacroID()) {
     return;
@@ -154,26 +198,26 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
 
   auto Diagnostic =
       diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0")
-      << StartsWithFunction->getName() << FindFun->getName() << Neg;
+      << ReplacementFunction->getName() << FindFun->getName() << Neg;
 
-  // Remove possible arguments after search expression and ' [!=]= 0' suffix.
+  // Remove possible arguments after search expression and ' [!=]= .+' suffix.
   Diagnostic << FixItHint::CreateReplacement(
       CharSourceRange::getTokenRange(
           Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
                                      *Result.SourceManager, getLangOpts()),
           ComparisonExpr->getEndLoc()),
       ")");
 
-  // Remove possible '0 [!=]= ' prefix.
+  // Remove possible '.+ [!=]= ' prefix.
   Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
       ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc()));
 
-  // Replace method name by 'starts_with'.
+  // Replace method name by '(starts|ends)_with'.
   // Remove possible arguments before search expression.
   Diagnostic << FixItHint::CreateReplacement(
       CharSourceRange::getCharRange(FindExpr->getExprLoc(),
                                     SearchExpr->getBeginLoc()),
-      (StartsWithFunction->getName() + "(").str());
+      (ReplacementFunction->getName() + "(").str());
 
   // Add possible negation '!'.
   if (Neg) {
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
@@ -14,7 +14,7 @@
 namespace clang::tidy::modernize {
 
 /// Checks for common roundabout ways to express ``starts_with`` and
-/// ``ends_with`` and suggests replacing with ``starts_with`` when the method is
+/// ``ends_with`` and suggests replacing with the simpler method when it is
 /// available. Notably, this will work with ``std::string`` and
 /// ``std::string_view``.
 ///
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -202,6 +202,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-nullptr>` check to also recognize
   ``NULL``/``__null`` (but not ``0``) when used with a templated type.
 
+- Improved :doc:`modernize-use-starts-ends-with
+  <clang-tidy/checks/modernize/use-starts-ends-with>` check to handle two cases
+  that can be replaced with ``ends_with``
+
 - Improved :doc:`modernize-use-std-format
   <clang-tidy/checks/modernize/use-std-format>` check to support replacing
   member function calls too and to only expand macros starting with ``PRI``
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
@@ -4,15 +4,21 @@ modernize-use-starts-ends-with
 ==============================
 
 Checks for common roundabout ways to express ``starts_with`` and ``ends_with``
-and suggests replacing with ``starts_with`` when the method is available.
-Notably, this will work with ``std::string`` and ``std::string_view``.
+and suggests replacing with the simpler method when it is available. Notably, 
+this will work with ``std::string`` and ``std::string_view``.
 
 .. code-block:: c++
 
   std::string s = "...";
   if (s.find("prefix") == 0) { /* do something */ }
   if (s.rfind("prefix", 0) == 0) { /* do something */ }
   if (s.compare(0, strlen("prefix"), "prefix") == 0) { /* do something */ }
+  if (s.compare(s.size() - strlen("suffix"), strlen("suffix"), "suffix") == 0) {
+    /* do something */
+  }
+  if (s.rfind("suffix") == (s.length() - 6)) {
+    /* do something */
+  }
 
 becomes
 
@@ -22,3 +28,5 @@ becomes
   if (s.starts_with("prefix")) { /* do something */ }
   if (s.starts_with("prefix")) { /* do something */ }
   if (s.starts_with("prefix")) { /* do something */ }
+  if (s.ends_with("suffix")) { /* do something */ }
+  if (s.ends_with("suffix")) { /* do something */ }
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -64,6 +64,10 @@ struct basic_string {
   constexpr bool starts_with(C ch) const noexcept;
   constexpr bool starts_with(const C* s) const;
 
+  constexpr bool ends_with(std::basic_string_view<C, T> sv) const noexcept;
+  constexpr bool ends_with(C ch) const noexcept;
+  constexpr bool ends_with(const C* s) const;
+
   _Type& operator[](size_type);
   const _Type& operator[](size_type) const;
 
@@ -108,6 +112,10 @@ struct basic_string_view {
   constexpr bool starts_with(C ch) const noexcept;
   constexpr bool starts_with(const C* s) const;
 
+  constexpr bool ends_with(basic_string_view sv) const noexcept;
+  constexpr bool ends_with(C ch) const noexcept;
+  constexpr bool ends_with(const C* s) const;
+
   constexpr int compare(basic_string_view sv) const noexcept;
 
   static constexpr size_t npos = -1;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
