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

[Mandy Chung]

On 3/11/19 8:48 AM, Adam Farley8 wrote:
> 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.

I will sponsor this patch for you when CSR is approved.

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

No additional action from your end.  It's already in finalized state.

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

Which release are you thinking to backport to?

IMO I don't think this fix is critical for existing releases
and this fix has behavioral change.

Mandy

> 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