[SemaCXX] Implement CWG2137 (list-initialization from objects of the same type)

[MitalAshok]

Closes #77638, #24186

Rebased from https://reviews.llvm.org/D156032, see there for more information.

Implements wording change in CWG2137 in the first commit.

This also implements an approach to CWG2311 in the second commit, because too much code that relies on T{ T_prvalue } being an elision would break. Because that issue is still open and the CWG issue doesn't provide wording to fix the issue, there may be different behaviours on other compilers.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-libcxx

Author: Mital Ashok (MitalAshok)

Changes

Closes #77638, #24186

Rebased from <https://reviews.llvm.org/D156032>, see there for more information.

Implements wording change in CWG2137 in the first commit.

This also implements an approach to CWG2311 in the second commit, because too much code that relies on T{ T_prvalue } being an elision would break. Because that issue is still open and the CWG issue doesn't provide wording to fix the issue, there may be different behaviours on other compilers.

---

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

8 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+5)
• (modified) clang/lib/Sema/SemaInit.cpp (+30-10)
• (modified) clang/lib/Sema/SemaOverload.cpp (+28-10)
• (modified) clang/test/CXX/drs/dr14xx.cpp (-10)
• (modified) clang/test/CXX/drs/dr21xx.cpp (+40)
• (modified) clang/test/CXX/drs/dr23xx.cpp (+84)
• (modified) clang/www/cxx_dr_status.html (+2-2)
• (modified) libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp (+20-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 59732962caac65..66d388bb954bda 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -199,6 +199,11 @@ C++2c Feature Support
 
 Resolutions to C++ Defect Reports
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Implemented `CWG2137 <https://wg21.link/CWG2137>`_ which allows
+  list-initialization from objects of the same type.
+- Implemented `CWG2311 <https://wg21.link/CWG2311>`_: given a prvalue ``e`` of object type
+  ``T``, ``T{e}`` will try to resolve an initializer list constructor and will use it if successful (CWG2137).
+  Otherwise, if there is no initializer list constructor, the copy will be elided as if it was ``T(e)``.
 
 C Language Changes
 ------------------
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 408ee5f775804b..2822a8d5d78b3e 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -4199,7 +4199,7 @@ static OverloadingResult ResolveConstructorOverload(
 /// \param IsListInit     Is this list-initialization?
 /// \param IsInitListCopy Is this non-list-initialization resulting from a
 ///                       list-initialization from {x} where x is the same
-///                       type as the entity?
+///                       aggregate type as the entity?
 static void TryConstructorInitialization(Sema &S,
                                          const InitializedEntity &Entity,
                                          const InitializationKind &Kind,
@@ -4229,6 +4229,14 @@ static void TryConstructorInitialization(Sema &S,
         Entity.getKind() !=
             InitializedEntity::EK_LambdaToBlockConversionBlockElement);
 
+  bool CopyElisionPossible = false;
+  auto ElideConstructor = [&] {
+    // Convert qualifications if necessary.
+    Sequence.AddQualificationConversionStep(DestType, VK_PRValue);
+    if (ILE)
+      Sequence.RewrapReferenceInitList(DestType, ILE);
+  };
+
   // C++17 [dcl.init]p17:
   //     - If the initializer expression is a prvalue and the cv-unqualified
   //       version of the source type is the same class as the class of the
@@ -4241,11 +4249,17 @@ static void TryConstructorInitialization(Sema &S,
   if (S.getLangOpts().CPlusPlus17 && !RequireActualConstructor &&
       UnwrappedArgs.size() == 1 && UnwrappedArgs[0]->isPRValue() &&
       S.Context.hasSameUnqualifiedType(UnwrappedArgs[0]->getType(), DestType)) {
-    // Convert qualifications if necessary.
-    Sequence.AddQualificationConversionStep(DestType, VK_PRValue);
-    if (ILE)
-      Sequence.RewrapReferenceInitList(DestType, ILE);
-    return;
+    if (ILE && !DestType->isAggregateType()) {
+      // CWG2311: T{ prvalue_of_type_T } is not eligible for copy elision
+      // Make this an elision if this won't call an initializer-list
+      // constructor. (Always on an aggregate type or check constructors first.)
+      assert(!IsInitListCopy &&
+             "IsInitListCopy only possible with aggregate types");
+      CopyElisionPossible = true;
+    } else {
+      ElideConstructor();
+      return;
+    }
   }
 
   const RecordType *DestRecordType = DestType->getAs<RecordType>();
@@ -4290,6 +4304,12 @@ static void TryConstructorInitialization(Sema &S,
           S, Kind.getLocation(), Args, CandidateSet, DestType, Ctors, Best,
           CopyInitialization, AllowExplicit,
           /*OnlyListConstructors=*/true, IsListInit, RequireActualConstructor);
+
+    if (CopyElisionPossible && Result == OR_No_Viable_Function) {
+      // No initializer list candidate
+      ElideConstructor();
+      return;
+    }
   }
 
   // C++11 [over.match.list]p1:
@@ -4571,9 +4591,9 @@ static void TryListInitialization(Sema &S,
     return;
   }
 
-  // C++11 [dcl.init.list]p3, per DR1467:
-  // - If T is a class type and the initializer list has a single element of
-  //   type cv U, where U is T or a class derived from T, the object is
+  // C++11 [dcl.init.list]p3, per DR1467 and DR2137:
+  // - If T is an aggregate class and the initializer list has a single element
+  //   of type cv U, where U is T or a class derived from T, the object is
   //   initialized from that element (by copy-initialization for
   //   copy-list-initialization, or by direct-initialization for
   //   direct-list-initialization).
@@ -4584,7 +4604,7 @@ static void TryListInitialization(Sema &S,
   // - Otherwise, if T is an aggregate, [...] (continue below).
   if (S.getLangOpts().CPlusPlus11 && InitList->getNumInits() == 1 &&
       !IsDesignatedInit) {
-    if (DestType->isRecordType()) {
+    if (DestType->isRecordType() && DestType->isAggregateType()) {
       QualType InitType = InitList->getInit(0)->getType();
       if (S.Context.hasSameUnqualifiedType(InitType, DestType) ||
           S.IsDerivedFrom(InitList->getBeginLoc(), InitType, DestType)) {
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 64bc3851980272..30bc2036cf781c 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1562,19 +1562,37 @@ TryUserDefinedConversion(Sema &S, Expr *From, QualType ToType,
     //   called for those cases.
     if (CXXConstructorDecl *Constructor
           = dyn_cast<CXXConstructorDecl>(ICS.UserDefined.ConversionFunction)) {
-      QualType FromCanon
-        = S.Context.getCanonicalType(From->getType().getUnqualifiedType());
+      QualType FromType;
+      SourceLocation FromLoc;
+      // C++11 [over.ics.list]p6, per DR2137:
+      // C++17 [over.ics.list]p6:
+      //   If C is not an initializer-list constructor and the initializer list
+      //   has a single element of type cv U, where U is X or a class derived
+      //   from X, the implicit conversion sequence has Exact Match rank if U is
+      //   X, or Conversion rank if U is derived from X.
+      if (const auto *InitList = dyn_cast<InitListExpr>(From);
+          InitList && InitList->getNumInits() == 1 &&
+          !S.isInitListConstructor(Constructor)) {
+        const Expr *SingleInit = InitList->getInit(0);
+        FromType = SingleInit->getType();
+        FromLoc = SingleInit->getBeginLoc();
+      } else {
+        FromType = From->getType();
+        FromLoc = From->getBeginLoc();
+      }
+      QualType FromCanon =
+          S.Context.getCanonicalType(FromType.getUnqualifiedType());
       QualType ToCanon
         = S.Context.getCanonicalType(ToType).getUnqualifiedType();
       if (Constructor->isCopyConstructor() &&
           (FromCanon == ToCanon ||
-           S.IsDerivedFrom(From->getBeginLoc(), FromCanon, ToCanon))) {
+           S.IsDerivedFrom(FromLoc, FromCanon, ToCanon))) {
         // Turn this into a "standard" conversion sequence, so that it
         // gets ranked with standard conversion sequences.
         DeclAccessPair Found = ICS.UserDefined.FoundConversionFunction;
         ICS.setStandard();
         ICS.Standard.setAsIdentityConversion();
-        ICS.Standard.setFromType(From->getType());
+        ICS.Standard.setFromType(FromType);
         ICS.Standard.setAllToTypes(ToType);
         ICS.Standard.CopyConstructor = Constructor;
         ICS.Standard.FoundCopyConstructor = Found;
@@ -5320,18 +5338,18 @@ TryListConversion(Sema &S, InitListExpr *From, QualType ToType,
       IsDesignatedInit)
     return Result;
 
-  // Per DR1467:
-  //   If the parameter type is a class X and the initializer list has a single
-  //   element of type cv U, where U is X or a class derived from X, the
-  //   implicit conversion sequence is the one required to convert the element
-  //   to the parameter type.
+  // Per DR1467 and DR2137:
+  //   If the parameter type is an aggregate class X and the initializer list
+  //   has a single element of type cv U, where U is X or a class derived from
+  //   X, the implicit conversion sequence is the one required to convert the
+  //   element to the parameter type.
   //
   //   Otherwise, if the parameter type is a character array [... ]
   //   and the initializer list has a single element that is an
   //   appropriately-typed string literal (8.5.2 [dcl.init.string]), the
   //   implicit conversion sequence is the identity conversion.
   if (From->getNumInits() == 1 && !IsDesignatedInit) {
-    if (ToType->isRecordType()) {
+    if (ToType->isRecordType() && ToType->isAggregateType()) {
       QualType InitType = From->getInit(0)->getType();
       if (S.Context.hasSameUnqualifiedType(InitType, ToType) ||
           S.IsDerivedFrom(From->getBeginLoc(), InitType, ToType))
diff --git a/clang/test/CXX/drs/dr14xx.cpp b/clang/test/CXX/drs/dr14xx.cpp
index d262f6f9dcab79..4c29d03a6e117a 100644
--- a/clang/test/CXX/drs/dr14xx.cpp
+++ b/clang/test/CXX/drs/dr14xx.cpp
@@ -488,16 +488,6 @@ namespace dr1467 {  // dr1467: 3.7 c++11
     }
   } // nonaggregate
 
-  namespace SelfInitIsNotListInit {
-    struct S {
-      S();
-      explicit S(S &);
-      S(const S &);
-    };
-    S s1;
-    S s2 = {s1}; // ok, not list-initialization so we pick the non-explicit constructor
-  }
-
   struct NestedInit { int a, b, c; };
   NestedInit ni[1] = {{NestedInit{1, 2, 3}}};
 
diff --git a/clang/test/CXX/drs/dr21xx.cpp b/clang/test/CXX/drs/dr21xx.cpp
index a7e50df3f374be..9d2016262712d1 100644
--- a/clang/test/CXX/drs/dr21xx.cpp
+++ b/clang/test/CXX/drs/dr21xx.cpp
@@ -11,6 +11,16 @@
 // cxx98-error@-1 {{variadic macros are a C99 feature}}
 #endif
 
+namespace std {
+  __extension__ typedef __SIZE_TYPE__ size_t;
+
+  template<typename E> struct initializer_list {
+    const E *p; size_t n;
+    initializer_list(const E *p, size_t n);
+    initializer_list();
+  };
+}
+
 namespace dr2100 { // dr2100: 12
   template<const int *P, bool = true> struct X {};
   template<typename T> struct A {
@@ -132,6 +142,36 @@ namespace dr2126 { // dr2126: 12
 #endif
 }
 
+namespace dr2137 { // dr2137: 18
+#if __cplusplus >= 201103L
+  struct Q {
+    Q();
+    Q(Q&&);
+    Q(std::initializer_list<Q>) = delete; // expected-note 2 {{has been explicitly marked deleted here}}
+  };
+
+  Q x = Q { Q() }; // expected-error {{call to deleted constructor}}
+
+  int f(Q); // expected-note {{passing argument to parameter here}}
+  int y = f({ Q() }); // expected-error {{call to deleted constructor}}
+
+  struct U {
+    U();
+    U(const U&);
+  };
+
+  struct Derived : U {
+    Derived();
+    Derived(const Derived&);
+  } d;
+
+  int g(Derived);
+  int g(U(&&)[1]) = delete;
+
+  int z = g({ d });
+#endif
+}
+
 namespace dr2140 { // dr2140: 9
 #if __cplusplus >= 201103L
   union U { int a; decltype(nullptr) b; };
diff --git a/clang/test/CXX/drs/dr23xx.cpp b/clang/test/CXX/drs/dr23xx.cpp
index d2f4e7652ab568..8aa04c917846fa 100644
--- a/clang/test/CXX/drs/dr23xx.cpp
+++ b/clang/test/CXX/drs/dr23xx.cpp
@@ -6,6 +6,16 @@
 // RUN: %clang_cc1 -std=c++23 %s -verify=expected,since-cxx11,since-cxx14,since-cxx17,since-cxx20 -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -std=c++2c %s -verify=expected,since-cxx11,since-cxx14,since-cxx17,since-cxx20 -fexceptions -fcxx-exceptions -pedantic-errors 2>&1 | FileCheck %s
 
+namespace std {
+  __extension__ typedef __SIZE_TYPE__ size_t;
+
+  template<typename E> struct initializer_list {
+    const E *p; size_t n;
+    initializer_list(const E *p, size_t n);
+    initializer_list();
+  };
+}
+
 #if __cplusplus >= 201103L
 namespace dr2303 { // dr2303: 12
 template <typename... T>
@@ -47,6 +57,80 @@ void g() {
 } //namespace dr2303
 #endif
 
+namespace dr2311 {  // dr2311: 18 open
+#if __cplusplus >= 201707L
+template<typename T>
+void test() {
+  // Ensure none of these expressions try to call a move constructor.
+  T a = T{T(0)};
+  T b{T(0)};
+  auto c{T(0)};
+  T d = {T(0)};
+  auto e = {T(0)};
+#if __cplusplus >= 202302L
+  auto f = auto{T(0)};
+#endif
+  void(*fn)(T);
+  fn({T(0)});
+}
+
+struct NonMovable {
+  NonMovable(int);
+  NonMovable(NonMovable&&) = delete;
+};
+struct NonMovableNonApplicableIList {
+  NonMovableNonApplicableIList(int);
+  NonMovableNonApplicableIList(NonMovableNonApplicableIList&&) = delete;
+  NonMovableNonApplicableIList(std::initializer_list<int>);
+};
+struct ExplicitMovable {
+  ExplicitMovable(int);
+  explicit ExplicitMovable(ExplicitMovable&&);
+};
+struct ExplicitNonMovable {
+  ExplicitNonMovable(int);
+  explicit ExplicitNonMovable(ExplicitNonMovable&&) = delete;
+};
+struct ExplicitNonMovableNonApplicableIList {
+  ExplicitNonMovableNonApplicableIList(int);
+  explicit ExplicitNonMovableNonApplicableIList(ExplicitNonMovableNonApplicableIList&&) = delete;
+  ExplicitNonMovableNonApplicableIList(std::initializer_list<int>);
+};
+struct CopyOnly {
+  CopyOnly(int);
+  CopyOnly(const CopyOnly&);
+  CopyOnly(CopyOnly&&) = delete;
+};
+struct ExplicitCopyOnly {
+  ExplicitCopyOnly(int);
+  explicit ExplicitCopyOnly(const ExplicitCopyOnly&);
+  explicit ExplicitCopyOnly(ExplicitCopyOnly&&) = delete;
+};
+
+template void test<NonMovable>();
+template void test<NonMovableNonApplicableIList>();
+template void test<ExplicitMovable>();
+template void test<ExplicitNonMovable>();
+template void test<ExplicitNonMovableNonApplicableIList>();
+template void test<CopyOnly>();
+template void test<ExplicitCopyOnly>();
+
+struct any {
+    template<typename T>
+    any(T&&);
+};
+
+template<typename T>
+struct X {
+    X();
+    X(T) = delete;  // expected-note {{'X' has been explicitly marked deleted here}}
+};
+
+X<std::initializer_list<any>> x{ X<std::initializer_list<any>>() };  // expected-error {{call to deleted constructor of 'X<std::initializer_list<any>>'}}
+
+#endif
+}
+
 // dr2331: na
 
 #if __cplusplus >= 201103L
diff --git a/clang/www/cxx_dr_status.html b/clang/www/cxx_dr_status.html
index 4a3ed19161f9a5..391c611480820c 100755
--- a/clang/www/cxx_dr_status.html
+++ b/clang/www/cxx_dr_status.html
@@ -12630,7 +12630,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
     <td><a href="https://cplusplus.github.io/CWG/issues/2137.html">2137</a></td>
     <td>CD4</td>
     <td>List-initialization from object of same type</td>
-    <td class="unknown" align="center">Unknown</td>
+    <td class="unreleased" align="center">Clang 18</td>
   </tr>
   <tr id="2138">
     <td><a href="https://cplusplus.github.io/CWG/issues/2138.html">2138</a></td>
@@ -13674,7 +13674,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
     <td><a href="https://cplusplus.github.io/CWG/issues/2311.html">2311</a></td>
     <td>open</td>
     <td>Missed case for guaranteed copy elision</td>
-    <td align="center">Not resolved</td>
+    <td class="unreleased" align="center">Clang 18</td>
   </tr>
   <tr id="2312">
     <td><a href="https://cplusplus.github.io/CWG/issues/2312.html">2312</a></td>
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
index 3b2d093eb34d49..30fdb19fd3aebe 100644
--- a/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
+++ b/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
@@ -121,7 +121,26 @@ int main(int, char**)
         test_pair_rv<CopyOnly, CopyOnly&>();
         test_pair_rv<CopyOnly, CopyOnly&&>();
 
-        test_pair_rv<ExplicitTypes::CopyOnly, ExplicitTypes::CopyOnly>();
+        /* For ExplicitTypes::CopyOnly, two of the viable candidates for initializing from a non-const xvalue are:
+         *   pair(const pair&);  // (defaulted copy constructor)
+         *   template<class U1, class U2> explicit pair(const pair<U1, U2>&&); [U1 = ExplicitTypes::CopyOnly, U2 = int]
+         * This results in diverging behavior for test_convertible which uses copy-list-initialization
+         * Prior to CWG2137, this would have selected the first (non-explicit) ctor as explicit ctors would not be considered
+         * Afterwards, it should select the second since it is a better match, and then failed because it is explicit
+         *
+         * This may change with future defect reports, and some compilers only have partial support for CWG2137,
+         * so use std::is_convertible directly to avoid a copy-list-initialization
+         */
+        {
+          using P1  = std::pair<ExplicitTypes::CopyOnly, int>;
+          using P2  = std::pair<int, ExplicitTypes::CopyOnly>;
+          using UP1 = std::pair<ExplicitTypes::CopyOnly, int>&&;
+          using UP2 = std::pair<int, ExplicitTypes::CopyOnly>&&;
+          static_assert(std::is_constructible<P1, UP1>::value, "");
+          static_assert(std::is_convertible<P1, UP1>::value, "");
+          static_assert(std::is_constructible<P2, UP2>::value, "");
+          static_assert(std::is_convertible<P2, UP2>::value, "");
+        }
         test_pair_rv<ExplicitTypes::CopyOnly, ExplicitTypes::CopyOnly&, true, false>();
         test_pair_rv<ExplicitTypes::CopyOnly, ExplicitTypes::CopyOnly&&, true, false>();
 

[cor3ntin]

I think all the comments made on phab have been applied, and I do believe this implements the DRs (or their intent)

Please give a few days for other folks to look at it before merging.
@zygoloid in particular as he opened CW2311

[cor3ntin]

@mordante ping for visibility (you approved that on Phab already)

[mordante]

Thanks @cor3ntin. The patch still looks fine from libc++'s PoV.

[Endilll]

Looks perfect to me now. Thank you!

[cor3ntin]

@MitalAshok the bot found some trailing whitespace issues

[MitalAshok]

@cor3ntin It looks like it was in an unrelated file. I've rebased and the format check is passing now

[cor3ntin]

Do you want me to merge on your behalf?

[MitalAshok]

@cor3ntin Still waiting for Windows checks, but yes, could you merge if you please

[cor3ntin]

Let me merge that lest I forget. Our windows bots have a lot of issues

[alexfh]

We've started seeing call to constructor of 'SomeType' is ambiguous errors after this patch on some code. In the case I closely inspected the code looks like https://gcc.godbolt.org/z/3MbWf69cx:

#include <vector>
struct A {
    A() {}
};

struct B {
    explicit B(A);
    explicit B(std::vector<A>);
};

struct C {
    B b;
    C() : b({A()}) {}
};

Is the new error expected? (I would totally agree with it, just making sure. And the behavior is now different to that of GCC trunk.)

[alexfh]

Actually, on the second look it doesn't seem to be too ambiguous to me. I think std::vector<A> should be a strictly better match in this case.

[yxsamliu]

This PR caused a regression for AMDMIGraphX https://github.com/ROCm/AMDMIGraphX/blob/develop/src/targets/gpu/include/migraphx/gpu/prefix_scan_sum.hpp#L57

A reduced test case is:

#include <vector>
#include <stdio.h>
using namespace std;

struct A {
    int x;
    A(int x_) : x(x_) {}
    A(const vector<A>& a) {}
};

int main() {
    vector<A> a{1,2};
    vector<A> b{a};

    printf("%ld\n", b.size());
}

For variable b, it is expected to be constructed by copy ctor of vector, and have 2 elements.

With this patch, a is first converted to A(a), then b is constructed with one element A(a).

Is this expected or a bug? Thanks.

gcc has the same behavior https://godbolt.org/z/hTb795xWP

[yxsamliu]

The issue above seems to be a clang bug to me according to https://cplusplus.github.io/CWG/issues/1467.html

List-initialization of an object or reference of type T is defined as follows:

If T is a class type and the initializer list has a single element of type cv T or a class type derived from T, the object is initialized from that element.

Therefore b should be initialized from a instead of A(a).

[yxsamliu]

This is mostly just a call to coordinate a bit better. I would suggest that if the patch author is engaged on the thread, it would make sense to tell them the schedule on which you plan to revert so they have an opportunity to tell you if that's going to be disruptive.

This part captures one particular issue with my way of action I happened to miss. Thanks for putting this clearly @AaronBallman! Coordination was indeed far from perfect here. And your suggestion totally makes sense.

While I realize this is the 'policy', I'm asking for compassion, particularly with new developers who are engaging.

This is also a good point @erichkeane. I'll try to consider this as well.

Any plan to reland this PR? cuDF/hipDF depends on it. Also the subtle difference of behavior about list initialization between clang/gcc causes issues that are difficult to debug. Thanks.