Re: [PATCH] c++: parameter pack inside constexpr if [PR101764]

[Patrick Palka <ppalka@redhat.com>]

On Thu, 2 Sep 2021, Jason Merrill wrote:

> On 8/30/21 10:05 PM, Patrick Palka wrote:
> > Here when partially substituting into the pack expansion, substitution
> > into the constexpr if yields a still-dependent tree, so tsubst_expr
> > returns an IF_STMT with an unsubstituted IF_COND and with
> > IF_STMT_EXTRA_ARGS added to.  Hence after partial substitution
> > the pack expansion pattern still refers to the parameter pack 'ts' of
> > level 2 (and it's thus represented in the new
> > PACK_EXPANSION_PARAMETER_PACKS)
> > even though the partially instantiated generic lambda admits only one
> > level of template arguments.
> 
> > This causes us to crash during the
> > subsequent instantiation with the lambda's template arguments because of
> > the level mismatch.  (Likewise when the constexpr if is replaced by a
> > requires-expr, which too uses the extra args mechanism for delaying
> > partial instantiation.)
> 
> > So essentially, a pack expansion pattern that contains a parameter pack
> > inside an "extra args" tree doesn't play well with partial substitution
> > thereof.  This patch fixes this by forcing such pack expansions to use
> > the extra args mechanism as well.
> 
> Why is this specific to parameter packs?  Won't non-pack template parameters
> also suffer from the level mismatch?

IIUC it's specific to parameter packs because each parameter pack in the
pattern is also recorded in PACK_EXPANSION_PARAMETER_PACKS, which
tsubst_pack_expansion looks at to extra all argument packs from 'args'
that are relevant to the pattern.

I should clarify it's during the loop over PACK_EXPANSION_PARAMETER_PACKS
that we crash, because we fail to find an argument pack for 'ts' (which
still has the unlowered level 2), and we trip over the assert:

	{
	  /* We can't substitute for this parameter pack.  We use a flag as
	     well as the missing_level counter because function parameter
	     packs don't have a level.  */
	  gcc_assert (processing_template_decl || is_auto (parm_pack));
	  unsubstituted_packs = true;
	}

For non-pack template parameters (even those within extra args trees),
ordinary substitution is sufficient and does the right thing.

> I'd think it would be simpler to just
> note when the pattern contains a constexpr if or requires-expression, for
> which we can't substitute into the pattern like a pack expansion, and know we
> need to use extra args in that case.

Sounds good.  We'd force the extra args mechanism more than is strictly
necessary, but IIUC that should be harmless.

> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> > 
> > 	PR c++/101764
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (PACK_EXPANSION_FORCE_EXTRA_ARGS_P): New accessor
> > 	macro.
> > 	* pt.c (uses_extra_args_mechanism_p): New function.
> > 	(find_parameter_pack_data::found_pack_within_extra_args_tree_p):
> > 	New data member.
> > 	(find_parameter_pack_data::inside_extra_args_tree_p): Likewise.
> > 	(find_parameter_packs_r): Detect parameter packs within "extra
> > 	args" trees and set found_pack_within_extra_args_tree_p
> > 	appropriately.
> > 	(make_pack_expansion): Set PACK_EXPANSION_FORCE_EXTRA_ARGS_P if
> > 	found_pack_within_extra_args_tree_p.
> > 	(use_pack_expansion_extra_args_p): Return true if there were
> > 	unsubstituted packs and PACK_EXPANSION_FORCE_EXTRA_ARGS_P.
> > 	(tsubst_pack_expansion): Pass the pack expansion to
> > 	use_pack_expansion_extra_args_p.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp1z/constexpr-if35.C: New test.
> > ---
> >   gcc/cp/cp-tree.h                            |  5 ++
> >   gcc/cp/pt.c                                 | 69 ++++++++++++++++++++-
> >   gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C | 18 ++++++
> >   3 files changed, 90 insertions(+), 2 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
> > 
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index ce7ca53a113..06dec495428 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -493,6 +493,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
> >         CONSTRUCTOR_C99_COMPOUND_LITERAL (in CONSTRUCTOR)
> >         OVL_NESTED_P (in OVERLOAD)
> >         DECL_MODULE_EXPORT_P (in _DECL)
> > +      PACK_EXPANSION_FORCE_EXTRA_ARGS_P (in *_PACK_EXPANSION)
> >      4: IDENTIFIER_MARKED (IDENTIFIER_NODEs)
> >         TREE_HAS_CONSTRUCTOR (in INDIRECT_REF, SAVE_EXPR, CONSTRUCTOR,
> >   	  CALL_EXPR, or FIELD_DECL).
> > @@ -3902,6 +3903,10 @@ struct GTY(()) lang_decl {
> >   /* True iff this pack expansion is for auto... in lambda init-capture.  */
> >   #define PACK_EXPANSION_AUTO_P(NODE) TREE_LANG_FLAG_2 (NODE)
> >   +/* True if we must use PACK_EXPANSION_EXTRA_ARGS to avoid partial
> > +   substitution into this pack expansion.  */
> > +#define PACK_EXPANSION_FORCE_EXTRA_ARGS_P(NODE) TREE_LANG_FLAG_3 (NODE)
> > +
> >   /* True iff the wildcard can match a template parameter pack.  */
> >   #define WILDCARD_PACK_P(NODE) TREE_LANG_FLAG_0 (NODE)
> >   diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index fcf3ac31b25..a92dff88d9d 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -3855,6 +3855,20 @@ expand_builtin_pack_call (tree call, tree args,
> > tsubst_flags_t complain,
> >     return NULL_TREE;
> >   }
> >   +/* Return true if the tree T uses the extra args mechanism for
> > +   deferring partial substitution into it.  */
> > +
> > +static bool
> > +uses_extra_args_mechanism_p (tree t)
> > +{
> > +  return (PACK_EXPANSION_P (t)
> > +	  || TREE_CODE (t) == REQUIRES_EXPR
> > +	  || (TREE_CODE (t) == IF_STMT
> > +	      && IF_STMT_CONSTEXPR_P (t)));
> > +}
> > +
> > +static tree find_parameter_packs_r (tree *, int *, void*);
> > +
> >   /* Structure used to track the progress of find_parameter_packs_r.  */
> >   struct find_parameter_pack_data
> >   {
> > @@ -3867,6 +3881,16 @@ struct find_parameter_pack_data
> >       /* True iff we're making a type pack expansion.  */
> >     bool type_pack_expansion_p;
> > +
> > +  /* True iff we found a pack inside a subtree that uses the extra
> > +     args mechanism.  */
> > +  bool found_pack_within_extra_args_tree_p = false;
> > +
> > +private:
> > +  /* True iff find_parameter_packs_r is currently visiting a tree
> > +     that uses the extra args mechanism or a subtree thereof.  */
> > +  bool inside_extra_args_tree_p = false;
> > +  friend tree find_parameter_packs_r (tree *, int *, void*);
> >   };
> >     /* Identifies all of the argument packs that occur in a template
> > @@ -3968,6 +3992,37 @@ find_parameter_packs_r (tree *tp, int *walk_subtrees,
> > void* data)
> >         *ppd->parameter_packs = tree_cons (NULL_TREE, t,
> > *ppd->parameter_packs);
> >       }
> >   +  if (!ppd->inside_extra_args_tree_p
> 
> Why this flag?

With the revised patch below this flag is gone, but: it was needed
because inside this branch we walk again into 't' (rather than directly
into its subtrees), so without setting/checking inside_extra_args_tree_p
we'd get infinite recursion.  It allows us to avoid having to
specifically recurse into each subtree of IF_STMT / REQUIRES_EXPR here
(and any future extra args trees that get added).

> 
> > +      && uses_extra_args_mechanism_p (t)
> > +      && !PACK_EXPANSION_P (t))
> > +    {
> > +      /* This tree is using the extra args mechanism.  Update *ppd and
> > recurse
> > +	 so that we can try to detect a parameter pack within.  */
> 
> This comment needs to mention why pack expansions are different.  And it seems
> wrong to say that the tree *is using* the mechanism; it's a tree that would
> use the mechanism in case of partial instantiation.

Ack; I should probably say 'has (the extra args mechanism)' instead of
'uses' throughout.

> 
> > +      tree parameter_packs = NULL_TREE;
> > +      hash_set<tree> visited;
> > +
> > +	{
> > +	  auto iovr = make_temp_override (ppd->inside_extra_args_tree_p,
> > +					  true);
> > +	  auto povr = make_temp_override (ppd->parameter_packs,
> > +					  &parameter_packs);
> > +	  auto vovr = make_temp_override (ppd->visited,
> > +					  &visited);
> 
> Why override these?

This too is also gone with the revised patch, but: we needed to override
e.g. 'visited' because if a pack appears earlier in the pattern and then
later in the extra args tree, we wouldn't walk into the pack a second
time and so wouldn't notice it in inside the extra args tree.

Here's v2, which takes the simpler approach:

-- >8 --

	PR c++/101764

gcc/cp/ChangeLog:

	* cp-tree.h (PACK_EXPANSION_FORCE_EXTRA_ARGS_P): New accessor
	macro.
	* pt.c (has_extra_args_mechanism_p): New function.
	(find_parameter_pack_data::found_extra_args_tree_p):
	New data member.
	(find_parameter_packs_r): Set found_extra_args_tree_p
	appropriately.
	(make_pack_expansion): Set PACK_EXPANSION_FORCE_EXTRA_ARGS_P if
	found_extra_args_tree_p.
	(use_pack_expansion_extra_args_p): Return true if there were
	unsubstituted packs and PACK_EXPANSION_FORCE_EXTRA_ARGS_P.
	(tsubst_pack_expansion): Pass the pack expansion to
	use_pack_expansion_extra_args_p.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1z/constexpr-if35.C: New test.
---
 gcc/cp/cp-tree.h                            |  5 +++
 gcc/cp/pt.c                                 | 35 +++++++++++++++++++--
 gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C | 18 +++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index ceb53591926..706a51bd80c 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -493,6 +493,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
       CONSTRUCTOR_C99_COMPOUND_LITERAL (in CONSTRUCTOR)
       OVL_NESTED_P (in OVERLOAD)
       DECL_MODULE_EXPORT_P (in _DECL)
+      PACK_EXPANSION_FORCE_EXTRA_ARGS_P (in *_PACK_EXPANSION)
    4: IDENTIFIER_MARKED (IDENTIFIER_NODEs)
       TREE_HAS_CONSTRUCTOR (in INDIRECT_REF, SAVE_EXPR, CONSTRUCTOR,
 	  CALL_EXPR, or FIELD_DECL).
@@ -3903,6 +3904,10 @@ struct GTY(()) lang_decl {
 /* True iff this pack expansion is for auto... in lambda init-capture.  */
 #define PACK_EXPANSION_AUTO_P(NODE) TREE_LANG_FLAG_2 (NODE)
 
+/* True if we must use PACK_EXPANSION_EXTRA_ARGS to avoid partial
+   instantiation of this pack expansion.  */
+#define PACK_EXPANSION_FORCE_EXTRA_ARGS_P(NODE) TREE_LANG_FLAG_3 (NODE)
+
 /* True iff the wildcard can match a template parameter pack.  */
 #define WILDCARD_PACK_P(NODE) TREE_LANG_FLAG_0 (NODE)
 
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 1b81501386b..224dd9ebd2b 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -3855,6 +3855,18 @@ expand_builtin_pack_call (tree call, tree args, tsubst_flags_t complain,
   return NULL_TREE;
 }
 
+/* Return true if the tree T has the extra args mechanism for
+   avoiding partial instantiation.  */
+
+static bool
+has_extra_args_mechanism_p (const_tree t)
+{
+  return (PACK_EXPANSION_P (t) /* PACK_EXPANSION_EXTRA_ARGS  */
+	  || TREE_CODE (t) == REQUIRES_EXPR /* REQUIRES_EXPR_EXTRA_ARGS  */
+	  || (TREE_CODE (t) == IF_STMT
+	      && IF_STMT_CONSTEXPR_P (t))); /* IF_STMT_EXTRA_ARGS  */
+}
+
 /* Structure used to track the progress of find_parameter_packs_r.  */
 struct find_parameter_pack_data
 {
@@ -3867,6 +3879,9 @@ struct find_parameter_pack_data
 
   /* True iff we're making a type pack expansion.  */
   bool type_pack_expansion_p;
+
+  /* True iff we found a subtree that has the extra args mechanism.  */
+  bool found_extra_args_tree_p = false;
 };
 
 /* Identifies all of the argument packs that occur in a template
@@ -3968,6 +3983,9 @@ find_parameter_packs_r (tree *tp, int *walk_subtrees, void* data)
       *ppd->parameter_packs = tree_cons (NULL_TREE, t, *ppd->parameter_packs);
     }
 
+  if (has_extra_args_mechanism_p (t) && !PACK_EXPANSION_P (t))
+    ppd->found_extra_args_tree_p = true;
+
   if (TYPE_P (t))
     cp_walk_tree (&TYPE_CONTEXT (t),
 		  &find_parameter_packs_r, ppd, ppd->visited);
@@ -4229,6 +4247,14 @@ make_pack_expansion (tree arg, tsubst_flags_t complain)
   PACK_EXPANSION_PARAMETER_PACKS (result) = parameter_packs;
 
   PACK_EXPANSION_LOCAL_P (result) = at_function_scope_p ();
+  if (ppd.found_extra_args_tree_p)
+    /* If the pattern of this pack expansion contains a subtree that has
+       the extra args mechanism for avoiding partial instantiation, then
+       force this pack expansion to also use extra args.  Otherwise
+       partial instantiation of this pack expansion may not lower the
+       level of some parameter packs within the pattern, which would
+       confuse tsubst_pack_expansion later (PR101764).  */
+    PACK_EXPANSION_FORCE_EXTRA_ARGS_P (result) = true;
 
   return result;
 }
@@ -12405,10 +12431,15 @@ make_argument_pack_select (tree arg_pack, unsigned index)
     substitution.  */
 
 static bool
-use_pack_expansion_extra_args_p (tree parm_packs,
+use_pack_expansion_extra_args_p (tree t,
+				 tree parm_packs,
 				 int arg_pack_len,
 				 bool has_empty_arg)
 {
+  if (has_empty_arg
+      && PACK_EXPANSION_FORCE_EXTRA_ARGS_P (t))
+    return true;
+
   /* If one pack has an expansion and another pack has a normal
      argument or if one pack has an empty argument and an another
      one hasn't then tsubst_pack_expansion cannot perform the
@@ -13161,7 +13192,7 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain,
 
   /* We cannot expand this expansion expression, because we don't have
      all of the argument packs we need.  */
-  if (use_pack_expansion_extra_args_p (packs, len, unsubstituted_packs))
+  if (use_pack_expansion_extra_args_p (t, packs, len, unsubstituted_packs))
     {
       /* We got some full packs, but we can't substitute them in until we
 	 have values for all the packs.  So remember these until then.  */
diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
new file mode 100644
index 00000000000..cae690ba82c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
@@ -0,0 +1,18 @@
+// PR c++/101764
+// { dg-do compile { target c++17 } }
+
+void g(...);
+
+template<class>
+auto f() {
+  return [](auto... ts) {
+    g([] { if constexpr (decltype(ts){0}); }...);
+#if __cpp_concepts
+    g(requires { decltype(ts){0}; }...);
+#endif
+  };
+}
+
+int main() {
+  f<int>()('a', true);
+}
-- 
2.33.0.328.g8b7c11b866