Hiya Mandy, I've updated the webrev with your proposal, plus a few tweaks for formatting.
http://cr.openjdk.java.net/~afarley/8216558.2/webrev Let me know what you think. Best Regards Adam Farley IBM Runtimes Mandy Chung <mandy.ch...@oracle.com> wrote on 26/03/2019 03:51:29: > From: Mandy Chung <mandy.ch...@oracle.com> > To: Adam Farley8 <adam.far...@uk.ibm.com>, Joe Darcy <joe.da...@oracle.com> > Cc: core-libs-dev <core-libs-dev@openjdk.java.net> > Date: 26/03/2019 03:51 > Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails > to throw IllegalAccessException for final fields > > > On 3/25/19 11:36 AM, Adam Farley8 wrote: > Addendum: URL for new webrev: http://cr.openjdk.java.net/~afarley/ > 8216558.1/webrev/ > > Thanks for sending a versioned webrev. > > 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? > > > Each test case has a set of inaccessible fields under normal access checks. > For MH_UNREFLECT_GETTER_ACCESSIBLE and MH_UNREFLECT_SETTER_ACCESSIBLE > cases, before the patch, all fields are accessible as setAccessible(true) > is called on all fields. That's the special casing for these cases and > expect actualFieldNames is empty. > > This patch now changes the set of inaccessible fields for unreflectSetter > (static final fields are inaccessible). A cleaner way to fix this is > to filter a given set of inaccessible fields and return the set of > inaccessible fields applicable to a FieldLookup case. > > Attached is my suggested patch. It adds a new FieldLookup::inaccessibeFields > method and the XXX_ACCESSIBLE Lookup overrides it to do the filtering. > > Mandy > > > diff --git a/test/jdk/java/lang/invoke/VarHandles/accessibility/ > TestFieldLookupAccessibility.java b/test/jdk/java/lang/invoke/ > VarHandles/accessibility/TestFieldLookupAccessibility.java > --- a/test/jdk/java/lang/invoke/VarHandles/accessibility/ > TestFieldLookupAccessibility.java > +++ b/test/jdk/java/lang/invoke/VarHandles/accessibility/ > TestFieldLookupAccessibility.java > @@ -22,7 +22,7 @@ > */ > > /* @test > - * @bug 8152645 > + * @bug 8152645 8216558 > * @summary test field lookup accessibility of MethodHandles and VarHandles > * @compile TestFieldLookupAccessibility.java > * pkg/A.java pkg/B_extends_A.java pkg/C.java > @@ -96,6 +96,12 @@ > Object lookup(MethodHandles.Lookup l, Field f) throws Exception { > return l.unreflectGetter(cloneAndSetAccessible(f)); > } > + > + // Setting the accessibility bit of a Field grants access under > + // all conditions for MethodHandler getters > + Set<String> inaccessibleFields(Set<String> inaccessibleFields) { > + return new HashSet<>(); > + } > }, > MH_UNREFLECT_SETTER() { > Object lookup(MethodHandles.Lookup l, Field f) throws Exception { > @@ -103,13 +109,25 @@ > } > > boolean isAccessible(Field f) { > - return f.isAccessible() || !Modifier.isFinal > (f.getModifiers()); > + return f.isAccessible() && !Modifier.isStatic > (f.getModifiers()) || !Modifier.isFinal(f.getModifiers()); > } > }, > MH_UNREFLECT_SETTER_ACCESSIBLE() { > Object lookup(MethodHandles.Lookup l, Field f) throws Exception { > return l.unreflectSetter(cloneAndSetAccessible(f)); > } > + > + boolean isAccessible(Field f) { > + return !(Modifier.isStatic(f.getModifiers()) && > Modifier.isFinal(f.getModifiers())); > + } > + > + // Setting the accessibility bit of a Field grants > access to non-static > + // final fields for MethodHandler setters > + Set<String> inaccessibleFields(Set<String> inaccessibleFields) { > + Set<String> result = new HashSet<>(); > + inaccessibleFields.stream().filter(f -> f.contains > ("_static_")).forEach(result::add); > + return result; > + } > }, > VH() { > Object lookup(MethodHandles.Lookup l, Field f) throws Exception { > @@ -142,6 +160,10 @@ > return true; > } > > + Set<String> inaccessibleFields(Set<String> inaccessibleFields) { > + return new HashSet<>(inaccessibleFields); > + } > + > static Field cloneAndSetAccessible(Field f) throws Exception { > // Clone to avoid mutating source field > f = f.getDeclaringClass().getDeclaredField(f.getName()); > @@ -180,7 +202,7 @@ > @Test(dataProvider = "lookupProvider") > public void test(FieldLookup fl, Class<?> src, > MethodHandles.Lookup l, Set<String> inaccessibleFields) { > // Add to the expected failures all inaccessible fields due > to accessibility modifiers > - Set<String> expected = new HashSet<>(inaccessibleFields); > + Set<String> expected = fl.inaccessibleFields(inaccessibleFields); > Map<Field, Throwable> actual = new HashMap<>(); > > for (Field f : fields(src)) { > @@ -203,12 +225,9 @@ > if (!actualFieldNames.equals(expected)) { > if (actualFieldNames.isEmpty()) { > // Setting the accessibility bit of a Field grants > access under > - // all conditions for MethodHander getters and setters > - if (fl != FieldLookup.MH_UNREFLECT_GETTER_ACCESSIBLE && > - fl != FieldLookup.MH_UNREFLECT_SETTER_ACCESSIBLE) { > + // all conditions for MethodHandler getters > Assert.assertEquals(actualFieldNames, expected, > "No accessibility failures:"); > } > - } > else { > Assert.assertEquals(actualFieldNames, expected, > "Accessibility failures differ:"); > } 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
