| Message ID | 66b426d7.050a0220.2958fc.0959@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] c++/modules: Handle instantiating already tsubsted template friend classes [PR115801] | expand |
On 8/7/24 10:00 PM, Nathaniel Shead wrote: > On Wed, Aug 07, 2024 at 09:12:13PM -0400, Patrick Palka wrote: >> 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. >>> > > Ah right, I see; a full version of the testcase I guess might be > > template<class T> > struct A { > template<class U> struct B; > > template<class U> > struct C { > template<class V> friend struct A::B; > private: > int x; > }; > }; > > template <class T> > template <class U> > struct A<T>::B { > int foo(A<int*>::C<long> c) { return c.x; } > }; > > template struct A<int*>::C<long>; > template struct A<int*>::B<long>; > template struct A<double*>::B<long>; // !!! > > Which is currently correctly rejected by GCC but with my proposed change > is no longer rejected. > > How does this patch look instead? This passes dg.exp and modules.exp > still, still waiting for full regtest to complete. OK. > -- >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 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 particular case to work. > > We do need to be careful to continue to do name lookup to handle > templates from an outer current instantiation though; this patch adds a > new testcase for this as well. This should not impact modules (because > exportingness will only affect namespace lookup). > > PR c++/115801 > > gcc/cp/ChangeLog: > > * pt.cc (tsubst_friend_class): Return the type immediately when > no tsubsting or name lookup 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. > * g++.dg/template/friend82.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > Reviewed-by: Patrick Palka <ppalka@redhat.com> > Reviewed-by: Jason Merrill <jason@redhat.com> > --- > gcc/cp/pt.cc | 8 ++++ > .../g++.dg/modules/tpl-friend-16_a.C | 40 +++++++++++++++++++ > .../g++.dg/modules/tpl-friend-16_b.C | 17 ++++++++ > gcc/testsuite/g++.dg/template/friend82.C | 23 +++++++++++ > 4 files changed, 88 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 > create mode 100644 gcc/testsuite/g++.dg/template/friend82.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 2db59213c54..8abd092116b 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -11732,6 +11732,14 @@ tsubst_friend_class (tree friend_tmpl, tree args) > return TREE_TYPE (tmpl); > } > > + if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) == 1) > + /* The template has already been fully substituted, e.g. for > + > + template <typename> friend class ::C; > + > + so we can just 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); > 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" } > diff --git a/gcc/testsuite/g++.dg/template/friend82.C b/gcc/testsuite/g++.dg/template/friend82.C > new file mode 100644 > index 00000000000..28a057dd23e > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/friend82.C > @@ -0,0 +1,23 @@ > +// { dg-do compile } > + > +template<class T> > +struct A { > + template<class U> struct B; > + > + template<class U> > + struct C { > + template<class V> friend struct A::B; > + private: > + int x; > + }; > +}; > + > +template <class T> > +template <class U> > +struct A<T>::B { > + int foo(A<int*>::C<long> c) { return c.x; } // { dg-error "private" } > +}; > + > +template struct A<int*>::C<long>; > +template struct A<int*>::B<long>; // { dg-bogus "" } > +template struct A<double*>::B<long>; // { dg-message "required from here" }
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 2db59213c54..8abd092116b 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -11732,6 +11732,14 @@ tsubst_friend_class (tree friend_tmpl, tree args) return TREE_TYPE (tmpl); } + if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) == 1) + /* The template has already been fully substituted, e.g. for + + template <typename> friend class ::C; + + so we can just 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); 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" } diff --git a/gcc/testsuite/g++.dg/template/friend82.C b/gcc/testsuite/g++.dg/template/friend82.C new file mode 100644 index 00000000000..28a057dd23e --- /dev/null +++ b/gcc/testsuite/g++.dg/template/friend82.C @@ -0,0 +1,23 @@ +// { dg-do compile } + +template<class T> +struct A { + template<class U> struct B; + + template<class U> + struct C { + template<class V> friend struct A::B; + private: + int x; + }; +}; + +template <class T> +template <class U> +struct A<T>::B { + int foo(A<int*>::C<long> c) { return c.x; } // { dg-error "private" } +}; + +template struct A<int*>::C<long>; +template struct A<int*>::B<long>; // { dg-bogus "" } +template struct A<double*>::B<long>; // { dg-message "required from here" }