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 <nathanielosh...@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
> >
> >
>
