[libcxx-commits] [libcxx] 16f3492 - [libc++] restrict the expected conversion constructor not compete against copy constructor (#96101)

[via libcxx-commits]

Author: Hui
Date: 2024-06-26T12:10:59+01:00
New Revision: 16f349251fabacfdba4acac3b25baf0e6890c1a0

URL: https://github.com/llvm/llvm-project/commit/16f349251fabacfdba4acac3b25baf0e6890c1a0
DIFF: https://github.com/llvm/llvm-project/commit/16f349251fabacfdba4acac3b25baf0e6890c1a0.diff

LOG: [libc++] restrict the expected conversion constructor not compete against copy constructor (#96101)

fixes #92676

So right now clang does not like 
```
std::expected<std::any, int> e1;
auto e2 = e1;

```

So basically when clang tries to do overload resolution of `auto e2 =
e1;`

It finds

```
expected(const expected&);  // 1. This is OK
expected(const expected<_Up, _OtherErr>&)  requires __can_convert; // 2. This needs to check its constraints
```

Then in `__can_convert`, one of the check is 

```
_Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>
```
which is checking 
```
is_constructible<std::any, expected<_Up, _OtherErr>&>
```

Then it looks at `std::any`'s constructor
```
  template < class _ValueType,
             class _Tp = decay_t<_ValueType>,
             class     = enable_if_t< !is_same<_Tp, any>::value && !__is_inplace_type<_ValueType>::value &&
                                      is_copy_constructible<_Tp>::value> >
  any(_ValueType&& __value);
```
In the above, `is_copy_constructible<_Tp>` expands to
```
is_copy_constructible<std::expected<std::any, int>>
```

And the above goes back to the original thing we asked : copy the
`std::expected`, which goes to the overload resolution again.

```
expected(const expected&);
expected(const expected<_Up, _OtherErr>&)  requires __can_convert;
```

So the second overload results in a logical cycle. 

I am not a language lawyer. We could argue that clang should give up on
the second overload which has logical cycle, as the first overload is a
perfect match.

Anyway, the fix in this patch tries to short-circuiting the second
overload's constraint check: that is, if the argument matches exact same
`expected<T, E>`, we give up immediately and let the copy constructor to
deal with it

Added: 
    

Modified: 
    libcxx/include/__expected/expected.h
    libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 0f994e297a877..f618b20603e60 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -507,7 +507,9 @@ class expected : private __expected_base<_Tp, _Err> {
       _And< is_constructible<_Tp, _UfQual>,
             is_constructible<_Err, _OtherErrQual>,
             _If<_Not<is_same<remove_cv_t<_Tp>, bool>>::value,
-                _And< _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>,
+                _And< 
+                      _Not<_And<is_same<_Tp, _Up>, is_same<_Err, _OtherErr>>>, // use the copy constructor instead, see #92676
+                      _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>,
                       _Not<is_constructible<_Tp, expected<_Up, _OtherErr>>>,
                       _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>&>>,
                       _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>>>,

diff  --git a/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp
index 581df51207da2..ba9831750c842 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp
@@ -62,6 +62,16 @@ static_assert(!std::is_trivially_copy_constructible_v<std::expected<CopyableNonT
 static_assert(!std::is_trivially_copy_constructible_v<std::expected<int, CopyableNonTrivial>>);
 static_assert(!std::is_trivially_copy_constructible_v<std::expected<CopyableNonTrivial, CopyableNonTrivial>>);
 
+struct Any {
+  constexpr Any()                      = default;
+  constexpr Any(const Any&)            = default;
+  constexpr Any& operator=(const Any&) = default;
+
+  template <class T>
+    requires(!std::is_same_v<Any, std::decay_t<T>> && std::is_copy_constructible_v<std::decay_t<T>>)
+  constexpr Any(T&&) {}
+};
+
 constexpr bool test() {
   // copy the value non-trivial
   {
@@ -109,6 +119,16 @@ constexpr bool test() {
     assert(!e2.has_value());
   }
 
+  {
+    // TODO(LLVM 20): Remove once we drop support for Clang 17
+#if defined(TEST_CLANG_VER) && TEST_CLANG_VER >= 1800
+    // https://github.com/llvm/llvm-project/issues/92676
+    std::expected<Any, int> e1;
+    auto e2 = e1;
+    assert(e2.has_value());
+#endif
+  }
+
   return true;
 }