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

[Patrick Palka <ppalka@redhat.com>]

On Tue, 8 Mar 2022, Jason Merrill wrote:

> On 3/8/22 14:38, Patrick Palka wrote:
> > On Tue, 8 Mar 2022, Jason Merrill wrote:
> > 
> > > 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?
> > 
> > That seems to work nicely :) All xfailed tests for the static data
> > member initialization case now also pass.
> > 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> > 
> > -- >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
> > seems to do the right thing thanks to code in build_aggr_init that sets
> > LOOKUP_ONLYCONVERTING when the initializer represents copy-initialization.
> > 
> > This patch fixes this by making cp_finish_decl set LOOKUP_ONLYCONVERTING
> > by inspecting the initializer like build_aggr_init does, so that callers
> > don't need to explicitly pass this flag.
> > 
> > 	PR c++/102137
> > 	PR c++/87820
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (is_copy_initialization): Declare.
> > 	* decl.cc (cp_finish_decl): Set LOOKUP_ONLYCONVERTING
> > 	when is_copy_initialization is true.
> > 	* init.cc (build_aggr_init): Split out copy-initialization
> > 	check into ...
> > 	(is_copy_initialization): ... here.
> > 	* pt.cc (instantiate_decl): Pass 0 instead of
> > 	LOOKUP_ONLYCONVERTING as flags to cp_finish_decl.
> 
> Any reason not to use LOOKUP_NORMAL, both here and in tsubst_expr?  OK either
> way.

Thanks a lot.  I'm not really sure what the consequences of using
LOOKUP_NORMAL (which implies LOOKUP_PROTECT) instead of 0 would be here,
and there's a bunch of other callers of cp_finish_decl which pass 0 or
some other value that excludes LOOKUP_PROTECT.  So I figure such a change
would be better off as a separate patch that changes/audits all such
cp_finish_decl at once.

> 
> > 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/decl.cc                                |  3 +
> >   gcc/cp/init.cc                                | 20 +++--
> >   gcc/cp/pt.cc                                  |  3 +-
> >   gcc/testsuite/g++.dg/cpp0x/explicit15.C       | 83 +++++++++++++++++++
> >   .../g++.dg/cpp1z/class-deduction108.C         | 78 +++++++++++++++++
> >   6 files changed, 181 insertions(+), 7 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/decl.cc b/gcc/cp/decl.cc
> > index 199ac768d43..5452f7d9cb3 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -7968,6 +7968,9 @@ cp_finish_decl (tree decl, tree init, bool
> > init_const_expr_p,
> >     if (type == error_mark_node)
> >       return;
> >   +  if (VAR_P (decl) && is_copy_initialization (init))
> > +    flags |= LOOKUP_ONLYCONVERTING;
> > +
> >     /* Warn about register storage specifiers except when in GNU global
> >        or local register variable extension.  */
> >     if (VAR_P (decl) && DECL_REGISTER (decl) && asmspec_tree == NULL_TREE)
> > 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..e8b5d8fbb73 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -26602,8 +26602,7 @@ instantiate_decl (tree d, bool defer_ok, bool
> > expl_inst_class_mem_p)
> >   	      const_init
> >   		= DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (code_pattern);
> >   	      cp_finish_decl (d, init, /*init_const_expr_p=*/const_init,
> > -			      /*asmspec_tree=*/NULL_TREE,
> > -			      LOOKUP_ONLYCONVERTING);
> > +			      /*asmspec_tree=*/NULL_TREE, 0);
> >   	    }
> >   	  if (enter_context)
> >   	    pop_nested_class ();
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/explicit15.C
> > b/gcc/testsuite/g++.dg/cpp0x/explicit15.C
> > new file mode 100644
> > index 00000000000..076d8b31631
> > --- /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" }
> > +  static constexpr T t2 = {a}; // { dg-error "cannot convert" }
> > +  static constexpr T t4{a}; // { dg-bogus "cannot convert" }
> > +  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" }
> > +  static constexpr int n2 = {t}; // { dg-error "cannot convert" }
> > +  static constexpr int n4{t}; // { dg-bogus "cannot convert" }
> > +  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
> > } }
> > +  static inline T t2 = {a}; // { dg-error "cannot convert" "" { target
> > c++17 } }
> > +  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 } }
> > +  static inline int n2 = {t}; // { dg-error "cannot convert" "" { target
> > c++17 } }
> > +  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..e82c4bafb04
> > --- /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" }
> > +  static inline TT x2 = {0}; // { dg-error "explicit deduction guide" }
> > +  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" }
> > +  static inline A a2 = {t}; // { dg-error "explicit deduction guide" }
> > +  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" }
> > +  static constexpr TT x2 = {0}; // { dg-error "explicit deduction guide" }
> > +  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" }
> > +  static constexpr A a2 = {t}; // { dg-error "explicit deduction guide" }
> > +  static constexpr A a4{t};
> > +  static constexpr A a5{};
> > +};
> > +
> > +template struct E<A>;
> > +template struct F<int>;
> 
>