[libc++] Fix std::optional-related type deduction

[ianloic]

Summary

When clang a constructor invocation where there are single-argument
constructors that take either std::optional or some other type the
checks in _CheckOptionalArgsConstructor fail becaues that helper tries
(and fails) to take an rvalue-reference to void.

A simple example is here: https://godbolt.org/z/bPdzYfcnv
This works just fine with other C++ standard library implementations.

This change uses std::add_rvalue_reference_t<T> rather than T&& to avoid
the compilation error.

This fixes bug https://github.com/llvm/llvm-project/issues/61692 and
maybe https://github.com/llvm/llvm-project/issues/48959.

[phosek]

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ambigous_constructor.pass.cpp

30

Nit: please make sure there's a newline at the end of the file.

[ianloic]

Added trailing blank line.

[philnik]

This changes behaviour, in a not necessarily conforming way. I don't know why other standard library implementations accept your construct while libc++ doesn't, but this is definitely not the right fix. Interestingly, implementing the constructor yourself works with one standard but not the other: https://godbolt.org/z/GKKGP46fs, and both bug reports use std::string. Maybe there lies the real issue? You should probably reduce the reproducer further to find out where the problem actually is.

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ambigous_constructor.pass.cpp

20–21

Please don't use using in the tests.

34

Please don't use reserved identifiers in the tests.

[ianloic]

Reduce test case, address comments.

[ianloic]

In D147560#4244469, @philnik wrote:

This changes behaviour, in a not necessarily conforming way. I don't know why other standard library implementations accept your construct while libc++ doesn't, but this is definitely not the right fix. Interestingly, implementing the constructor yourself works with one standard but not the other: https://godbolt.org/z/GKKGP46fs, and both bug reports use std::string. Maybe there lies the real issue? You should probably reduce the reproducer further to find out where the problem actually is.

I can make a test-case without std::string: https://godbolt.org/z/Edr58onYP
but I'm still working on understanding more deeply what's going on with it. I'm trying to reduce the std::optional implementation to find where it's really going wrong.

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ambigous_constructor.pass.cpp

20–21

No worries, I was copying the style of a neighboring test.

[ianloic]

A different approach

[EricWF]

I think this is a bandaid over a bigger clang bug.

I don't want to accept a patch to placate clang here until we know what's actually going on.

[ianloic]

The underling clang issue has been fixed.

libcxx/include/optional

static_assert(!is_reference_v<value_type>,
         "instantiation of optional with a reference type is ill-formed");
     static_assert(is_destructible_v<value_type>,
         "instantiation of optional with a non-destructible type is ill-formed");
     static_assert(!is_array_v<value_type>,
         "instantiation of optional with an array type is ill-formed");
 
     // LWG2756: conditionally explicit conversion from _Up
     struct _CheckOptionalArgsConstructor {
-      template <class _Up>
+      template <class _Up, class _UpRR = add_rvalue_reference_t<_Up>>
       static constexpr bool __enable_implicit() {
-          return is_constructible_v<_Tp, _Up&&> &&
-                 is_convertible_v<_Up&&, _Tp>;
+          return is_constructible_v<_Tp, _UpRR> &&
+                 is_convertible_v<_UpRR, _Tp>;
       }
 
-      template <class _Up>
+      template <class _Up, class _UpRR = add_rvalue_reference_t<_Up>>
       static constexpr bool __enable_explicit() {
-          return is_constructible_v<_Tp, _Up&&> &&
-                 !is_convertible_v<_Up&&, _Tp>;
+          return is_constructible_v<_Tp, _UpRR> &&
+                 !is_convertible_v<_UpRR, _Tp>;
       }
     };
     template <class _Up>
     using _CheckOptionalArgsCtor = _If<
         _IsNotSame<__remove_cvref_t<_Up>, in_place_t>::value &&
         _IsNotSame<__remove_cvref_t<_Up>, optional>::value,
         _CheckOptionalArgsConstructor,
         __check_tuple_constructor_fail

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/ambigous_constructor.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+// <optional>
+
+#include <optional>
+
+#include "test_macros.h"
+
+struct Bar {
+  int s;
+};
+
+struct Foo {
+  Foo(Bar) {}
+  Foo(std::optional<int>) {}
+};
+
+int main(int, char**) {
+  Foo foo{{.s = 42}};
+
+  return 0;
+}