RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields

[Adam Farley8]

Hi Mandy,

Thank you for explaining. :)

Unfortunately I'm only a mere Author, and I cannot submit test runs on the 
shared test server.

I have run the test locally, and it passes against a patched build and 
fails correctly agaionst an unpatched build, so I think we're good to go.

Can you tell me if there's something I can do to help move the CSR 
forward?

Also, can you tell me if an additional CSR would need to be created for 
other releases if this fix gets backported?

Best Regards

Adam Farley 
IBM Runtimes


Mandy Chung <mandy.chung at oracle.com> wrote on 07/03/2019 23:17:15:

> From: Mandy Chung <mandy.chung at oracle.com>
> To: Adam Farley8 <adam.farley at uk.ibm.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>
> Date: 07/03/2019 23:19
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> 
> 
> On 3/7/19 9:44 AM, Adam Farley8 wrote:
> > Hi Mandy,
> > 
> > Since you have created a new work item to cover the test work, do let
> > me know if you want the test code removed from the webrev for the
> > original issue.
> 
> You push what you have.  Your fix should come with a regression test.
> Please make sure you do jdk-submit to verify the test work.
> 
> > Also, by "nobody is meant to be changing final fields anyway", I was
> > explaining my decision to setAccessible(true) on the final fields by
> > default, rather than setting it via a control bit as you suggested.
> > Seemed to save code while exposing us to minimal risk, because the
> > field is meant to be immutable (final).
> > 
> > Lastly, I'm sorry to see you weren't happy with the quality of my test
> > code. Your changes seem much larger in scale, and quite different to
> > my changes. Could you explain the benefits of your approach, vs simply
> > adding non-static final fields to my code?
> 
> First, I dont see my test update is large scale.  Second, it's not
> about the quality of the test code.  The size of the change is not
> the sole factor to consider but that seems to bother you.  Anyway,
> the test itself isn't easy to work with.  You have done the fix to
> include the regression test which is okay.
> 
> Your patch made me to look at the test closer which I started from
> the version in the repo, not based on your version.  Once you push
> your fix, I can generate a new webrev to see the diff that I can
> revise on top of your version.
> 
> I change a couple things.  The test uses Error in HasFields.CASES to
> indicate a negative test.  For a final field, it has no write access
> and it is a negative test case except unreflectSetter on an instance
> final field whose accessible flag is true.  That explains why
> Error.class is in the third element of the new case added for final
> field that actually reflects if a test case is positive or negative
> when calling testAccessor.   What you added is treating setter on
> final field is positive test case and then returns when IAE is thrown.
> It does the work but passing incorrect information.
> 
> I uncovered some unnecessary test cases for findSetter on
> static fields and findStaticSetter on non-static fields.  It's an
> existing test issue and I think they  should be filtered out and
> that's why CASES is separated into two separate ArrayLists and the new 
> static cases(int testMode) method.  I see that you used a new kind for
> static final field that may be a good approach.  When I pull down your
> change, I will send out a revision (that's why I wait for your patch
> to go in first and see any thing I should change).
> 
> Hope this helps.
> 
> Mandy
> 

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU