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