release/22.x: [libc++] Disable mistakenly enabled optional<T&> constructors for optional<T> (#194446)#195472
Merged
Merged
Conversation
Member
Author
|
@frederick-vs-ja What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-libcxx Author: llvmbot ChangesBackport 239189c Requested by: @frederick-vs-ja Full diff: https://github.com/llvm/llvm-project/pull/195472.diff 5 Files Affected:
diff --git a/libcxx/include/optional b/libcxx/include/optional
index 12fbcdfa5c5d6..4d85ac1e0ff5e 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -828,6 +828,10 @@ private:
template <class _Up>
constexpr static bool __libcpp_opt_ref_ctor_deleted =
is_lvalue_reference_v<_Tp> && reference_constructs_from_temporary_v<_Tp, _Up>;
+
+ template <class _Up>
+ constexpr static bool __ref_ctor_enabled =
+ is_lvalue_reference_v<_Tp> && !reference_constructs_from_temporary_v<_Tp, _Up>;
# endif
// LWG2756: conditionally explicit conversion from _Up
@@ -935,7 +939,7 @@ public:
template <class _Up, enable_if_t<_CheckOptionalLikeCtor<_Up, _Up const&>::template __enable_implicit<_Up>(), int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 optional(const optional<_Up>& __v)
# if _LIBCPP_STD_VER >= 26
- noexcept(is_lvalue_reference_v<_Tp> && is_nothrow_constructible_v<_Tp&, _Up&>)
+ noexcept(is_lvalue_reference_v<_Tp> && is_nothrow_constructible_v<_Tp&, const _Up&>)
# endif
{
this->__construct_from(__v);
@@ -943,7 +947,7 @@ public:
template <class _Up, enable_if_t<_CheckOptionalLikeCtor<_Up, _Up const&>::template __enable_explicit<_Up>(), int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit optional(const optional<_Up>& __v)
# if _LIBCPP_STD_VER >= 26
- noexcept(is_lvalue_reference_v<_Tp> && is_nothrow_constructible_v<_Tp&, _Up&>)
+ noexcept(is_lvalue_reference_v<_Tp> && is_nothrow_constructible_v<_Tp&, const _Up&>)
# endif
{
this->__construct_from(__v);
@@ -981,25 +985,25 @@ public:
// optional(optional<U>& rhs)
template <class _Up>
- requires(!__libcpp_opt_ref_ctor_deleted<_Up>) && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
- (!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, _Up&>
+ requires __ref_ctor_enabled<_Up&> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) && (!is_same_v<_Tp&, _Up>) &&
+ is_constructible_v<_Tp&, _Up&>
_LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_Up&, _Tp&>)
optional(optional<_Up>& __rhs) noexcept(is_nothrow_constructible_v<_Tp&, _Up&>) {
this->__construct_from(__rhs);
}
template <class _Up>
- requires __libcpp_opt_ref_ctor_deleted<_Up> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
+ requires __libcpp_opt_ref_ctor_deleted<_Up&> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
(!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, _Up&>
constexpr explicit optional(optional<_Up>& __rhs) noexcept = delete;
// optional(const optional<U>&)
template <class _Up, enable_if_t<_CheckOptionalLikeCtor<_Up, _Up const&>::template __enable_implicit<_Up>(), int> = 0>
- requires __libcpp_opt_ref_ctor_deleted<_Up>
+ requires __libcpp_opt_ref_ctor_deleted<const _Up&>
optional(const optional<_Up>&) = delete;
template <class _Up, enable_if_t<_CheckOptionalLikeCtor<_Up, _Up const&>::template __enable_explicit<_Up>(), int> = 0>
- requires __libcpp_opt_ref_ctor_deleted<_Up>
+ requires __libcpp_opt_ref_ctor_deleted<const _Up&>
explicit optional(const optional<_Up>&) = delete;
// optional(optional<U>&&)
@@ -1013,16 +1017,16 @@ public:
// optional(const optional<U>&&)
template <class _Up>
- requires(!__libcpp_opt_ref_ctor_deleted<_Up>) && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
- (!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, _Up>
+ requires __ref_ctor_enabled<const _Up> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
+ (!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, const _Up>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit(!is_convertible_v<const _Up, _Tp&>)
optional(const optional<_Up>&& __v) noexcept(is_nothrow_constructible_v<_Tp&, const _Up>) {
this->__construct_from(std::move(__v));
}
template <class _Up>
- requires __libcpp_opt_ref_ctor_deleted<_Up> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
- (!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, _Up>
+ requires __libcpp_opt_ref_ctor_deleted<const _Up> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
+ (!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, const _Up>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 optional(const optional<_Up>&& __v) noexcept = delete;
# endif
diff --git a/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/const_optional_U.pass.cpp b/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/const_optional_U.pass.cpp
index 597110bcb54fe..844317c8570d9 100644
--- a/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/const_optional_U.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/const_optional_U.pass.cpp
@@ -151,6 +151,37 @@ constexpr bool test_ref() {
assert(*o2 == 1);
}
+ {
+ const std::optional<int> o1{2};
+ std::optional<const int&> o2(o1);
+ assert(*o2 == 2);
+ assert(&(*o2) == &(*o1));
+ }
+
+ {
+ const std::optional<ReferenceConversion<int>> o1({1, 2});
+ std::optional<const int&> o2(o1);
+ assert(o2.has_value());
+ assert(&(*o2) == &o1->lvalue);
+ assert(*o2 == 1);
+ }
+
+ {
+ const std::optional<ReferenceConversion<int>> o1({1, 2});
+ std::optional<const int&> o2(std::move(o1));
+ assert(o2.has_value());
+ assert(&(*o2) == &o1->rvalue);
+ assert(*o2 == 2);
+ }
+
+ {
+ const std::optional<ReferenceConversionThrows<int>> o1({1, 2});
+ ASSERT_NOT_NOEXCEPT(std::optional<const int&>(o1));
+ ASSERT_NOT_NOEXCEPT(std::optional<const int&>(std::move(o1)));
+ }
+
+ static_assert(!std::is_constructible_v<std::optional<int>, const std::optional<ConstRValueOnly<int>>&&>);
+
return true;
}
#endif
diff --git a/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/optional_U.pass.cpp b/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/optional_U.pass.cpp
index 39a80efa475f7..03082033cd4dc 100644
--- a/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/optional_U.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/optional_U.pass.cpp
@@ -144,6 +144,36 @@ constexpr bool test_ref() {
assert(*o2 == 1);
}
+ {
+ std::optional<int> o1(1);
+ std::optional<const int&> o2(o1);
+ assert(o2.has_value());
+ assert(*o2 == 1);
+ assert(&(*o2) == &(*o1));
+ }
+
+ {
+ std::optional<ReferenceConversion<int>> o1({1, 2});
+ std::optional<const int&> o2(o1);
+ assert(o2.has_value());
+ assert(*o2 == 1);
+ assert(&(*o2) == &o1->lvalue);
+ }
+
+ {
+ std::optional<ReferenceConversion<int>> o1({1, 2});
+ std::optional<const int&> o2(std::move(o1));
+ assert(o2.has_value());
+ assert(*o2 == 2);
+ assert(&(*o2) == &o1->rvalue);
+ }
+
+ {
+ std::optional<ReferenceConversionThrows<int>> o1({1, 2});
+ ASSERT_NOT_NOEXCEPT(std::optional<const int&>(o1));
+ ASSERT_NOT_NOEXCEPT(std::optional<const int&>(std::move(o1)));
+ }
+
return true;
}
#endif
@@ -164,6 +194,11 @@ int main(int, char**) {
test<Z>(std::move(rhs), true);
}
+#if TEST_STD_VER >= 26
+ // GH: #194415
+ static_assert(!std::is_constructible_v<std::optional<int>, std::optional<LValueOnly<int>>&>);
+#endif
+
static_assert(!(std::is_constructible<optional<X>, optional<Z>>::value), "");
#if TEST_STD_VER >= 26
diff --git a/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ref_constructs_from_temporary.verify.cpp b/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ref_constructs_from_temporary.verify.cpp
index 53ac77a2e5620..d4c95c302d817 100644
--- a/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ref_constructs_from_temporary.verify.cpp
+++ b/libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ref_constructs_from_temporary.verify.cpp
@@ -29,12 +29,10 @@ void test() {
const std::optional<int> co(1);
std::optional<int> o0(1);
- // expected-error-re@*:* 10 {{call to deleted constructor of 'std::optional<{{.*}}>'}}
+ // expected-error-re@*:* 8 {{call to deleted constructor of 'std::optional<{{.*}}>'}}
std::optional<const int&> o1{1}; // optional(U&&)
- std::optional<const int&> o2{o0}; // optional(optional<U>&)
- std::optional<const int&> o3{co}; // optional(const optional<U>&)
- std::optional<const int&> o4{std::move(o0)}; // optional(optional<U>&&&)
- std::optional<const int&> o5{std::move(co)}; // optional(optional<U>&&&)
+ std::optional<const int&> o4{std::move(o0)}; // optional(optional<U>&&)
+ std::optional<const int&> o5{std::move(co)}; // optional(optional<U>&&)
std::optional<const X&> o6{1}; // optional(U&&)
std::optional<const X&> o7{o0}; // optional(optional<U>&)
diff --git a/libcxx/test/std/utilities/optional/optional.object/optional_helper_types.h b/libcxx/test/std/utilities/optional/optional.object/optional_helper_types.h
index 704d99acb15e9..d9b17a0b91f3b 100644
--- a/libcxx/test/std/utilities/optional/optional.object/optional_helper_types.h
+++ b/libcxx/test/std/utilities/optional/optional.object/optional_helper_types.h
@@ -21,8 +21,9 @@ struct ReferenceConversion {
constexpr ReferenceConversion(T lval, T rval) : lvalue(lval), rvalue(rval) {}
constexpr operator T&() & noexcept { return lvalue; }
-
+ constexpr operator const T&() const& noexcept { return lvalue; }
constexpr operator T&() && noexcept { return rvalue; }
+ constexpr operator const T&() const&& noexcept { return rvalue; }
};
template <typename T>
@@ -42,6 +43,14 @@ struct ReferenceConversionThrows {
return lvalue;
}
+ constexpr operator const T&() const& {
+ if (throws) {
+ TEST_THROW(1);
+ }
+
+ return lvalue;
+ }
+
constexpr operator T&() && {
if (throws) {
TEST_THROW(2);
@@ -49,6 +58,34 @@ struct ReferenceConversionThrows {
return rvalue;
}
+
+ constexpr operator const T&() const&& {
+ if (throws) {
+ TEST_THROW(2);
+ }
+
+ return rvalue;
+ }
+};
+
+template <typename T>
+struct LValueOnly {
+ T val{};
+
+ constexpr operator T&() & noexcept { return val; }
+ constexpr operator T&() const& = delete;
+ constexpr operator T&() && = delete;
+ constexpr operator T&() const&& = delete;
+};
+
+template <typename T>
+struct ConstRValueOnly {
+ mutable T val{};
+
+ constexpr operator T&() & = delete;
+ constexpr operator T&() const& = delete;
+ constexpr operator T&() && = delete;
+ constexpr operator T&() const&& { return val; };
};
#endif
|
e9556ad to
141ebc5
Compare
frederick-vs-ja
approved these changes
May 3, 2026
frederick-vs-ja
left a comment
Contributor
There was a problem hiding this comment.
LGTM as-is. Although I have no idea why CI doesn't run yet.
Contributor
|
@ldionne as the maintainer for libcxx, what do you think about taking this fix in the 22.x release branch? I'm not too familiar with libcxx, so would like the opinion of a maintainer whether this is a critical bug fix or serious enough that it should be backported. |
Contributor
|
ping @ldionne |
Contributor
|
ping @ldionne, do you approve of this change being backported to 22.x? |
Contributor
|
ping. We have only less than a week before the 22.x branch is closed. |
ldionne
approved these changes
Jun 15, 2026
ldionne
left a comment
Member
There was a problem hiding this comment.
Apologies for missing the repeated pings. I think we should cherry-pick this change.
…optional<T>` (llvm#194446) Resolves llvm#194415 - A constructor specifically meant for `optional<T&>` was left enabled for `optional<T>` - Fix it, and add a test to check for regression. - This patch also corrects the constraints for `optional(optional<U>&)` and `optional(const optional<U>&)` , as they were incorrectly disallowing [valid conversions](https://godbolt.org/z/1r5Ea7z5M) - Also, correct the `noexcept` specification. - Add tests for both corrections. (cherry picked from commit 239189c)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport 239189c
Requested by: @frederick-vs-ja