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

[Adam Farley8]

Hi Mandy,

The webrev has been updated with the new test: 
http://cr.openjdk.java.net/~afarley/8216558/webrev/

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.

Open for review.

Best Regards

Adam Farley 
IBM Runtimes


Mandy Chung <mandy.chung at oracle.com> wrote on 01/03/2019 17:50:49:

> 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: 01/03/2019 17:50
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> Hi Adam,
> 
> You can add a new test for this specific test case.
> MethodHandlesGeneralTest is a general test that you either update
> it to fit in existing testing framework or add a new test which
> will be simpler for you.  Please include the test in the webrev
> for easier review.
> 
> Mandy
> 
> On 3/1/19 2:31 AM, Adam Farley8 wrote:
> > Hi Mandy,
> > 
> > Historically, the bigger the change I propose, the more months it 
takes 
> > the OpenJDK community to approve.
> > 
> > I'm not sure I can justify adding an entire class to test a very 
> > specific case.
> > 
> > Additionally, HasFields seems excessively complex. I don't understand 
> > why it feels the
> > need to spend 34 lines of code parsing, processing, and storing 20 
> > minimal variables it already has.
> > 
> > Seems like an informative comment, followed by a littany of 
> > "cases.add..." would have been a simpler choice.
> > 
> > Could you tell me if I'm missing something?
> > 
> > Best Regards
> > 
> > Adam Farley
> > IBM Runtimes
> > 
> > 
> > Mandy Chung <mandy.chung at oracle.com> wrote on 21/02/2019 17:37:54:
> > 
> >> 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: 21/02/2019 17:41
> >> Subject: Re: RFR: JDK-8216558:  Lookup.unreflectSetter(Field) fails
> >> to throw IllegalAccessException for final fields
> >> 
> >> Hi Adam,
> >> 
> >> On 2/14/19 3:16 AM, Adam Farley8 wrote:
> >> > Hi Mandy,
> >> > 
> >> > Apologies for the delay.
> >> 
> >> Same here as I returned from vacation yesterday.
> >> 
> >> > Could you review this cdiff as a proposal for the jtreg test?
> >> > 
> >> > Made sense to modify the existing test set for MethodHandle rather 
than
> >> > add a new one.
> >> 
> >> Yes it'd be better if you extend the MethodHandlesGeneralTest and
> >> MethodHandlesTest to test the write access.
> >> 
> >> To add the test cases properly, MethodHandlesTest.HasFields
> >> should be modified to include the read-only fields.  It might
> >> be cleaner to add a new HasReadOnlyFields class and maybe a new
> >> TEST_SET_ACCESSIBLE bit that requests to call setAccessible(true)
> >> and testSetter expects unreflectSetter to throw exception on
> >> static final fields (possibly additional bits might be needed).
> >> 
> >> 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
> 

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