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 <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
>
>
