Message ID | 20231129154512.428173-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: wrong ambiguity in accessing static field [PR112744] | expand |
On Wed, 29 Nov 2023, Marek Polacek wrote: > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > Now that I'm posting this patch, I think you'll probably want me to use > ba_any unconditionally. That works too; g++.dg/tc1/dr52.C just needs > a trivial testsuite tweak: > 'C' is not an accessible base of 'X' > v. > 'C' is an inaccessible base of 'X' > We should probably unify those messages... > > -- >8 -- > Given > > struct A { constexpr static int a = 0; }; > struct B : A {}; > struct C : A {}; > struct D : B, C {}; > > we give the "'A' is an ambiguous base of 'D'" error for > > D{}.A::a; > > which seems wrong: 'a' is a static data member so there is only one copy > so it can be unambiguously referred to even if there are multiple A > objects. clang++/MSVC/icx agree. > > PR c++/112744 > > gcc/cp/ChangeLog: > > * typeck.cc (finish_class_member_access_expr): When accessing > a static data member, use ba_any for lookup_base. > > gcc/testsuite/ChangeLog: > > * g++.dg/lookup/scoped11.C: New test. > * g++.dg/lookup/scoped12.C: New test. > * g++.dg/lookup/scoped13.C: New test. > --- > gcc/cp/typeck.cc | 21 ++++++++++++++++++--- > gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++ > gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++ > gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++ > 4 files changed, 60 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index e995fb6ddd7..c4de8bb2616 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -3476,7 +3476,7 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, > name, scope); > return error_mark_node; > } > - > + > if (TREE_SIDE_EFFECTS (object)) > val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val); > return val; > @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, > return error_mark_node; > } > > + /* NAME may refer to a static data member, in which case there is > + one copy of the data member that is shared by all the objects of > + the class. So NAME can be unambiguously referred to even if > + there are multiple indirect base classes containing NAME. */ > + const base_access ba = [scope, name] () > + { > + if (identifier_p (name)) > + { > + tree m = lookup_member (scope, name, /*protect=*/0, > + /*want_type=*/false, tf_none); > + if (!m || VAR_P (m)) > + return ba_any; I wonder if we want to return ba_check_bit instead of ba_any so that we still check access of the selected base? struct A { constexpr static int a = 0; }; struct D : private A {}; void f() { D{}.A::a; // #1 GCC (and Clang) currently rejects } template<class T> void g() { D{}.T::a; // #2 GCC currently rejects, Clang accepts?! } template void g<A>(); > + } > + return ba_check; > + } (); > + > /* Find the base of OBJECT_TYPE corresponding to SCOPE. */ > - access_path = lookup_base (object_type, scope, ba_check, > - NULL, complain); > + access_path = lookup_base (object_type, scope, ba, NULL, complain); > if (access_path == error_mark_node) > return error_mark_node; > if (!access_path) > diff --git a/gcc/testsuite/g++.dg/lookup/scoped11.C b/gcc/testsuite/g++.dg/lookup/scoped11.C > new file mode 100644 > index 00000000000..be743522fce > --- /dev/null > +++ b/gcc/testsuite/g++.dg/lookup/scoped11.C > @@ -0,0 +1,14 @@ > +// PR c++/112744 > +// { dg-do compile } > + > +struct A { const static int a = 0; }; > +struct B : A {}; > +struct C : A {}; > +struct D : B, C {}; > + > +int main() > +{ > + D d; > + (void) d.a; > + (void) d.A::a; > +} > diff --git a/gcc/testsuite/g++.dg/lookup/scoped12.C b/gcc/testsuite/g++.dg/lookup/scoped12.C > new file mode 100644 > index 00000000000..ffa145598fd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/lookup/scoped12.C > @@ -0,0 +1,14 @@ > +// PR c++/112744 > +// { dg-do compile } > + > +class A { const static int a = 0; }; > +struct B : A {}; > +struct C : A {}; > +struct D : B, C {}; > + > +int main() > +{ > + D d; > + (void) d.a; // { dg-error "private" } > + (void) d.A::a; // { dg-error "private" } > +} > diff --git a/gcc/testsuite/g++.dg/lookup/scoped13.C b/gcc/testsuite/g++.dg/lookup/scoped13.C > new file mode 100644 > index 00000000000..970e1aa833e > --- /dev/null > +++ b/gcc/testsuite/g++.dg/lookup/scoped13.C > @@ -0,0 +1,14 @@ > +// PR c++/112744 > +// { dg-do compile } > + > +struct A { const static int a = 0; }; > +struct B : A {}; > +struct C : A {}; > +struct D : B, C {}; > + > +int main() > +{ > + D d; > + (void) d.x; // { dg-error ".struct D. has no member named .x." } > + (void) d.A::x; // { dg-error ".struct A. has no member named .x." } > +} > > base-commit: 3d104d93a7011146b0870ab160613147adb8d9b3 > -- > 2.42.0 > >
On Wed, Nov 29, 2023 at 12:23:46PM -0500, Patrick Palka wrote: > On Wed, 29 Nov 2023, Marek Polacek wrote: > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > Now that I'm posting this patch, I think you'll probably want me to use > > ba_any unconditionally. That works too; g++.dg/tc1/dr52.C just needs > > a trivial testsuite tweak: > > 'C' is not an accessible base of 'X' > > v. > > 'C' is an inaccessible base of 'X' > > We should probably unify those messages... > > > > -- >8 -- > > Given > > > > struct A { constexpr static int a = 0; }; > > struct B : A {}; > > struct C : A {}; > > struct D : B, C {}; > > > > we give the "'A' is an ambiguous base of 'D'" error for > > > > D{}.A::a; > > > > which seems wrong: 'a' is a static data member so there is only one copy > > so it can be unambiguously referred to even if there are multiple A > > objects. clang++/MSVC/icx agree. > > > > PR c++/112744 > > > > gcc/cp/ChangeLog: > > > > * typeck.cc (finish_class_member_access_expr): When accessing > > a static data member, use ba_any for lookup_base. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/lookup/scoped11.C: New test. > > * g++.dg/lookup/scoped12.C: New test. > > * g++.dg/lookup/scoped13.C: New test. > > --- > > gcc/cp/typeck.cc | 21 ++++++++++++++++++--- > > gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++ > > gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++ > > 4 files changed, 60 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C > > create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C > > > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > index e995fb6ddd7..c4de8bb2616 100644 > > --- a/gcc/cp/typeck.cc > > +++ b/gcc/cp/typeck.cc > > @@ -3476,7 +3476,7 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, > > name, scope); > > return error_mark_node; > > } > > - > > + > > if (TREE_SIDE_EFFECTS (object)) > > val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val); > > return val; > > @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, > > return error_mark_node; > > } > > > > + /* NAME may refer to a static data member, in which case there is > > + one copy of the data member that is shared by all the objects of > > + the class. So NAME can be unambiguously referred to even if > > + there are multiple indirect base classes containing NAME. */ > > + const base_access ba = [scope, name] () > > + { > > + if (identifier_p (name)) > > + { > > + tree m = lookup_member (scope, name, /*protect=*/0, > > + /*want_type=*/false, tf_none); > > + if (!m || VAR_P (m)) > > + return ba_any; > > I wonder if we want to return ba_check_bit instead of ba_any so that we > still check access of the selected base? That would certainly make sense to me. I didn't do that because I'd not seen ba_check_bit being used except as part of ba_check, but that may not mean much. So either I can tweak the lambda to return ba_check_bit rather than ba_any or use ba_check_bit unconditionally. Any opinions on that? > struct A { constexpr static int a = 0; }; > struct D : private A {}; > > void f() { > D{}.A::a; // #1 GCC (and Clang) currently rejects > } > > template<class T> > void g() { > D{}.T::a; // #2 GCC currently rejects, Clang accepts?! > } > > template void g<A>(); Thanks for looking at the patch and the testcase. I'll add it. > > + } > > + return ba_check; > > + } (); > > + > > /* Find the base of OBJECT_TYPE corresponding to SCOPE. */ > > - access_path = lookup_base (object_type, scope, ba_check, > > - NULL, complain); > > + access_path = lookup_base (object_type, scope, ba, NULL, complain); > > if (access_path == error_mark_node) > > return error_mark_node; > > if (!access_path) > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped11.C b/gcc/testsuite/g++.dg/lookup/scoped11.C > > new file mode 100644 > > index 00000000000..be743522fce > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped11.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile } > > + > > +struct A { const static int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.a; > > + (void) d.A::a; > > +} > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped12.C b/gcc/testsuite/g++.dg/lookup/scoped12.C > > new file mode 100644 > > index 00000000000..ffa145598fd > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped12.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile } > > + > > +class A { const static int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.a; // { dg-error "private" } > > + (void) d.A::a; // { dg-error "private" } > > +} > > diff --git a/gcc/testsuite/g++.dg/lookup/scoped13.C b/gcc/testsuite/g++.dg/lookup/scoped13.C > > new file mode 100644 > > index 00000000000..970e1aa833e > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/lookup/scoped13.C > > @@ -0,0 +1,14 @@ > > +// PR c++/112744 > > +// { dg-do compile } > > + > > +struct A { const static int a = 0; }; > > +struct B : A {}; > > +struct C : A {}; > > +struct D : B, C {}; > > + > > +int main() > > +{ > > + D d; > > + (void) d.x; // { dg-error ".struct D. has no member named .x." } > > + (void) d.A::x; // { dg-error ".struct A. has no member named .x." } > > +} > > > > base-commit: 3d104d93a7011146b0870ab160613147adb8d9b3 > > -- > > 2.42.0 > > > > > Marek
On 11/29/23 10:45, Marek Polacek wrote: > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > Now that I'm posting this patch, I think you'll probably want me to use > ba_any unconditionally. That works too; g++.dg/tc1/dr52.C just needs > a trivial testsuite tweak: > 'C' is not an accessible base of 'X' > v. > 'C' is an inaccessible base of 'X' > We should probably unify those messages... Hmm, won't using ba_any unconditionally break ambiguous base checking for non-static data members? > @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, > return error_mark_node; > } > > + /* NAME may refer to a static data member, in which case there is > + one copy of the data member that is shared by all the objects of > + the class. So NAME can be unambiguously referred to even if > + there are multiple indirect base classes containing NAME. */ > + const base_access ba = [scope, name] () Why a lambda? > + { > + if (identifier_p (name)) > + { > + tree m = lookup_member (scope, name, /*protect=*/0, > + /*want_type=*/false, tf_none); > + if (!m || VAR_P (m)) Do you want shared_member_p here? Jason
On 11/29/23 12:43, Marek Polacek wrote: > On Wed, Nov 29, 2023 at 12:23:46PM -0500, Patrick Palka wrote: >> On Wed, 29 Nov 2023, Marek Polacek wrote: >> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>> >>> Now that I'm posting this patch, I think you'll probably want me to use >>> ba_any unconditionally. That works too; g++.dg/tc1/dr52.C just needs >>> a trivial testsuite tweak: >>> 'C' is not an accessible base of 'X' >>> v. >>> 'C' is an inaccessible base of 'X' >>> We should probably unify those messages... >>> >>> -- >8 -- >>> Given >>> >>> struct A { constexpr static int a = 0; }; >>> struct B : A {}; >>> struct C : A {}; >>> struct D : B, C {}; >>> >>> we give the "'A' is an ambiguous base of 'D'" error for >>> >>> D{}.A::a; >>> >>> which seems wrong: 'a' is a static data member so there is only one copy >>> so it can be unambiguously referred to even if there are multiple A >>> objects. clang++/MSVC/icx agree. >>> >>> PR c++/112744 >>> >>> gcc/cp/ChangeLog: >>> >>> * typeck.cc (finish_class_member_access_expr): When accessing >>> a static data member, use ba_any for lookup_base. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/lookup/scoped11.C: New test. >>> * g++.dg/lookup/scoped12.C: New test. >>> * g++.dg/lookup/scoped13.C: New test. >>> --- >>> gcc/cp/typeck.cc | 21 ++++++++++++++++++--- >>> gcc/testsuite/g++.dg/lookup/scoped11.C | 14 ++++++++++++++ >>> gcc/testsuite/g++.dg/lookup/scoped12.C | 14 ++++++++++++++ >>> gcc/testsuite/g++.dg/lookup/scoped13.C | 14 ++++++++++++++ >>> 4 files changed, 60 insertions(+), 3 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/lookup/scoped11.C >>> create mode 100644 gcc/testsuite/g++.dg/lookup/scoped12.C >>> create mode 100644 gcc/testsuite/g++.dg/lookup/scoped13.C >>> >>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc >>> index e995fb6ddd7..c4de8bb2616 100644 >>> --- a/gcc/cp/typeck.cc >>> +++ b/gcc/cp/typeck.cc >>> @@ -3476,7 +3476,7 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, >>> name, scope); >>> return error_mark_node; >>> } >>> - >>> + >>> if (TREE_SIDE_EFFECTS (object)) >>> val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val); >>> return val; >>> @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, >>> return error_mark_node; >>> } >>> >>> + /* NAME may refer to a static data member, in which case there is >>> + one copy of the data member that is shared by all the objects of >>> + the class. So NAME can be unambiguously referred to even if >>> + there are multiple indirect base classes containing NAME. */ >>> + const base_access ba = [scope, name] () >>> + { >>> + if (identifier_p (name)) >>> + { >>> + tree m = lookup_member (scope, name, /*protect=*/0, >>> + /*want_type=*/false, tf_none); >>> + if (!m || VAR_P (m)) >>> + return ba_any; >> >> I wonder if we want to return ba_check_bit instead of ba_any so that we >> still check access of the selected base? > > That would certainly make sense to me. I didn't do that because > I'd not seen ba_check_bit being used except as part of ba_check, > but that may not mean much. > > So either I can tweak the lambda to return ba_check_bit rather > than ba_any or use ba_check_bit unconditionally. Any opinions on that? The relevant passage seems to be https://eel.is/c++draft/class.access.base#6 after DR 52, which seems to have clarified that the pointer conversion only applies to non-static members. >> struct A { constexpr static int a = 0; }; >> struct D : private A {}; >> >> void f() { >> D{}.A::a; // #1 GCC (and Clang) currently rejects >> } I see that MSVC also rejects it, while EDG accepts. https://eel.is/c++draft/class.access.base#5.1 seems to say that a is accessible when named in A. https://eel.is/c++draft/expr.ref#7 also only constrains references to non-static members. But first we need to look up A in D, and A's injected-class-name looked up as a member of D is not accessible; it's private, and f() is not a friend, and we correctly complain about that. If we avoid the lookup of A in D with D{}.::A::a; clang accepts it, which is consistent with accepting the template version, and seems correct. So, I think ba_any is what we want here. Jason
On Wed, Nov 29, 2023 at 01:58:31PM -0500, Jason Merrill wrote: > On 11/29/23 10:45, Marek Polacek wrote: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > Now that I'm posting this patch, I think you'll probably want me to use > > ba_any unconditionally. That works too; g++.dg/tc1/dr52.C just needs > > a trivial testsuite tweak: > > 'C' is not an accessible base of 'X' > > v. > > 'C' is an inaccessible base of 'X' > > We should probably unify those messages... > > Hmm, won't using ba_any unconditionally break ambiguous base checking for > non-static data members? Yes. I thought not but that's only because we weren't properly testing that case (added scoped14.C, patch to follow). So that settles that. > > @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, > > return error_mark_node; > > } > > + /* NAME may refer to a static data member, in which case there is > > + one copy of the data member that is shared by all the objects of > > + the class. So NAME can be unambiguously referred to even if > > + there are multiple indirect base classes containing NAME. */ > > + const base_access ba = [scope, name] () > > Why a lambda? Only so that I can set 'ba' once and make it const. I don't believe it deserves a named function. It seems more readable than using a ?:. > > + { > > + if (identifier_p (name)) > > + { > > + tree m = lookup_member (scope, name, /*protect=*/0, > > + /*want_type=*/false, tf_none); > > + if (!m || VAR_P (m)) > > Do you want shared_member_p here? That looks like the right thing to use, thanks. Marek
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index e995fb6ddd7..c4de8bb2616 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -3476,7 +3476,7 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, name, scope); return error_mark_node; } - + if (TREE_SIDE_EFFECTS (object)) val = build2 (COMPOUND_EXPR, TREE_TYPE (val), object, val); return val; @@ -3493,9 +3493,24 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p, return error_mark_node; } + /* NAME may refer to a static data member, in which case there is + one copy of the data member that is shared by all the objects of + the class. So NAME can be unambiguously referred to even if + there are multiple indirect base classes containing NAME. */ + const base_access ba = [scope, name] () + { + if (identifier_p (name)) + { + tree m = lookup_member (scope, name, /*protect=*/0, + /*want_type=*/false, tf_none); + if (!m || VAR_P (m)) + return ba_any; + } + return ba_check; + } (); + /* Find the base of OBJECT_TYPE corresponding to SCOPE. */ - access_path = lookup_base (object_type, scope, ba_check, - NULL, complain); + access_path = lookup_base (object_type, scope, ba, NULL, complain); if (access_path == error_mark_node) return error_mark_node; if (!access_path) diff --git a/gcc/testsuite/g++.dg/lookup/scoped11.C b/gcc/testsuite/g++.dg/lookup/scoped11.C new file mode 100644 index 00000000000..be743522fce --- /dev/null +++ b/gcc/testsuite/g++.dg/lookup/scoped11.C @@ -0,0 +1,14 @@ +// PR c++/112744 +// { dg-do compile } + +struct A { const static int a = 0; }; +struct B : A {}; +struct C : A {}; +struct D : B, C {}; + +int main() +{ + D d; + (void) d.a; + (void) d.A::a; +} diff --git a/gcc/testsuite/g++.dg/lookup/scoped12.C b/gcc/testsuite/g++.dg/lookup/scoped12.C new file mode 100644 index 00000000000..ffa145598fd --- /dev/null +++ b/gcc/testsuite/g++.dg/lookup/scoped12.C @@ -0,0 +1,14 @@ +// PR c++/112744 +// { dg-do compile } + +class A { const static int a = 0; }; +struct B : A {}; +struct C : A {}; +struct D : B, C {}; + +int main() +{ + D d; + (void) d.a; // { dg-error "private" } + (void) d.A::a; // { dg-error "private" } +} diff --git a/gcc/testsuite/g++.dg/lookup/scoped13.C b/gcc/testsuite/g++.dg/lookup/scoped13.C new file mode 100644 index 00000000000..970e1aa833e --- /dev/null +++ b/gcc/testsuite/g++.dg/lookup/scoped13.C @@ -0,0 +1,14 @@ +// PR c++/112744 +// { dg-do compile } + +struct A { const static int a = 0; }; +struct B : A {}; +struct C : A {}; +struct D : B, C {}; + +int main() +{ + D d; + (void) d.x; // { dg-error ".struct D. has no member named .x." } + (void) d.A::x; // { dg-error ".struct A. has no member named .x." } +}