[clang] P2266 implicit moves STL workaround

[mizvekov]

Summary

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 <[e-mail]>

[mizvekov]

.

[mizvekov]

format.

[mizvekov]

.

[aaron.ballman]

Thank you for working on this! We're testing the patch out internally to see if it solves issues for us with some of the failures we're seeing.

clang/lib/Sema/SemaStmt.cpp

3314

This can return nullptr, so we should probably return false in that case; WDYT?

3317–3321

Can you use D->isInStdNamespace() instead?

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3314

Yes I missed that, you are absolutely right!

3317–3321

It doesn't look like isInStdNamespace is equivalent here, even though the name suggests otherwise.
This is a small helper, all it does is:

bool Decl::isInStdNamespace() const {
  const DeclContext *DC = getDeclContext();
  return DC && DC->isStdNamespace();
}

It helps you check isStdNamespace from a Decl, without having to through a DeclContext yourself.

Now if we really need this workaround to apply 'recursively' like this, that is a different question which I am not sure.

[aaron.ballman]

clang/lib/Sema/SemaStmt.cpp

3317–3321

Good point -- isInStdNamespace() only looks if the declaration is in the std namespace, not whether any declaration context containing the declaration is in std. I don't have an issue with the current approach, was mostly thinking if there were ways to simplify it.

[mizvekov]

• Small simplification to implementation, don't need to cast to NamespaceDecl.
• Simplification to tests: Use some evil prep hackery to avoid needing multiple files.
• Verify result of getReferencedDeclOfCallee is non-null.

No tests added for the last one, as I cannot think how this could actually happen in this instance.
That would suggest that maybe we should assert instead, but then it seems that if there were
such a case, it's most likely that we would want to return false anyway.
So the assert would be basically fishing for a test case. WDYT?

[mibintc]

Intel compiles VS2019 #include files regularly with clang, and the file <filesystem> compiled with -std:c++latest encounters this error report, which @aaron.ballman suggests is related to this effort.

In file included from tst_incl_filesystem.cpp:2:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\filesystem(3099,20): error: call to deleted constructor of 'unique_ptr<wchar_t []>'
            return _Cleaned_link;
                   ^~~~~~~~~~~~~
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\memory(3566,5): note: 'unique_ptr' has been explicitly marked deleted here
    unique_ptr(const unique_ptr&) = delete;
    ^
In file included from tst_incl_filesystem.cpp:2:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\filesystem(3125,16): error: call to deleted constructor of 'unique_ptr<wchar_t []>'
        return _Cleaned_link;
               ^~~~~~~~~~~~~
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\memory(3566,5): note: 'unique_ptr' has been explicitly marked deleted here
    unique_ptr(const unique_ptr&) = delete;

I used creduce with the same command line, looking for that error diagnostic with std:c++latest versus no diagnostic with std:c++20 ; this smaller test case, with a few edits, is the result

# 1 "test.cpp" 1
# 2 "test.cpp" 3
namespace std {
template <int a> struct b { static constexpr c = a; };
template <bool a> using d = b<a>;
using e = d<true>;
template <bool> using f = int;
template <bool, class, class> using g = int;
template <class...> constexpr am = e ::c;
template <class> constexpr au = false;
class h {
public:
  template <class i> h(i);
  template <class> using j = f<am<>>;
  template <class = j<g<au<int>, int, int>>> h(h &) = delete;
};
h k() {
  h l(new wchar_t);
  return l;
}
}
# 22 "test.cpp" 2

[aaron.ballman]

In D105951#2880654, @mibintc wrote:

Intel compiles VS2019 #include files regularly with clang, and the file <filesystem> compiled with -std:c++latest encounters this error report, which @aaron.ballman suggests is related to this effort.

I think we might be in an awkward situation where some MSVC STL code needs the implicit cast in order to work, such as:

In file included from c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\string:11:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\xstring(4714,12): error: non-const lvalue reference to type 'basic_istream<...>' cannot bind to a temporary of type 'basic_istream<...>'
    return _Istr;
           ^~~~~

This issue was fixed with the previous workaround. But other MSVC STL code fails without the implicit cast, such as:

In file included from tst_incl_filesystem.cpp:2:
c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29910/include\filesystem(3099,20): error: call to deleted constructor of 'unique_ptr<wchar_t []>'
            return _Cleaned_link;
                   ^~~~~~~~~~~~~

(At least, Melanie's reduced example fails with this patch, but if you remove the system header or namespace std bits from the test, then it passes.)

[mizvekov]

Patch update:

• Fixes issue where the workaround would completely supress implicit moves.
• Improve tests to cover this issue.
• Stop applying the workaround to throw expressions, as we have not observed any problems there.

In D105951#2880654, @mibintc wrote:

Intel compiles VS2019 #include files regularly with clang, and the file <filesystem> compiled with -std:c++latest encounters this error report, which @aaron.ballman suggests is related to this effort.

I used creduce with the same command line, looking for that error diagnostic with std:c++latest versus no diagnostic with std:c++20 ; this smaller test case, with a few edits, is the result

Thanks for testing!

In D105951#2880920, @aaron.ballman wrote:

I think we might be in an awkward situation where some MSVC STL code needs the implicit cast in order to work, such as:
....
(At least, Melanie's reduced example fails with this patch, but if you remove the system header or namespace std bits from the test, then it passes.)

No, that was totally my fault! :-)

I did not intend for the workaround to completely disable implicit moves, just that it would fall back to the C++20 behavior.

The problem is that the code path for this is split around multiple places, and while suppressing simpler implicit moves here, I should have undone the suppressing of the C++20 implicit move code path elsewhere!

Sigh, I forgot it was there because I was working on another patch that completely removed that second code path...
Sorry for that!

[mizvekov]

While we are still reviewing this and it's probably going to take longer, I went ahead made a DR for fixing the same issue in main: https://reviews.llvm.org/D106303

[STL_MSFT]

FYI, we merged https://github.com/microsoft/STL/pull/2025 fixing the 2 affected return _Istr; occurrences in <string> and <xstring> for VS 2022 17.0 Preview 4. If you find any other affected return statements, please let us know ASAP (as the VS release process locks down the release branch far in advance).

[mizvekov]

In D105951#2894888, @STL_MSFT wrote:

FYI, we merged https://github.com/microsoft/STL/pull/2025 fixing the 2 affected return _Istr; occurrences in <string> and <xstring> for VS 2022 17.0 Preview 4. If you find any other affected return statements, please let us know ASAP (as the VS release process locks down the release branch far in advance).

Thanks!

We could relax the workaround even more, and apply them just to those affected member functions.
That would help finding more issues, if there are any.
But at the same time we are super close to creating the release branch...

[mizvekov]

Rebase

[mizvekov]

If you think landing this in 13 after the release branch is created will be complicated, then please consider this message a gentle Ping :)

Frankly, I am satisfied with the mechanics of this workaround as is.

• After another look, I think doing something more targeted will be unduly convoluted, as we would want to target particular function overloads, one which is an operator>>.
• The STL will still be tested upstream, with "-fno-ms-compatibility".

[mibintc]

I tested this patch downstream on Windows with Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29334 for x64

I used c++20, x++latest default and c++17 with the stl header files (standalone include files--not usage) and didn't find any problems.

c:/Program files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.28.29333/

[aaron.ballman]

LGTM, thank you!

clang/lib/Frontend/InitPreprocessor.cpp

if (LangOpts.CPlusPlus20) {
     //Builder.defineMacro("__cpp_coroutines", "201902L");
     Builder.defineMacro("__cpp_designated_initializers", "201707L");
     Builder.defineMacro("__cpp_impl_three_way_comparison", "201907L");
     //Builder.defineMacro("__cpp_modules", "201907L");
     Builder.defineMacro("__cpp_using_enum", "201907L");
   }
   // C++2b features.
   if (LangOpts.CPlusPlus2b) {
-    if (!LangOpts.MSVCCompat)
-      Builder.defineMacro("__cpp_implicit_move", "202011L");
     Builder.defineMacro("__cpp_size_t_suffix", "202011L");
   }
   if (LangOpts.Char8)
     Builder.defineMacro("__cpp_char8_t", "201811L");
   Builder.defineMacro("__cpp_impl_destroying_delete", "201806L");
 
   // TS features.
   if (LangOpts.Coroutines)

clang/lib/Sema/SemaStmt.cpp

Sema::ActOnBreakStmt(SourceLocation BreakLoc, Scope *CurScope) {
   if (S->isOpenMPLoopScope())
     return StmtError(Diag(BreakLoc, diag::err_omp_loop_cannot_use_stmt)
                      << "break");
   CheckJumpOutOfSEHFinally(*this, BreakLoc, *S);
 
   return new (Context) BreakStmt(BreakLoc);
 }
 
+static bool CheckSimplerImplicitMovesMSVCWorkaround(const Sema &S,
+                                                    const Expr &E) {
+  if (!S.getLangOpts().MSVCCompat)
+    return false;
+  const Decl *D = E.getReferencedDeclOfCallee();
+  if (!S.SourceMgr.isInSystemHeader(D->getLocation()))
+    return false;
+  for (const DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent()) {
+    if (const auto *NS = dyn_cast<NamespaceDecl>(DC))
+      if (NS->isStdNamespace())
+        return true;
+  }
+  return false;
+}
+
 /// Determine whether the given expression might be move-eligible or
 /// copy-elidable in either a (co_)return statement or throw expression,
 /// without considering function return type, if applicable.
 ///
 /// \param E The expression being returned from the function or block,
 /// being thrown, or being co_returned from a coroutine. This expression
 /// might be modified by the implementation.
 ///
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))) {
+      (ForceCXX2b || (getLangOpts().CPlusPlus2b &&
+                      !CheckSimplerImplicitMovesMSVCWorkaround(*this, *E)))) {
     E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(),
                                  CK_NoOp, E, nullptr, VK_XValue,
                                  FPOptionsOverride());
   }
   return Res;
 }
 
 /// Determine whether the given NRVO candidate variable is move-eligible or

clang/test/SemaCXX/Inputs/sys/cxx2b-p2266-disable-with-msvc-compat-sys.hpp

+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}}
+
+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}}
+} // 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}}
+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}}
+} // 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}}
+
+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}}
+} // 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}}
+} // namespace foo
+
+} // namespace std

clang/test/SemaCXX/Inputs/usr/cxx2b-p2266-disable-with-msvc-compat-usr.hpp

+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}}
+
+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}}
+} // 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}}
+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}}
+} // 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}}
+
+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}}
+} // 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}}
+} // namespace foo
+
+} // namespace std

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

-// 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 -I %S/Inputs/usr -isystem %S/Inputs/sys                    -verify=cxx2b,new %s
+// RUN: %clang_cc1 -std=c++2b -fsyntax-only -I %S/Inputs/usr -isystem %S/Inputs/sys -fms-compatibility -verify=cxx2b,old %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -I %S/Inputs/usr -isystem %S/Inputs/sys                    -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.
-
-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}}
-};
-struct MoveOnly {
-  MoveOnly();
-  MoveOnly(MoveOnly &&);
-};
-MoveOnly &&rref();
-
-MoveOnly &&test1(MoveOnly &&w) {
-  return w; // old-error {{cannot bind to lvalue of type}}
-}
-
-CopyOnly test2(bool b) {
-  static CopyOnly w1;
-  CopyOnly w2;
-  if (b) {
-    return w1;
-  } else {
-    return w2; // new-error {{no matching constructor for initialization}}
-  }
-}
-
-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}}
-
-MoveOnly &&test4() {
-  MoveOnly &&x = rref();
-  return x; // old-error {{cannot bind to lvalue of type}}
-}
-
-void test5() try {
-  CopyOnly x;
-  throw x; // new-error {{no matching constructor for initialization}}
-} catch (...) {
-}
+//        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.
+
+#if __cpluscplus > 202002L && __cpp_implicit_move < 202011L
+#error "__cpp_implicit_move not defined correctly"
+#endif
+
+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 {
+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
+
+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}}
+} // namespace std
+} // namespace foo
+
+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}}
+
+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}}
+} // 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}}
+} // namespace foo
+
+} // namespace std
+#include <cxx2b-p2266-disable-with-msvc-compat-usr.hpp>
+#include <cxx2b-p2266-disable-with-msvc-compat-sys.hpp>