[PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp]

> Yes, thanks; it would take a lot to make me request less comments.

Awesome.

> The second lines of arguments are indented one space too far in both these calls.

Oops! I will fix that.

> Well, I guess it could be using a declaration of the same name from another base.

 Yes I had been worrying about that.

> But in that case you could end up with an overload set
> containing both the decl we're complaining about and another of the same
> name from another base, in which case the lookup result would include
> both, and so the comparison would fail and we would fall through to the
> private base assumption.

I think technically yes ... but also no since such code would not
compile anyways, plus oddly it seems to work anyway. For instance
(assuming I'm understanding correctly), if you do this (with the patch
applied):

class A
{
  protected:
  int i;
};

class A2
{
  protected:
  int i;
};

class B:public A, public A2
{
  private:
  using A::i, A2::i;
};

class C:public B
{
  void f()
  {
    A::i = 0;
  }
};

You get:

error: redeclaration of ‘using A::i’
   using A::i;

note: previous declaration ‘using A2::i’
    using A2::i;

error: redeclaration of ‘using A2::i’
   using A::i, A2::i;

previous declaration ‘using A::i’
   using A::i, A2::i;

In member function ‘void C::f()’:
error: ‘int A::i’ is private within this context
   A::i = 0;

note: declared private here
   using A::i, A2::i;

Which seems to work (well ... more like fail to compile ... as
expected). Maybe you're imagining a different situation to me?

You can even use void f() { i = 0; } and void f() { A2::i = 0; } and
you seem to get the same results either which way (although, to be
fair, if you do A2::i = 0, it suddenly doesn't complain about it being
private anymore [no idea why], it just complains about the
redeclaration , and once you fix the redeclaration, it THEN complains
about being private, so it's got a bit of a quirk - don't think that's
related to the patch though).

> But checking the name is a simple way to skip irrelevant usings.

That does sound like a better way of doing it. Would I just do
cp_tree_equal (DECL_NAME (blah1), DECL_NAME (blah2)) [assuming
DECL_NAME returns a tree], or perhaps DECL_NAME (blah1) == DECL_NAME
(blah2)?

> Maybe also check that the using is TREE_PRIVATE?

Would that be necessary? Maybe if you wanted to sanity-check it I
suppose. We know for sure that PARENT_BINFO has private access to
DECL. If we find a using statement introducing DECL in PARENT_BINFO,
then surely the using statement must (by definition) have been
private? If it were not private, then the child class would have been
able to access it, and enforce_access wouldn't have thrown an error.
It doesn't seem to be the case that DECL could be private for any
other reason other than the using decl being private.

Let me know your thoughts and I will update the patch. Thanks for your help.

Anthony


On Thu, 4 Feb 2021 at 16:33, Jason Merrill <jason@redhat.com> wrote:
>
> On 2/4/21 11:24 AM, Jason Merrill wrote:
> > On 2/4/21 10:02 AM, Anthony Sharp via Gcc-patches wrote:
> >> Hello,
> >>
> >> New bugfix for PR19377
> >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19377). This is
> >> basically an extension of what I did before for PR17314 except it also
> >> fixes this other bug.
> >>
> >> I hope I didn't over-comment in the code ... better to say too much
> >> than too little! It's a niche bug so I thought it could do with a
> >> little explanation.
> >
> > Yes, thanks; it would take a lot to make me request less comments.
> >
> >> +      if (TREE_CODE (parent_field) == USING_DECL)
> >> +       {
> >> +         if (cp_tree_equal (decl,
> >> +                            lookup_member (parent_binfo,
> >> +                                           DECL_NAME (parent_field),
> >> +                                           /*protect=*/0,
> >> +                                           /*want_type=*/false,
> >> +                                           tf_warning_or_error)))
> >
> > Isn't it sufficient to check that the names match?
>
> Well, I guess it could be using a declaration of the same name from
> another base.  But in that case you could end up with an overload set
> containing both the decl we're complaining about and another of the same
> name from another base, in which case the lookup result would include
> both, and so the comparison would fail and we would fall through to the
> private base assumption.
>
> But checking the name is a simple way to skip irrelevant usings.
> Maybe also check that the using is TREE_PRIVATE?
>
> >>           tree parent_binfo = get_parent_with_private_access (decl,
> >> -
> >> basetype_path);
> >> +
> >> basetype_path);
> > ...
> >> +             diag_location = get_class_access_diagnostic_decl
> >> (parent_binfo,
> >> +
> >> diag_decl);
> >
> > The second lines of arguments are indented one space too far in both
> > these calls.
> >
> > Jason
>