Re: [PATCH] c++: template friend with variadic constraints [PR108066]

[Jason Merrill <jason@redhat.com>]

On 12/22/22 12:34, Patrick Palka wrote:
> On Thu, 15 Dec 2022, Jason Merrill wrote:
> 
>> On 12/15/22 14:31, Patrick Palka wrote:
>>> On Thu, 15 Dec 2022, Patrick Palka wrote:
>>>
>>>> On Thu, 15 Dec 2022, Jason Merrill wrote:
>>>>
>>>>> On 12/12/22 12:20, Patrick Palka wrote:
>>>>>> When instantiating a constrained hidden template friend, we need to
>>>>>> substitute into its constraints for sake of declaration matching.
>>>>>
>>>>> Hmm, we shouldn't need to do declaration matching when there's only a
>>>>> single
>>>>> declaration.
>>>>
>>>> Oops, indeed.  It looks like in this case we're not calling
>>>> maybe_substitute_reqs_for during declaration matching, but rather from
>>>> tsubst_friend_function and specifically for the non-trailing requirements:
>>>>
>>>>     if (TREE_CODE (new_friend) == TEMPLATE_DECL)
>>>>       {
>>>>         DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (new_friend) = false;
>>>>         DECL_USE_TEMPLATE (DECL_TEMPLATE_RESULT (new_friend)) = 0;
>>>>         DECL_SAVED_TREE (DECL_TEMPLATE_RESULT (new_friend))
>>>>           = DECL_SAVED_TREE (DECL_TEMPLATE_RESULT (decl));
>>>>
>>>>         /* Substitute TEMPLATE_PARMS_CONSTRAINTS so that parameter levels
>>>> will
>>>>            match in decls_match.  */
>>>>         tree parms = DECL_TEMPLATE_PARMS (new_friend);
>>>>         tree treqs = TEMPLATE_PARMS_CONSTRAINTS (parms);
>>>>         treqs = maybe_substitute_reqs_for (treqs, new_friend);
>>>>         if (treqs != TEMPLATE_PARMS_CONSTRAINTS (parms))
>>>>           {
>>>>             TEMPLATE_PARMS_CONSTRAINTS (parms) = treqs;
>>>>             /* As well as each TEMPLATE_PARM_CONSTRAINTS.  */
>>>>             tsubst_each_template_parm_constraints (parms, args,
>>>>                                                    tf_warning_or_error);
>>>>           }
>>>>       }
>>>>
>>>> I'll adjust the commit message appropriately.
>>>>
>>>>>
>>>>>> For
>>>>>> this substitution we use a full argument vector whose outer levels
>>>>>> correspond to the class's arguments and innermost level corresponds to
>>>>>> the template's level-lowered generic arguments.
>>>>>>
>>>>>> But for A<int>::f here, for which the relevant argument vector is
>>>>>> {{int}, {Us...}}, this substitution triggers the assert in
>>>>>> use_pack_expansion_extra_args_p since one argument is a pack expansion
>>>>>> and the other isn't.
>>>>>>
>>>>>> And for A<int, int>::f, for which the relevant argument vector is
>>>>>> {{int, int}, {Us...}}, the use_pack_expansion_extra_args_p assert
>>>>>> would
>>>>>> also trigger but we first get a bogus "mismatched argument pack
>>>>>> lengths"
>>>>>> error from tsubst_pack_expansion.
>>>>>>
>>>>>> These might ultimately be bugs in tsubst_pack_expansion, but it seems
>>>>>> we can work around them by tweaking the constraint substitution in
>>>>>> maybe_substitute_reqs_for to only use the friend's outer arguments in
>>>>>> the case of a template friend.
>>>>>
>>>>> Yes, this is how we normally substitute a member template during class
>>>>> instantiation: with the class' template args, not its own.  The assert
>>>>> seems
>>>>> to be enforcing that.
>>>>
>>>> Ah, makes snese.
>>>>
>>>>>
>>>>>> This should be otherwise equivalent to
>>>>>> substituting using the full arguments, since the template's innermost
>>>>>> arguments are just its generic arguments with level=1.
>>>>>>
>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
>>>>>> for
>>>>>> trunk/12?
>>>>>>
>>>>>> 	PR c++/108066
>>>>>> 	PR c++/108067
>>>>>>
>>>>>> gcc/cp/ChangeLog:
>>>>>>
>>>>>> 	* constraint.cc (maybe_substitute_reqs_for): For a template
>>>>>> 	friend, substitute using only its outer arguments.  Remove
>>>>>> 	dead uses_template_parms test.
>>>>>
>>>>> I don't see any removal?
>>>>
>>>> Oops, I reverted that change before posting the patch but forgot to adjust
>>>> the
>>>> ChangeLog as well.  Removing the uses_template_parms test seems to be safe
>>>> but
>>>> it's not necessary to fix the bug.
>>>>
>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> 	* g++.dg/cpp2a/concepts-friend12.C: New test.
>>>>>> ---
>>>>>>     gcc/cp/constraint.cc                          |  8 +++++++
>>>>>>     .../g++.dg/cpp2a/concepts-friend12.C          | 22
>>>>>> +++++++++++++++++++
>>>>>>     2 files changed, 30 insertions(+)
>>>>>>     create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
>>>>>>
>>>>>> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
>>>>>> index d4cd92ec3b4..f9d1009c9fe 100644
>>>>>> --- a/gcc/cp/constraint.cc
>>>>>> +++ b/gcc/cp/constraint.cc
>>>>>> @@ -1352,6 +1352,14 @@ maybe_substitute_reqs_for (tree reqs,
>>>>>> const_tree
>>>>>> decl)
>>>>>>           tree tmpl = DECL_TI_TEMPLATE (decl);
>>>>>>           tree gargs = generic_targs_for (tmpl);
>>>>>>           processing_template_decl_sentinel s;
>>>>>> +      if (PRIMARY_TEMPLATE_P (tmpl))
>>>>>> +	{
>>>>>> +	  if (TEMPLATE_ARGS_DEPTH (gargs) == 1)
>>>>>> +	    return reqs;
>>>>>> +	  ++processing_template_decl;
>>>>>> +	  gargs = copy_node (gargs);
>>>>>> +	  --TREE_VEC_LENGTH (gargs);
>>>>>
>>>>> Maybe instead of messing with TEMPLATE_ARGS_DEPTH we want to grab the
>>>>> targs
>>>>> for DECL_FRIEND_CONTEXT instead of decl itself?
>>>>
>>>> IIUC DECL_FRIEND_CONTEXT wouldn't give us the right args if the template
>>>> friend
>>>> is declared inside a partial specialization:
>>>>
>>>>     template<class T, class U>
>>>>     concept C = __is_same(T, U);
>>>>
>>>>     template<class T>
>>>>     struct A;
>>>>
>>>>     template<class T>
>>>>     struct A<T*> {
>>>>       template<class U>
>>>>         requires (__is_same(T, U))
>>>>       friend void f() { };
>>>>     };
>>>>
>>>>     template struct A<int*>;
>>>>
>>>> The DECL_FRIEND_CONTEXT of f is A<int*>, whose TYPE_TI_ARGS are {int*},
>>>> relative to the primary template instead of the partial specialization.
>>>> So
>>>> we'd wrongly end up substituting T=int* instead of T=int into f's
>>>> TEMPLATE_PARMS_CONSTRAINTS.
>>>>
>>>> r12-6950-g76dc465aaf1b74 fixed a similar issue when substituting into
>>>> class-scope deduction guides declared inside a partial specialization.
>>
>> Ah, so this is a workaround for not being able to get at the template args
>> used in substituting A<int*> from A<int*> itself.
>>
>> Maybe factor this out from here and ctor_deduction_guides_for into an
>> outer_template_args function with a comment to that effect?
> 
> Like so?  Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK.

>> Maybe it would make sense to update CLASSTYPE_TEMPLATE_INFO for A<int*> after
>> we choose a partial specialization in instantiate_class_template, but I'm sure
>> a bunch of places would need to be adjusted to handle that.
> 
> *nod* Having that information readily available instead of having to
> call most_specialized_partial_spec would be nice.
> 
> -- >8 --
> 
> 
> Subject: [PATCH] c++: template friend with variadic constraints [PR107853]
> 
> 	PR c++/107853
> 
> gcc/cp/ChangeLog:
> 
> 	* constraint.cc (maybe_substitute_reqs_for): For a template
> 	friend, substitute using only its outer arguments via
> 	outer_template_args.
> 	* cp-tree.h (outer_template_args): Declare.
> 	* pt.cc (outer_template_args): Define, factored out and
> 	generalized from ...
> 	(ctor_deduction_guides_for): ... here.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-friend12.C: New test.
> 	* g++.dg/cpp2a/concepts-friend13.C: New test.
> ---
>   gcc/cp/constraint.cc                          |  7 ++--
>   gcc/cp/cp-tree.h                              |  1 +
>   gcc/cp/pt.cc                                  | 35 +++++++++++++------
>   .../g++.dg/cpp2a/concepts-friend12.C          | 21 +++++++++++
>   .../g++.dg/cpp2a/concepts-friend13.C          | 20 +++++++++++
>   5 files changed, 71 insertions(+), 13 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C
> 
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index 37eae03afdb..247a8278d40 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -1350,11 +1350,12 @@ maybe_substitute_reqs_for (tree reqs, const_tree decl)
>     if (DECL_UNIQUE_FRIEND_P (decl) && DECL_TEMPLATE_INFO (decl))
>       {
>         tree tmpl = DECL_TI_TEMPLATE (decl);
> -      tree gargs = generic_targs_for (tmpl);
> +      tree outer_args = outer_template_args (tmpl);
>         processing_template_decl_sentinel s;
> -      if (uses_template_parms (gargs))
> +      if (PRIMARY_TEMPLATE_P (tmpl)
> +	  || uses_template_parms (outer_args))
>   	++processing_template_decl;
> -      reqs = tsubst_constraint (reqs, gargs,
> +      reqs = tsubst_constraint (reqs, outer_args,
>   				tf_warning_or_error, NULL_TREE);
>       }
>     return reqs;
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 541d760492d..1f4967c2ba0 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7056,6 +7056,7 @@ extern tree maybe_set_retval_sentinel		(void);
>   extern tree template_parms_to_args		(tree);
>   extern tree template_parms_level_to_args	(tree);
>   extern tree generic_targs_for			(tree);
> +extern tree outer_template_args			(tree);
>   
>   /* in expr.cc */
>   extern tree cplus_expand_constant		(tree);
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index e68c74913f5..c0593e9cac6 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -4958,6 +4958,29 @@ generic_targs_for (tree tmpl)
>     return template_parms_to_args (DECL_TEMPLATE_PARMS (tmpl));
>   }
>   
> +/* Return the template arguments corresponding to the template
> +   parameters of TMPL's enclosing scope.  When TMPL is a member
> +   template of a partial specialization, this returns the arguments
> +   for the partial specialization as opposed to those for the primary
> +   template, which is the main difference between this function and
> +   simply inspecting e.g. the TYPE_TI_ARGS of TMPL's DECL_CONTEXT.  */
> +
> +tree
> +outer_template_args (tree tmpl)
> +{
> +  tree ti = get_template_info (DECL_TEMPLATE_RESULT (tmpl));
> +  if (!ti)
> +    return NULL_TREE;
> +  tree args = TI_ARGS (ti);
> +  if (!PRIMARY_TEMPLATE_P (tmpl))
> +    return args;
> +  if (TMPL_ARGS_DEPTH (args) == 1)
> +    return NULL_TREE;
> +  args = copy_node (args);
> +  --TREE_VEC_LENGTH (args);
> +  return args;
> +}
> +
>   /* Update the declared TYPE by doing any lookups which were thought to be
>      dependent, but are not now that we know the SCOPE of the declarator.  */
>   
> @@ -30081,16 +30104,8 @@ alias_ctad_tweaks (tree tmpl, tree uguides)
>   static tree
>   ctor_deduction_guides_for (tree tmpl, tsubst_flags_t complain)
>   {
> -  tree type = TREE_TYPE (tmpl);
> -  tree outer_args = NULL_TREE;
> -  if (DECL_CLASS_SCOPE_P (tmpl)
> -      && CLASSTYPE_TEMPLATE_INSTANTIATION (DECL_CONTEXT (tmpl)))
> -    {
> -      outer_args = copy_node (CLASSTYPE_TI_ARGS (type));
> -      gcc_assert (TMPL_ARGS_DEPTH (outer_args) > 1);
> -      --TREE_VEC_LENGTH (outer_args);
> -      type = TREE_TYPE (most_general_template (tmpl));
> -    }
> +  tree outer_args = outer_template_args (tmpl);
> +  tree type = TREE_TYPE (most_general_template (tmpl));
>   
>     tree cands = NULL_TREE;
>   
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C b/gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
> new file mode 100644
> index 00000000000..9687be5931a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
> @@ -0,0 +1,21 @@
> +// PR c++/107853
> +// { dg-do compile { target c++20 } }
> +
> +template<class T, class U>
> +concept C = __is_same(T, U);
> +
> +template<class... Ts>
> +struct A {
> +  template<class... Us>
> +    requires (C<Ts, Us> && ...)
> +  friend void f(A, A<Us...>) { }
> +};
> +
> +int main() {
> +  A<int> x;
> +  f(x, x);
> +  A<int, int> y;
> +  f(y, y);
> +  A<char> z;
> +  f(x, z); // { dg-error "no match" }
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C b/gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C
> new file mode 100644
> index 00000000000..3cc4505a7ef
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C
> @@ -0,0 +1,20 @@
> +// Verify we substitute the correct outer template arguments
> +// when instantiating a constrained template friend declared
> +// inside a partial specialization.
> +// { dg-do compile { target c++20 } }
> +
> +template<class U>
> +  requires __is_same(int*, U)
> +void f() { };
> +
> +template<class T>
> +struct A;
> +
> +template<class T>
> +struct A<T*> {
> +  template<class U>
> +    requires __is_same(T, U)
> +  friend void f() { } // { dg-bogus "redefinition" }
> +};
> +
> +template struct A<int*>;