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

[Adam Farley8]

Hi Mandy,

Answers below. :)

Mandy Chung <mandy.chung at oracle.com> wrote on 22/03/2019 00:35:00:

> 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: 22/03/2019 00:35
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
>  217                 //If this is a USA test, then only the fraction
> of the expected failures will occur; those which are both static and 
final.
>  218                 if (fl != 
> FieldLookup.MH_UNREFLECT_SETTER_ACCESSIBLE && 
> actualFieldNames.stream().anyMatch(s->!(s.contains("static")&&
> (s.contains("final")))))
> 
> What is a USA test?

UNREFLECT_SETTER_ACCESSIBLE. 

I was trying to be brief, and I lost readability. 

Will re-upload with the expanded name.

> 
> Why the assertion becomes conditional?

Because we only need this check if the test is USA.

Previously, setting a variable as accessible gave us a setter without 
errors 100% of the time.

The test expects this, and anticipates an empty list of variables which 
threw errors when passed to unreflectSetter.

Since we now throw an exception when calling the unreflectSetter method, 
even when the variable is accessible, this check ensures that the only 
variables throwing an exception are the static final fields.

> 
> Have you considered another way doing it?

Yep, several. This seemed the simplest.

Other options I considered were:

--------------

1) Changing the test to store a list of variables, rather than just their 
names, so the "isAccessible" method could be used instead.

Discarded because it makes the array compare slower for every test.

2) Editing the array of anticipated excludeds during test execution, to 
remove the exceptions we don't expect to see.

Discarded because the change seemed too bulky to accommodate a single 
test.

3) Make the "isAccessible" methods smarter, and rely on them exclusively 
to determine the list of anticipated exceptions.

Ditto

---------------------

Option 3 could work. I prefer the one-liner stream check.

Thoughts?

- Adam

> 
> Mandy

> 
> On 3/21/19 9:51 AM, Adam Farley8 wrote:
> Hi Mandy, 
> 
> Good spot. Have updated the webrev. 
> 
> http://cr.openjdk.java.net/~afarley/8216558/webrev/
> 
> Could you review the proposed change? 
> 
> Also, I ran the invoke tests as advised, and the only one that 
> fails is "LFGarbageCollectedTest.java", which also fails in the 
> same way when run against a non-patched java, so I think we're ok. 
> 
> Best Regards
> 
> Adam Farley 
> IBM Runtimes
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