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

[Patrick Palka <ppalka@redhat.com>]

On Thu, 6 Aug 2020, Patrick Palka wrote:

> On Wed, 5 Aug 2020, Jason Merrill wrote:
> 
> > 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.
> 
> Ah okay, that makes sense.
> 
> It also looks like the question of perform a single full substitution
> (during auto deduction) vs two substitutions may also be a correctness
> issue in light of SFINAE.  Consider the following testcase:
> 
>     template<typename T, typename U>
>     concept C = true;
> 
>     auto f(auto x) -> C<decltype(x.fail())> auto { return 0; }
>     auto f(auto x, ...) { return 1; }
> 
>     int a = f(0);
> 
> If we do a single substitution, then the substitution failure is a hard
> error and we'll reject this testcase.  If we do two substitutions, then
> it's a SFINAE error and we select the second overload.  Would we be
> correct to issue a hard error here?

Please ignore the previous message :)  I had missed that the
substitution failure should result in the constraint evaluating to false
as part of satisfaction.  Sorry for the noise.

> 
> > 
> > > 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 "" }
> > >   }
> > >   
> > 
> > 
>