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

[Mandy Chung]

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.

Mandy