Skip to content

[libc++] Disable mistakenly enabled optional<T&> constructors for optional<T>#194446

Merged
frederick-vs-ja merged 5 commits into
llvm:mainfrom
smallp-o-p:optional-#194415
Apr 29, 2026
Merged

[libc++] Disable mistakenly enabled optional<T&> constructors for optional<T>#194446
frederick-vs-ja merged 5 commits into
llvm:mainfrom
smallp-o-p:optional-#194415

Conversation

@smallp-o-p

@smallp-o-p smallp-o-p commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Resolves #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
  • Also, correct the noexcept specification.
  • Add tests for both corrections.

@smallp-o-p smallp-o-p requested a review from a team as a code owner April 27, 2026 19:47
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 27, 2026
@llvmbot

llvmbot commented Apr 27, 2026

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-libcxx

Author: William Tran-Viet (smallp-o-p)

Changes

Resolves #194415

  • A constructor specifically meant for optional&lt;T&amp;&gt; was left enabled for optional&lt;T&gt;
  • Fix it, and add a test to check for regression.

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

3 Files Affected:

  • (modified) libcxx/include/optional (+8-4)
  • (modified) libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/optional_U.pass.cpp (+5)
  • (modified) libcxx/test/std/utilities/optional/optional.object/optional_helper_types.h (+10)
diff --git a/libcxx/include/optional b/libcxx/include/optional
index 9eed886aa4636..f1a2c5567a49a 100644
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -893,6 +893,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 __libcpp_enable_opt_ref_ctor =
+      is_lvalue_reference_v<_Tp> && !reference_constructs_from_temporary_v<_Tp, _Up>;
 #    endif
 
   // LWG2756: conditionally explicit conversion from _Up
@@ -1046,8 +1050,8 @@ 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 __libcpp_enable_opt_ref_ctor<_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);
@@ -1078,8 +1082,8 @@ 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 __libcpp_enable_opt_ref_ctor<_Up> && (!is_same_v<remove_cvref_t<_Tp>, optional<_Up>>) &&
+             (!is_same_v<_Tp&, _Up>) && is_constructible_v<_Tp&, _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));
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..ce86193f5ea4e 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
@@ -164,6 +164,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_helper_types.h b/libcxx/test/std/utilities/optional/optional.object/optional_helper_types.h
index 704d99acb15e9..e119a32bd5522 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
@@ -51,4 +51,14 @@ struct ReferenceConversionThrows {
   }
 };
 
+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;
+};
+
 #endif

@smallp-o-p smallp-o-p changed the title [libc++] Disable optional<T&> constructor for optional<T> [libc++] Disable mistakenly enabled optional<T&> constructors for optional<T> Apr 27, 2026
@smallp-o-p smallp-o-p changed the title [libc++] Disable mistakenly enabled optional<T&> constructors for optional<T> [libc++] Disable mistakenly enabled optional<T&> constructors for optional<T> Apr 28, 2026
Comment thread libcxx/include/optional Outdated
@frederick-vs-ja frederick-vs-ja merged commit 239189c into llvm:main Apr 29, 2026
80 checks passed
@Zingam

Zingam commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

This issue looks serious enough. Should this be backported to LLVM22?

kirthana14m pushed a commit to ROCm/llvm-project that referenced this pull request Apr 30, 2026
…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.
@smallp-o-p

Copy link
Copy Markdown
Contributor Author

This issue looks serious enough. Should this be backported to LLVM22?

I have no objections to that.

@frederick-vs-ja

frederick-vs-ja commented May 2, 2026

Copy link
Copy Markdown
Contributor

Let's backport this.

@frederick-vs-ja

Copy link
Copy Markdown
Contributor

/cherry-pick 239189c

@smallp-o-p

smallp-o-p commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

/cherry-pick 239189c

I also identified one last conformance issue relating to optional<T&> constructor constraints. I plan on fixing it early this week, is there anything special I need to worry about to get that future commit into the 22.x minor release, preferably with this one?

@llvmbot

llvmbot commented May 2, 2026

Copy link
Copy Markdown
Member

/pull-request #195472

@frederick-vs-ja

Copy link
Copy Markdown
Contributor

I also identified one last conformance issue relating to optional<T&> constructor constraints. I plan on fixing it early this week, is there anything special I need to worry about to get that future commit into the 22.x minor release, preferably with this one?

No IMO. I think it's fine to open a new PR and then separately backport it.

dyung pushed a commit to llvmbot/llvm-project that referenced this pull request Jun 15, 2026
…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)
daunabomba pushed a commit to daunabomba/llvm-project that referenced this pull request Jun 17, 2026
…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)
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

Development

Successfully merging this pull request may close these issues.

[libc++] incorrect constructor in std::optional::optional in C++26

4 participants