[clang-tidy][modernize-use-starts-ends-with] Fix operator rewriting false negative

[nicovank]

In C++20, operator!= can be rewritten by negating operator==. This is the case for std::string, where operator!= is not provided hence relying on this rewriting.

Cover this case by matching binaryOperation and adding one case to isNegativeComparison.

This is only relevant in the newly added substr case, previous cases used a simple BinaryOperator. Release notes already mention adding this new case so I don't think further comments there are necessary.

Testing on non-mock:

> cat tmp.cpp
#include <string>
void f(std::string u, std::string v) { u.substr(0, v.size()) != v; }

# Before change, no warning.
> ./build/bin/clang-tidy -checks="-*,modernize-use-starts-ends-with" tmp.cpp -- -std=c++20

# After change.
> ./build/bin/clang-tidy -checks="-*,modernize-use-starts-ends-with" tmp.cpp -- -std=c++20
tmp.cpp:2:42: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
    2 | void f(std::string u, std::string v) { u.substr(0, v.size()) != v; }
      |                                          ^~~~~~ ~~~~~~~~~~~~~~~~~
      |                                        ! starts_with v)

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Nicolas van Kempen (nicovank)

Changes

In C++20, operator!= can be rewritten by negating operator==. This is the case for std::string, where operator!= is not provided hence relying on this rewriting.

Cover this case by matching binaryOperation and adding one case to isNegativeComparison.

This is only relevant in the newly added substr case, previous cases used a simple BinaryOperator. Release notes already mention adding this new case so I don't think further comments there are necessary.

Testing on non-mock:

> cat tmp.cpp
#include <string>
void f(std::string u, std::string v) { u.substr(0, v.size()) != v; }

# Before change, no warning.
> ./build/bin/clang-tidy -checks="-*,modernize-use-starts-ends-with" tmp.cpp -- -std=c++20

# After change.
> ./build/bin/clang-tidy -checks="-*,modernize-use-starts-ends-with" tmp.cpp -- -std=c++20
tmp.cpp:2:42: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
    2 | void f(std::string u, std::string v) { u.substr(0, v.size()) != v; }
      |                                          ^~~~~~ ~~~~~~~~~~~~~~~~~
      |                                        ! starts_with v)

---

Full diff: https://github.com/llvm/llvm-project/pull/117837.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp (+7-3)
• (modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string (+6-4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp (+10)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 234f6790a7ed9b..ab72c6796bb4cd 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -20,12 +20,16 @@ using namespace clang::ast_matchers;
 namespace clang::tidy::modernize {
 
 static bool isNegativeComparison(const Expr *ComparisonExpr) {
-  if (const auto *BO = llvm::dyn_cast<BinaryOperator>(ComparisonExpr))
-    return BO->getOpcode() == BO_NE;
+  if (const auto *Op = llvm::dyn_cast<BinaryOperator>(ComparisonExpr))
+    return Op->getOpcode() == BO_NE;
 
   if (const auto *Op = llvm::dyn_cast<CXXOperatorCallExpr>(ComparisonExpr))
     return Op->getOperator() == OO_ExclaimEqual;
 
+  if (const auto *Op =
+          llvm::dyn_cast<CXXRewrittenBinaryOperator>(ComparisonExpr))
+    return Op->getOperator() == BO_NE;
+
   return false;
 }
 
@@ -185,7 +189,7 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
 
   // Case 6: X.substr(0, LEN(Y)) [!=]= Y -> starts_with.
   Finder->addMatcher(
-      cxxOperatorCallExpr(
+      binaryOperation(
           hasAnyOperatorName("==", "!="),
           hasOperands(
               expr().bind("needle"),
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 51f68174169067..7e579e5319752a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -136,10 +136,6 @@ bool operator==(const std::string&, const std::string&);
 bool operator==(const std::string&, const char*);
 bool operator==(const char*, const std::string&);
 
-bool operator!=(const std::string&, const std::string&);
-bool operator!=(const std::string&, const char*);
-bool operator!=(const char*, const std::string&);
-
 bool operator==(const std::wstring&, const std::wstring&);
 bool operator==(const std::wstring&, const wchar_t*);
 bool operator==(const wchar_t*, const std::wstring&);
@@ -148,9 +144,15 @@ bool operator==(const std::string_view&, const std::string_view&);
 bool operator==(const std::string_view&, const char*);
 bool operator==(const char*, const std::string_view&);
 
+#if __cplusplus < 202002L
+bool operator!=(const std::string&, const std::string&);
+bool operator!=(const std::string&, const char*);
+bool operator!=(const char*, const std::string&);
+
 bool operator!=(const std::string_view&, const std::string_view&);
 bool operator!=(const std::string_view&, const char*);
 bool operator!=(const char*, const std::string_view&);
+#endif
 
 size_t strlen(const char* str);
 }
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
index 18a65f7f1cf260..8699ca03ba3313 100644
--- 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
@@ -320,3 +320,13 @@ void test_substr() {
 
     str.substr(0, strlen("hello123")) == "hello";
 }
+
+void test_operator_rewriting(std::string str, std::string prefix) {
+  str.substr(0, prefix.size()) == prefix;
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr
+  // CHECK-FIXES: str.starts_with(prefix);
+
+  str.substr(0, prefix.size()) != prefix;
+  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr
+  // CHECK-FIXES: !str.starts_with(prefix);
+}

[5chmidti]

LGTM