Re: [PATCH] c++: dependent constraint on placeholder return type [PR96443]

[Jason Merrill <jason@redhat.com>]

On 8/4/20 8:00 PM, Patrick Palka wrote:
> On Tue, 4 Aug 2020, Patrick Palka wrote:
> 
>> In the testcase below, we never substitute function-template arguments
>> into f15<int>'s placeholder-return-type constraint, which leads to us
>> incorrectly rejecting this instantiation in do_auto_deduction due to
>> satisfaction failure (of the constraint SameAs<int, decltype(x)>).
>>
>> The fact that we incorrectly reject this testcase is masked by the
>> other instantiation f15<char>, which we correctly reject and diagnose
>> (by accident).
>>
>> A good place to do this missing substitution seems to be during
>> TEMPLATE_TYPE_PARM level lowering.  So this patch adds a call to
>> tsubst_constraint there, and also adds dg-bogus directives to this
>> testcase wherever we expect instantiation to succeed. (So without the
>> substitution fix, this last dg-bogus would FAIL).

>> Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and
>> range-v3 projects.  Does this look OK to commit?
>>
>> gcc/cp/ChangeLog:
>>
>> 	PR c++/96443
>> 	* pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into
>> 	the constraints on a placeholder type when its level.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c++/96443
>> 	* g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus wherever we expect
>> 	instantiation to succeed.
> 
> Looking back at this patch with fresh eyes, I realized that the commit
> message is not the best.  I rewrote the commit message to hopefully be
> more coherent below:
> 
> -- >8 --
> 
> Subject: [PATCH] c++: dependent constraint on placeholder return type
>   [PR96443]
> 
> In the testcase concepts-ts1.C, we're incorrectly rejecting the call to
> 'f15(0)' due to satisfaction failure of the function's
> placeholder-return-type constraint.
> 
> The testcase doesn't spot this rejection because the error we emit for
> the constraint failure points to f15's return statement instead of the
> call site, and we already have a dg-error at the return statement to
> verify the (correct) rejection of the call f15('a').  So in order to
> verify that we indeed accept the call 'f15(0)', we need to add a
> dg-bogus directive at the call site to look for the "required from here"
> diagnostic line that generally accompanies an instantiation failure.
> 
> As for why satisfaction failure occurs, it turns out that we never
> substitute the template arguments of a function template specialization
> in to its placeholder-return-type constraint.  So in this case during
> do_auto_deduction, we end up checking satisfaction of the still-dependent
> constraint SameAs<int, decltype(x)> from do_auto_deduction, which fails
> because it's dependent.
> 
> A good place to do this missing substitution seems to be during
> TEMPLATE_TYPE_PARM level lowering; so this patch adds a call to
> tsubst_constraint there.

Doing substitution seems like the wrong approach here; requirements 
should never be substituted except as part of satisfaction calculation 
(or, rarely, for declaration matching).  If we aren't communicating all 
the necessary template arguments to the later satisfaction, that's what 
we need to fix.

> Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and
> range-v3 projects.  Does this look OK to commit?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/96443
> 	* pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into
> 	the constraints on a placeholder type when reducing its level.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/96443
> 	* g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus to the call to f15
> 	that we expect to accept.
> ---
>   gcc/cp/pt.c                               | 7 ++++---
>   gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index e7496002c1c..9f3426f8249 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>   
>                   if (TREE_CODE (t) == TEMPLATE_TYPE_PARM)
>   		  {
> -		    /* Propagate constraints on placeholders since they are
> -		       only instantiated during satisfaction.  */
> +		    /* Substitute constraints on placeholders when reducing
> +		       their level.  */
>   		    if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t))
> -		      PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr;
> +		      PLACEHOLDER_TYPE_CONSTRAINTS (r)
> +			= tsubst_constraint (constr, args, complain, in_decl);
>   		    else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t))
>   		      {
>   			pl = tsubst_copy (pl, args, complain, in_decl);
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C
> index 1cefe3b243f..a116cac4ea4 100644
> --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C
> @@ -40,7 +40,7 @@ void driver()
>     f3('a'); // { dg-error "" }
>     f4(0, 0);
>     f4(0, 'a'); // { dg-error "" }
> -  f15(0);
> +  f15(0); // { dg-bogus "" }
>     f15('a'); // { dg-message "" }
>   }
>   
>