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

[Adam Farley8]

Hi Mandy, 

I've made the changes to the webrev and uploaded the corrected version.

I've also uploaded the zips in the csr and bug.

As for the test, I have mentioned that the test is attached to the bug. I 
attached the same test to the CSR.

Are you saying that the test supplied is not a suitable test?

Please advise.

Best Regards

Adam Farley 
IBM Runtimes


Mandy Chung <mandy.chung at oracle.com> wrote on 30/01/2019 18:22:40:

> 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: 30/01/2019 18:22
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> Hi Adam,
> 
> On 1/30/19 7:52 AM, Adam Farley8 wrote:
> > Hi Mandy,
> > 
> > CSR has been raised: https://urldefense.proofpoint.com/v2/url?
> u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8218061&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=wVe5GwdYZTp4EVs77yXBcAfCyKsPsjBCVBjF0XjirEs&s=1DFz7ShyytN1Bj8a3YtRyTpwbT1Cn1o-
> Jf-ec5OvF7w&e=
> 
> Thanks for doing it.  A couple comments:
> 
> I think the compatibility risk should be low rather than minimal.
> Even the code shouldn't be doing that, unreflectSetter does
> allow to set a static final field in the current implementation.
> It's low because Field::set throws exception if it's a static
> final field and so it's expected that very few existing libraries
> would use unreflectSetter.
> 
> FWIW.  There are few existing libraries hacking the reflection
> to change their static final fields but very few.
> 
> Since the spec diff is small, I suggest to cut-n-paste the diff
> on the javadoc to the specification section and easier to see
> the spec change.  webrev is not generally needed for CSR but
> welcome to include that if it helps the review.
> 
> I made some minor edit to the CSR to make the problem and solution
> clearer.
> 
> > The webrev I used includes John's comments, your additions, the 
> > one-line-IF,
> > and I also took the liberty of moving the unreflectField method 
underneath
> > the unreflectSetter method, because it seemed strange that it was 
located
> > between unreflectSetter and unreflectGetter.
> > 
> > Online webrev has been updated to include these changes:
> > https://urldefense.proofpoint.com/v2/url?
> u=http-3A__cr.openjdk.java.net_-7Eafarley_8216558_webrev&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=wVe5GwdYZTp4EVs77yXBcAfCyKsPsjBCVBjF0XjirEs&s=0KYjj1f6lxz1DSEcK1UnPJ3ozG9-
> VwiUx2-B_u9EaD0&e=
> 
> Looks good but the patch still needs a regression test. Please
> include the regression test for code review.
> 
> Formatting nit: a space is missing between 'if' and '(' and
> plase use the 4-space identation of the throw.
> 
> 1901             if(isSetter && field.isStatic() && field.isFinal())
> 1902                     throw field.makeAccessException("static final 
> field has no write access", this);
> 
> 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