[clang-tidy] Improve `performance-move-const-arg` message when no move constructor is available

[AMS21]

Summary

We now display a simple note if the reason is that the used class does not
support move semantics.

This fixes llvm#62550

[AMS21]

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

345

I've tried to add

// CHECK-MESSAGES: [[@LINE+1]]:1: note: 'NonMoveConstructable' is not move constructible

here. But doing so actually fails the tests and I'm not quite sure why.

[PiotrZSL]

Overall looks ok,
Add some test with records being used via typedef.

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

117

make sure you use here canonical type, or it may not work correctly with typedefs, like typedefs to records.
same in line 120, in fact only on line 154 would be good to use original type.

209

getLocation ()

211

<< RecordDecl
should work, do not use getName, it could cause crash for unnamed records.

clang-tools-extra/docs/ReleaseNotes.rst

378

"check to warn when ..."

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

345

There are 2 ways to deal with this.
If you use CHECK-MESSAGES, then order of issues is checked, and that why for current notes you may see thing like @LINE-27, so you would need to put it into places when its emited.
Other way is to use CHECK-NOTES, then all warnings are checked out of order.
So simply I would put those CHECK-MESSAGES, after a warning in lines 256, 372, 384, 387

[AMS21]

Implement suggested changes
Add tests with typedefs

[PiotrZSL]

+- LGTM

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

210

You need t check RecordDecl here also.
like if (...; RecordDecl && !(RecordDecl->hasMoveConstructor() && RecordDecl->hasMoveAssignment()))

212

maybe this should be constructible/assignable depend on what we are missing (constructor, assign operator)...
Simply this may be strange to get warn about construct when we assign.
Still things like std::optional cold use move constructor instead of assign operator on assign.
So this wound need to be checked... It's fine if we won't be very detailed here

clang-tools-extra/docs/ReleaseNotes.rst

378

remove :

[AMS21]

Implemented suggested changes

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

345

Ah yes that makes total sense. Thanks for clarifying that :)

[PiotrZSL]

LGTM

clang-tools-extra/docs/ReleaseNotes.rst

378–379

NOTE: check to warn when no move constructor is available. -> check to warn when move special member functions are not available. or something similar, to not limit this only to construction

[AMS21]

Improve doc

[AMS21]

As always if there are no more problems, I would kindly ask for someone to push this on my behalf :)

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

void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
   // Skipping matchers which have been matched.
   if (!ReceivingExpr && AlreadyCheckedMoves.contains(CallMove))
     return;
 
   if (ReceivingExpr)
     AlreadyCheckedMoves.insert(CallMove);
 
   const Expr *Arg = CallMove->getArg(0);
+  const QualType ArgType = Arg->getType();
   SourceManager &SM = Result.Context->getSourceManager();
 
   CharSourceRange MoveRange =
       CharSourceRange::getCharRange(CallMove->getSourceRange());
   CharSourceRange FileMoveRange =
       Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
   if (!FileMoveRange.isValid())
     return;
 
-  bool IsConstArg = Arg->getType().isConstQualified();
-  bool IsTriviallyCopyable =
-      Arg->getType().isTriviallyCopyableType(*Result.Context);
+  bool IsConstArg = ArgType.isConstQualified();
+  bool IsTriviallyCopyable = ArgType.isTriviallyCopyableType(*Result.Context);
 
   if (IsConstArg || IsTriviallyCopyable) {
-    if (const CXXRecordDecl *R = Arg->getType()->getAsCXXRecordDecl()) {
+    if (const CXXRecordDecl *R = ArgType->getAsCXXRecordDecl()) {
       // According to [expr.prim.lambda]p3, "whether the closure type is
       // trivially copyable" property can be changed by the implementation of
       // the language, so we shouldn't rely on it when issuing diagnostics.
       if (R->isLambda())
         return;
       // Don't warn when the type is not copyable.
       for (const auto *Ctor : R->ctors()) {
         if (Ctor->isCopyConstructor() && Ctor->isDeleted())
const auto *Var =
                        "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();
+                  << ArgType;
       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);
if (IsRVRefParam) {
            DiagnosticIDs::Note)
           << (InvocationParm->getFunctionScopeIndex() + 1) << FunctionName
           << *InvocationParmType << ExpectParmTypeName;
     }
   } else if (ReceivingExpr && CheckMoveToConstRef) {
     if ((*InvocationParmType)->isRValueReferenceType())
       return;
 
+    {
       auto Diag = diag(FileMoveRange.getBegin(),
                        "passing result of std::move() as a const reference "
                        "argument; no move will actually happen");
 
       replaceCallWithArg(CallMove, Diag, SM, getLangOpts());
     }
+
+    if (const CXXRecordDecl *RecordDecl = ArgType->getAsCXXRecordDecl();
+        !RecordDecl->hasMoveConstructor() || !RecordDecl->hasMoveAssignment())
+      diag(RecordDecl->getSourceRange().getBegin(),
+           "'%0' is not move constructible", DiagnosticIDs::Note)
+          << RecordDecl->getName();
+  }
 }
 
 } // namespace clang::tidy::performance

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed false positive in :doc:`modernize-use-equals-default
   functions containing macros or preprocessor directives, and out-of-line special
   member functions in unions.
 
 - Improved :doc:`modernize-use-override
   <clang-tidy/checks/modernize/use-override>` check with new
   `IgnoreTemplateInstantiations` option to optionally ignore virtual function
   overrides that are part of template instantiations.
 
+- Improved :doc:`performance-move-const-arg
+  <clang-tidy/checks/performance/move-const-arg>`: warning when no move
+  constructor is available.
+
 - Improved :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>`: warn on ``const &&``
   constructors.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 

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

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]
 }
+
+namespace issue_62550 {
+
+struct NonMoveConstructable {
+  NonMoveConstructable();
+  NonMoveConstructable(const NonMoveConstructable&);
+  NonMoveConstructable& operator=(const NonMoveConstructable&);
+  NonMoveConstructable& operator=(NonMoveConstructable&&);
+};
+
+void testNonMoveConstructible() {
+  NonMoveConstructable t1;
+  NonMoveConstructable t2{std::move(t1)};
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
+}
+
+struct NonMoveAssignable {
+  NonMoveAssignable();
+  NonMoveAssignable(const NonMoveAssignable&);
+  NonMoveAssignable(NonMoveAssignable&&);
+
+  NonMoveAssignable& operator=(const NonMoveAssignable&);
+};
+
+void testNonMoveAssignable() {
+  NonMoveAssignable t1;
+  NonMoveAssignable t2;
+
+  t2 = std::move(t1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
+}
+
+struct NonMoveable {
+  NonMoveable();
+  NonMoveable(const NonMoveable&);
+  NonMoveable& operator=(const NonMoveable&);
+};
+
+void testNonMoveable() {
+  NonMoveable t1;
+  NonMoveable t2{std::move(t1)};
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
+
+  t1 = std::move(t2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
+}
+
+} // namespace issue_62550