Re: RFR: 6378384 (reflect) subclass can’t access superclass’s protected fields and methods by reflection

[Peter Levart]

Hi Mandy,


On 10/17/2016 10:52 PM, Mandy Chung wrote:
>> On Oct 16, 2016, at 11:18 AM, Peter Levart <peter.levart at gmail.com> wrote:
>>
>>
>> I think specifying both is more verbose on one hand, but OTOH it allows one to visually inspect all the cases and think of each and every one in isolation to see if it is valid in a particular caller/member/target arrangement. The test infrastructure verifies that the test case covers all the MemberFactory(s) so one must only verify each individual allowed / denied MemberFactory.
> I understand the intention there.  But when each test case enumerates 20 constants, it’s getting harder to review what’s going on and catch any issue.

It now enumerates only 12 constants and I think it's quite the opposite. 
Take the following for example. If only the allowed members were listed:

          ok &= new Test()
.current("b.PublicSub").member("a.PublicSuper").target("b.PublicSub")
              .allowed(PROTECTED_INSTANCE_F_M, PUBLIC_INSTANCE_F_M, 
PROTECTED_STATIC_F_M,
                       PUBLIC_STATIC_F_M, PUBLIC_C)
              .perform();

...you would review the members that are allowed and then you should ask 
yourself: "Which are the ones that are denied? All the rest. What are 
they?", or: "Could there be any other that should be allowed? Which one?"

It's much easier if they are explicitly listed:

          ok &= new Test()
.current("b.PublicSub").member("a.PublicSuper").target("b.PublicSub")
              .allowed(PROTECTED_INSTANCE_F_M, PUBLIC_INSTANCE_F_M, 
PROTECTED_STATIC_F_M,
                       PUBLIC_STATIC_F_M, PUBLIC_C)
              .denied (PRIVATE_INSTANCE_F_M, PACKAGE_INSTANCE_F_M, 
PRIVATE_STATIC_F_M,
                       PACKAGE_STATIC_F_M, PRIVATE_C, PACKAGE_C,
                       PROTECTED_C)
              .perform();


And besides, you don't really have to review them all. The fact that 
running the test on unpatched JDK 9 finds just two differences:

- access to protected static method from subclass in another package
- access to protected static field from subclass in another package

...is a reassurance that the patch does exactly what it should. No less, 
no more.

>>>     Builder::allowAll and Builder::denyAll would be useful.  allowAccessMember of a specific modifier can imply both field and method.
>> This is a good idea. Here's a modified test (no changes to patched files, just tests):
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.03/
> I like these grouping and it does help.  One more nit: would be good to replace the class name strings with constant variables.  I don’t need a new webrev.

Then I would have to have a mapping from class name -> constant name for 
the generator. Besides, constant names would not be any prettier than 
class name string literals. At least now it is obvious to anyone what 
package a particular class belongs to:

     "a.Package" vs. A_PACKAGE ?

Note that I can't use a.Package.class literal(s) here (thought I would 
like to) as they don't compile if they refer to a package-private class 
from another package.

I would like to keep those things unchanged, If you don't mind.

> thanks
> Mandy

Regards, Peter