Re: [PATCH] c++: argument pack expansion inside constraint [PR100138]

[Patrick Palka <ppalka@redhat.com>]

On Sat, May 8, 2021 at 8:42 AM Jason Merrill <jason@redhat.com> wrote:
>
> On 5/7/21 12:33 PM, Patrick Palka wrote:
> > This PR is about CTAD but the underlying problems are more general;
> > CTAD is a good trigger for them because of the necessary substitution
> > into constraints that deduction guide generation entails.
> >
> > In the testcase below, when generating the implicit deduction guide for
> > the constrained constructor template for A, we substitute the generic
> > flattening map 'tsubst_args' into the constructor's constraints.  During
> > this substitution, tsubst_pack_expansion returns a rebuilt pack
> > expansion for sizeof...(xs), but it's neglecting to carry over the
> > PACK_EXPANSION_LOCAL_P (and PACK_EXPANSION_SIZEOF_P) flag from the
> > original tree to the rebuilt one.  The flag is otherwise unset on the
> > original tree[1] but set for the rebuilt tree from make_pack_expansion
> > only because we're doing the CTAD at function scope (inside main).  This
> > leads us to crash when substituting into the pack expansion during
> > satisfaction because we don't have local_specializations set up (it'd be
> > set up for us if PACK_EXPANSION_LOCAL_P is unset)
> >
> > Similarly, when substituting into a constraint we need to set
> > cp_unevaluated since constraints are unevaluated operands.  This avoids
> > a crash during CTAD for C below.
> >
> > [1]: Although the original pack expansion is in a function context, I
> > guess it makes sense that PACK_EXPANSION_LOCAL_P is unset for it because
> > we can't rely on local specializations (which are formed when
> > substituting into the function declaration) during satisfaction.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, also tested on
> > cmcstl2 and range-v3, does this look OK for trunk?
>
> OK.

Would it be ok to backport this patch to the 11 branch given its
impact on concepts (or perhaps backport only part of it, say all but
the PACK_EXPANSION_LOCAL_P propagation since that part just avoids
ICEing on the invalid portions of the testcase)?

>
> > gcc/cp/ChangeLog:
> >
> >       PR c++/100138
> >       * constraint.cc (tsubst_constraint): Set up cp_unevaluated.
> >       (satisfy_atom): Set up iloc_sentinel before calling
> >       cxx_constant_value.
> >       * pt.c (tsubst_pack_expansion): When returning a rebuilt pack
> >       expansion, carry over PACK_EXPANSION_LOCAL_P and
> >       PACK_EXPANSION_SIZEOF_P from the original pack expansion.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR c++/100138
> >       * g++.dg/cpp2a/concepts-ctad4.C: New test.
> > ---
> >   gcc/cp/constraint.cc                        |  6 ++++-
> >   gcc/cp/pt.c                                 |  2 ++
> >   gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C | 25 +++++++++++++++++++++
> >   3 files changed, 32 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C
> >
> > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> > index 0709695fd08..30fccc46678 100644
> > --- a/gcc/cp/constraint.cc
> > +++ b/gcc/cp/constraint.cc
> > @@ -2747,6 +2747,7 @@ tsubst_constraint (tree t, tree args, tsubst_flags_t complain, tree in_decl)
> >     /* We also don't want to evaluate concept-checks when substituting the
> >        constraint-expressions of a declaration.  */
> >     processing_constraint_expression_sentinel s;
> > +  cp_unevaluated u;
> >     tree expr = tsubst_expr (t, args, complain, in_decl, false);
> >     return expr;
> >   }
> > @@ -3005,7 +3006,10 @@ satisfy_atom (tree t, tree args, sat_info info)
> >
> >     /* Compute the value of the constraint.  */
> >     if (info.noisy ())
> > -    result = cxx_constant_value (result);
> > +    {
> > +      iloc_sentinel ils (EXPR_LOCATION (result));
> > +      result = cxx_constant_value (result);
> > +    }
> >     else
> >       {
> >         result = maybe_constant_value (result, NULL_TREE,
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index 36a8cb5df5d..0d27dd1af65 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -13203,6 +13203,8 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
> >         else
> >       result = tsubst (pattern, args, complain, in_decl);
> >         result = make_pack_expansion (result, complain);
> > +      PACK_EXPANSION_LOCAL_P (result) = PACK_EXPANSION_LOCAL_P (t);
> > +      PACK_EXPANSION_SIZEOF_P (result) = PACK_EXPANSION_SIZEOF_P (t);
> >         if (PACK_EXPANSION_AUTO_P (t))
> >       {
> >         /* This is a fake auto... pack expansion created in add_capture with
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C
> > new file mode 100644
> > index 00000000000..95a3a22dd04
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C
> > @@ -0,0 +1,25 @@
> > +// PR c++/100138
> > +// { dg-do compile { target c++20 } }
> > +
> > +template <class T>
> > +struct A {
> > +  A(T, auto... xs) requires (sizeof...(xs) != 0) { }
> > +};
> > +
> > +constexpr bool f(...) { return true; }
> > +
> > +template <class T>
> > +struct B {
> > +  B(T, auto... xs) requires (f(xs...)); // { dg-error "constant expression" }
> > +};
> > +
> > +template <class T>
> > +struct C {
> > +  C(T, auto x) requires (f(x)); // { dg-error "constant expression" }
> > +};
> > +
> > +int main() {
> > +  A x{1, 2}; // { dg-bogus "" }
> > +  B y{1, 2}; // { dg-error "deduction|no match" }
> > +  C z{1, 2}; // { dg-error "deduction|no match" }
> > +}
> >
>