Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

**-Wreturn-std-move**

This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`.
Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is `stdext::inplace_function<Sig, N>` which implements conversion via a conversion operator rather than a converting constructor; see https://github.com/WG21-SG14/SG14/issues/125#issue-297201412
Another place where this has come up in the wild, but where the fix ended up being different, was

    try { ... } catch (ExceptionType ex) {
        throw ex;
    }

where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

**-Wreturn-std-move-in-c++11**

This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

**Discussion**

Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

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

Patch by Arthur O'Dwyer.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@329914 91177308-0d34-0410-b5e6-96231b3b80d8

1 files changed, 110 insertions, 17 deletions

diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index 8963f4c564..c3a627f1d3 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -2905,7 +2905,8 @@ bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
   if (VD->getKind() != Decl::Var &&
       !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
     return false;
-  if (VD->isExceptionVariable()) return false;
+  if (!(CESK & CES_AllowExceptionVariables) && VD->isExceptionVariable())
+    return false;
 
   // ...automatic...
   if (!VD->hasLocalStorage()) return false;
@@ -2937,6 +2938,11 @@ bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
 /// returned lvalues as rvalues in certain cases (to prefer move construction),
 /// then falls back to treating them as lvalues if that failed.
 ///
+/// \param ConvertingConstructorsOnly If true, follow [class.copy]p32 and reject
+/// resolutions that find non-constructors, such as derived-to-base conversions
+/// or `operator T()&&` member functions. If false, do consider such
+/// conversion sequences.
+///
 /// \param Res We will fill this in if move-initialization was possible.
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
@@ -2946,6 +2952,7 @@ static void TryMoveInitialization(Sema& S,
                                   const VarDecl *NRVOCandidate,
                                   QualType ResultType,
                                   Expr *&Value,
+                                  bool ConvertingConstructorsOnly,
                                   ExprResult &Res) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
                             CK_NoOp, Value, VK_XValue);
@@ -2966,22 +2973,39 @@ static void TryMoveInitialization(Sema& S,
       continue;
 
     FunctionDecl *FD = Step.Function.Function;
-    if (isa<CXXConstructorDecl>(FD)) {
-      // C++14 [class.copy]p32:
-      // [...] If the first overload resolution fails or was not performed,
-      // or if the type of the first parameter of the selected constructor
-      // is not an rvalue reference to the object's type (possibly
-      // cv-qualified), overload resolution is performed again, considering
-      // the object as an lvalue.
-      const RValueReferenceType *RRefType =
-          FD->getParamDecl(0)->getType()->getAs<RValueReferenceType>();
-      if (!RRefType)
-        break;
-      if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
-                                            NRVOCandidate->getType()))
-        break;
+    if (ConvertingConstructorsOnly) {
+      if (isa<CXXConstructorDecl>(FD)) {
+        // C++14 [class.copy]p32:
+        // [...] If the first overload resolution fails or was not performed,
+        // or if the type of the first parameter of the selected constructor
+        // is not an rvalue reference to the object's type (possibly
+        // cv-qualified), overload resolution is performed again, considering
+        // the object as an lvalue.
+        const RValueReferenceType *RRefType =
+            FD->getParamDecl(0)->getType()->getAs<RValueReferenceType>();
+        if (!RRefType)
+          break;
+        if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
+                                              NRVOCandidate->getType()))
+          break;
+      } else {
+        continue;
+      }
     } else {
-      continue;
+      if (isa<CXXConstructorDecl>(FD)) {
+        // Check that overload resolution selected a constructor taking an
+        // rvalue reference. If it selected an lvalue reference, then we
+        // didn't need to cast this thing to an rvalue in the first place.
+        if (!isa<RValueReferenceType>(FD->getParamDecl(0)->getType()))
+          break;
+      } else if (isa<CXXMethodDecl>(FD)) {
+        // Check that overload resolution selected a conversion operator
+        // taking an rvalue reference.
+        if (cast<CXXMethodDecl>(FD)->getRefQualifier() != RQ_RValue)
+          break;
+      } else {
+        continue;
+      }
     }
 
     // Promote "AsRvalue" to the heap, since we now need this
@@ -3019,13 +3043,82 @@ Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
   ExprResult Res = ExprError();
 
   if (AllowNRVO) {
+    bool AffectedByCWG1579 = false;
+
     if (!NRVOCandidate) {
       NRVOCandidate = getCopyElisionCandidate(ResultType, Value, CES_Default);
+      if (NRVOCandidate &&
+          !getDiagnostics().isIgnored(diag::warn_return_std_move_in_cxx11,
+                                      Value->getExprLoc())) {
+        const VarDecl *NRVOCandidateInCXX11 =
+            getCopyElisionCandidate(ResultType, Value, CES_FormerDefault);
+        AffectedByCWG1579 = (!NRVOCandidateInCXX11);
+      }
     }
 
     if (NRVOCandidate) {
       TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-                            Res);
+                            true, Res);
+    }
+
+    if (!Res.isInvalid() && AffectedByCWG1579) {
+      QualType QT = NRVOCandidate->getType();
+      if (QT.getNonReferenceType()
+                     .getUnqualifiedType()
+                     .isTriviallyCopyableType(Context)) {
+        // Adding 'std::move' around a trivially copyable variable is probably
+        // pointless. Don't suggest it.
+      } else {
+        // Common cases for this are returning unique_ptr<Derived> from a
+        // function of return type unique_ptr<Base>, or returning T from a
+        // function of return type Expected<T>. This is totally fine in a
+        // post-CWG1579 world, but was not fine before.
+        assert(!ResultType.isNull());
+        SmallString<32> Str;
+        Str += "std::move(";
+        Str += NRVOCandidate->getDeclName().getAsString();
+        Str += ")";
+        Diag(Value->getExprLoc(), diag::warn_return_std_move_in_cxx11)
+            << Value->getSourceRange()
+            << NRVOCandidate->getDeclName() << ResultType << QT;
+        Diag(Value->getExprLoc(), diag::note_add_std_move_in_cxx11)
+            << FixItHint::CreateReplacement(Value->getSourceRange(), Str);
+      }
+    } else if (Res.isInvalid() &&
+               !getDiagnostics().isIgnored(diag::warn_return_std_move,
+                                           Value->getExprLoc())) {
+      const VarDecl *FakeNRVOCandidate =
+          getCopyElisionCandidate(QualType(), Value, CES_AsIfByStdMove);
+      if (FakeNRVOCandidate) {
+        QualType QT = FakeNRVOCandidate->getType();
+        if (QT->isLValueReferenceType()) {
+          // Adding 'std::move' around an lvalue reference variable's name is
+          // dangerous. Don't suggest it.
+        } else if (QT.getNonReferenceType()
+                       .getUnqualifiedType()
+                       .isTriviallyCopyableType(Context)) {
+          // Adding 'std::move' around a trivially copyable variable is probably
+          // pointless. Don't suggest it.
+        } else {
+          ExprResult FakeRes = ExprError();
+          Expr *FakeValue = Value;
+          TryMoveInitialization(*this, Entity, FakeNRVOCandidate, ResultType,
+                                FakeValue, false, FakeRes);
+          if (!FakeRes.isInvalid()) {
+            bool IsThrow =
+                (Entity.getKind() == InitializedEntity::EK_Exception);
+            SmallString<32> Str;
+            Str += "std::move(";
+            Str += FakeNRVOCandidate->getDeclName().getAsString();
+            Str += ")";
+            Diag(Value->getExprLoc(), diag::warn_return_std_move)
+                << Value->getSourceRange()
+                << FakeNRVOCandidate->getDeclName() << IsThrow;
+            Diag(Value->getExprLoc(), diag::note_add_std_move)
+                << FixItHint::CreateReplacement(Value->getSourceRange(), Str);
+          }
+        }
+      }
     }
   }