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

[Jason Merrill <jason@redhat.com>]

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?

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.

> Here's a concrete testcase that we'd begin to reject if we used
> the args from DECL_FRIEND_CONTEXT for substitution:
> 
>    // Verify we substitute the correct outer template arguments
>    // when instantiating a constrained template friend.
>    // { 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*>;