[clang] P2266 implicit moves STL workaround

This patch replaces the workaround for simpler implicit moves
implemented in D105518.

The Microsoft STL currently has some issues with P2266.

Where before, with -fms-compatibility, we would disable simpler
implicit moves globally, with this change, we disable it only
when the returned expression is in a context contained by
std namespace and is located within a system header.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Reviewed By: aaron.ballman, mibintc

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

clang/include/clang/Sema/Sema.h

@@ -4785,20 +4785,24 @@ public:
     bool isMoveEligible() const { return S != None; };
     bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; }
   };
-  NamedReturnInfo getNamedReturnInfo(Expr *&E, bool ForceCXX2b = false);
+  enum class SimplerImplicitMoveMode { ForceOff, Normal, ForceOn };
+  NamedReturnInfo getNamedReturnInfo(
+      Expr *&E, SimplerImplicitMoveMode Mode = SimplerImplicitMoveMode::Normal);
   NamedReturnInfo getNamedReturnInfo(const VarDecl *VD);
   const VarDecl *getCopyElisionCandidate(NamedReturnInfo &Info,
                                          QualType ReturnType);
 
-  ExprResult PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
-                                             const NamedReturnInfo &NRInfo,
-                                             Expr *Value);
+  ExprResult
+  PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
+                                  const NamedReturnInfo &NRInfo, Expr *Value,
+                                  bool SupressSimplerImplicitMoves = false);
 
   StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                              Scope *CurScope);
   StmtResult BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
   StmtResult ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
-                                     NamedReturnInfo &NRInfo);
+                                     NamedReturnInfo &NRInfo,
+                                     bool SupressSimplerImplicitMoves);
 
   StmtResult ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                              bool IsVolatile, unsigned NumOutputs,

clang/lib/Frontend/InitPreprocessor.cpp

@@ -598,8 +598,7 @@ static void InitializeCPlusPlusFeatureTestMacros(const LangOptions &LangOpts,
   }
   // C++2b features.
   if (LangOpts.CPlusPlus2b) {
-    if (!LangOpts.MSVCCompat)
-      Builder.defineMacro("__cpp_implicit_move", "202011L");
+    Builder.defineMacro("__cpp_implicit_move", "202011L");
     Builder.defineMacro("__cpp_size_t_suffix", "202011L");
   }
   if (LangOpts.Char8)

clang/lib/Sema/SemaCoroutine.cpp

@@ -977,7 +977,7 @@ StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
   VarDecl *Promise = FSI->CoroutinePromise;
   ExprResult PC;
   if (E && (isa<InitListExpr>(E) || !E->getType()->isVoidType())) {
-    getNamedReturnInfo(E, /*ForceCXX2b=*/true);
+    getNamedReturnInfo(E, SimplerImplicitMoveMode::ForceOn);
     PC = buildPromiseCall(*this, Promise, Loc, "return_value", E);
   } else {
     E = MakeFullDiscardedValueExpr(E).get();

clang/lib/Sema/SemaStmt.cpp

@@ -3322,7 +3322,8 @@ Sema::ActOnBreakStmt(SourceLocation BreakLoc, Scope *CurScope) {
 /// \returns An aggregate which contains the Candidate and isMoveEligible
 /// and isCopyElidable methods. If Candidate is non-null, it means
 /// isMoveEligible() would be true under the most permissive language standard.
-Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E, bool ForceCXX2b) {
+Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E,
+                                               SimplerImplicitMoveMode Mode) {
   if (!E)
     return NamedReturnInfo();
   // - in a return statement in a function [where] ...
@@ -3334,13 +3335,10 @@ Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E, bool ForceCXX2b) {
   if (!VD)
     return NamedReturnInfo();
   NamedReturnInfo Res = getNamedReturnInfo(VD);
-  // FIXME: We supress simpler implicit move here (unless ForceCXX2b is true)
-  //        in msvc compatibility mode just as a temporary work around,
-  //        as the MSVC STL has issues with this change.
-  //        We will come back later with a more targeted approach.
   if (Res.Candidate && !E->isXValue() &&
-      (ForceCXX2b ||
-       (getLangOpts().CPlusPlus2b && !getLangOpts().MSVCCompat))) {
+      (Mode == SimplerImplicitMoveMode::ForceOn ||
+       (Mode != SimplerImplicitMoveMode::ForceOff &&
+        getLangOpts().CPlusPlus2b))) {
     E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(),
                                  CK_NoOp, E, nullptr, VK_XValue,
                                  FPOptionsOverride());
@@ -3480,15 +3478,10 @@ VerifyInitializationSequenceCXX98(const Sema &S,
 /// This routine implements C++20 [class.copy.elision]p3, which attempts to
 /// treat returned lvalues as rvalues in certain cases (to prefer move
 /// construction), then falls back to treating them as lvalues if that failed.
-ExprResult
-Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
-                                      const NamedReturnInfo &NRInfo,
-                                      Expr *Value) {
-  // FIXME: We force P1825 implicit moves here in msvc compatibility mode
-  // because we are disabling simpler implicit moves as a temporary
-  // work around, as the MSVC STL has issues with this change.
-  // We will come back later with a more targeted approach.
-  if ((!getLangOpts().CPlusPlus2b || getLangOpts().MSVCCompat) &&
+ExprResult Sema::PerformMoveOrCopyInitialization(
+    const InitializedEntity &Entity, const NamedReturnInfo &NRInfo, Expr *Value,
+    bool SupressSimplerImplicitMoves) {
+  if ((!getLangOpts().CPlusPlus2b || SupressSimplerImplicitMoves) &&
       NRInfo.isMoveEligible()) {
     ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
                               CK_NoOp, Value, VK_XValue, FPOptionsOverride());
@@ -3529,7 +3522,8 @@ static bool hasDeducedReturnType(FunctionDecl *FD) {
 ///
 StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
                                          Expr *RetValExp,
-                                         NamedReturnInfo &NRInfo) {
+                                         NamedReturnInfo &NRInfo,
+                                         bool SupressSimplerImplicitMoves) {
   // If this is the first return we've seen, infer the return type.
   // [expr.prim.lambda]p4 in C++11; block literals follow the same rules.
   CapturingScopeInfo *CurCap = cast<CapturingScopeInfo>(getCurFunction());
@@ -3660,7 +3654,8 @@ StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
     // the C version of which boils down to CheckSingleAssignmentConstraints.
     InitializedEntity Entity = InitializedEntity::InitializeResult(
         ReturnLoc, FnRetType, NRVOCandidate != nullptr);
-    ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRInfo, RetValExp);
+    ExprResult Res = PerformMoveOrCopyInitialization(
+        Entity, NRInfo, RetValExp, SupressSimplerImplicitMoves);
     if (Res.isInvalid()) {
       // FIXME: Cleanup temporaries here, anyway?
       return StmtError();
@@ -3870,15 +3865,37 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
   return R;
 }
 
+static bool CheckSimplerImplicitMovesMSVCWorkaround(const Sema &S,
+                                                    const Expr *E) {
+  if (!E || !S.getLangOpts().CPlusPlus2b || !S.getLangOpts().MSVCCompat)
+    return false;
+  const Decl *D = E->getReferencedDeclOfCallee();
+  if (!D || !S.SourceMgr.isInSystemHeader(D->getLocation()))
+    return false;
+  for (const DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent()) {
+    if (DC->isStdNamespace())
+      return true;
+  }
+  return false;
+}
+
 StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
   // Check for unexpanded parameter packs.
   if (RetValExp && DiagnoseUnexpandedParameterPack(RetValExp))
     return StmtError();
 
-  NamedReturnInfo NRInfo = getNamedReturnInfo(RetValExp);
+  // HACK: We supress simpler implicit move here in msvc compatibility mode
+  // just as a temporary work around, as the MSVC STL has issues with
+  // this change.
+  bool SupressSimplerImplicitMoves =
+      CheckSimplerImplicitMovesMSVCWorkaround(*this, RetValExp);
+  NamedReturnInfo NRInfo = getNamedReturnInfo(
+      RetValExp, SupressSimplerImplicitMoves ? SimplerImplicitMoveMode::ForceOff
+                                             : SimplerImplicitMoveMode::Normal);
 
   if (isa<CapturingScopeInfo>(getCurFunction()))
-    return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp, NRInfo);
+    return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp, NRInfo,
+                                   SupressSimplerImplicitMoves);
 
   QualType FnRetType;
   QualType RelatedRetType;
@@ -4069,8 +4086,8 @@ StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
       // we have a non-void function with an expression, continue checking
       InitializedEntity Entity = InitializedEntity::InitializeResult(
           ReturnLoc, RetType, NRVOCandidate != nullptr);
-      ExprResult Res =
-          PerformMoveOrCopyInitialization(Entity, NRInfo, RetValExp);
+      ExprResult Res = PerformMoveOrCopyInitialization(
+          Entity, NRInfo, RetValExp, SupressSimplerImplicitMoves);
       if (Res.isInvalid()) {
         // FIXME: Clean up temporaries here anyway?
         return StmtError();

clang/test/SemaCXX/cxx2b-p2266-disable-with-msvc-compat.cpp

@@ -1,52 +1,153 @@
-// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fcxx-exceptions                    -verify=new %s
-// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fcxx-exceptions -fms-compatibility -verify=old %s
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions                    -verify=old %s
+// RUN: %clang_cc1 -std=c++2b -fsyntax-only                    -verify=cxx2b,new %s
+// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fms-compatibility -verify=cxx2b,old %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only                    -verify=cxx20,old %s
 
 // FIXME: This is a test for a temporary workaround where we disable simpler implicit moves
-//        when compiling with -fms-compatibility, because the MSVC STL does not compile.
-//        A better workaround is under discussion.
-//        The test cases here are just a copy from `CXX/class/class.init/class.copy.elision/p3.cpp`,
-//        so feel free to delete this file when the workaround is not needed anymore.
+//        in the STL when compiling with -fms-compatibility, because of issues with the
+//        implementation there.
+//        Feel free to delete this file when the workaround is not needed anymore.
 
-struct CopyOnly {
-  CopyOnly(); // new-note {{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
-  // new-note@-1 {{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
-  CopyOnly(CopyOnly &); // new-note {{candidate constructor not viable: expects an lvalue for 1st argument}}
-  // new-note@-1 {{candidate constructor not viable: expects an lvalue for 1st argument}}
+#if __INCLUDE_LEVEL__ == 0
+
+#if __cpluscplus > 202002L && __cpp_implicit_move < 202011L
+#error "__cpp_implicit_move not defined correctly"
+#endif
+
+struct nocopy {
+  nocopy(nocopy &&);
 };
-struct MoveOnly {
-  MoveOnly();
-  MoveOnly(MoveOnly &&);
-};
-MoveOnly &&rref();
 
-MoveOnly &&test1(MoveOnly &&w) {
-  return w; // old-error {{cannot bind to lvalue of type}}
-}
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy mt3(nocopy x) { return x; }
 
-CopyOnly test2(bool b) {
-  static CopyOnly w1;
-  CopyOnly w2;
-  if (b) {
-    return w1;
-  } else {
-    return w2; // new-error {{no matching constructor for initialization}}
-  }
-}
+namespace {
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy mt3(nocopy x) { return x; }
+} // namespace
 
-template <class T> T &&test3(T &&x) { return x; } // old-error {{cannot bind to lvalue of type}}
-template MoveOnly &test3<MoveOnly &>(MoveOnly &);
-template MoveOnly &&test3<MoveOnly>(MoveOnly &&); // old-note {{in instantiation of function template specialization}}
+namespace foo {
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+namespace std {
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy mt3(nocopy x) { return x; }
+} // namespace std
+} // namespace foo
 
-MoveOnly &&test4() {
-  MoveOnly &&x = rref();
-  return x; // old-error {{cannot bind to lvalue of type}}
-}
+namespace std {
 
-void test5() try {
-  CopyOnly x;
-  throw x; // new-error {{no matching constructor for initialization}}
-} catch (...) {
-}
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy mt3(nocopy x) { return x; }
 
-MoveOnly test6(MoveOnly x) { return x; }
+namespace {
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy mt3(nocopy x) { return x; }
+} // namespace
+
+namespace foo {
+int &&mt1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &mt2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy mt3(nocopy x) { return x; }
+} // namespace foo
+
+} // namespace std
+
+#include __FILE__
+
+#define SYSTEM
+#include __FILE__
+
+#elif !defined(SYSTEM)
+
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+
+namespace {
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+} // namespace
+
+namespace foo {
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+namespace std {
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+} // namespace std
+} // namespace foo
+
+namespace std {
+
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+
+namespace {
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+} // namespace
+
+namespace foo {
+int &&ut1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &ut2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy ut3(nocopy x) { return x; }
+} // namespace foo
+
+} // namespace std
+
+#else
+
+#pragma GCC system_header
+
+int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+
+namespace {
+int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+} // namespace
+
+namespace foo {
+int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+namespace std {
+int &&st1(int &&x) { return x; } // cxx20-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // cxx2b-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+} // namespace std
+} // namespace foo
+
+namespace std {
+
+int &&st1(int &&x) { return x; } // old-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // new-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+
+namespace {
+int &&st1(int &&x) { return x; } // old-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // new-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+} // namespace
+
+namespace foo {
+int &&st1(int &&x) { return x; } // old-error {{cannot bind to lvalue}}
+int &st2(int &&x) { return x; }  // new-error {{cannot bind to a temporary}}
+nocopy st3(nocopy x) { return x; }
+} // namespace foo
+
+} // namespace std
+
+#endif