[clang] [clang-tools-extra] [clang-tidy] Fix false in unnecessary-value-param inside templates (PR #98488)

[Dmitry Polukhin via cfe-commits]

https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/98488

>From a05c4ca2c2e61653e7bd8d3a2f5faa6b86daa615 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Thu, 11 Jul 2024 07:01:49 -0700
Subject: [PATCH 1/2] [clang-tidy] Fix false in unnecessary-value-param inside
 templates

Summary:
If callExpr is type dependent, there is no way to analyze individual arguments
until template specialization. Before this diff only calls with dependent
callees were skipped so unnecessary-value-param was processing arguments that
had non-dependent type that gave false positives because the call was not fully
resolved till specialization. So now instead of checking type dependent callee,
the whole expression will be checked for type dependent.

Test Plan: check-clang-tools
---
 .../performance/unnecessary-value-param.cpp   | 34 +++++++++++++++++++
 clang/lib/Analysis/ExprMutationAnalyzer.cpp   |  9 +++--
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
index d578eedd94a39..b1696ad3c3d59 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
@@ -2,6 +2,31 @@
 
 // CHECK-FIXES: #include <utility>
 
+namespace std {
+template <typename>
+struct remove_reference;
+
+template <typename _Tp>
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
+}
+} // namespace std
+
 struct ExpensiveToCopyType {
   const ExpensiveToCopyType & constReference() const {
     return *this;
@@ -402,3 +427,12 @@ int templateSpecializationFunction(ExpensiveToCopyType E) {
   // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
   return 0;
 }
+
+struct B {
+  static void bar(ExpensiveMovableType a, ExpensiveMovableType b);
+};
+
+template <typename T>
+void NegativeCallWithDependentAndNondependentArgs(ExpensiveMovableType a, T b) {
+    B::bar(std::move(a), b);
+}
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 3b3782fa1db9a..70a1d7b52ffe9 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -404,15 +404,14 @@ ExprMutationAnalyzer::Analyzer::findDirectMutation(const Expr *Exp) {
             memberExpr(hasObjectExpression(canResolveToExpr(Exp)))),
       nonConstReferenceType());
   const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
-  const auto TypeDependentCallee =
-      callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),
-                        cxxDependentScopeMemberExpr(),
-                        hasType(templateTypeParmType()), isTypeDependent())));
 
   const auto AsNonConstRefArg = anyOf(
       callExpr(NonConstRefParam, NotInstantiated),
       cxxConstructExpr(NonConstRefParam, NotInstantiated),
-      callExpr(TypeDependentCallee, hasAnyArgument(canResolveToExpr(Exp))),
+      // If the call is type-dependent, we can't properly process any
+      // argument because required type conversions and implicit casts
+      // will be inserted only after specialization.
+      callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
       cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
       // Previous False Positive in the following Code:
       // `template <typename T> void f() { int i = 42; new Type<T>(i); }`

>From dbcdb0655318b7ba674cf00c7a9da9917a056102 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Thu, 11 Jul 2024 07:35:21 -0700
Subject: [PATCH 2/2] Apply git-clang-format

---
 clang/lib/Analysis/ExprMutationAnalyzer.cpp | 34 ++++++++++-----------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 70a1d7b52ffe9..6d726ae44104e 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -405,23 +405,23 @@ ExprMutationAnalyzer::Analyzer::findDirectMutation(const Expr *Exp) {
       nonConstReferenceType());
   const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
 
-  const auto AsNonConstRefArg = anyOf(
-      callExpr(NonConstRefParam, NotInstantiated),
-      cxxConstructExpr(NonConstRefParam, NotInstantiated),
-      // If the call is type-dependent, we can't properly process any
-      // argument because required type conversions and implicit casts
-      // will be inserted only after specialization.
-      callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
-      cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
-      // Previous False Positive in the following Code:
-      // `template <typename T> void f() { int i = 42; new Type<T>(i); }`
-      // Where the constructor of `Type` takes its argument as reference.
-      // The AST does not resolve in a `cxxConstructExpr` because it is
-      // type-dependent.
-      parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
-      // If the initializer is for a reference type, there is no cast for
-      // the variable. Values are cast to RValue first.
-      initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
+  const auto AsNonConstRefArg =
+      anyOf(callExpr(NonConstRefParam, NotInstantiated),
+            cxxConstructExpr(NonConstRefParam, NotInstantiated),
+            // If the call is type-dependent, we can't properly process any
+            // argument because required type conversions and implicit casts
+            // will be inserted only after specialization.
+            callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
+            cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
+            // Previous False Positive in the following Code:
+            // `template <typename T> void f() { int i = 42; new Type<T>(i); }`
+            // Where the constructor of `Type` takes its argument as reference.
+            // The AST does not resolve in a `cxxConstructExpr` because it is
+            // type-dependent.
+            parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
+            // If the initializer is for a reference type, there is no cast for
+            // the variable. Values are cast to RValue first.
+            initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
 
   // Captured by a lambda by reference.
   // If we're initializing a capture with 'Exp' directly then we're initializing