[clang-tools-extra] [clang-tidy][modernize-use-starts-ends-with] Fix operator rewriting false negative (PR #117837)

[Nicolas van Kempen via cfe-commits]

https://github.com/nicovank created https://github.com/llvm/llvm-project/pull/117837


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)
```


>From 65400949047a15424bcbb8bb2a1e1f515e886cf8 Mon Sep 17 00:00:00 2001
From: Nicolas van Kempen <nvankemp at gmail.com>
Date: Tue, 26 Nov 2024 22:26:20 -0500
Subject: [PATCH] [clang-tidy][modernize-use-starts-ends-with] Fix operator
 rewriting false negative

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)
```
---
 .../clang-tidy/modernize/UseStartsEndsWithCheck.cpp    | 10 +++++++---
 .../test/clang-tidy/checkers/Inputs/Headers/string     | 10 ++++++----
 .../checkers/modernize/use-starts-ends-with.cpp        | 10 ++++++++++
 3 files changed, 23 insertions(+), 7 deletions(-)

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);
+}