[libc++] Make future_error constructor standard-compliant

[ldionne]

Summary

This patch removes the non compliant constructor of std::future_error
and adds the standards compliant constructor in C++17 instead.

Note that we can't support the constructor as an extension in all
standard modes because it uses delegating constructors, which require
C++11. We could in theory support the constructor as an extension in
C++11 and C++14 only, however I believe it is acceptable not to do that
since I expect the breakage from this patch will be minimal.

If it turns out that more code than we expect is broken by this, we can
reconsider that decision.

This was found during D99515.

[curdeius]

libcxx/include/future

540

Would it be better this way (probably with some specific error message)?

[• Quuxplusone]

libcxx/include/future

530–539

make_error_code should remain called via ADL; it's a customization point.
https://boostorg.github.io/outcome/motivation/plug_error_code.html
If your de-ADL'ed version passes the test suite, then we need more tests around this.

The pre-existing ctor sets up logic_error(__ec.message()) appropriately, so we want to find a way of duplicating that behavior. Also, I recommend adding a test for the message under test/libcxx/!

I'm suggesting that there be a private constructor from error_code (to avoid calling the user's make_error_code twice), but that it be tagged with a private tag to prevent people from stumbling into it accidentally. Your current patch leaves the existing ctor in place, but marks it private in C++17 and later; I think that's more likely to lead to confusion.

struct MyErrC { operator future_errc() const; operator error_code() const; };
auto err = std::future_error(MyErrC());

https://godbolt.org/z/rzs6K5a16

1372–1373

Maybe pull up the )); from the next line while you're here. Ditto line 1493.

libcxx/test/std/thread/futures/futures.future_error/ctor.fail.cpp

26 ↗

(On Diff #334074)

Per my suggestion above, I think "accidentally stumbling into a private constructor" is never a good thing. This specific error message indicates that we should do better.

libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp

32–33

Good; but how come these lines work in C++14 mode? Implicit conversion from future_errc to error_code, right?
In that case, your changes in test/std/thread/futures/futures.future_error/code.pass.cpp are too conservative; you can eliminate the #else condition and just run those tests in all modes.

[curdeius]

libcxx/include/future

530–539

Concerning make_error_code, isn't it a customisation point in the ctor of error_code only? Here we know that we call it with future_errc argument and we should call std-provided overload, no?

[• Quuxplusone]

libcxx/include/future

530–539

Ah, true. Okay, I retract that objection: it doesn't matter whether we qualify it or not. I have no strong opinion on whether it's better to consistently unqualify make_error_code everywhere it appears, or to consistently qualify every call except those that require ADL for correctness.

[curdeius]

• Apply review comments.
• Add future_error::__tag and rewrite ctors.
• Test noexcept.

[curdeius]

I'd love to hear your input on ABI breakage.

libcxx/include/future

530–539

I exposed __tag in all standard modes, because otherwise, I'd need to handle the construction of future_error in C++17 onwards and differently in previous standards.

539

What about ABI? Doing this change will certainly break it. What's the policy for that?
And why the ctor was in future.cpp and not inline? Is there something I should know about?

libcxx/test/std/thread/futures/futures.future_error/code.pass.cpp

23

I test noexcept in all standards modes, even if previously (pre-C++17) code() and what() were marked as throw() (which seems to be equivalent).

libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp

32–33

Yes, implicit conversion to error_code.

[ldionne]

libcxx/include/future

539

Indeed, this is an ABI break, and in fact the CI did catch it:

SYMBOL REMOVED: _ZNSt3__112future_errorC1ENS_10error_codeE
    {'is_defined': True, 'name': '_ZNSt3__112future_errorC1ENS_10error_codeE', 'type': 'FUNC'}
 
SYMBOL REMOVED: _ZNSt3__112future_errorC2ENS_10error_codeE
    {'is_defined': True, 'name': '_ZNSt3__112future_errorC2ENS_10error_codeE', 'type': 'FUNC'}

Perhaps we could define the constructor but make it private before C++17. That way, it is still part of the ABI but it can't be accessed by users. WDYT?

libcxx/src/future.cpp

208

This might not have been the case when you wrote that patch, however since then we always build libc++ with C++20. So we shouldn't need to use the __tag anywhere. In fact, I don't think we even need a __tag constructor at all, now.

[ldionne]

[Github PR transition cleanup]

Commandeering to finish.

[ldionne]

Rebase the patch, avoid ABI break, make 100% conforming (at the cost of potential breakage).

[ldionne]

Update patch message.

[ldionne]

Heads up for vendors. I'll ship this at the end of the week if nobody chimes in. If this causes problems after being landed, there's a few ways to try and reduce the scope of breakage.

[ldionne]

Adjust ignore_format.txt

[ldionne]

XFAIL test on Windows

[ldionne]

Fix XFAIL for real.

libcxx/docs/ReleaseNotes/18.rst

 
 - The "safe" mode is now controlled via the new generalized support for hardening. The ``LIBCXX_ENABLE_ASSERTIONS``
   CMake variable that was used to enable the safe mode is now deprecated and setting it will trigger an error; use the
   ``LIBCXX_HARDENING_MODE`` variable with the value ``safe`` instead. Similarly, the ``_LIBCPP_ENABLE_ASSERTIONS`` macro
   is deprecated (setting it to ``1`` still enables the safe mode in this release while also issuing a deprecation
   warning). ``_LIBCPP_ENABLE_ASSERTIONS`` will be removed entirely in the next release and setting it will become an
   error. See :ref:`the hardening documentation <using-hardening-modes>` for more details.
 
+- The non-conforming constructor ``std::future_error(std::error_code)`` has been removed. Please use the
+  ``std::future_error(std::future_errc)`` constructor provided in C++17 instead.
+
 Upcoming Deprecations and Removals
 ----------------------------------
 
 LLVM 18
 ~~~~~~~
 
 - The ``_LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED`` macro will not be honored anymore in LLVM 18.
   Please see the updated documentation about the safe libc++ mode and in particular the ``_LIBCPP_VERBOSE_ABORT``

libcxx/include/future

 };
 
 template <> struct is_error_code_enum<future_errc> : public true_type { };
 error_code make_error_code(future_errc e) noexcept;
 error_condition make_error_condition(future_errc e) noexcept;
 
 const error_category& future_category() noexcept;
 
-class future_error
-    : public logic_error
-{
+class future_error : public logic_error {
 public:
-    future_error(error_code ec);  // exposition only
-    explicit future_error(future_errc); // C++17
+    explicit future_error(future_errc e); // since C++17
+
     const error_code& code() const noexcept;
     const char*       what() const noexcept;
+
+private:
+    error_code ec_;             // exposition only
 };
 
 template <class R>
 class promise
 {
 public:
     promise();
     template <class Allocator>

 
 inline _LIBCPP_INLINE_VISIBILITY
 error_condition
 make_error_condition(future_errc __e) _NOEXCEPT
 {
     return error_condition(static_cast<int>(__e), future_category());
 }
 
+_LIBCPP_NORETURN inline _LIBCPP_HIDE_FROM_ABI
+#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+_LIBCPP_AVAILABILITY_FUTURE_ERROR
+#endif
+void __throw_future_error(future_errc __ev);
+
 class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_FUTURE_ERROR future_error
     : public logic_error
 {
     error_code __ec_;
+
+    future_error(error_code);
+    friend void __throw_future_error(future_errc);
+    template <class> friend class promise;
+
 public:
-    future_error(error_code __ec);
+#if _LIBCPP_STD_VER >= 17
+    _LIBCPP_HIDE_FROM_ABI explicit future_error(future_errc __ec) : future_error(std::make_error_code(__ec)) {}
+#endif
 
     _LIBCPP_INLINE_VISIBILITY
     const error_code& code() const _NOEXCEPT {return __ec_;}
 
     _LIBCPP_HIDE_FROM_ABI future_error(const future_error&) _NOEXCEPT = default;
     ~future_error() _NOEXCEPT override;
 };
 
-_LIBCPP_NORETURN inline _LIBCPP_INLINE_VISIBILITY
-#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
-_LIBCPP_AVAILABILITY_FUTURE_ERROR
-#endif
+// Declared above std::future_error
 void __throw_future_error(future_errc __ev)
 {
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
     throw future_error(make_error_code(__ev));
 #else
     (void)__ev;
     _LIBCPP_VERBOSE_ABORT("future_error was thrown in -fno-exceptions mode");
 #endif
promise<_Rp>::promise(allocator_arg_t, const _Alloc& __a0)
     __state_ = _VSTD::addressof(*__hold.release());
 }
 
 template <class _Rp>
 promise<_Rp>::~promise()
 {
     if (__state_)
     {
         if (!__state_->__has_value() && __state_->use_count() > 1)
-            __state_->set_exception(make_exception_ptr(
-                      future_error(make_error_code(future_errc::broken_promise))
-                                                      ));
+            __state_->set_exception(make_exception_ptr(future_error(make_error_code(future_errc::broken_promise))));
         __state_->__release_shared();
     }
 }
 
 template <class _Rp>
 future<_Rp>
 promise<_Rp>::get_future()
 {

 }
 
 template <class _Rp>
 promise<_Rp&>::~promise()
 {
     if (__state_)
     {
         if (!__state_->__has_value() && __state_->use_count() > 1)
-            __state_->set_exception(make_exception_ptr(
-                      future_error(make_error_code(future_errc::broken_promise))
-                                                      ));
+            __state_->set_exception(make_exception_ptr(future_error(make_error_code(future_errc::broken_promise))));
         __state_->__release_shared();
     }
 }
 
 template <class _Rp>
 future<_Rp&>
 promise<_Rp&>::get_future()
 {

libcxx/src/future.cpp

 }
 
 promise<void>::~promise()
 {
     if (__state_)
     {
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
         if (!__state_->__has_value() && __state_->use_count() > 1)
-            __state_->set_exception(make_exception_ptr(
-                      future_error(make_error_code(future_errc::broken_promise))
-                                                      ));
+            __state_->set_exception(make_exception_ptr(future_error(future_errc::broken_promise)));
 #endif // _LIBCPP_HAS_NO_EXCEPTIONS
         __state_->__release_shared();
     }
 }
 
 future<void>
 promise<void>::get_future()
 {
     if (__state_ == nullptr)

libcxx/test/std/thread/futures/futures.future_error/code.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: no-threads
 
 // <future>
-
+//
 // class future_error
-//     future_error(error_code __ec);  // exposition only
-//     explicit future_error(future_errc _Ev) : __ec_(make_error_code(_Ev)) {} // C++17
-
-// const error_code& code() const throw();
+//
+// const error_code& code() const noexcept;
 
-#include <future>
 #include <cassert>
+#include <future>
+#include <utility>
 
 #include "test_macros.h"
 
-int main(int, char**)
-{
-    {
-        std::error_code ec = std::make_error_code(std::future_errc::broken_promise);
-        std::future_error f(ec);
-        assert(f.code() == ec);
-    }
-    {
-        std::error_code ec = std::make_error_code(std::future_errc::future_already_retrieved);
-        std::future_error f(ec);
-        assert(f.code() == ec);
-    }
-    {
-        std::error_code ec = std::make_error_code(std::future_errc::promise_already_satisfied);
-        std::future_error f(ec);
-        assert(f.code() == ec);
-    }
-    {
-        std::error_code ec = std::make_error_code(std::future_errc::no_state);
-        std::future_error f(ec);
-        assert(f.code() == ec);
-    }
-#if TEST_STD_VER > 14
-    {
-        std::future_error f(std::future_errc::broken_promise);
-        assert(f.code() == std::make_error_code(std::future_errc::broken_promise));
-    }
-    {
-        std::future_error f(std::future_errc::no_state);
-        assert(f.code() == std::make_error_code(std::future_errc::no_state));
-    }
+int main(int, char**) {
+  ASSERT_NOEXCEPT(std::declval<std::future_error const&>().code());
+  ASSERT_SAME_TYPE(decltype(std::declval<std::future_error const&>().code()), std::error_code const&);
+
+  // Before C++17, we can't construct std::future_error directly in a standards-conforming way
+#if TEST_STD_VER >= 17
+    std::future_error const f(std::future_errc::broken_promise);
+    std::error_code const& code = f.code();
+    assert(code == std::make_error_code(std::future_errc::broken_promise));
+    std::future_error const f(std::future_errc::future_already_retrieved);
+    std::error_code const& code = f.code();
+    assert(code == std::make_error_code(std::future_errc::future_already_retrieved));
+    std::future_error const f(std::future_errc::promise_already_satisfied);
+    std::error_code const& code = f.code();
+    assert(code == std::make_error_code(std::future_errc::promise_already_satisfied));
+    std::future_error const f(std::future_errc::no_state);
+    std::error_code const& code = f.code();
+    assert(code == std::make_error_code(std::future_errc::no_state));
 #endif
 
   return 0;
 }

libcxx/test/std/thread/futures/futures.future_error/ctor.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: no-threads
+// UNSUPPORTED: c++03, c++11, c++14
+
+// <future>
+
+// class future_error
+//
+// explicit future_error(future_errc e); // since C++17
+
+#include <cassert>
+#include <future>
+#include <type_traits>
+
+int main(int, char**) {
+  {
+    std::future_error f(std::future_errc::broken_promise);
+    assert(f.code() == std::make_error_code(std::future_errc::broken_promise));
+  }
+  {
+    std::future_error f(std::future_errc::future_already_retrieved);
+    assert(f.code() == std::make_error_code(std::future_errc::future_already_retrieved));
+  }
+  {
+    std::future_error f(std::future_errc::promise_already_satisfied);
+    assert(f.code() == std::make_error_code(std::future_errc::promise_already_satisfied));
+  }
+  {
+    std::future_error f(std::future_errc::no_state);
+    assert(f.code() == std::make_error_code(std::future_errc::no_state));
+  }
+
+  // Make sure the constructor is explicit
+  static_assert(!std::is_convertible_v<std::future_errc, std::future_error>);
+
+  return 0;
+}

libcxx/test/std/thread/futures/futures.future_error/types.compile.pass.cpp

 
 // <future>
 
 // class future_error : public logic_error {...};
 
 #include <future>
 #include <type_traits>
 
-#include "test_macros.h"
-
-int main(int, char**)
-{
-    static_assert((std::is_convertible<std::future_error*,
-                                       std::logic_error*>::value), "");
-
-  return 0;
-}
+static_assert(std::is_convertible<std::future_error*, std::logic_error*>::value, "");

libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: no-threads
 
 // LWG 2056 changed the values of future_errc, so if we're using new headers
 // with an old library we'll get incorrect messages.
 //
 // XFAIL: stdlib=apple-libc++ && target={{.+}}-apple-macosx10.{{9|10|11}}
 
-// <future>
+// VC Runtime's std::exception::what() method is not marked as noexcept, so
+// this fails.
+// UNSUPPORTED: target=x86_64-pc-windows-msvc
 
+// <future>
+//
 // class future_error
+//
+// const char* what() const noexcept;
 
-// const char* what() const throw();
-
-#include <future>
-#include <cstring>
 #include <cassert>
+#include <future>
+#include <string_view>
+#include <utility>
 
 #include "test_macros.h"
 
-int main(int, char**)
-{
-    {
-        std::future_error f(std::make_error_code(std::future_errc::broken_promise));
-        LIBCPP_ASSERT(std::strcmp(f.what(), "The associated promise has been destructed prior "
-                      "to the associated state becoming ready.") == 0);
-    }
-    {
-        std::future_error f(std::make_error_code(std::future_errc::future_already_retrieved));
-        LIBCPP_ASSERT(std::strcmp(f.what(), "The future has already been retrieved from "
-                      "the promise or packaged_task.") == 0);
-    }
-    {
-        std::future_error f(std::make_error_code(std::future_errc::promise_already_satisfied));
-        LIBCPP_ASSERT(std::strcmp(f.what(), "The state of the promise has already been set.") == 0);
-    }
-    {
-        std::future_error f(std::make_error_code(std::future_errc::no_state));
-        LIBCPP_ASSERT(std::strcmp(f.what(), "Operation not permitted on an object without "
-                      "an associated state.") == 0);
-    }
+int main(int, char**) {
+  ASSERT_NOEXCEPT(std::declval<std::future_error const&>().what());
+  ASSERT_SAME_TYPE(decltype(std::declval<std::future_error const&>().what()), char const*);
+
+  // Before C++17, we can't construct std::future_error directly in a standards-conforming way
+#if TEST_STD_VER >= 17
+    std::future_error const f(std::future_errc::broken_promise);
+    char const* what = f.what();
+    LIBCPP_ASSERT(what == std::string_view{"The associated promise has been destructed prior "
+                                           "to the associated state becoming ready."});
+    std::future_error f(std::future_errc::future_already_retrieved);
+    char const* what = f.what();
+    LIBCPP_ASSERT(what == std::string_view{"The future has already been retrieved from "
+                                           "the promise or packaged_task."});
+    std::future_error f(std::future_errc::promise_already_satisfied);
+    char const* what = f.what();
+    LIBCPP_ASSERT(what == std::string_view{"The state of the promise has already been set."});
+    std::future_error f(std::future_errc::no_state);
+    char const* what = f.what();
+    LIBCPP_ASSERT(what == std::string_view{"Operation not permitted on an object without "
+                                           "an associated state."});
+#endif
 
   return 0;
 }

libcxx/utils/data/ignore_format.txt

 libcxx/test/std/thread/futures/futures.async/async_race.pass.cpp
 libcxx/test/std/thread/futures/futures.async/async.verify.cpp
 libcxx/test/std/thread/futures/futures.errors/default_error_condition.pass.cpp
 libcxx/test/std/thread/futures/futures.errors/equivalent_error_code_int.pass.cpp
 libcxx/test/std/thread/futures/futures.errors/equivalent_int_error_condition.pass.cpp
 libcxx/test/std/thread/futures/futures.errors/future_category.pass.cpp
 libcxx/test/std/thread/futures/futures.errors/make_error_code.pass.cpp
 libcxx/test/std/thread/futures/futures.errors/make_error_condition.pass.cpp
-libcxx/test/std/thread/futures/futures.future_error/code.pass.cpp
-libcxx/test/std/thread/futures/futures.future_error/types.pass.cpp
-libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp
 libcxx/test/std/thread/futures/futures.overview/future_errc.pass.cpp
 libcxx/test/std/thread/futures/futures.overview/future_status.pass.cpp
 libcxx/test/std/thread/futures/futures.overview/is_error_code_enum_future_errc.pass.cpp
 libcxx/test/std/thread/futures/futures.overview/launch.pass.cpp
 libcxx/test/std/thread/futures/futures.promise/alloc_ctor.pass.cpp
 libcxx/test/std/thread/futures/futures.promise/copy_assign.verify.cpp
 libcxx/test/std/thread/futures/futures.promise/copy_ctor.verify.cpp
 libcxx/test/std/thread/futures/futures.promise/default.pass.cpp