c++: Recursive unification with packs and explicit targs [PR94628]

This PR seems to be similar to PR c++/43382, except that the recursive call to
the variadic function with trailing return type in this testcase is additionally
given some explicit template arguments.

In the first testcase below, when resolving the recursive call to 'select',
fn_type_unification first substitutes in the call's explicit template arguments
before doing unification, and so during this substitution the template argument
pack for Args is incomplete.

Since the pack is incomplete, the substitution of 'args...' in the trailing
return type decltype(f(args...)) is handled by the unsubstituted_packs case of
tsubst_pack_expansion.  But the handling of this case happens _before_ we reset
local_specializations, and so the substitution ends up reusing the old binding
for 'args' from local_specializations rather than building a new one.

This patch fixes this issue by setting up local_specializations sooner in
tsubst_pack_expansion, before the handling of the unsubstituted_packs case.
It also adds a new policy to local_specialization_stack so that we could use the
class here to conditionally replace local_specializations.

Passes 'make check-c++', does this look OK to commit after bootstrap/regtesting?

gcc/cp/ChangeLog:

	PR c++/94628
	* cp-tree.h (lss_policy::lss_inherit): New enumerator.
	* pt.c (local_specialization_stack::local_specialization_stack): Handle
	an lss_inherit policy.
	(local_specialization_stack::~local_specialization_stack): Likewise.
	(tsubst_pack_expansion): Use a local_specialization_stack instead of
	manually saving and restoring local_specializations.  Conditionally
	replace local_specializations sooner, before the handling of the
	unsubstituted_packs case.

gcc/testsuite/ChangeLog:

	PR c++/94628
	* g++.dg/cpp0x/variadic179.C: New test.
	* g++.dg/cpp0x/variadic180.C: New test.
---
 gcc/cp/cp-tree.h                         |  2 +-
 gcc/cp/pt.c                              | 38 +++++++++++-------------
 gcc/testsuite/g++.dg/cpp0x/variadic179.C | 16 ++++++++++
 gcc/testsuite/g++.dg/cpp0x/variadic180.C | 25 ++++++++++++++++
 4 files changed, 59 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic179.C
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic180.C

On 4/19/20 12:55 PM, Patrick Palka wrote:
> This PR seems to be similar to PR c++/43382, except that the recursive call to
> the variadic function with trailing return type in this testcase is additionally
> given some explicit template arguments.
> 
> In the first testcase below, when resolving the recursive call to 'select',
> fn_type_unification first substitutes in the call's explicit template arguments
> before doing unification, and so during this substitution the template argument
> pack for Args is incomplete.
> 
> Since the pack is incomplete, the substitution of 'args...' in the trailing
> return type decltype(f(args...)) is handled by the unsubstituted_packs case of
> tsubst_pack_expansion.  But the handling of this case happens _before_ we reset
> local_specializations, and so the substitution ends up reusing the old binding
> for 'args' from local_specializations rather than building a new one.
> 
> This patch fixes this issue by setting up local_specializations sooner in
> tsubst_pack_expansion, before the handling of the unsubstituted_packs case.
> It also adds a new policy to local_specialization_stack so that we could use the
> class here to conditionally replace local_specializations.
> 
> Passes 'make check-c++', does this look OK to commit after bootstrap/regtesting?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/94628
> 	* cp-tree.h (lss_policy::lss_inherit): New enumerator.
> 	* pt.c (local_specialization_stack::local_specialization_stack): Handle
> 	an lss_inherit policy.
> 	(local_specialization_stack::~local_specialization_stack): Likewise.
> 	(tsubst_pack_expansion): Use a local_specialization_stack instead of
> 	manually saving and restoring local_specializations.  Conditionally
> 	replace local_specializations sooner, before the handling of the
> 	unsubstituted_packs case.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/94628
> 	* g++.dg/cpp0x/variadic179.C: New test.
> 	* g++.dg/cpp0x/variadic180.C: New test.
> ---
>   gcc/cp/cp-tree.h                         |  2 +-
>   gcc/cp/pt.c                              | 38 +++++++++++-------------
>   gcc/testsuite/g++.dg/cpp0x/variadic179.C | 16 ++++++++++
>   gcc/testsuite/g++.dg/cpp0x/variadic180.C | 25 ++++++++++++++++
>   4 files changed, 59 insertions(+), 22 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic179.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic180.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 000badc21ac..5b3b9507474 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -5424,7 +5424,7 @@ enum unification_kind_t {
>   // An RAII class used to create a new pointer map for local
>   // specializations. When the stack goes out of scope, the
>   // previous pointer map is restored.
> -enum lss_policy { lss_blank, lss_copy };
> +enum lss_policy { lss_blank, lss_copy, lss_inherit };
>   class local_specialization_stack
>   {
>   public:
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 9e39f46a090..9cea663514d 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -83,7 +83,9 @@ static tree cur_stmt_expr;
>   local_specialization_stack::local_specialization_stack (lss_policy policy)
>     : saved (local_specializations)
>   {
> -  if (policy == lss_blank || !saved)
> +  if (policy == lss_inherit)
> +    ;

Maybe assert that saved is non-null?

lss_inherit suggests to me that we could add things and then pop back to 
the previous state, which is the actual meaning of lss_copy.  Maybe 
lss_nop or lss_same instead?

OK with those changes.

> +  else if (policy == lss_blank || !saved)
>       local_specializations = new hash_map<tree, tree>;
>     else
>       local_specializations = new hash_map<tree, tree>(*saved);
> @@ -91,8 +93,11 @@ local_specialization_stack::local_specialization_stack (lss_policy policy)
>   
>   local_specialization_stack::~local_specialization_stack ()
>   {
> -  delete local_specializations;
> -  local_specializations = saved;
> +  if (local_specializations != saved)
> +    {
> +      delete local_specializations;
> +      local_specializations = saved;
> +    }
>   }
>   
>   /* True if we've recursed into fn_type_unification too many times.  */
> @@ -12694,7 +12699,6 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
>     bool unsubstituted_fn_pack = false;
>     int i, len = -1;
>     tree result;
> -  hash_map<tree, tree> *saved_local_specializations = NULL;
>     bool need_local_specializations = false;
>     int levels;
>   
> @@ -12893,7 +12897,15 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
>   	= build_extra_args (pattern, args, complain);
>         return t;
>       }
> -  else if (unsubstituted_packs)
> +
> +  /* If NEED_LOCAL_SPECIALIZATIONS then we're in a late-specified return
> +     type, so create our own local specializations map; the current map is
> +     either NULL or (in the case of recursive unification) might have
> +     bindings that we don't want to use or alter.  */
> +  local_specialization_stack lss (need_local_specializations
> +				  ? lss_blank : lss_inherit);
> +
> +  if (unsubstituted_packs)
>       {
>         /* There were no real arguments, we're just replacing a parameter
>   	 pack with another version of itself. Substitute into the
> @@ -12910,16 +12922,6 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
>   
>     gcc_assert (len >= 0);
>   
> -  if (need_local_specializations)
> -    {
> -      /* We're in a late-specified return type, so create our own local
> -	 specializations map; the current map is either NULL or (in the
> -	 case of recursive unification) might have bindings that we don't
> -	 want to use or alter.  */
> -      saved_local_specializations = local_specializations;
> -      local_specializations = new hash_map<tree, tree>;
> -    }
> -
>     /* For each argument in each argument pack, substitute into the
>        pattern.  */
>     result = make_tree_vec (len);
> @@ -12966,12 +12968,6 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
>           }
>       }
>   
> -  if (need_local_specializations)
> -    {
> -      delete local_specializations;
> -      local_specializations = saved_local_specializations;
> -    }
> -
>     /* If the dependent pack arguments were such that we end up with only a
>        single pack expansion again, there's no need to keep it in a TREE_VEC.  */
>     if (len == 1 && TREE_CODE (result) == TREE_VEC
> diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic179.C b/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> new file mode 100644
> index 00000000000..f04d3f753ca
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> @@ -0,0 +1,16 @@
> +// PR c++/94628
> +// { dg-do compile { target c++11 } }
> +
> +int f(int, int);
> +int f(int);
> +
> +template<class...Args>
> +auto select(Args... args) -> decltype(f(args...))
> +{
> +  if (sizeof...(Args) > 1)
> +    return select<char>(7);
> +  else
> +    return 0;
> +}
> +
> +int a = select(0, 1);
> diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic180.C b/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> new file mode 100644
> index 00000000000..e8fcdd0a64d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> @@ -0,0 +1,25 @@
> +// PR c++/94628
> +// A variant of variadic101.C where the recursive call to deref
> +// has its first template argument explicitly given.
> +// { dg-do compile { target c++11 } }
> +
> +template<class T>
> +struct Container
> +{ T f() const; };
> +
> +template<class T>
> +T deref(const T& t)
> +{ return t; }
> +
> +
> +template <class T, class... Args>
> +auto
> +deref(const T& u, int r, Args... args)
> +-> decltype(deref(u.f(), args...))
> +{ return deref<decltype(u.f())>(u.f(), args...); }
> +
> +int main(void)
> +{
> +    Container<Container<int>> v;
> +    deref(v,1,2);
> +}
>

On Mon, 20 Apr 2020, Jason Merrill wrote:
> On 4/19/20 12:55 PM, Patrick Palka wrote:
> > This PR seems to be similar to PR c++/43382, except that the recursive call
> > to
> > the variadic function with trailing return type in this testcase is
> > additionally
> > given some explicit template arguments.
> > 
> > In the first testcase below, when resolving the recursive call to 'select',
> > fn_type_unification first substitutes in the call's explicit template
> > arguments
> > before doing unification, and so during this substitution the template
> > argument
> > pack for Args is incomplete.
> > 
> > Since the pack is incomplete, the substitution of 'args...' in the trailing
> > return type decltype(f(args...)) is handled by the unsubstituted_packs case
> > of
> > tsubst_pack_expansion.  But the handling of this case happens _before_ we
> > reset
> > local_specializations, and so the substitution ends up reusing the old
> > binding
> > for 'args' from local_specializations rather than building a new one.
> > 
> > This patch fixes this issue by setting up local_specializations sooner in
> > tsubst_pack_expansion, before the handling of the unsubstituted_packs case.
> > It also adds a new policy to local_specialization_stack so that we could use
> > the
> > class here to conditionally replace local_specializations.
> > 
> > Passes 'make check-c++', does this look OK to commit after
> > bootstrap/regtesting?
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/94628
> > 	* cp-tree.h (lss_policy::lss_inherit): New enumerator.
> > 	* pt.c (local_specialization_stack::local_specialization_stack):
> > Handle
> > 	an lss_inherit policy.
> > 	(local_specialization_stack::~local_specialization_stack): Likewise.
> > 	(tsubst_pack_expansion): Use a local_specialization_stack instead of
> > 	manually saving and restoring local_specializations.  Conditionally
> > 	replace local_specializations sooner, before the handling of the
> > 	unsubstituted_packs case.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c++/94628
> > 	* g++.dg/cpp0x/variadic179.C: New test.
> > 	* g++.dg/cpp0x/variadic180.C: New test.
> > ---
> >   gcc/cp/cp-tree.h                         |  2 +-
> >   gcc/cp/pt.c                              | 38 +++++++++++-------------
> >   gcc/testsuite/g++.dg/cpp0x/variadic179.C | 16 ++++++++++
> >   gcc/testsuite/g++.dg/cpp0x/variadic180.C | 25 ++++++++++++++++
> >   4 files changed, 59 insertions(+), 22 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic179.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > 
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 000badc21ac..5b3b9507474 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -5424,7 +5424,7 @@ enum unification_kind_t {
> >   // An RAII class used to create a new pointer map for local
> >   // specializations. When the stack goes out of scope, the
> >   // previous pointer map is restored.
> > -enum lss_policy { lss_blank, lss_copy };
> > +enum lss_policy { lss_blank, lss_copy, lss_inherit };
> >   class local_specialization_stack
> >   {
> >   public:
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index 9e39f46a090..9cea663514d 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -83,7 +83,9 @@ static tree cur_stmt_expr;
> >   local_specialization_stack::local_specialization_stack (lss_policy policy)
> >     : saved (local_specializations)
> >   {
> > -  if (policy == lss_blank || !saved)
> > +  if (policy == lss_inherit)
> > +    ;
> 
> Maybe assert that saved is non-null?

Hmm, it looks like adding an assert to that effect breaks substituting
into the TYPE_ARG_TYPES of a variadic function template, because
local_specializations is not set up until we finish substituting into
the decl and start instantiating the body.

> 
> lss_inherit suggests to me that we could add things and then pop back to the
> previous state, which is the actual meaning of lss_copy.  Maybe lss_nop or
> lss_same instead?

Sounds good, I will commit the patch with lss_inherit changed to
lss_nop shortly (and without the assert I suppose).

> 
> OK with those changes.
> 
> > +  else if (policy == lss_blank || !saved)
> >       local_specializations = new hash_map<tree, tree>;
> >     else
> >       local_specializations = new hash_map<tree, tree>(*saved);
> > @@ -91,8 +93,11 @@ local_specialization_stack::local_specialization_stack
> > (lss_policy policy)
> >     local_specialization_stack::~local_specialization_stack ()
> >   {
> > -  delete local_specializations;
> > -  local_specializations = saved;
> > +  if (local_specializations != saved)
> > +    {
> > +      delete local_specializations;
> > +      local_specializations = saved;
> > +    }
> >   }
> >     /* True if we've recursed into fn_type_unification too many times.  */
> > @@ -12694,7 +12699,6 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> >     bool unsubstituted_fn_pack = false;
> >     int i, len = -1;
> >     tree result;
> > -  hash_map<tree, tree> *saved_local_specializations = NULL;
> >     bool need_local_specializations = false;
> >     int levels;
> >   @@ -12893,7 +12897,15 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> >   	= build_extra_args (pattern, args, complain);
> >         return t;
> >       }
> > -  else if (unsubstituted_packs)
> > +
> > +  /* If NEED_LOCAL_SPECIALIZATIONS then we're in a late-specified return
> > +     type, so create our own local specializations map; the current map is
> > +     either NULL or (in the case of recursive unification) might have
> > +     bindings that we don't want to use or alter.  */
> > +  local_specialization_stack lss (need_local_specializations
> > +				  ? lss_blank : lss_inherit);
> > +
> > +  if (unsubstituted_packs)
> >       {
> >         /* There were no real arguments, we're just replacing a parameter
> >   	 pack with another version of itself. Substitute into the
> > @@ -12910,16 +12922,6 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> >       gcc_assert (len >= 0);
> >   -  if (need_local_specializations)
> > -    {
> > -      /* We're in a late-specified return type, so create our own local
> > -	 specializations map; the current map is either NULL or (in the
> > -	 case of recursive unification) might have bindings that we don't
> > -	 want to use or alter.  */
> > -      saved_local_specializations = local_specializations;
> > -      local_specializations = new hash_map<tree, tree>;
> > -    }
> > -
> >     /* For each argument in each argument pack, substitute into the
> >        pattern.  */
> >     result = make_tree_vec (len);
> > @@ -12966,12 +12968,6 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> >           }
> >       }
> >   -  if (need_local_specializations)
> > -    {
> > -      delete local_specializations;
> > -      local_specializations = saved_local_specializations;
> > -    }
> > -
> >     /* If the dependent pack arguments were such that we end up with only a
> >        single pack expansion again, there's no need to keep it in a
> > TREE_VEC.  */
> >     if (len == 1 && TREE_CODE (result) == TREE_VEC
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > b/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > new file mode 100644
> > index 00000000000..f04d3f753ca
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > @@ -0,0 +1,16 @@
> > +// PR c++/94628
> > +// { dg-do compile { target c++11 } }
> > +
> > +int f(int, int);
> > +int f(int);
> > +
> > +template<class...Args>
> > +auto select(Args... args) -> decltype(f(args...))
> > +{
> > +  if (sizeof...(Args) > 1)
> > +    return select<char>(7);
> > +  else
> > +    return 0;
> > +}
> > +
> > +int a = select(0, 1);
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > b/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > new file mode 100644
> > index 00000000000..e8fcdd0a64d
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > @@ -0,0 +1,25 @@
> > +// PR c++/94628
> > +// A variant of variadic101.C where the recursive call to deref
> > +// has its first template argument explicitly given.
> > +// { dg-do compile { target c++11 } }
> > +
> > +template<class T>
> > +struct Container
> > +{ T f() const; };
> > +
> > +template<class T>
> > +T deref(const T& t)
> > +{ return t; }
> > +
> > +
> > +template <class T, class... Args>
> > +auto
> > +deref(const T& u, int r, Args... args)
> > +-> decltype(deref(u.f(), args...))
> > +{ return deref<decltype(u.f())>(u.f(), args...); }
> > +
> > +int main(void)
> > +{
> > +    Container<Container<int>> v;
> > +    deref(v,1,2);
> > +}
> > 
> 
>