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