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

[Patrick Palka <ppalka@redhat.com>]

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?

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