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

[Adam Farley8]

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.

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? 

No judgement, just looking to learn here. :)

Best Regards

Adam Farley 
IBM Runtimes


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

> 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 15:40
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> Hi Adam,
> 
> On 3/6/19 8:28 AM, Adam Farley8 wrote:
> > Hi Mandy,
> > 
> > The webrev has been updated with the new test:
> > 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=jQfLigt7_Y_u9yffi19X55OptDevspJHn76z12Z_XZI&s=93WLChJClpzd43Gy4CUKWGrdi5AZcRXLyAn93S1irj8&e=
> 
> Looks okay although I think the test adds isFinal check for the new test
> case is meant to be say static final field.
> 
> > Note that I included a handful of small improvements, and that the 
final
> > fields are all setAccessible by default, because (a) it seemed cleaner
> > than adding a new control bit, and (b) nobody is meant to be changing
> > final fields anyway.
> 
> I'm not sure what you tried to say about "nobody is meant to be changing
> final fields".  There should be tests covering all cases.
> 
> In any case, since you chose to modify MethodHandlesGeneralTest, it's 
> okay to add tests for the static final fields which is what this fix
> is about.  I created a JBS issue to add test cases for the instance
> final fields:
>     https://urldefense.proofpoint.com/v2/url?
> u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8220282&d=DwIC-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=jQfLigt7_Y_u9yffi19X55OptDevspJHn76z12Z_XZI&s=fIIn31odNd2SEKaj2zUF4qc03FJhqnQzROj09QrS0Jw&e=
> 
> FYI.  I have a patch for it and will send out for review.
>    https://urldefense.proofpoint.com/v2/url?
> u=http-3A__cr.openjdk.java.net_-7Emchung_jdk13_webrevs_8220282_webrev.
> 00_&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=jQfLigt7_Y_u9yffi19X55OptDevspJHn76z12Z_XZI&s=YhNHbETeM1kVO2frjr6aXSMgZCc69B3kDqVEiOHVSCg&e=
> 
> 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