Re: [PATCH] c++: detecting copy-init context during CTAD [PR102137]

[Jason Merrill <jason@redhat.com>]

On 3/8/22 11:36, Patrick Palka wrote:
> On Mon, 7 Mar 2022, Jason Merrill wrote:
> 
>> On 3/7/22 10:47, Patrick Palka wrote:
>>> On Fri, 4 Mar 2022, Jason Merrill wrote:
>>>
>>>> On 3/4/22 14:24, Patrick Palka wrote:
>>>>> Here we're failing to communicate to cp_finish_decl from tsubst_expr
>>>>> that we're in a copy-initialization context (via the
>>>>> LOOKUP_ONLYCONVERTING
>>>>> flag), which causes do_class_deduction to always consider explicit
>>>>> deduction guides when performing CTAD for a templated variable
>>>>> initializer.
>>>>>
>>>>> We could fix this by passing LOOKUP_ONLYCONVERTING appropriately when
>>>>> calling cp_finish_decl from tsubst_expr, but it seems do_class_deduction
>>>>> can determine if we're in a copy-init context by simply inspecting the
>>>>> initializer, and thus render its flags parameter unnecessary, which is
>>>>> what this patch implements.  (If we were to fix this in tsubst_expr
>>>>> instead, I think we'd have to inspect the initializer in the same way
>>>>> in order to detect a copy-init context?)
>>>>
>>>> Hmm, does this affect conversions as well?
>>>>
>>>> Looks like it does:
>>>>
>>>> struct A
>>>> {
>>>>     explicit operator int();
>>>> };
>>>>
>>>> template <class T> void f()
>>>> {
>>>>     T t = A();
>>>> }
>>>>
>>>> int main()
>>>> {
>>>>     f<int>(); // wrongly accepted
>>>> }
>>>>
>>>> The reverse, initializing via an explicit constructor, is caught by code
>>>> in
>>>> build_aggr_init much like the code your patch adds to do_auto_deduction;
>>>> perhaps we should move/copy that code to cp_finish_decl?
>>>
>>> Ah, makes sense.  Moving that code from build_aggr_init to
>>> cp_finish_decl broke things, but using it in both spots seems to work
>>> well.  And I suppose we might as well use it in do_class_deduction too,
>>> since doing so lets us remove the flags parameter.
>>
>> Before removing the flags parameter please try asserting that it now matches
>> is_copy_initialization and see if anything breaks.
> 
> I added to do_class_deduction:
> 
>    gcc_assert (bool(flags & LOOKUP_ONLYCONVERTING) == is_copy_initialization (init));
> 
> Turns out removing the flags parameter breaks CTAD for new-expressions
> of the form 'new TT(x)' because in this case build_new passes just 'x'
> as the initializer to do_auto_deduction (as opposed to a single TREE_LIST),
> for which is_copy_initialization returns true even though it's really
> direct initalization.
> 
> Also turns out we're similarly not passing the right LOOKUP_* flags to
> cp_finish_decl from instantiate_body, which breaks consideration of
> explicit conversions/deduction guides when instantiating the initializer
> of a static data member.  I added some xfailed testcases for these
> situations.

Maybe we want to check is_copy_initialization in cp_finish_decl?

> Here's a patch that keeps the flags parameter of do_auto_deduction, and
> only changes the call to cp_finish_decl from tsubst_expr:
> 
> -- >8 --
> 
> Subject: [PATCH] c++: detecting copy-init context during CTAD [PR102137]
> 
> Here we're failing to communicate to cp_finish_decl from tsubst_expr
> that we're in a copy-initialization context (via the LOOKUP_ONLYCONVERTING
> flag), which causes us to always consider explicit deduction guides when
> performing CTAD for a templated variable initializer.
> 
> It turns out this bug also affects consideration of explicit conversion
> operators for the same reason.  But consideration of explicit constructors
> is unaffected and seems to work correctly thanks to code in build_aggr_init
> that sets LOOKUP_ONLYCONVERTING when the initializer represents
> copy-initialization.
> 
> This patch factors out the copy-initialization check from build_aggr_init
> and reuses it in tsubst_expr for sake of cp_finish_decl.  This fixes
> consideration of explicit dguides/conversion when instantiating the
> initializer of block-scope variables, but the static data member case is
> still similarly broken since those are handled from instantiate_body
> not tsubst_expr.
> 
> 	PR c++/102137
> 	PR c++/87820
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (is_copy_initialization): Declare.
> 	* init.cc (build_aggr_init): Split out copy-initialization
> 	check into ...
> 	(is_copy_initialization): ... here.
> 	(tsubst_expr) <case VAR_DECL>: Pass LOOKUP_ONLYCONVERTING
> 	to cp_finish_decl when is_copy_initialization is true.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/explicit15.C: New test.
> 	* g++.dg/cpp1z/class-deduction108.C: New test.
> ---
>   gcc/cp/cp-tree.h                              |  1 +
>   gcc/cp/init.cc                                | 20 +++--
>   gcc/cp/pt.cc                                  |  6 +-
>   gcc/testsuite/g++.dg/cpp0x/explicit15.C       | 83 +++++++++++++++++++
>   .../g++.dg/cpp1z/class-deduction108.C         | 78 +++++++++++++++++
>   5 files changed, 182 insertions(+), 6 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/explicit15.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index ac723901098..fd76909ca75 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7039,6 +7039,7 @@ extern void emit_mem_initializers		(tree);
>   extern tree build_aggr_init			(tree, tree, int,
>                                                    tsubst_flags_t);
>   extern int is_class_type			(tree, int);
> +extern bool is_copy_initialization		(tree);
>   extern tree build_zero_init			(tree, tree, bool);
>   extern tree build_value_init			(tree, tsubst_flags_t);
>   extern tree build_value_init_noctor		(tree, tsubst_flags_t);
> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
> index 545d904c0f9..d5faa0dc519 100644
> --- a/gcc/cp/init.cc
> +++ b/gcc/cp/init.cc
> @@ -2019,11 +2019,7 @@ build_aggr_init (tree exp, tree init, int flags, tsubst_flags_t complain)
>         return stmt_expr;
>       }
>   
> -  if (init && init != void_type_node
> -      && TREE_CODE (init) != TREE_LIST
> -      && !(TREE_CODE (init) == TARGET_EXPR
> -	   && TARGET_EXPR_DIRECT_INIT_P (init))
> -      && !DIRECT_LIST_INIT_P (init))
> +  if (is_copy_initialization (init))
>       flags |= LOOKUP_ONLYCONVERTING;
>   
>     is_global = begin_init_stmts (&stmt_expr, &compound_stmt);
> @@ -2331,6 +2327,20 @@ is_class_type (tree type, int or_else)
>     return 1;
>   }
>   
> +/* Returns true iff the variable initializer INIT represents
> +   copy-initialization (and therefore we must set LOOKUP_ONLYCONVERTING
> +   when processing it).  */
> +
> +bool
> +is_copy_initialization (tree init)
> +{
> +  return (init && init != void_type_node
> +	  && TREE_CODE (init) != TREE_LIST
> +	  && !(TREE_CODE (init) == TARGET_EXPR
> +	       && TARGET_EXPR_DIRECT_INIT_P (init))
> +	  && !DIRECT_LIST_INIT_P (init));
> +}
> +
>   /* Build a reference to a member of an aggregate.  This is not a C++
>      `&', but really something which can have its address taken, and
>      then act as a pointer to member, for example TYPE :: FIELD can have
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 402365285cf..18e2252c482 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -18606,7 +18606,11 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
>   			TREE_TYPE (asmspec_tree) = char_array_type_node;
>   		      }
>   
> -		    cp_finish_decl (decl, init, const_init, asmspec_tree, 0);
> +
> +		    int flags = LOOKUP_NORMAL;
> +		    if (VAR_P (decl) && is_copy_initialization (init))
> +		      flags |= LOOKUP_ONLYCONVERTING;
> +		    cp_finish_decl (decl, init, const_init, asmspec_tree, flags);
>   
>   		    if (ndecl != error_mark_node)
>   		      cp_finish_decomp (ndecl, first, cnt);
> diff --git a/gcc/testsuite/g++.dg/cpp0x/explicit15.C b/gcc/testsuite/g++.dg/cpp0x/explicit15.C
> new file mode 100644
> index 00000000000..48563c8d0b2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/explicit15.C
> @@ -0,0 +1,83 @@
> +// PR c++/87820
> +// { dg-do compile { target c++11 } }
> +
> +struct A {
> +  constexpr explicit operator int() const { return 0; }
> +};
> +
> +template<class T>
> +void f() {
> +  A a;
> +  T t1 = a; // { dg-error "cannot convert" }
> +  T t2 = {a}; // { dg-error "cannot convert" }
> +  T t3(a);
> +  T t4{a};
> +  T t5 = T(a);
> +  T t6 = T{a};
> +  new T(a);
> +  new T{a};
> +}
> +
> +template<class T>
> +void g() {
> +  T t;
> +  int n1 = t; // { dg-error "cannot convert" }
> +  int n2 = {t}; // { dg-error "cannot convert" }
> +  int n3(t);
> +  int n4{t};
> +  int n5 = int(t);
> +  int n6 = int{t};
> +  new int(t);
> +  new int{t};
> +}
> +
> +template void f<int>();
> +template void g<A>();
> +
> +template<class T>
> +struct B {
> +  static constexpr A a{};
> +  static constexpr T t1 = a; // { dg-error "cannot convert" "" { xfail c++17 } }
> +  static constexpr T t2 = {a}; // { dg-error "cannot convert" "" { xfail c++17 } }
> +  static constexpr T t4{a}; // { dg-bogus "cannot convert" "" { xfail c++14_down } }
> +  static constexpr T t5 = T(a);
> +  static constexpr T t6 = T{a};
> +};
> +
> +template<class T>
> +struct C {
> +  static constexpr T t{};
> +  static constexpr int n1 = t; // { dg-error "cannot convert" "" { xfail c++17 } }
> +  static constexpr int n2 = {t}; // { dg-error "cannot convert" "" { xfail c++17 } }
> +  static constexpr int n4{t}; // { dg-bogus "cannot convert" "" { xfail c++14_down } }
> +  static constexpr int n5 = int(t);
> +  static constexpr int n6 = int{t};
> +};
> +
> +template struct B<int>;
> +template struct C<A>;
> +
> +#if __cpp_inline_variables
> +template<class T>
> +struct D {
> +  static inline A a;
> +  static inline T t1 = a; // { dg-error "cannot convert" "" { target c++17  xfail *-*-* } }
> +  static inline T t2 = {a}; // { dg-error "cannot convert" "" { target c++17  xfail *-*-* } }
> +  static inline T t4{a};
> +  static inline T t5 = T(a);
> +  static inline T t6 = T{a};
> +};
> +
> +template<class T>
> +struct E {
> +  static inline T t;
> +  static inline int n1 = t; // { dg-error "cannot convert" "" { target c++17 xfail *-*-* } }
> +  static inline int n2 = {t}; // { dg-error "cannot convert" "" { target c++17 xfail *-*-* } }
> +  static inline int n4{t};
> +  static inline int n5 = int(t);
> +  static inline int n6 = int{t};
> +};
> +
> +template struct D<int>;
> +template struct E<A>;
> +#endif
> diff --git a/gcc/testsuite/g++.dg/cpp1z/class-deduction108.C b/gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
> new file mode 100644
> index 00000000000..5c9f85652ed
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1z/class-deduction108.C
> @@ -0,0 +1,78 @@
> +// PR c++/102137
> +// { dg-do compile { target c++17 } }
> +
> +template<class T>
> +struct A {
> +  constexpr A() { }
> +  constexpr A(int) { }
> +};
> +
> +explicit A(...) -> A<int>;
> +
> +template<template<class> class TT>
> +void f() {
> +  TT x1 = 0; // { dg-error "deduction|no match" }
> +  TT x2 = {0}; // { dg-error "explicit deduction guide" }
> +  TT x3(0);
> +  TT x4{0};
> +  TT x5;
> +  new TT(0);
> +  new TT{0};
> +  new TT();
> +  new TT{};
> +  new TT;
> +}
> +
> +template<class T>
> +void g(T t) {
> +  A a1 = t; // { dg-error "deduction|no match" }
> +  A a2 = {t}; // { dg-error "explicit deduction guide" }
> +  A a3(t);
> +  A a4{t};
> +  A a5;
> +  new A(t);
> +  new A{t};
> +}
> +
> +template void f<A>();
> +template void g(int);
> +
> +template<template<class> class TT>
> +struct B {
> +  static inline TT x1 = 0; // { dg-error "deduction|no match" "" { xfail *-*-* } }
> +  static inline TT x2 = {0}; // { dg-error "explicit deduction guide" "" { xfail *-*-* } }
> +  static inline TT x4{0};
> +  static inline TT x5;
> +};
> +
> +template<class T>
> +struct C {
> +  static inline T t;
> +  static inline A a1 = t; // { dg-error "deduction|no match" "" { xfail *-*-* } }
> +  static inline A a2 = {t}; // { dg-error "explicit deduction guide" "" { xfail *-*-* } }
> +  static inline A a4{t};
> +  static inline A a5{};
> +};
> +
> +template struct B<A>;
> +template struct C<int>;
> +
> +template<template<class> class TT>
> +struct E {
> +  static constexpr TT x1 = 0; // { dg-error "deduction|no match" "" { xfail *-*-* } }
> +  static constexpr TT x2 = {0}; // { dg-error "explicit deduction guide" "" { xfail *-*-* } }
> +  static constexpr TT x4{0};
> +  static constexpr TT x5{};
> +};
> +
> +template<class T>
> +struct F {
> +  static constexpr T t{};
> +  static constexpr A a1 = t; // { dg-error "deduction|no match" "" { xfail *-*-* } }
> +  static constexpr A a2 = {t}; // { dg-error "explicit deduction guide" "" { xfail *-*-* } }
> +  static constexpr A a4{t};
> +  static constexpr A a5{};
> +};
> +
> +template struct E<A>;
> +template struct F<int>;