[libc++] Implement LWG4072: std::optional comparisons: constrain harder - #209968
Conversation
|
Hello @HumanThe2nd 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
llvmorg-github-actions
Bot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Dan Shan (HumanThe2nd) ChangesResolves #118345 (LWG4072: std::optional comparisons: constrain harder) The heterogeneous operators (==, !=, <, <=, >, >=) for arguments T and U don't check that T or U are not a std::optional themselves. This allowed the operators to cause ambiguous overload resolution instead of falling back to optional's own operators. Added As required by the LLVM Project's AI use policy:
Full diff: https://github.com/llvm/llvm-project/pull/209968.diff 7 Files Affected:
diff --git a/libcxx/include/optional b/libcxx/include/optional
index 2499479348892..eb93af6857751 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -1543,7 +1543,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr strong_ordering operator<=>(const optional<_Tp>&
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() == std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() == std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Up>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator==(const optional<_Tp>& __x, const _Up& __v) {
if (__x.has_value())
@@ -1554,7 +1555,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator==(const optional<_Tp>& __x, const
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() == std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() == std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Tp>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator==(const _Tp& __v, const optional<_Up>& __x) {
if (__x.has_value())
@@ -1565,7 +1567,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator==(const _Tp& __v, const optional<_
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() != std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() != std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Up>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator!=(const optional<_Tp>& __x, const _Up& __v) {
if (__x.has_value())
@@ -1576,7 +1579,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator!=(const optional<_Tp>& __x, const
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() != std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() != std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Tp>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator!=(const _Tp& __v, const optional<_Up>& __x) {
if (__x.has_value())
@@ -1586,7 +1590,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator!=(const _Tp& __v, const optional<_
template < class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() < std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() < std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Up>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator<(const optional<_Tp>& __x, const _Up& __v) {
if (__x.has_value())
@@ -1596,7 +1601,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator<(const optional<_Tp>& __x, const _
template < class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() < std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() < std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Tp>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator<(const _Tp& __v, const optional<_Up>& __x) {
if (__x.has_value())
@@ -1607,7 +1613,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator<(const _Tp& __v, const optional<_U
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() <= std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() <= std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Up>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator<=(const optional<_Tp>& __x, const _Up& __v) {
if (__x.has_value())
@@ -1618,7 +1625,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator<=(const optional<_Tp>& __x, const
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() <= std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() <= std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Tp>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator<=(const _Tp& __v, const optional<_Up>& __x) {
if (__x.has_value())
@@ -1628,7 +1636,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator<=(const _Tp& __v, const optional<_
template < class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() > std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() > std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Up>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator>(const optional<_Tp>& __x, const _Up& __v) {
if (__x.has_value())
@@ -1638,7 +1647,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator>(const optional<_Tp>& __x, const _
template < class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() > std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() > std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Tp>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator>(const _Tp& __v, const optional<_Up>& __x) {
if (__x.has_value())
@@ -1649,7 +1659,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator>(const _Tp& __v, const optional<_U
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() >= std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() >= std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Up>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator>=(const optional<_Tp>& __x, const _Up& __v) {
if (__x.has_value())
@@ -1660,7 +1671,8 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool operator>=(const optional<_Tp>& __x, const
template <
class _Tp,
class _Up,
- enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() >= std::declval<const _Up&>()), bool>,
+ enable_if_t<__is_core_convertible_v<decltype(std::declval<const _Tp&>() >= std::declval<const _Up&>()), bool> &&
+ !__is_std_optional<_Tp>::value,
int> = 0>
_LIBCPP_HIDE_FROM_ABI constexpr bool operator>=(const _Tp& __v, const optional<_Up>& __x) {
if (__x.has_value())
diff --git a/libcxx/test/std/utilities/optional/optional.relops/equal.pass.cpp b/libcxx/test/std/utilities/optional/optional.relops/equal.pass.cpp
index 6915b604d4ef4..dba9cbd78146c 100644
--- a/libcxx/test/std/utilities/optional/optional.relops/equal.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.relops/equal.pass.cpp
@@ -29,6 +29,9 @@ static_assert(HasOperatorEqual<std::optional<EqualityComparable>, std::optional<
static_assert(!HasOperatorEqual<std::optional<NonComparable>>);
static_assert(!HasOperatorEqual<std::optional<EqualityComparable>, std::optional<NonComparable>>);
+static_assert(!HasOperatorEqual<std::optional<void*>, std::optional<int>>);
+static_assert(!HasOperatorEqual<std::optional<int>, std::optional<void*>>);
+
#endif
using std::optional;
diff --git a/libcxx/test/std/utilities/optional/optional.relops/greater_equal.pass.cpp b/libcxx/test/std/utilities/optional/optional.relops/greater_equal.pass.cpp
index d14e7df6abc80..b31f5047f1090 100644
--- a/libcxx/test/std/utilities/optional/optional.relops/greater_equal.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.relops/greater_equal.pass.cpp
@@ -26,6 +26,9 @@ static_assert(!HasOperatorGreaterThanEqual<std::optional<NonComparable>>);
static_assert(!HasOperatorGreaterThanEqual<std::optional<EqualityComparable>>);
static_assert(!HasOperatorGreaterThanEqual<std::optional<ThreeWayComparable>, std::optional<NonComparable>>);
+static_assert(!HasOperatorGreaterThanEqual<std::optional<void*>, std::optional<int>>);
+static_assert(!HasOperatorGreaterThanEqual<std::optional<int>, std::optional<void*>>);
+
#endif
using std::optional;
diff --git a/libcxx/test/std/utilities/optional/optional.relops/greater_than.pass.cpp b/libcxx/test/std/utilities/optional/optional.relops/greater_than.pass.cpp
index f19ea23e8ae78..dd0e039d1d342 100644
--- a/libcxx/test/std/utilities/optional/optional.relops/greater_than.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.relops/greater_than.pass.cpp
@@ -26,6 +26,9 @@ static_assert(!HasOperatorGreaterThan<std::optional<NonComparable>>);
static_assert(!HasOperatorGreaterThan<std::optional<EqualityComparable>>);
static_assert(!HasOperatorGreaterThan<std::optional<ThreeWayComparable>, std::optional<NonComparable>>);
+static_assert(!HasOperatorGreaterThan<std::optional<void*>, std::optional<int>>);
+static_assert(!HasOperatorGreaterThan<std::optional<int>, std::optional<void*>>);
+
#endif
using std::optional;
diff --git a/libcxx/test/std/utilities/optional/optional.relops/less_equal.pass.cpp b/libcxx/test/std/utilities/optional/optional.relops/less_equal.pass.cpp
index bd81a79f1265e..9e472e158e3b5 100644
--- a/libcxx/test/std/utilities/optional/optional.relops/less_equal.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.relops/less_equal.pass.cpp
@@ -26,6 +26,9 @@ static_assert(!HasOperatorLessThanEqual<std::optional<NonComparable>>);
static_assert(!HasOperatorLessThanEqual<std::optional<EqualityComparable>>);
static_assert(!HasOperatorLessThanEqual<std::optional<ThreeWayComparable>, std::optional<NonComparable>>);
+static_assert(!HasOperatorLessThanEqual<std::optional<void*>, std::optional<int>>);
+static_assert(!HasOperatorLessThanEqual<std::optional<int>, std::optional<void*>>);
+
#endif
using std::optional;
diff --git a/libcxx/test/std/utilities/optional/optional.relops/less_than.pass.cpp b/libcxx/test/std/utilities/optional/optional.relops/less_than.pass.cpp
index c87bfd6f73e54..9c231dff56f51 100644
--- a/libcxx/test/std/utilities/optional/optional.relops/less_than.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.relops/less_than.pass.cpp
@@ -26,6 +26,9 @@ static_assert(!HasOperatorLessThan<std::optional<NonComparable>>);
static_assert(!HasOperatorLessThan<std::optional<EqualityComparable>>);
static_assert(!HasOperatorLessThan<std::optional<ThreeWayComparable>, std::optional<NonComparable>>);
+static_assert(!HasOperatorLessThan<std::optional<void*>, std::optional<int>>);
+static_assert(!HasOperatorLessThan<std::optional<int>, std::optional<void*>>);
+
#endif
using std::optional;
diff --git a/libcxx/test/std/utilities/optional/optional.relops/not_equal.pass.cpp b/libcxx/test/std/utilities/optional/optional.relops/not_equal.pass.cpp
index 1b7d2621c9193..6507a61426f87 100644
--- a/libcxx/test/std/utilities/optional/optional.relops/not_equal.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.relops/not_equal.pass.cpp
@@ -29,6 +29,9 @@ static_assert(HasOperatorNotEqual<std::optional<EqualityComparable>, std::option
static_assert(!HasOperatorNotEqual<std::optional<NonComparable>>);
static_assert(!HasOperatorNotEqual<std::optional<EqualityComparable>, std::optional<NonComparable>>);
+static_assert(!HasOperatorNotEqual<std::optional<void*>, std::optional<int>>);
+static_assert(!HasOperatorNotEqual<std::optional<int>, std::optional<void*>>);
+
#endif
using std::optional;
|
HumanThe2nd
changed the title
|
Ping |
842c78d to
e6fec6c
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
e6fec6c to
6d91f7f
Compare
|
@HumanThe2nd Please resolve the merge conflict. The culprit PR: https://github.com/llvm/llvm-project/pull/206644/changes |
6d91f7f to
2ca9ce4
Compare
246e541 to
7e10f5c
Compare
|
Ping |
Have you tested the changes individually to see the effects? While on the surface the changes might look OK, my concerns are that this PR is not quite ready yet but I am not quite sure what needs to be done next. |
I tested the affected files on a local build though llvm-lit, which all passed. I recall it failed on libunwind which this PR doesn't touch. Could a rerun on the checks please be done? |
|
I tested the code both with and without the change, and the new tests do pass in both cases. Though, I also wasn't able to reproduce the ambiguity from the documented example or similar cases, suggesting it's resolved elsewhere, possibly clang's template partial ordering. So this PR mandates wording of LWG4072, but doesn't fix a reproducible bug as originally suggested. Would appreciate thoughts on how to proceed, thanks. |
|
Hmm, LWG4072 looks like a workaround for some compiler bug or a defect in the C++ core language. I attempted to remove the resolution of LWG4072 in libstdc++'s |
Ping, Since the issue doesn't reproduce on GCC as documented above, I could implement this as a consistency patch without a test, or use an unconventional method to test it. could someone please provide preferred actions? |
|
I'd ask @jwakely what was wrong when initially P2944R3 in libc++. Was there actually infinite meta-recursion without LWG4072? |
|
No, not infinite meta-recursion. But without LWG4072 the overload set is changed in undesirable ways. Here's a very contrived example showing how it leads to an error outside the immediate context, by considering #include <optional>
#include <type_traits>
struct A { };
template<typename T> requires std::is_class_v<T>
bool
operator==(const A& a, const T& t)
{
return t.equals(a);
}
template<typename T>
bool try_cmp()
{
std::optional<A> a;
std::optional<T> i;
if constexpr (requires { a == i; })
return a == i;
return false;
}
int main()
{
try_cmp<int>();
}With the changes to Maybe a better solution would have been to make But that's not what P2944R3 did. Instead it made it get constrained away, and then the |
|
Even if you consider my example to be too contrived (the If we constrain the best match, what do the other overloads do? In this case, P2944R3 meant that we started to use the other overloads for comparing two optionals, and those other overloads were never meant to be used in that case. |
|
@HumanThe2nd I'm implementing LWG4072 for MSVC STL in microsoft/STL#6424 now. I think you can copy these relative test cases. @jwakely Thanks again for finding these cases out! |
b85bb14 to
0588b58
Compare
Thank you for providing the references and explanations. I've integrated them into this PR, attempting to match the architecture here. (credit @frederick-vs-ja and @jwakely) The local tests fail all assertions without the LWG4072 changes and pass with them included. If someone could review again, it would be appreciated. |
14be67c to
1b752a9
Compare
1b752a9 to
d5e7d74
Compare
d5e7d74 to
46441a1
Compare
d5d4f72 to
289e7f5
Compare
289e7f5 to
74501e6
Compare
frederick-vs-ja
left a comment
frederick-vs-ja
left a comment
There was a problem hiding this comment.
Finally, let's change this line as the following.
"`LWG4072 <https://wg21.link/LWG4072>`__","``std::optional`` comparisons: constrain harder","2024-11 (Wrocław)","|Complete|","24","`#118345 <https://github.com/llvm/llvm-project/issues/118345>`__",""
As required by the LLVM Project's AI use policy:
- The fix and test processes were revised and verified with AI assistance.
Also, is there any part of this PR still using the original AI generation? If no, let's drop this from the PR description which will be used as squashed commit message by default.
I used AI to check my work and fix typos/mistakes. Should I still emit it from the initial comment? |
I think this should be fine... I slightly reworded the PR description, mostly avoided duplicating of the LWG issue title. Please double-check my changes to the description. Feel free to furtherly reword it. |
Thank you again for reviewing it, I think it looks good now. I don't have permissions to merge as a new contributor. Would you like any other actions from me? |
|
I think we can merge this PR now. Would you mind to make you mail address public on GitHub? If the commit isn't associated with the proper mail address, there will possibly be weird reply messages from bots in your future PR for LLVM. |
a6cdcb3 to
15af8d4
Compare
|
I reverted it to the newest version before the mistake and set my email to public but can't confirm it from my side. Would it be possible for you to verify please? Sorry for the trouble. |
|
The mail address seems OK now. Let's wait for CI becoming green. |
frederick-vs-ja
added
the
pending-ci
frederick-vs-ja
removed
the
pending-ci
|
@HumanThe2nd Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
5 participants
Resolves #118345.
Previously, the heterogeneous comparison operators (
==,!=,<,<=,>,>=) for argumentsTandUdon't check thatTorUare not astd::optionalthemselves. This allowed the operators to cause ambiguous overload resolution instead of falling back tooptional's own operators, which possibly caused hard error when there should not be a matched overload.This patch implements the resolution by adding
!__is_std_optional_v<_Up>(and the_Tpequivalent for the reversed) to the constraints in all twelve involved operators.As required by the LLVM Project's AI use policy: