Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

PragmaTwice
Copy link
Contributor

@PragmaTwice PragmaTwice commented Nov 9, 2023

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

@PragmaTwice PragmaTwice requested a review from a team as a code owner November 9, 2023 16:01
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 9, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 9, 2023

@llvm/pr-subscribers-libcxx

Author: Twice (PragmaTwice)

Changes

This patch includes the fix for LWG3940 (std::expected&lt;void, E&gt;::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
 }
 

Copy link

github-actions bot commented Nov 9, 2023

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

Copy link
Contributor

@huixie90 huixie90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with all issues are addressed. Thank you!

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

// MoveOnly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja Dec 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@huixie90 huixie90 self-assigned this Nov 15, 2023
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patch!

@PragmaTwice
Copy link
Contributor Author

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 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
@PragmaTwice
Copy link
Contributor Author

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?

Copy link
Contributor

@huixie90 huixie90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@PragmaTwice
Copy link
Contributor Author

@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
Copy link
Contributor Author

PragmaTwice commented Jan 3, 2024

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

@huixie90
Copy link
Contributor

huixie90 commented Jan 3, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

PragmaTwice commented Jan 4, 2024

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?

@PragmaTwice
Copy link
Contributor Author

The CI finally passed now : )

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI finally passed now : )

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

@PragmaTwice
Copy link
Contributor Author

PragmaTwice commented Jan 9, 2024

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
Copy link
Contributor

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

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yes looks fine to me.

@huixie90 huixie90 merged commit 7ded345 into llvm:main Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants