[libc++] Implement LWG3940: std::expected<void, E>::value() also needs E to be copy constructible #71819

PragmaTwice · 2023-11-09T16:01:10Z

This patch includes the fix for LWG3940 (std::expected<void, E>::value() also needs E to be copy constructible)

…s E to be copy constructible)

llvmbot · 2023-11-09T16:01:45Z

@llvm/pr-subscribers-libcxx

Author: Twice (PragmaTwice)

Changes

This patch includes the fix for LWG3940 (std::expected<void, E>::value() also needs E to be copy constructible)

TODO: maybe add more tests and check compiler error messages precisely via llvm-lit

Full diff: https://github.com/llvm/llvm-project/pull/71819.diff

4 Files Affected:

(modified) libcxx/docs/Status/Cxx2cIssues.csv (+1-1)
(modified) libcxx/include/__expected/expected.h (+3)
(added) libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp (+27)
(modified) libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp (-11)

diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index a928b69ea10c489..099a19d02888f4c 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -15,7 +15,7 @@
 "`3927 <https://wg21.link/LWG3927>`__","Unclear preconditions for ``operator[]`` for sequence containers","Varna June 2023","|Nothing To Do|","",""
 "`3935 <https://wg21.link/LWG3935>`__","``template<class X> constexpr complex& operator=(const complex<X>&)`` has no specification","Varna June 2023","|Complete|","3.4",""
 "`3938 <https://wg21.link/LWG3938>`__","Cannot use ``std::expected`` monadic ops with move-only ``error_type``","Varna June 2023","|Complete|","18.0",""
-"`3940 <https://wg21.link/LWG3940>`__","``std::expected<void, E>::value()`` also needs ``E`` to be copy constructible","Varna June 2023","","",""
+"`3940 <https://wg21.link/LWG3940>`__","``std::expected<void, E>::value()`` also needs ``E`` to be copy constructible","Varna June 2023","|Complete|","18.0",""
 "","","","","",""
 "`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""
 "`3892 <https://wg21.link/LWG3892>`__","Incorrect formatting of nested ranges and tuples","Not Yet Adopted","|Complete|","17.0","|format|"
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index bf16c8f720d2681..c6441f1651b0855 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -1187,12 +1187,15 @@ class expected<_Tp, _Err> {
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr void value() const& {
+    static_assert(is_copy_constructible_v<_Err>, "error_type has to be copy constructible");
     if (!__has_val_) {
       std::__throw_bad_expected_access<_Err>(__union_.__unex_);
     }
   }
 
   _LIBCPP_HIDE_FROM_ABI constexpr void value() && {
+    static_assert(is_copy_constructible_v<_Err> && is_move_constructible_v<_Err>,
+                  "error_type has to be both copy constructible and move constructible");
     if (!__has_val_) {
       std::__throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
     }
diff --git a/libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp b/libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp
new file mode 100644
index 000000000000000..37c06e629e9443e
--- /dev/null
+++ b/libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+// 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: c++03, c++11, c++14, c++17, c++20
+
+// constexpr void value() &&;
+// Mandates: is_copy_constructible_v<E> is true and is_move_constructible_v<E> is true.
+
+#include <expected>
+
+#include "MoveOnly.h"
+
+void test() {
+  // MoveOnly
+  std::expected<void, MoveOnly> e(std::unexpect, 5);
+
+  // COMPILE FAIL: MoveOnly is not copy constructible
+  std::move(e).value();
+}
+
+int main(int, char**) {
+  test();
+}
diff --git a/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp b/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
index 0f02db19c7b75e7..a24d67c57e19b59 100644
--- a/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
@@ -65,17 +65,6 @@ void testException() {
     }
   }
 
-  // MoveOnly
-  {
-    std::expected<void, MoveOnly> e(std::unexpect, 5);
-    try {
-      std::move(e).value();
-      assert(false);
-    } catch (const std::bad_expected_access<MoveOnly>& ex) {
-      assert(ex.error() == 5);
-    }
-  }
-
 #endif // TEST_HAS_NO_EXCEPTIONS
 }

github-actions · 2023-11-09T16:12:15Z

✅ With the latest revision this PR passed the C/C++ code formatter.

libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp

huixie90

LGTM with all issues are addressed. Thank you!

libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.verify.cpp

huixie90 · 2023-11-15T17:34:28Z

libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp

@@ -65,17 +65,6 @@ void testException() {
    }
  }

-  // MoveOnly


Out of interest, according to the LWG issue, this test should never work. Do you know why there was not an issue before?

Yeah due to LWG3940 it cannot pass the compiler. But I guess before this the expected::value implementation can actually accept move-only types.

Out of interest, according to the LWG issue, this test should never work. Do you know why there was not an issue before?

This is because of Clang's non-conforming acception of move-only exception types, which is reported as #53224.

Why would this extension be non-conforming? A warning is missing, but other than that I don't see how it wouldn't be conforming.

It seems that throw NonCopyable(args) also needs to result in substitution failure, while Clang currently disagrees (example). See also this article.

mordante

Thanks for your patch!

libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.verify.cpp

PragmaTwice · 2023-11-17T13:15:01Z

It seems the CI has failed, but I'm not sure if it is related to my changes, since I don't touch these failed test cases.

std/containers/container.adaptors/container.adaptors.format/format.functions.format.pass.cpp' FAILED

PragmaTwice · 2023-11-21T12:41:47Z

There are still some random CI errors, but seems not related to my changes.

Hi @huixie90 @mordante, could you review this PR again when you are free?

libcxx/include/__expected/expected.h

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp

huixie90

LGTM

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp

mordante

@PragmaTwice There are some review comments still open. Do you have time to address these? Then we can merge this patch.

PragmaTwice · 2023-12-25T13:02:25Z

@PragmaTwice There are some review comments still open. Do you have time to address these? Then we can merge this patch.

Apologies for my delayed response. I intend to complete it this week.

PragmaTwice · 2024-01-03T08:19:58Z

Hi @mordante @huixie90, I think all review comments have been addressed now. Could you review it again?

huixie90 · 2024-01-03T16:04:26Z

Hi @mordante @huixie90, I think all review comments have been addressed now. Could you review it again?

Hi thanks for the update. There 2 expected tests in the CI that seem to be failing. Do you know why it is the case?

PragmaTwice · 2024-01-04T02:02:22Z

Hi @mordante @huixie90, I think all review comments have been addressed now. Could you review it again?

Hi thanks for the update. There 2 expected tests in the CI that seem to be failing. Do you know why it is the case?

Yeah I've seen these failed cases.
But I cannot reproduce on my side, and it seems not related to my changes.

I will try to merge the latest main branch and rerun the CI.

# | error: 'expected-error' diagnostics seen but not expected: 
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1378: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1378: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1389: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1389: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1400: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1400: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1412: excess elements in struct initializer
# |   File /home/runner/_work/llvm-project/llvm-project/build/generic-modules/include/c++/v1/__expected/expected.h Line 1412: excess elements in struct initializer

PragmaTwice · 2024-01-04T03:02:34Z

https://github.com/llvm/llvm-project/actions/runs/7404652886/job/20147039107?pr=71819

# .---command stderr------------
# | error: 'expected-error' diagnostics expected but not seen: 
# |   File * Line * (directive at /home/runner/_work/llvm-project/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp:46): call to deleted constructor of 'MoveOnly'
# |   File * Line * (directive at /home/runner/_work/llvm-project/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp:47): call to deleted constructor of 'CopyOnly'
# |   File * Line * (directive at /home/runner/_work/llvm-project/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp:48): call to deleted constructor of 'CopyOnly'
# | 3 errors generated.
# `-----------------------------

It seems in some specific version/config of clang, these error messages do not appear.

May I ask if there is any method to workaround these non-important error messages?

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp

PragmaTwice · 2024-01-06T07:24:32Z

The CI finally passed now : )

mordante

The CI finally passed now : )

Great. A few minor comment and then it LGTM. I'll leave the final approval to @huixie90

libcxx/include/__expected/expected.h

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp

PragmaTwice · 2024-01-09T14:00:14Z

The CI finally passed now : )

Great. A few minor comment and then it LGTM. I'll leave the final approval to @huixie90

Thank you! Comments has been addressed now. cc @huixie90

huixie90 · 2024-01-17T16:07:36Z

LGTM. @mordante could you please have another look if the current state addressed your concerns?

mordante

LGTM. @mordante could you please have another look if the current state addressed your concerns?

Yes looks fine to me.

[libc++] Implement LWG3940 (std::expected<void, E>::value() also need…

3e4b73d

…s E to be copy constructible)

PragmaTwice requested a review from a team as a code owner November 9, 2023 16:01

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 9, 2023

[libcxx] format test

2d1c43f

huixie90 reviewed Nov 15, 2023

View reviewed changes

libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp Outdated Show resolved Hide resolved

PragmaTwice added 2 commits November 15, 2023 23:49

use .verify.cpp instead of .compile.fail.cpp

0767b57

format code

a5ea305

huixie90 reviewed Nov 15, 2023

View reviewed changes

huixie90 self-assigned this Nov 15, 2023

mordante reviewed Nov 15, 2023

View reviewed changes

libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.verify.cpp Outdated Show resolved Hide resolved

rename & clean up

cc85707

PragmaTwice changed the title ~~[libc++] Implement LWG3940 (std::expected<void, E>::value() also needs E to be copy constructible)~~ [libc++] Implement LWG3940: std::expected<void, E>::value() also needs E to be copy constructible Nov 18, 2023

trigger the CI again

7d6b115

mordante reviewed Nov 24, 2023

View reviewed changes

libcxx/include/__expected/expected.h Outdated Show resolved Hide resolved

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp Show resolved Hide resolved

huixie90 approved these changes Nov 29, 2023

View reviewed changes

huixie90 reviewed Dec 13, 2023

View reviewed changes

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp Show resolved Hide resolved

mordante requested changes Dec 24, 2023

View reviewed changes

add more tests

5cf3dc6

Merge branch 'main' into lwg3940

0404b8d

huixie90 reviewed Jan 5, 2024

View reviewed changes

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp Outdated Show resolved Hide resolved

exclude no-rtti and no-exceptions

4ef228b

Merge branch 'main' into lwg3940

8f558c5

mordante reviewed Jan 7, 2024

View reviewed changes

libcxx/include/__expected/expected.h Outdated Show resolved Hide resolved

libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp Outdated Show resolved Hide resolved

remove static_assert message & move expected-error position

2614080

mordante approved these changes Jan 18, 2024

View reviewed changes

huixie90 merged commit 7ded345 into llvm:main Jan 19, 2024

[libc++] Implement LWG3940: std::expected<void, E>::value() also needs E to be copy constructible #71819

[libc++] Implement LWG3940: std::expected<void, E>::value() also needs E to be copy constructible #71819

Uh oh!

Conversation

PragmaTwice commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 9, 2023

Uh oh!

github-actions bot commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

huixie90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

huixie90 Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

frederick-vs-ja Dec 26, 2023

Choose a reason for hiding this comment

Uh oh!

philnik777 Dec 26, 2023

Choose a reason for hiding this comment

Uh oh!

frederick-vs-ja Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PragmaTwice commented Nov 17, 2023

Uh oh!

PragmaTwice commented Nov 21, 2023

Uh oh!

Uh oh!

Uh oh!

huixie90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

PragmaTwice commented Dec 25, 2023

Uh oh!

PragmaTwice commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huixie90 commented Jan 3, 2024

Uh oh!

PragmaTwice commented Jan 4, 2024

Uh oh!

PragmaTwice commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PragmaTwice commented Jan 6, 2024

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PragmaTwice commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huixie90 commented Jan 17, 2024

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PragmaTwice commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

frederick-vs-ja Dec 26, 2023 •

edited

Loading

PragmaTwice commented Jan 3, 2024 •

edited

Loading

PragmaTwice commented Jan 4, 2024 •

edited

Loading

PragmaTwice commented Jan 9, 2024 •

edited

Loading