[clang-tidy] Fix wrong FixIt in performance-move-const-arg

There are incorrect Fixit and missing warnings:
case :
A trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters. Removing std::move will cause compilation errors.
```
void showInt(int&&) {}
void testInt() {
    int a = 10;
    // expect: warning + nofix
    showInt(std::move(a));  // showInt(a) <--- wrong fix
}

struct Tmp {};
void showTmp(Tmp&&) {}
void testTmp() {
    Tmp t;
    // expect: warning + nofix
    showTmp(std::move(t));  // showTmp(t) <--- wrong fix
}
```

Reviewed By: aaron.ballman, Quuxplusone

Differential Revision: https://reviews.llvm.org/D107450

Author: Sockke

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp

@@ -47,17 +47,62 @@ void MoveConstArgCheck::registerMatchers(MatchFinder *Finder) {
 
   Finder->addMatcher(MoveCallMatcher, this);
 
+  auto ConstTypeParmMatcher =
+      qualType(references(isConstQualified())).bind("invocation-parm-type");
+  auto RValueTypeParmMatcher =
+      qualType(rValueReferenceType()).bind("invocation-parm-type");
+  // Matches respective ParmVarDecl for a CallExpr or CXXConstructExpr.
+  auto ArgumentWithParamMatcher = forEachArgumentWithParam(
+      MoveCallMatcher, parmVarDecl(anyOf(hasType(ConstTypeParmMatcher),
+                                         hasType(RValueTypeParmMatcher)))
+                           .bind("invocation-parm"));
+  // Matches respective types of arguments for a CallExpr or CXXConstructExpr
+  // and it works on calls through function pointers as well.
+  auto ArgumentWithParamTypeMatcher = forEachArgumentWithParamType(
+      MoveCallMatcher, anyOf(ConstTypeParmMatcher, RValueTypeParmMatcher));
+
   Finder->addMatcher(
-      invocation(forEachArgumentWithParam(
-                     MoveCallMatcher,
-                     parmVarDecl(hasType(references(isConstQualified())))))
+      invocation(anyOf(ArgumentWithParamMatcher, ArgumentWithParamTypeMatcher))
           .bind("receiving-expr"),
       this);
 }
 
+bool IsRValueReferenceParam(const Expr *Invocation,
+                            const QualType &InvocationParmType,
+                            const Expr *Arg) {
+  if (Invocation && InvocationParmType->isRValueReferenceType() &&
+      Arg->isLValue()) {
+    if (!Invocation->getType()->isRecordType())
+      return true;
+    else {
+      if (const auto *ConstructCallExpr =
+              dyn_cast<CXXConstructExpr>(Invocation)) {
+        if (const auto *ConstructorDecl = ConstructCallExpr->getConstructor()) {
+          if (!ConstructorDecl->isCopyOrMoveConstructor() &&
+              !ConstructorDecl->isDefaultConstructor())
+            return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
   const auto *ReceivingExpr = Result.Nodes.getNodeAs<Expr>("receiving-expr");
+  const auto *InvocationParm =
+      Result.Nodes.getNodeAs<ParmVarDecl>("invocation-parm");
+  const auto *InvocationParmType =
+      Result.Nodes.getNodeAs<QualType>("invocation-parm-type");
+
+  // Skipping matchers which have been matched.
+  if (!ReceivingExpr && AlreadyCheckedMoves.contains(CallMove))
+    return;
+
+  if (ReceivingExpr)
+    AlreadyCheckedMoves.insert(CallMove);
+
   const Expr *Arg = CallMove->getArg(0);
   SourceManager &SM = Result.Context->getSourceManager();
 
@@ -90,20 +135,68 @@ void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
       return;
 
     bool IsVariable = isa<DeclRefExpr>(Arg);
+    // std::move shouldn't be removed when an lvalue wrapped by std::move is
+    // passed to the function with an rvalue reference parameter.
+    bool IsRVRefParam =
+        IsRValueReferenceParam(ReceivingExpr, *InvocationParmType, Arg);
     const auto *Var =
         IsVariable ? dyn_cast<DeclRefExpr>(Arg)->getDecl() : nullptr;
-    auto Diag = diag(FileMoveRange.getBegin(),
-                     "std::move of the %select{|const }0"
-                     "%select{expression|variable %4}1 "
-                     "%select{|of the trivially-copyable type %5 }2"
-                     "has no effect; remove std::move()"
-                     "%select{| or make the variable non-const}3")
-                << IsConstArg << IsVariable << IsTriviallyCopyable
-                << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
-                << Arg->getType();
 
-    replaceCallWithArg(CallMove, Diag, SM, getLangOpts());
+    {
+      auto Diag = diag(FileMoveRange.getBegin(),
+                       "std::move of the %select{|const }0"
+                       "%select{expression|variable %5}1 "
+                       "%select{|of the trivially-copyable type %6 }2"
+                       "has no effect%select{; remove std::move()|}3"
+                       "%select{| or make the variable non-const}4")
+                  << IsConstArg << IsVariable << IsTriviallyCopyable
+                  << IsRVRefParam
+                  << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
+                  << Arg->getType();
+      if (!IsRVRefParam)
+        replaceCallWithArg(CallMove, Diag, SM, getLangOpts());
+    }
+    if (IsRVRefParam) {
+      // Generate notes for an invocation with an rvalue reference parameter.
+      const auto *ReceivingCallExpr = dyn_cast<CallExpr>(ReceivingExpr);
+      const auto *ReceivingConstructExpr =
+          dyn_cast<CXXConstructExpr>(ReceivingExpr);
+      // Skipping the invocation which is a template instantiation.
+      if ((!ReceivingCallExpr || !ReceivingCallExpr->getDirectCallee() ||
+           ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) &&
+          (!ReceivingConstructExpr ||
+           !ReceivingConstructExpr->getConstructor() ||
+           ReceivingConstructExpr->getConstructor()->isTemplateInstantiation()))
+        return;
+
+      const NamedDecl *FunctionName = nullptr;
+      FunctionName =
+          ReceivingCallExpr
+              ? ReceivingCallExpr->getDirectCallee()->getUnderlyingDecl()
+              : ReceivingConstructExpr->getConstructor()->getUnderlyingDecl();
+
+      QualType NoRefType = (*InvocationParmType)->getPointeeType();
+      PrintingPolicy PolicyWithSuppressedTag(getLangOpts());
+      PolicyWithSuppressedTag.SuppressTagKeyword = true;
+      PolicyWithSuppressedTag.SuppressUnwrittenScope = true;
+      std::string ExpectParmTypeName =
+          NoRefType.getAsString(PolicyWithSuppressedTag);
+      if (!NoRefType->isPointerType()) {
+        NoRefType.addConst();
+        ExpectParmTypeName =
+            NoRefType.getAsString(PolicyWithSuppressedTag) + " &";
+      }
+
+      diag(InvocationParm->getLocation(),
+           "consider changing the %ordinal0 parameter of %1 from %2 to '%3'",
+           DiagnosticIDs::Note)
+          << (InvocationParm->getFunctionScopeIndex() + 1) << FunctionName
+          << *InvocationParmType << ExpectParmTypeName;
+    }
   } else if (ReceivingExpr) {
+    if ((*InvocationParmType)->isRValueReferenceType())
+      return;
+
     auto Diag = diag(FileMoveRange.getBegin(),
                      "passing result of std::move() as a const reference "
                      "argument; no move will actually happen");

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h

@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
@@ -36,6 +37,7 @@ class MoveConstArgCheck : public ClangTidyCheck {
 
 private:
   const bool CheckTriviallyCopyableMove;
+  llvm::DenseSet<const CallExpr *> AlreadyCheckedMoves;
 };
 
 } // namespace performance

clang-tools-extra/docs/ReleaseNotes.rst

@@ -175,6 +175,9 @@ Changes in existing checks
   option to control whether to warn on narrowing integer to floating-point
   conversions.
 
+- Improved :doc:`performance-move-const-arg` check.
+
+  Removed a wrong FixIt for trivially copyable objects wrapped by ``std::move()`` and passed to an rvalue reference parameter. Removal of ``std::move()`` would break the code.
 
 Removed checks
 ^^^^^^^^^^^^^^

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp

@@ -246,3 +246,97 @@ void lambda2() {
   };
   f(MoveSemantics());
 }
+
+void showInt(int &&v);
+void showInt(int v1, int &&v2);
+void showPointer(const char *&&s);
+void showPointer2(const char *const &&s);
+void showTriviallyCopyable(TriviallyCopyable &&obj);
+void showTriviallyCopyablePointer(const TriviallyCopyable *&&obj);
+void testFunctions() {
+  int a = 10;
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &'
+  showInt(int());
+  showInt(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &'
+  const char* s = "";
+  showPointer(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *'
+  showPointer2(std::move(s)); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  showTriviallyCopyable(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  showTriviallyCopyablePointer(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+}
+template <class T>
+void forwardToShowInt(T && t) {
+  showInt(static_cast<T &&>(t));
+}
+void testTemplate() {
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}
+
+struct Tmp {
+  Tmp();
+  Tmp(int &&a);
+  Tmp(int v1, int &&a);
+  Tmp(const char *&&s);
+  Tmp(TriviallyCopyable&& obj);
+  Tmp(const TriviallyCopyable *&&obj);
+  void showTmp(TriviallyCopyable&& t);
+  static void showTmpStatic(TriviallyCopyable&& t);
+};
+void testMethods() {
+  Tmp t;
+  int a = 10;
+  Tmp t1(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &'
+  Tmp t2(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &'
+  const char* s = "";
+  Tmp t3(std::move(s));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *'
+  TriviallyCopyable *obj = new TriviallyCopyable();
+  Tmp t4(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-21]]:27: note: consider changing the 1st parameter of 'Tmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  Tmp t5(std::move(obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-23]]:34: note: consider changing the 1st parameter of 'Tmp' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *'
+  t.showTmp(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-25]]:36: note: consider changing the 1st parameter of 'showTmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+  Tmp::showTmpStatic(std::move(*obj));
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-27]]:49: note: consider changing the 1st parameter of 'showTmpStatic' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &'
+}
+
+void showA(A &&v) {}
+void testA() {
+  A a;
+  showA(std::move(a));
+}
+
+void testFuncPointer() {
+  int a = 10;
+  void (*choice)(int, int &&);
+  choice = showInt;
+  choice(std::move(a), std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; remove std::move() [performance-move-const-arg]
+  // CHECK-FIXES: choice(a, std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-3]]:24: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+}