c++/modules: Handle instantiating qualified template friend classes [PR115801]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

Another potential approach would be to go searching for this unexported
type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by
expanding on the lookup_imported_temploid_friend hack.  I'm still not
exactly sure how name lookup for template friends is supposed to behave
though, specifically in terms of when and where they should conflict
with other entities with the same name.

The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2
and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to
the usual rules in [basic.link] and [basic.scope.scope], but how these
all are supposed to interact isn't super clear to me right now.

Additionally I wonder if maybe the better approach long-term would be to
focus on getting textual redefinitions working first, and then reuse
whatever logic we build for that to handle template friends rather than
relying on finding these hidden 'mergeable' slots first.

-- >8 --

With modules it may be the case that a template friend class provided
with a qualified name is not found by name lookup at instantiation time,
due to the class not being exported from its module.  This causes issues
in tsubst_friend_class which did not handle this case.

This patch fixes the issue by just doing nothing when it detects this
case; this causes the named class to be added as a pending friend which
will be resolved once a definition is seen (lazy loaded).

	PR c++/115801

gcc/cp/ChangeLog:

	* pt.cc (tsubst_friend_class): Handle friend_tmpl without extra
	template parms.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/tpl-friend-16_a.C: New test.
	* g++.dg/modules/tpl-friend-16_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/pt.cc                                  |  9 +++++
 .../g++.dg/modules/tpl-friend-16_a.C          | 40 +++++++++++++++++++
 .../g++.dg/modules/tpl-friend-16_b.C          | 17 ++++++++
 3 files changed, 66 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C

On 8/6/24 2:35 AM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> Another potential approach would be to go searching for this unexported
> type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by
> expanding on the lookup_imported_temploid_friend hack.  I'm still not
> exactly sure how name lookup for template friends is supposed to behave
> though, specifically in terms of when and where they should conflict
> with other entities with the same name.

CWG2588 tried to clarify this in https://eel.is/c++draft/basic#link-4.8 
-- if there's a matching reachable declaration, the friend refers to it 
even if it isn't visible to name lookup.

It seems like an oversight that the new second bullet refers 
specifically to functions, it seems natural for it to apply to classes 
as well.

So, they correspond but do not conflict because they declare the same 
entity.

> The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2
> and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to
> the usual rules in [basic.link] and [basic.scope.scope], but how these
> all are supposed to interact isn't super clear to me right now.
> 
> Additionally I wonder if maybe the better approach long-term would be to
> focus on getting textual redefinitions working first, and then reuse
> whatever logic we build for that to handle template friends rather than
> relying on finding these hidden 'mergeable' slots first.

I have a WIP patch to allow textual redefinitions by teaching 
duplicate_decls that it's OK to redefine an imported GM entity, so 
check_module_override works.

My current plan is to then just token-skip the bodies.  This won't 
diagnose ODR problems, but our module merging doesn't do that 
consistently either.

> @@ -11800,6 +11800,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
>             input_location = saved_input_location;
>   	}
>       }
> +  else if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> +	   <= TMPL_ARGS_DEPTH (args))

This condition seems impossible normally; it's only satisfied in this 
testcase because friend_tmpl doesn't actually represent the friend 
declaration, it's already the named class template.  So the substitution 
in the next else fails because it was done already.

If this condition is true, we could set tmpl = friend_tmpl earlier, and 
skip doing name lookup at all.

It's interesting that the previous if does the same depth comparison, 
and that dates back to 2002; I wonder why it was needed then?

Jason

On Wed, Aug 07, 2024 at 01:44:31PM -0400, Jason Merrill wrote:
> On 8/6/24 2:35 AM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > 
> > Another potential approach would be to go searching for this unexported
> > type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by
> > expanding on the lookup_imported_temploid_friend hack.  I'm still not
> > exactly sure how name lookup for template friends is supposed to behave
> > though, specifically in terms of when and where they should conflict
> > with other entities with the same name.
> 
> CWG2588 tried to clarify this in https://eel.is/c++draft/basic#link-4.8 --
> if there's a matching reachable declaration, the friend refers to it even if
> it isn't visible to name lookup.
> 
> It seems like an oversight that the new second bullet refers specifically to
> functions, it seems natural for it to apply to classes as well.
> 
> So, they correspond but do not conflict because they declare the same
> entity.
> 

Right, makes sense.  OK, I'll work on filling out our testcases to make
sure that we cover everything under that interpretation and potentially
come back to making an ANY_REACHABLE flag for this.

> > The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2
> > and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to
> > the usual rules in [basic.link] and [basic.scope.scope], but how these
> > all are supposed to interact isn't super clear to me right now.
> > 
> > Additionally I wonder if maybe the better approach long-term would be to
> > focus on getting textual redefinitions working first, and then reuse
> > whatever logic we build for that to handle template friends rather than
> > relying on finding these hidden 'mergeable' slots first.
> 
> I have a WIP patch to allow textual redefinitions by teaching
> duplicate_decls that it's OK to redefine an imported GM entity, so
> check_module_override works.
> 
> My current plan is to then just token-skip the bodies.  This won't diagnose
> ODR problems, but our module merging doesn't do that consistently either.
> 
> > @@ -11800,6 +11800,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> >             input_location = saved_input_location;
> >   	}
> >       }
> > +  else if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> > +	   <= TMPL_ARGS_DEPTH (args))
> 
> This condition seems impossible normally; it's only satisfied in this
> testcase because friend_tmpl doesn't actually represent the friend
> declaration, it's already the named class template.  So the substitution in
> the next else fails because it was done already.
> 
> If this condition is true, we could set tmpl = friend_tmpl earlier, and skip
> doing name lookup at all.
> 
> It's interesting that the previous if does the same depth comparison, and
> that dates back to 2002; I wonder why it was needed then?
> 
> Jason
> 

Ah right, I see.  I think the depth comparison in the previous if
actually is for exactly the same reason, just for the normal case when
the template *is* found by name lookup, e.g. 

  template <typename> struct A {};
  template <typename> struct B {
    template <typename> friend struct ::A;
  };

This is g++.dg/template/friend5.  Here's an updated patch which is so
far very lightly tested, OK for trunk if full bootstrap+regtest
succeeds?

-- >8 --

With modules it may be the case that a template friend class provided
with a qualified name is not found by name lookup at instantiation time,
due to the class not being exported from its module.  This causes issues
in tsubst_friend_class which did not handle this case.

This is a more general issue, in fact, caused by the named friend class
not actually requiring tsubsting.  This was already worked around for
the "found by name lookup" case (g++.dg/template/friend5.C), but it
looks like there's no need to do name lookup at all for this to work.

	PR c++/115801

gcc/cp/ChangeLog:

	* pt.cc (tsubst_friend_class): Return the type directly when no
	tsubsting is required.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/tpl-friend-16_a.C: New test.
	* g++.dg/modules/tpl-friend-16_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/pt.cc                                  | 39 ++++++++++--------
 .../g++.dg/modules/tpl-friend-16_a.C          | 40 +++++++++++++++++++
 .../g++.dg/modules/tpl-friend-16_b.C          | 17 ++++++++
 3 files changed, 79 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 2db59213c54..ea00577fd37 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -11732,6 +11732,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
       return TREE_TYPE (tmpl);
     }
 
+  if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
+      <= TMPL_ARGS_DEPTH (args))
+    /* The template has already been subsituted, e.g. for
+
+	 template <typename> friend class ::C;
+
+       so we just need to return it directly.  */
+    return TREE_TYPE (friend_tmpl);
+
   tree context = CP_DECL_CONTEXT (friend_tmpl);
   if (TREE_CODE (context) == NAMESPACE_DECL)
     push_nested_namespace (context);
@@ -11764,23 +11773,19 @@ tsubst_friend_class (tree friend_tmpl, tree args)
 	   compatible with the attachment of the friend template.  */
 	module_may_redeclare (tmpl, friend_tmpl);
 
-      if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
-	  > TMPL_ARGS_DEPTH (args))
-	{
-	  tree parms = tsubst_template_parms (DECL_TEMPLATE_PARMS (friend_tmpl),
-					      args, tf_warning_or_error);
-	  tsubst_each_template_parm_constraints (parms, args,
-						 tf_warning_or_error);
-          location_t saved_input_location = input_location;
-          input_location = DECL_SOURCE_LOCATION (friend_tmpl);
-	  tree cons = get_constraints (friend_tmpl);
-	  ++processing_template_decl;
-	  cons = tsubst_constraint_info (cons, args, tf_warning_or_error,
-					 DECL_FRIEND_CONTEXT (friend_tmpl));
-	  --processing_template_decl;
-          redeclare_class_template (TREE_TYPE (tmpl), parms, cons);
-          input_location = saved_input_location;
-	}
+      tree parms = tsubst_template_parms (DECL_TEMPLATE_PARMS (friend_tmpl),
+					  args, tf_warning_or_error);
+      tsubst_each_template_parm_constraints (parms, args,
+					     tf_warning_or_error);
+      location_t saved_input_location = input_location;
+      input_location = DECL_SOURCE_LOCATION (friend_tmpl);
+      tree cons = get_constraints (friend_tmpl);
+      ++processing_template_decl;
+      cons = tsubst_constraint_info (cons, args, tf_warning_or_error,
+				     DECL_FRIEND_CONTEXT (friend_tmpl));
+      --processing_template_decl;
+      redeclare_class_template (TREE_TYPE (tmpl), parms, cons);
+      input_location = saved_input_location;
     }
   else
     {
diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
new file mode 100644
index 00000000000..e1cdcd98e1e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
@@ -0,0 +1,40 @@
+// PR c++/115801
+// { dg-additional-options "-fmodules-ts -Wno-global-module" }
+// { dg-module-cmi test }
+
+module;
+
+template <typename T> struct GMF;
+template <typename T> struct GMF_Hidden {
+  int go() { GMF<T> gmf; return gmf.x; }
+};
+
+template <typename T> struct GMF {
+private:
+  template <typename> friend struct ::GMF_Hidden;
+  int x = 1;
+};
+
+template <typename T> int test_gmf() {
+  GMF_Hidden<T> h; return h.go();
+}
+
+export module test;
+
+export using ::GMF;
+export using ::test_gmf;
+
+export template <typename> struct Attached;
+template <typename T> struct Attached_Hidden {
+  int go() { Attached<T> attached; return attached.x; }
+};
+
+template <typename T> struct Attached {
+private:
+  template <typename> friend struct ::Attached_Hidden;
+  int x = 2;
+};
+
+export template <typename T> int test_attached() {
+  Attached_Hidden<T> h; return h.go();
+}
diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
new file mode 100644
index 00000000000..d3484ab19b1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
@@ -0,0 +1,17 @@
+// PR c++/115801
+// { dg-additional-options "-fmodules-ts" }
+
+import test;
+
+int main() {
+  GMF<int> gmf;
+  Attached<int> attached;
+
+  int a = test_gmf<double>();
+  int b = test_attached<double>();
+
+  GMF_Hidden<int> gmf_hidden;  // { dg-error "not declared" }
+  Attached_Hidden<int> attached_hidden;  // { dg-error "not declared" }
+}
+
+// { dg-prune-output "expected primary-expression" }

On 8/7/24 7:45 PM, Nathaniel Shead wrote:
> On Wed, Aug 07, 2024 at 01:44:31PM -0400, Jason Merrill wrote:
>> On 8/6/24 2:35 AM, Nathaniel Shead wrote:
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>
>>> Another potential approach would be to go searching for this unexported
>>> type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by
>>> expanding on the lookup_imported_temploid_friend hack.  I'm still not
>>> exactly sure how name lookup for template friends is supposed to behave
>>> though, specifically in terms of when and where they should conflict
>>> with other entities with the same name.
>>
>> CWG2588 tried to clarify this in https://eel.is/c++draft/basic#link-4.8 --
>> if there's a matching reachable declaration, the friend refers to it even if
>> it isn't visible to name lookup.
>>
>> It seems like an oversight that the new second bullet refers specifically to
>> functions, it seems natural for it to apply to classes as well.
>>
>> So, they correspond but do not conflict because they declare the same
>> entity.
>>
> 
> Right, makes sense.  OK, I'll work on filling out our testcases to make
> sure that we cover everything under that interpretation and potentially
> come back to making an ANY_REACHABLE flag for this.
> 
>>> The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2
>>> and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to
>>> the usual rules in [basic.link] and [basic.scope.scope], but how these
>>> all are supposed to interact isn't super clear to me right now.
>>>
>>> Additionally I wonder if maybe the better approach long-term would be to
>>> focus on getting textual redefinitions working first, and then reuse
>>> whatever logic we build for that to handle template friends rather than
>>> relying on finding these hidden 'mergeable' slots first.
>>
>> I have a WIP patch to allow textual redefinitions by teaching
>> duplicate_decls that it's OK to redefine an imported GM entity, so
>> check_module_override works.
>>
>> My current plan is to then just token-skip the bodies.  This won't diagnose
>> ODR problems, but our module merging doesn't do that consistently either.
>>
>>> @@ -11800,6 +11800,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
>>>              input_location = saved_input_location;
>>>    	}
>>>        }
>>> +  else if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
>>> +	   <= TMPL_ARGS_DEPTH (args))
>>
>> This condition seems impossible normally; it's only satisfied in this
>> testcase because friend_tmpl doesn't actually represent the friend
>> declaration, it's already the named class template.  So the substitution in
>> the next else fails because it was done already.
>>
>> If this condition is true, we could set tmpl = friend_tmpl earlier, and skip
>> doing name lookup at all.
>>
>> It's interesting that the previous if does the same depth comparison, and
>> that dates back to 2002; I wonder why it was needed then?
>>
>> Jason
>>
> 
> Ah right, I see.  I think the depth comparison in the previous if
> actually is for exactly the same reason, just for the normal case when
> the template *is* found by name lookup, e.g.
> 
>    template <typename> struct A {};
>    template <typename> struct B {
>      template <typename> friend struct ::A;
>    };
> 
> This is g++.dg/template/friend5.  Here's an updated patch which is so
> far very lightly tested, OK for trunk if full bootstrap+regtest
> succeeds?
> 
> -- >8 --
> 
> With modules it may be the case that a template friend class provided
> with a qualified name is not found by name lookup at instantiation time,
> due to the class not being exported from its module.  This causes issues
> in tsubst_friend_class which did not handle this case.
> 
> This is a more general issue, in fact, caused by the named friend class
> not actually requiring tsubsting.  This was already worked around for
> the "found by name lookup" case (g++.dg/template/friend5.C), but it
> looks like there's no need to do name lookup at all for this to work.
> 
> 	PR c++/115801
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (tsubst_friend_class): Return the type directly when no
> 	tsubsting is required.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/tpl-friend-16_a.C: New test.
> 	* g++.dg/modules/tpl-friend-16_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/pt.cc                                  | 39 ++++++++++--------
>   .../g++.dg/modules/tpl-friend-16_a.C          | 40 +++++++++++++++++++
>   .../g++.dg/modules/tpl-friend-16_b.C          | 17 ++++++++
>   3 files changed, 79 insertions(+), 17 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 2db59213c54..ea00577fd37 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -11732,6 +11732,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
>         return TREE_TYPE (tmpl);
>       }
>   
> +  if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> +      <= TMPL_ARGS_DEPTH (args))
> +    /* The template has already been subsituted, e.g. for

"substituted"

OK with that typo fix.

Jason

On Thu, 8 Aug 2024, Nathaniel Shead wrote:

> On Wed, Aug 07, 2024 at 01:44:31PM -0400, Jason Merrill wrote:
> > On 8/6/24 2:35 AM, Nathaniel Shead wrote:
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > 
> > > Another potential approach would be to go searching for this unexported
> > > type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by
> > > expanding on the lookup_imported_temploid_friend hack.  I'm still not
> > > exactly sure how name lookup for template friends is supposed to behave
> > > though, specifically in terms of when and where they should conflict
> > > with other entities with the same name.
> > 
> > CWG2588 tried to clarify this in https://eel.is/c++draft/basic#link-4.8 --
> > if there's a matching reachable declaration, the friend refers to it even if
> > it isn't visible to name lookup.
> > 
> > It seems like an oversight that the new second bullet refers specifically to
> > functions, it seems natural for it to apply to classes as well.
> > 
> > So, they correspond but do not conflict because they declare the same
> > entity.
> > 
> 
> Right, makes sense.  OK, I'll work on filling out our testcases to make
> sure that we cover everything under that interpretation and potentially
> come back to making an ANY_REACHABLE flag for this.
> 
> > > The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2
> > > and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to
> > > the usual rules in [basic.link] and [basic.scope.scope], but how these
> > > all are supposed to interact isn't super clear to me right now.
> > > 
> > > Additionally I wonder if maybe the better approach long-term would be to
> > > focus on getting textual redefinitions working first, and then reuse
> > > whatever logic we build for that to handle template friends rather than
> > > relying on finding these hidden 'mergeable' slots first.
> > 
> > I have a WIP patch to allow textual redefinitions by teaching
> > duplicate_decls that it's OK to redefine an imported GM entity, so
> > check_module_override works.
> > 
> > My current plan is to then just token-skip the bodies.  This won't diagnose
> > ODR problems, but our module merging doesn't do that consistently either.
> > 
> > > @@ -11800,6 +11800,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> > >             input_location = saved_input_location;
> > >   	}
> > >       }
> > > +  else if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> > > +	   <= TMPL_ARGS_DEPTH (args))
> > 
> > This condition seems impossible normally; it's only satisfied in this
> > testcase because friend_tmpl doesn't actually represent the friend
> > declaration, it's already the named class template.  So the substitution in
> > the next else fails because it was done already.
> > 
> > If this condition is true, we could set tmpl = friend_tmpl earlier, and skip
> > doing name lookup at all.
> > 
> > It's interesting that the previous if does the same depth comparison, and
> > that dates back to 2002; I wonder why it was needed then?
> > 
> > Jason
> > 
> 
> Ah right, I see.  I think the depth comparison in the previous if
> actually is for exactly the same reason, just for the normal case when
> the template *is* found by name lookup, e.g. 
> 
>   template <typename> struct A {};
>   template <typename> struct B {
>     template <typename> friend struct ::A;
>   };
> 
> This is g++.dg/template/friend5.  Here's an updated patch which is so
> far very lightly tested, OK for trunk if full bootstrap+regtest
> succeeds?
> 
> -- >8 --
> 
> With modules it may be the case that a template friend class provided
> with a qualified name is not found by name lookup at instantiation time,
> due to the class not being exported from its module.  This causes issues
> in tsubst_friend_class which did not handle this case.
> 
> This is a more general issue, in fact, caused by the named friend class
> not actually requiring tsubsting.  This was already worked around for
> the "found by name lookup" case (g++.dg/template/friend5.C), but it
> looks like there's no need to do name lookup at all for this to work.
> 
> 	PR c++/115801
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (tsubst_friend_class): Return the type directly when no
> 	tsubsting is required.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/tpl-friend-16_a.C: New test.
> 	* g++.dg/modules/tpl-friend-16_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/pt.cc                                  | 39 ++++++++++--------
>  .../g++.dg/modules/tpl-friend-16_a.C          | 40 +++++++++++++++++++
>  .../g++.dg/modules/tpl-friend-16_b.C          | 17 ++++++++
>  3 files changed, 79 insertions(+), 17 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 2db59213c54..ea00577fd37 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -11732,6 +11732,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
>        return TREE_TYPE (tmpl);
>      }
>  
> +  if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> +      <= TMPL_ARGS_DEPTH (args))
> +    /* The template has already been subsituted, e.g. for
> +
> +	 template <typename> friend class ::C;
> +
> +       so we just need to return it directly.  */
> +    return TREE_TYPE (friend_tmpl);

IIUC I don't think this would do the right thing for a template friend
declaration that directly names a template from an outer current
instantiation:

    template<class T>
    struct A {
      template<class U> struct B;

      template<class U>
      struct C {
        template<class V> friend class A::B;
      };
    };

    template struct A<int*>::C<long>;

Here TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) is 2, and
so is TMPL_ARGS_DEPTH (args), so we'd exit early and return a fully
dependent TEMPLATE_DECL A<T>::B, but what we want to return is
A<int*>::B.

It should always be safe to exit early when
  TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) == 1
though, and that should cover the most common case.

> +
>    tree context = CP_DECL_CONTEXT (friend_tmpl);
>    if (TREE_CODE (context) == NAMESPACE_DECL)
>      push_nested_namespace (context);
> @@ -11764,23 +11773,19 @@ tsubst_friend_class (tree friend_tmpl, tree args)
>  	   compatible with the attachment of the friend template.  */
>  	module_may_redeclare (tmpl, friend_tmpl);
>  
> -      if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> -	  > TMPL_ARGS_DEPTH (args))
> -	{
> -	  tree parms = tsubst_template_parms (DECL_TEMPLATE_PARMS (friend_tmpl),
> -					      args, tf_warning_or_error);
> -	  tsubst_each_template_parm_constraints (parms, args,
> -						 tf_warning_or_error);
> -          location_t saved_input_location = input_location;
> -          input_location = DECL_SOURCE_LOCATION (friend_tmpl);
> -	  tree cons = get_constraints (friend_tmpl);
> -	  ++processing_template_decl;
> -	  cons = tsubst_constraint_info (cons, args, tf_warning_or_error,
> -					 DECL_FRIEND_CONTEXT (friend_tmpl));
> -	  --processing_template_decl;
> -          redeclare_class_template (TREE_TYPE (tmpl), parms, cons);
> -          input_location = saved_input_location;
> -	}
> +      tree parms = tsubst_template_parms (DECL_TEMPLATE_PARMS (friend_tmpl),
> +					  args, tf_warning_or_error);
> +      tsubst_each_template_parm_constraints (parms, args,
> +					     tf_warning_or_error);
> +      location_t saved_input_location = input_location;
> +      input_location = DECL_SOURCE_LOCATION (friend_tmpl);
> +      tree cons = get_constraints (friend_tmpl);
> +      ++processing_template_decl;
> +      cons = tsubst_constraint_info (cons, args, tf_warning_or_error,
> +				     DECL_FRIEND_CONTEXT (friend_tmpl));
> +      --processing_template_decl;
> +      redeclare_class_template (TREE_TYPE (tmpl), parms, cons);
> +      input_location = saved_input_location;
>      }
>    else
>      {
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
> new file mode 100644
> index 00000000000..e1cdcd98e1e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
> @@ -0,0 +1,40 @@
> +// PR c++/115801
> +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> +// { dg-module-cmi test }
> +
> +module;
> +
> +template <typename T> struct GMF;
> +template <typename T> struct GMF_Hidden {
> +  int go() { GMF<T> gmf; return gmf.x; }
> +};
> +
> +template <typename T> struct GMF {
> +private:
> +  template <typename> friend struct ::GMF_Hidden;
> +  int x = 1;
> +};
> +
> +template <typename T> int test_gmf() {
> +  GMF_Hidden<T> h; return h.go();
> +}
> +
> +export module test;
> +
> +export using ::GMF;
> +export using ::test_gmf;
> +
> +export template <typename> struct Attached;
> +template <typename T> struct Attached_Hidden {
> +  int go() { Attached<T> attached; return attached.x; }
> +};
> +
> +template <typename T> struct Attached {
> +private:
> +  template <typename> friend struct ::Attached_Hidden;
> +  int x = 2;
> +};
> +
> +export template <typename T> int test_attached() {
> +  Attached_Hidden<T> h; return h.go();
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> new file mode 100644
> index 00000000000..d3484ab19b1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> @@ -0,0 +1,17 @@
> +// PR c++/115801
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import test;
> +
> +int main() {
> +  GMF<int> gmf;
> +  Attached<int> attached;
> +
> +  int a = test_gmf<double>();
> +  int b = test_attached<double>();
> +
> +  GMF_Hidden<int> gmf_hidden;  // { dg-error "not declared" }
> +  Attached_Hidden<int> attached_hidden;  // { dg-error "not declared" }
> +}
> +
> +// { dg-prune-output "expected primary-expression" }
> -- 
> 2.43.2
> 
>

On Wed, 7 Aug 2024, Patrick Palka wrote:

> On Thu, 8 Aug 2024, Nathaniel Shead wrote:
> 
> > On Wed, Aug 07, 2024 at 01:44:31PM -0400, Jason Merrill wrote:
> > > On 8/6/24 2:35 AM, Nathaniel Shead wrote:
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > 
> > > > Another potential approach would be to go searching for this unexported
> > > > type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by
> > > > expanding on the lookup_imported_temploid_friend hack.  I'm still not
> > > > exactly sure how name lookup for template friends is supposed to behave
> > > > though, specifically in terms of when and where they should conflict
> > > > with other entities with the same name.
> > > 
> > > CWG2588 tried to clarify this in https://eel.is/c++draft/basic#link-4.8 --
> > > if there's a matching reachable declaration, the friend refers to it even if
> > > it isn't visible to name lookup.
> > > 
> > > It seems like an oversight that the new second bullet refers specifically to
> > > functions, it seems natural for it to apply to classes as well.
> > > 
> > > So, they correspond but do not conflict because they declare the same
> > > entity.
> > > 
> > 
> > Right, makes sense.  OK, I'll work on filling out our testcases to make
> > sure that we cover everything under that interpretation and potentially
> > come back to making an ANY_REACHABLE flag for this.
> > 
> > > > The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2
> > > > and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to
> > > > the usual rules in [basic.link] and [basic.scope.scope], but how these
> > > > all are supposed to interact isn't super clear to me right now.
> > > > 
> > > > Additionally I wonder if maybe the better approach long-term would be to
> > > > focus on getting textual redefinitions working first, and then reuse
> > > > whatever logic we build for that to handle template friends rather than
> > > > relying on finding these hidden 'mergeable' slots first.
> > > 
> > > I have a WIP patch to allow textual redefinitions by teaching
> > > duplicate_decls that it's OK to redefine an imported GM entity, so
> > > check_module_override works.
> > > 
> > > My current plan is to then just token-skip the bodies.  This won't diagnose
> > > ODR problems, but our module merging doesn't do that consistently either.
> > > 
> > > > @@ -11800,6 +11800,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> > > >             input_location = saved_input_location;
> > > >   	}
> > > >       }
> > > > +  else if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> > > > +	   <= TMPL_ARGS_DEPTH (args))
> > > 
> > > This condition seems impossible normally; it's only satisfied in this
> > > testcase because friend_tmpl doesn't actually represent the friend
> > > declaration, it's already the named class template.  So the substitution in
> > > the next else fails because it was done already.
> > > 
> > > If this condition is true, we could set tmpl = friend_tmpl earlier, and skip
> > > doing name lookup at all.
> > > 
> > > It's interesting that the previous if does the same depth comparison, and
> > > that dates back to 2002; I wonder why it was needed then?

I reckon the depth comparison in the previous if is equivalent to:

 if (DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (friend_tmpl))

But unfortunately we can't skip doing name lookup in that case due to
the mentioned example :/

> > > 
> > > Jason
> > > 
> > 
> > Ah right, I see.  I think the depth comparison in the previous if
> > actually is for exactly the same reason, just for the normal case when
> > the template *is* found by name lookup, e.g. 
> > 
> >   template <typename> struct A {};
> >   template <typename> struct B {
> >     template <typename> friend struct ::A;
> >   };
> > 
> > This is g++.dg/template/friend5.  Here's an updated patch which is so
> > far very lightly tested, OK for trunk if full bootstrap+regtest
> > succeeds?
> > 
> > -- >8 --
> > 
> > With modules it may be the case that a template friend class provided
> > with a qualified name is not found by name lookup at instantiation time,
> > due to the class not being exported from its module.  This causes issues
> > in tsubst_friend_class which did not handle this case.
> > 
> > This is a more general issue, in fact, caused by the named friend class
> > not actually requiring tsubsting.  This was already worked around for
> > the "found by name lookup" case (g++.dg/template/friend5.C), but it
> > looks like there's no need to do name lookup at all for this to work.
> > 
> > 	PR c++/115801
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.cc (tsubst_friend_class): Return the type directly when no
> > 	tsubsting is required.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/tpl-friend-16_a.C: New test.
> > 	* g++.dg/modules/tpl-friend-16_b.C: New test.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >  gcc/cp/pt.cc                                  | 39 ++++++++++--------
> >  .../g++.dg/modules/tpl-friend-16_a.C          | 40 +++++++++++++++++++
> >  .../g++.dg/modules/tpl-friend-16_b.C          | 17 ++++++++
> >  3 files changed, 79 insertions(+), 17 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
> >  create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 2db59213c54..ea00577fd37 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -11732,6 +11732,15 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> >        return TREE_TYPE (tmpl);
> >      }
> >  
> > +  if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> > +      <= TMPL_ARGS_DEPTH (args))
> > +    /* The template has already been subsituted, e.g. for
> > +
> > +	 template <typename> friend class ::C;
> > +
> > +       so we just need to return it directly.  */
> > +    return TREE_TYPE (friend_tmpl);
> 
> IIUC I don't think this would do the right thing for a template friend
> declaration that directly names a template from an outer current
> instantiation:
> 
>     template<class T>
>     struct A {
>       template<class U> struct B;
> 
>       template<class U>
>       struct C {
>         template<class V> friend class A::B;
>       };
>     };
> 
>     template struct A<int*>::C<long>;
> 
> Here TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) is 2, and
> so is TMPL_ARGS_DEPTH (args), so we'd exit early and return a fully
> dependent TEMPLATE_DECL A<T>::B, but what we want to return is
> A<int*>::B.
> 
> It should always be safe to exit early when
>   TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) == 1
> though, and that should cover the most common case.
> 
> > +
> >    tree context = CP_DECL_CONTEXT (friend_tmpl);
> >    if (TREE_CODE (context) == NAMESPACE_DECL)
> >      push_nested_namespace (context);
> > @@ -11764,23 +11773,19 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> >  	   compatible with the attachment of the friend template.  */
> >  	module_may_redeclare (tmpl, friend_tmpl);
> >  
> > -      if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl))
> > -	  > TMPL_ARGS_DEPTH (args))
> > -	{
> > -	  tree parms = tsubst_template_parms (DECL_TEMPLATE_PARMS (friend_tmpl),
> > -					      args, tf_warning_or_error);
> > -	  tsubst_each_template_parm_constraints (parms, args,
> > -						 tf_warning_or_error);
> > -          location_t saved_input_location = input_location;
> > -          input_location = DECL_SOURCE_LOCATION (friend_tmpl);
> > -	  tree cons = get_constraints (friend_tmpl);
> > -	  ++processing_template_decl;
> > -	  cons = tsubst_constraint_info (cons, args, tf_warning_or_error,
> > -					 DECL_FRIEND_CONTEXT (friend_tmpl));
> > -	  --processing_template_decl;
> > -          redeclare_class_template (TREE_TYPE (tmpl), parms, cons);
> > -          input_location = saved_input_location;
> > -	}
> > +      tree parms = tsubst_template_parms (DECL_TEMPLATE_PARMS (friend_tmpl),
> > +					  args, tf_warning_or_error);
> > +      tsubst_each_template_parm_constraints (parms, args,
> > +					     tf_warning_or_error);
> > +      location_t saved_input_location = input_location;
> > +      input_location = DECL_SOURCE_LOCATION (friend_tmpl);
> > +      tree cons = get_constraints (friend_tmpl);
> > +      ++processing_template_decl;
> > +      cons = tsubst_constraint_info (cons, args, tf_warning_or_error,
> > +				     DECL_FRIEND_CONTEXT (friend_tmpl));
> > +      --processing_template_decl;
> > +      redeclare_class_template (TREE_TYPE (tmpl), parms, cons);
> > +      input_location = saved_input_location;
> >      }
> >    else
> >      {
> > diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
> > new file mode 100644
> > index 00000000000..e1cdcd98e1e
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C
> > @@ -0,0 +1,40 @@
> > +// PR c++/115801
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> > +// { dg-module-cmi test }
> > +
> > +module;
> > +
> > +template <typename T> struct GMF;
> > +template <typename T> struct GMF_Hidden {
> > +  int go() { GMF<T> gmf; return gmf.x; }
> > +};
> > +
> > +template <typename T> struct GMF {
> > +private:
> > +  template <typename> friend struct ::GMF_Hidden;
> > +  int x = 1;
> > +};
> > +
> > +template <typename T> int test_gmf() {
> > +  GMF_Hidden<T> h; return h.go();
> > +}
> > +
> > +export module test;
> > +
> > +export using ::GMF;
> > +export using ::test_gmf;
> > +
> > +export template <typename> struct Attached;
> > +template <typename T> struct Attached_Hidden {
> > +  int go() { Attached<T> attached; return attached.x; }
> > +};
> > +
> > +template <typename T> struct Attached {
> > +private:
> > +  template <typename> friend struct ::Attached_Hidden;
> > +  int x = 2;
> > +};
> > +
> > +export template <typename T> int test_attached() {
> > +  Attached_Hidden<T> h; return h.go();
> > +}
> > diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> > new file mode 100644
> > index 00000000000..d3484ab19b1
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C
> > @@ -0,0 +1,17 @@
> > +// PR c++/115801
> > +// { dg-additional-options "-fmodules-ts" }
> > +
> > +import test;
> > +
> > +int main() {
> > +  GMF<int> gmf;
> > +  Attached<int> attached;
> > +
> > +  int a = test_gmf<double>();
> > +  int b = test_attached<double>();
> > +
> > +  GMF_Hidden<int> gmf_hidden;  // { dg-error "not declared" }
> > +  Attached_Hidden<int> attached_hidden;  // { dg-error "not declared" }
> > +}
> > +
> > +// { dg-prune-output "expected primary-expression" }
> > -- 
> > 2.43.2
> > 
> > 
>