RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields
mandy.chung at oracle.com
Mon Mar 11 22:49:27 UTC 2019
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.
> 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.
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
More information about the core-libs-dev