[Sema][CTAD] Allow user defined conversion for copy-list-initialization #94752

spaits · 2024-06-07T13:18:19Z

The following code:

#include <map>

int main() {
   std::map m1 = {std::pair{"foo", 2}, {"bar", 3}}; // guide #2
   std::map m2(m1.begin(), m1.end()); // guide #1
}

Is rejected by clang, but accepted by both gcc and msvc: https://godbolt.org/z/6v4fvabb5 .

So basically CTAD with copy-list-initialization is rejected.

Note that this exact code is also used in a cppreference article: https://en.cppreference.com/w/cpp/container/map/deduction_guides

I did some debugging and found out, that the reason why this does not work in clang is that, in the function Sema::DeduceTemplateSpecializationFromInitializer we explicitly suppress user conversion, if the initialization type is a copy initialization.

I checked the C++11 and C++20 standard drafts to see whether suppressing user conversion is the correct thing to do for user conversions. Based on the standard I don't think that it is correct.

13.3.1.4 Copy-initialization of class by user-defined conversion [over.match.copy]
Under the conditions specified in 8.5, as part of a copy-initialization of an object of class type, a user-defined
conversion can be invoked to convert an initializer expression to the type of the object being initialized.
Overload resolution is used to select the user-defined conversion to be invoked

So we could use user defined conversions according to the standard.

If a narrowing conversion is required to initialize any of the elements, the
program is ill-formed.

We should not do narrowing.

In copy-list-initialization, if an explicit constructor is chosen, the initialization is ill-formed.

We should not use explicit constructors.

Also a note:
In InitializationKind::InitKind there is no init kind called IK_CopyList but there is IK_DirectList, should we add IK_CopyList? Or it is unnecessary for now?

llvmbot · 2024-06-07T13:18:51Z

@llvm/pr-subscribers-clang

Author: Gábor Spaits (spaits)

Changes

This PR would solve issue #62925.

The following code:

#include &lt;map&gt;

int main() {
   std::map m1 = {std::pair{"foo", 2}, {"bar", 3}}; // guide #<!-- -->2
   std::map m2(m1.begin(), m1.end()); // guide #<!-- -->1
}

Is rejected by clang, but accepted by both gcc and msvc: https://godbolt.org/z/6v4fvabb5 .

So basically CTAD with copy-list-initialization is rejected.

Note that this exact code is also used in a cppreference article: https://en.cppreference.com/w/cpp/container/map/deduction_guides

I did some debugging and found out, that the reason why this does not work in clang is that, in the function Sema::DeduceTemplateSpecializationFromInitializer we explicitly suppress user conversion, if the initialization type is a copy initialization.

I checked the C++11 and C++20 standard drafts to see whether suppressing user conversion is the correct thing to do for user conversions. Based on the standard I don't think that it is correct.

13.3.1.4 Copy-initialization of class by user-defined conversion [over.match.copy]
Under the conditions specified in 8.5, as part of a copy-initialization of an object of class type, a user-defined
conversion can be invoked to convert an initializer expression to the type of the object being initialized.
Overload resolution is used to select the user-defined conversion to be invoked

So we could use user defined conversions according to the standard.

If a narrowing conversion is required to initialize any of the elements, the
program is ill-formed.

We should not do narrowing.

In copy-list-initialization, if an explicit constructor is chosen, the initialization is ill-formed.

We should not use explicit constructors.

Also a note:
In InitializationKind::InitKind there is no init kind called IK_CopyList but there is IK_DirectList, should we add IK_CopyList? Or it is unnecessary for now?

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

3 Files Affected:

(modified) clang/include/clang/Sema/Initialization.h (+1-1)
(modified) clang/lib/Sema/SemaInit.cpp (+2-4)
(added) clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp (+90)

diff --git a/clang/include/clang/Sema/Initialization.h b/clang/include/clang/Sema/Initialization.h
index f443e327eaf32..4b876db436b48 100644
--- a/clang/include/clang/Sema/Initialization.h
+++ b/clang/include/clang/Sema/Initialization.h
@@ -603,7 +603,7 @@ class InitializationKind {
     /// Normal context
     IC_Normal,
 
-    /// Normal context, but allows explicit conversion functionss
+    /// Normal context, but allows explicit conversion functions
     IC_ExplicitConvs,
 
     /// Implicit context (value initialization)
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index ed8b226a6b39f..211b6887befa3 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -10892,8 +10892,6 @@ QualType Sema::DeduceTemplateSpecializationFromInitializer(
     // FIXME: The "second phase of [over.match.list] case can also
     // theoretically happen here, but it's not clear whether we can
     // ever have a parameter of the right type.
-    bool SuppressUserConversions = Kind.isCopyInit();
-
     if (TD) {
       SmallVector<Expr *, 8> TmpInits;
       for (Expr *E : Inits)
@@ -10903,12 +10901,12 @@ QualType Sema::DeduceTemplateSpecializationFromInitializer(
           TmpInits.push_back(E);
       AddTemplateOverloadCandidate(
           TD, FoundDecl, /*ExplicitArgs=*/nullptr, TmpInits, Candidates,
-          SuppressUserConversions,
+          /*SuppressUserConversions=*/false,
           /*PartialOverloading=*/false, AllowExplicit, ADLCallKind::NotADL,
           /*PO=*/{}, AllowAggregateDeductionCandidate);
     } else {
       AddOverloadCandidate(GD, FoundDecl, Inits, Candidates,
-                           SuppressUserConversions,
+                           /*SuppressUserConversions=*/false,
                            /*PartialOverloading=*/false, AllowExplicit);
     }
   };
diff --git a/clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp b/clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp
new file mode 100644
index 0000000000000..21b1137158d5a
--- /dev/null
+++ b/clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value -std=c++20 %s
+namespace std
+{
+  typedef long unsigned int size_t;
+}
+
+namespace std
+{
+  template<class _E>
+    class initializer_list
+    {
+    public:
+      typedef _E value_type;
+      typedef const _E& reference;
+      typedef const _E& const_reference;
+      typedef size_t size_type;
+      typedef const _E* iterator;
+      typedef const _E* const_iterator;
+
+    private:
+      iterator _M_array;
+      size_type _M_len;
+
+
+      constexpr initializer_list(const_iterator __a, size_type __l)
+      : _M_array(__a), _M_len(__l) { }
+
+    public:
+      constexpr initializer_list() noexcept
+      : _M_array(0), _M_len(0) { }
+
+
+      constexpr size_type
+      size() const noexcept { return _M_len; }
+
+
+      constexpr const_iterator
+      begin() const noexcept { return _M_array; }
+
+
+      constexpr const_iterator
+      end() const noexcept { return begin() + size(); }
+    };
+
+  template<class _Tp>
+    constexpr const _Tp*
+    begin(initializer_list<_Tp> __ils) noexcept
+    { return __ils.begin(); }
+
+  template<class _Tp>
+    constexpr const _Tp*
+    end(initializer_list<_Tp> __ils) noexcept
+    { return __ils.end(); }
+}
+
+template<class T, class Y>
+class pair{
+    private:
+    T fst;
+    Y snd;
+    public:
+    pair(T f, Y s) : fst(f), snd(s) {}
+};
+
+template<class T, class Y>
+class map {
+    public:
+    map(std::initializer_list<pair<T, Y>>, int a = 4, int b = 5) {}
+};
+
+template<class T, class Y>
+class Contained {
+  public:
+  Contained(T, Y) {}
+};
+
+template<class T, class Y>
+class A {
+  public:
+  A(std::initializer_list<Contained<T, Y> >, int) {}
+};
+
+int main() {
+    map mOk ={pair{5, 'a'}, {6, 'b'}, {7, 'c'}};
+    map mNarrow ={pair{5, 'a'}, {6.0f, 'b'}, {7, 'c'}}; // expected-error {{type 'float' cannot be narrowed to 'int' in initializer list}} // expected-note {{insert an explicit cast to silence this issue}}
+
+    A aOk = {{Contained{5, 'c'}, {5, 'c'}}, 5};
+    A aNarrowNested = {{Contained{5, 'c'}, {5.0f, 'c'}}, 5}; // expected-error {{type 'float' cannot be narrowed to 'int' in initializer list}} // expected-note {{insert an explicit cast to silence this issue}}
+    A aNarrow = {{Contained{5, 'c'}, {5, 'c'}}, 5.0f}; // expected-error {{type 'float' cannot be narrowed to 'int' in initializer list}} // expected-note {{insert an explicit cast to silence this issue}}
+}

zyn0217

Thanks for working on this. While I didn't dig into the details in DeduceTemplateSpecializationFromInitializer(), I think the test could be improved before proceeding. I also wish @erichkeane and @hokein could take a look at this because they have more expertise in CTAD. :)

zyn0217 · 2024-06-09T04:03:16Z

clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp

+};
+
+int main() {
+    map mOk ={pair{5, 'a'}, {6, 'b'}, {7, 'c'}};


Could you try to simplify the test case a bit? There is so much boilerplate that it's hard to understand the issue.

spaits · 2024-06-09T12:22:55Z

@zyn0217 Thank you for checking my PR. I was able to simplify the std::initializer_list definition. I have also created two different "categories" for the test. One test contains almost the exact code as the one in the reported issue with the same class names. The other one is there to test whether we can handle conversions correctly on multiple levels of nesting (one level deep and top level, I have added this since in the code in the issue everything interesting happens on the first level).

spaits · 2024-06-17T13:08:02Z

@zyn0217 @cor3ntin Sorry to ping you. Could you please take another look at this PR? Are my assumptions about the standard correct? Are the tests good now or I should make other changes?

hokein

The change looks good to me. Please add a note in llvm-project/clang/docs/ReleaseNotes.rst.

clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp

hokein · 2024-06-18T06:21:20Z

clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp

@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value -std=c++20 %s


nit: please format the test file.

Applied clang format, nothing has really changed. I have changed the formatting by hand at some places.

yeah, that's expected, we disable the clang-format for all lit test files (see the clang/test/.clang-format).

hokein

thanks, looks good.

hokein · 2024-06-18T18:34:52Z

clang/test/SemaCXX/ctad-copy-init-list-narrowing.cpp

@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value -std=c++20 %s


yeah, that's expected, we disable the clang-format for all lit test files (see the clang/test/.clang-format).

spaits · 2024-06-18T18:39:19Z

thanks, looks good.

Thank you for reviewing. When the CI run finishes and it is successful I will merge this PR.

cor3ntin · 2024-06-20T14:29:06Z

clang/lib/Sema/SemaInit.cpp

    // FIXME: The "second phase of [over.match.list] case can also
    // theoretically happen here, but it's not clear whether we can
    // ever have a parameter of the right type.


Is that comment still useful?

It wasn't useful (or at least I could definitely not understand why it was there) even when I was debugging to find the solution. Since I did not understand it I did not touch it.

What do you think, could we remove it?

cor3ntin · 2024-06-20T14:31:20Z

clang/docs/ReleaseNotes.rst

+- User defined constructors are allowed for copy-list-initialization with CTAD.
+  The example code for deduction guides for std::map in
+  (`cppreference <https://en.cppreference.com/w/cpp/container/map/deduction_guides>`_)
+  will now work.
+  (#GH62925).
+


Not sure the link adds useful information. If you think the example is useful, feel free to add the examples in the release note directly (but the issue link is enough)

I will change it so only the issue link is there.

@cor3ntin

As discussed before with @cor3ntin before (#94752) here is the simplification of the release note written for the previously mentioned PR and the removal of a comment that is no longer useful. (Sorry for creating this PR this late.) Co-authored-by: Gabor Spaits <Gabor.Spaits@hightec-rt.com>

…on (llvm#94752) Fixes llvm#62925. The following code: ```cpp #include <map> int main() { std::map m1 = {std::pair{"foo", 2}, {"bar", 3}}; // guide llvm#2 std::map m2(m1.begin(), m1.end()); // guide llvm#1 } ``` Is rejected by clang, but accepted by both gcc and msvc: https://godbolt.org/z/6v4fvabb5 . So basically CTAD with copy-list-initialization is rejected. Note that this exact code is also used in a cppreference article: https://en.cppreference.com/w/cpp/container/map/deduction_guides I checked the C++11 and C++20 standard drafts to see whether suppressing user conversion is the correct thing to do for user conversions. Based on the standard I don't think that it is correct. ``` 13.3.1.4 Copy-initialization of class by user-defined conversion [over.match.copy] Under the conditions specified in 8.5, as part of a copy-initialization of an object of class type, a user-defined conversion can be invoked to convert an initializer expression to the type of the object being initialized. Overload resolution is used to select the user-defined conversion to be invoked ``` So we could use user defined conversions according to the standard. ``` If a narrowing conversion is required to initialize any of the elements, the program is ill-formed. ``` We should not do narrowing. ``` In copy-list-initialization, if an explicit constructor is chosen, the initialization is ill-formed. ``` We should not use explicit constructors.

[CTAD] Allow user defined conversion

8e1f3aa

spaits requested review from cor3ntin and AaronBallman June 7, 2024 13:18

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 7, 2024

spaits requested a review from zyn0217 June 7, 2024 19:30

zyn0217 reviewed Jun 9, 2024

View reviewed changes

zyn0217 requested review from hokein and erichkeane June 9, 2024 04:10

Simplify boiler plate and re-organize tests

8e33ac0

hokein reviewed Jun 18, 2024

View reviewed changes

spaits added 3 commits June 18, 2024 12:43

Simplify test

3076155

Format by hand

44b6f2c

Add release notes

026536a

hokein approved these changes Jun 18, 2024

View reviewed changes

spaits merged commit f80bd9b into llvm:main Jun 18, 2024
8 checks passed

cor3ntin reviewed Jun 20, 2024

View reviewed changes

spaits mentioned this pull request Jun 22, 2024

[Clang] Simplify release notes and remove irrelevant comment #96407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sema][CTAD] Allow user defined conversion for copy-list-initialization #94752

[Sema][CTAD] Allow user defined conversion for copy-list-initialization #94752

spaits commented Jun 7, 2024 •

edited

Loading

llvmbot commented Jun 7, 2024

zyn0217 left a comment

zyn0217 Jun 9, 2024

spaits commented Jun 9, 2024

spaits commented Jun 17, 2024

hokein left a comment

hokein Jun 18, 2024

spaits Jun 18, 2024

hokein Jun 18, 2024

hokein left a comment

hokein Jun 18, 2024

spaits commented Jun 18, 2024

cor3ntin Jun 20, 2024

spaits Jun 20, 2024 •

edited

Loading

cor3ntin Jun 20, 2024

spaits Jun 20, 2024

		@@ -0,0 +1,65 @@
		// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value -std=c++20 %s

[Sema][CTAD] Allow user defined conversion for copy-list-initialization #94752

[Sema][CTAD] Allow user defined conversion for copy-list-initialization #94752

Conversation

spaits commented Jun 7, 2024 • edited Loading

llvmbot commented Jun 7, 2024

zyn0217 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spaits commented Jun 9, 2024

spaits commented Jun 17, 2024

hokein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hokein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spaits commented Jun 18, 2024

Choose a reason for hiding this comment

spaits Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spaits commented Jun 7, 2024 •

edited

Loading

spaits Jun 20, 2024 •

edited

Loading