Re: [PATCH] c++: template friend with variadic constraints [PR108066]

[Patrick Palka <ppalka@redhat.com>]

On Thu, 15 Dec 2022, Jason Merrill wrote:

> On 12/15/22 14:31, Patrick Palka wrote:
> > On Thu, 15 Dec 2022, Patrick Palka wrote:
> > 
> > > On Thu, 15 Dec 2022, Jason Merrill wrote:
> > > 
> > > > On 12/12/22 12:20, Patrick Palka wrote:
> > > > > When instantiating a constrained hidden template friend, we need to
> > > > > substitute into its constraints for sake of declaration matching.
> > > > 
> > > > Hmm, we shouldn't need to do declaration matching when there's only a
> > > > single
> > > > declaration.
> > > 
> > > Oops, indeed.  It looks like in this case we're not calling
> > > maybe_substitute_reqs_for during declaration matching, but rather from
> > > tsubst_friend_function and specifically for the non-trailing requirements:
> > > 
> > >    if (TREE_CODE (new_friend) == TEMPLATE_DECL)
> > >      {
> > >        DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (new_friend) = false;
> > >        DECL_USE_TEMPLATE (DECL_TEMPLATE_RESULT (new_friend)) = 0;
> > >        DECL_SAVED_TREE (DECL_TEMPLATE_RESULT (new_friend))
> > >          = DECL_SAVED_TREE (DECL_TEMPLATE_RESULT (decl));
> > > 
> > >        /* Substitute TEMPLATE_PARMS_CONSTRAINTS so that parameter levels
> > > will
> > >           match in decls_match.  */
> > >        tree parms = DECL_TEMPLATE_PARMS (new_friend);
> > >        tree treqs = TEMPLATE_PARMS_CONSTRAINTS (parms);
> > >        treqs = maybe_substitute_reqs_for (treqs, new_friend);
> > >        if (treqs != TEMPLATE_PARMS_CONSTRAINTS (parms))
> > >          {
> > >            TEMPLATE_PARMS_CONSTRAINTS (parms) = treqs;
> > >            /* As well as each TEMPLATE_PARM_CONSTRAINTS.  */
> > >            tsubst_each_template_parm_constraints (parms, args,
> > >                                                   tf_warning_or_error);
> > >          }
> > >      }
> > > 
> > > I'll adjust the commit message appropriately.
> > > 
> > > > 
> > > > > For
> > > > > this substitution we use a full argument vector whose outer levels
> > > > > correspond to the class's arguments and innermost level corresponds to
> > > > > the template's level-lowered generic arguments.
> > > > > 
> > > > > But for A<int>::f here, for which the relevant argument vector is
> > > > > {{int}, {Us...}}, this substitution triggers the assert in
> > > > > use_pack_expansion_extra_args_p since one argument is a pack expansion
> > > > > and the other isn't.
> > > > > 
> > > > > And for A<int, int>::f, for which the relevant argument vector is
> > > > > {{int, int}, {Us...}}, the use_pack_expansion_extra_args_p assert
> > > > > would
> > > > > also trigger but we first get a bogus "mismatched argument pack
> > > > > lengths"
> > > > > error from tsubst_pack_expansion.
> > > > > 
> > > > > These might ultimately be bugs in tsubst_pack_expansion, but it seems
> > > > > we can work around them by tweaking the constraint substitution in
> > > > > maybe_substitute_reqs_for to only use the friend's outer arguments in
> > > > > the case of a template friend.
> > > > 
> > > > Yes, this is how we normally substitute a member template during class
> > > > instantiation: with the class' template args, not its own.  The assert
> > > > seems
> > > > to be enforcing that.
> > > 
> > > Ah, makes snese.
> > > 
> > > > 
> > > > > This should be otherwise equivalent to
> > > > > substituting using the full arguments, since the template's innermost
> > > > > arguments are just its generic arguments with level=1.
> > > > > 
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
> > > > > for
> > > > > trunk/12?
> > > > > 
> > > > > 	PR c++/108066
> > > > > 	PR c++/108067
> > > > > 
> > > > > gcc/cp/ChangeLog:
> > > > > 
> > > > > 	* constraint.cc (maybe_substitute_reqs_for): For a template
> > > > > 	friend, substitute using only its outer arguments.  Remove
> > > > > 	dead uses_template_parms test.
> > > > 
> > > > I don't see any removal?
> > > 
> > > Oops, I reverted that change before posting the patch but forgot to adjust
> > > the
> > > ChangeLog as well.  Removing the uses_template_parms test seems to be safe
> > > but
> > > it's not necessary to fix the bug.
> > > 
> > > > 
> > > > > gcc/testsuite/ChangeLog:
> > > > > 
> > > > > 	* g++.dg/cpp2a/concepts-friend12.C: New test.
> > > > > ---
> > > > >    gcc/cp/constraint.cc                          |  8 +++++++
> > > > >    .../g++.dg/cpp2a/concepts-friend12.C          | 22
> > > > > +++++++++++++++++++
> > > > >    2 files changed, 30 insertions(+)
> > > > >    create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
> > > > > 
> > > > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> > > > > index d4cd92ec3b4..f9d1009c9fe 100644
> > > > > --- a/gcc/cp/constraint.cc
> > > > > +++ b/gcc/cp/constraint.cc
> > > > > @@ -1352,6 +1352,14 @@ maybe_substitute_reqs_for (tree reqs,
> > > > > const_tree
> > > > > decl)
> > > > >          tree tmpl = DECL_TI_TEMPLATE (decl);
> > > > >          tree gargs = generic_targs_for (tmpl);
> > > > >          processing_template_decl_sentinel s;
> > > > > +      if (PRIMARY_TEMPLATE_P (tmpl))
> > > > > +	{
> > > > > +	  if (TEMPLATE_ARGS_DEPTH (gargs) == 1)
> > > > > +	    return reqs;
> > > > > +	  ++processing_template_decl;
> > > > > +	  gargs = copy_node (gargs);
> > > > > +	  --TREE_VEC_LENGTH (gargs);
> > > > 
> > > > Maybe instead of messing with TEMPLATE_ARGS_DEPTH we want to grab the
> > > > targs
> > > > for DECL_FRIEND_CONTEXT instead of decl itself?
> > > 
> > > IIUC DECL_FRIEND_CONTEXT wouldn't give us the right args if the template
> > > friend
> > > is declared inside a partial specialization:
> > > 
> > >    template<class T, class U>
> > >    concept C = __is_same(T, U);
> > > 
> > >    template<class T>
> > >    struct A;
> > > 
> > >    template<class T>
> > >    struct A<T*> {
> > >      template<class U>
> > >        requires (__is_same(T, U))
> > >      friend void f() { };
> > >    };
> > > 
> > >    template struct A<int*>;
> > > 
> > > The DECL_FRIEND_CONTEXT of f is A<int*>, whose TYPE_TI_ARGS are {int*},
> > > relative to the primary template instead of the partial specialization.
> > > So
> > > we'd wrongly end up substituting T=int* instead of T=int into f's
> > > TEMPLATE_PARMS_CONSTRAINTS.
> > > 
> > > r12-6950-g76dc465aaf1b74 fixed a similar issue when substituting into
> > > class-scope deduction guides declared inside a partial specialization.
> 
> Ah, so this is a workaround for not being able to get at the template args
> used in substituting A<int*> from A<int*> itself.
> 
> Maybe factor this out from here and ctor_deduction_guides_for into an
> outer_template_args function with a comment to that effect?

Like so?  Bootstrapped and regtested on x86_64-pc-linux-gnu.

> 
> Maybe it would make sense to update CLASSTYPE_TEMPLATE_INFO for A<int*> after
> we choose a partial specialization in instantiate_class_template, but I'm sure
> a bunch of places would need to be adjusted to handle that.

*nod* Having that information readily available instead of having to
call most_specialized_partial_spec would be nice.

-- >8 --


Subject: [PATCH] c++: template friend with variadic constraints [PR107853]

	PR c++/107853

gcc/cp/ChangeLog:

	* constraint.cc (maybe_substitute_reqs_for): For a template
	friend, substitute using only its outer arguments via
	outer_template_args.
	* cp-tree.h (outer_template_args): Declare.
	* pt.cc (outer_template_args): Define, factored out and
	generalized from ...
	(ctor_deduction_guides_for): ... here.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-friend12.C: New test.
	* g++.dg/cpp2a/concepts-friend13.C: New test.
---
 gcc/cp/constraint.cc                          |  7 ++--
 gcc/cp/cp-tree.h                              |  1 +
 gcc/cp/pt.cc                                  | 35 +++++++++++++------
 .../g++.dg/cpp2a/concepts-friend12.C          | 21 +++++++++++
 .../g++.dg/cpp2a/concepts-friend13.C          | 20 +++++++++++
 5 files changed, 71 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 37eae03afdb..247a8278d40 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -1350,11 +1350,12 @@ maybe_substitute_reqs_for (tree reqs, const_tree decl)
   if (DECL_UNIQUE_FRIEND_P (decl) && DECL_TEMPLATE_INFO (decl))
     {
       tree tmpl = DECL_TI_TEMPLATE (decl);
-      tree gargs = generic_targs_for (tmpl);
+      tree outer_args = outer_template_args (tmpl);
       processing_template_decl_sentinel s;
-      if (uses_template_parms (gargs))
+      if (PRIMARY_TEMPLATE_P (tmpl)
+	  || uses_template_parms (outer_args))
 	++processing_template_decl;
-      reqs = tsubst_constraint (reqs, gargs,
+      reqs = tsubst_constraint (reqs, outer_args,
 				tf_warning_or_error, NULL_TREE);
     }
   return reqs;
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 541d760492d..1f4967c2ba0 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7056,6 +7056,7 @@ extern tree maybe_set_retval_sentinel		(void);
 extern tree template_parms_to_args		(tree);
 extern tree template_parms_level_to_args	(tree);
 extern tree generic_targs_for			(tree);
+extern tree outer_template_args			(tree);
 
 /* in expr.cc */
 extern tree cplus_expand_constant		(tree);
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index e68c74913f5..c0593e9cac6 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -4958,6 +4958,29 @@ generic_targs_for (tree tmpl)
   return template_parms_to_args (DECL_TEMPLATE_PARMS (tmpl));
 }
 
+/* Return the template arguments corresponding to the template
+   parameters of TMPL's enclosing scope.  When TMPL is a member
+   template of a partial specialization, this returns the arguments
+   for the partial specialization as opposed to those for the primary
+   template, which is the main difference between this function and
+   simply inspecting e.g. the TYPE_TI_ARGS of TMPL's DECL_CONTEXT.  */
+
+tree
+outer_template_args (tree tmpl)
+{
+  tree ti = get_template_info (DECL_TEMPLATE_RESULT (tmpl));
+  if (!ti)
+    return NULL_TREE;
+  tree args = TI_ARGS (ti);
+  if (!PRIMARY_TEMPLATE_P (tmpl))
+    return args;
+  if (TMPL_ARGS_DEPTH (args) == 1)
+    return NULL_TREE;
+  args = copy_node (args);
+  --TREE_VEC_LENGTH (args);
+  return args;
+}
+
 /* Update the declared TYPE by doing any lookups which were thought to be
    dependent, but are not now that we know the SCOPE of the declarator.  */
 
@@ -30081,16 +30104,8 @@ alias_ctad_tweaks (tree tmpl, tree uguides)
 static tree
 ctor_deduction_guides_for (tree tmpl, tsubst_flags_t complain)
 {
-  tree type = TREE_TYPE (tmpl);
-  tree outer_args = NULL_TREE;
-  if (DECL_CLASS_SCOPE_P (tmpl)
-      && CLASSTYPE_TEMPLATE_INSTANTIATION (DECL_CONTEXT (tmpl)))
-    {
-      outer_args = copy_node (CLASSTYPE_TI_ARGS (type));
-      gcc_assert (TMPL_ARGS_DEPTH (outer_args) > 1);
-      --TREE_VEC_LENGTH (outer_args);
-      type = TREE_TYPE (most_general_template (tmpl));
-    }
+  tree outer_args = outer_template_args (tmpl);
+  tree type = TREE_TYPE (most_general_template (tmpl));
 
   tree cands = NULL_TREE;
 
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C b/gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
new file mode 100644
index 00000000000..9687be5931a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-friend12.C
@@ -0,0 +1,21 @@
+// PR c++/107853
+// { dg-do compile { target c++20 } }
+
+template<class T, class U>
+concept C = __is_same(T, U);
+
+template<class... Ts>
+struct A {
+  template<class... Us>
+    requires (C<Ts, Us> && ...)
+  friend void f(A, A<Us...>) { }
+};
+
+int main() {
+  A<int> x;
+  f(x, x);
+  A<int, int> y;
+  f(y, y);
+  A<char> z;
+  f(x, z); // { dg-error "no match" }
+}
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C b/gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C
new file mode 100644
index 00000000000..3cc4505a7ef
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-friend13.C
@@ -0,0 +1,20 @@
+// Verify we substitute the correct outer template arguments
+// when instantiating a constrained template friend declared
+// inside a partial specialization.
+// { dg-do compile { target c++20 } }
+
+template<class U>
+  requires __is_same(int*, U)
+void f() { };
+
+template<class T>
+struct A;
+
+template<class T>
+struct A<T*> {
+  template<class U>
+    requires __is_same(T, U)
+  friend void f() { } // { dg-bogus "redefinition" }
+};
+
+template struct A<int*>;
-- 
2.39.0.95.g7c2ef319c5