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

[via cfe-commits]

Author: Dmitry Polukhin
Date: 2024-07-18T09:55:36+01:00
New Revision: 1ce89899ad33a0d2976859d8d278dba4342cbb6b

URL: https://github.com/llvm/llvm-project/commit/1ce89899ad33a0d2976859d8d278dba4342cbb6b
DIFF: https://github.com/llvm/llvm-project/commit/1ce89899ad33a0d2976859d8d278dba4342cbb6b.diff

LOG: [clang-tidy] Fix false in unnecessary-value-param inside templates (#98488)

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

Added: 
    

Modified: 
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
    clang/lib/Analysis/ExprMutationAnalyzer.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 697b514ae1572..a23483e6df6d2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -450,7 +450,8 @@ Changes in existing checks
   <clang-tidy/checks/performance/unnecessary-value-param>` check
   detecting more cases for template functions including lambdas with ``auto``.
   E.g., ``std::sort(a.begin(), a.end(), [](auto x, auto y) { return a > b; });``
-  will be detected for expensive to copy types.
+  will be detected for expensive to copy types. Fixed false positives for
+  dependent call expressions.
 
 - Improved :doc:`readability-avoid-return-with-void-value
   <clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding

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 0dffaefa213a4..7c7ae43698929 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;
@@ -357,3 +382,12 @@ void fun() {
   ExpensiveToCopyType E;
   NegativeUsingConstructor S(E);
 }
+
+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..6d726ae44104e 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -404,25 +404,24 @@ 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))),
-      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