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

[John Rose]

On Jan 11, 2019, at 4:53 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> I think there is a problem knowing when "access check" means just access check and when it means "access check plus the special hack for setting final fields". I'm not reading any final field semantics into the existing text, because bytecode does not know about setAccessible and final fields.

The "bytecode behavior" of an unreflectSetter MH is clearly documented
by appealing to a call to Field::set.  That is a pre-existing specification
that proves that the observed behavior for statics is a bug, and for
non-statics is a feature.  No CSR is needed, and, David, you are simply
looking in the wrong places.  I admit that the right place is tricky to find.

> I can see an argument to be made for the case where setAccessible(true) has been called to disable language level access checks, and you want the MH to follow the same access rules. But that does not to me imply that the ability to write to a final field should be part of that. If that is intended then it needs to be very, very clearly spelt out.

Yes, the documentation should be improved, so that email threads like
this won't be necessary to tease out the meaning of the spec.  I placed
a proposal for *clarifying* (not *changing*) it in the bug comments.

> And as per my other email there is already a problem with the bytecode equivalence of MH semantics and final fields, because you can set final fields in <init> or <clinit> (though only once). I don't think MH captures that - nor can it.

That's true.  The most it can do is capture the effect, if any, of
setAcc(true) on a reflected Field.

> Which means we can't defer to bytecode semantics here but rather have to very clearly define how MH interact with final fields.

And that's already done, except for the "clearly" part.

Thanks for raising this issue!

— John

diff --git a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
@@ -421,6 +421,10 @@
      * because the desired class member is missing, or because the
      * desired class member is not accessible to the lookup class, or
      * because the lookup object is not trusted enough to access the member.
+     * In the case of a field setter function on a {@code final} field,
+     * finality enforcement is treated as a kind of access control,
+     * and the lookup will fail, except in special cases of
+     * {@link Lookup#unreflectSetter Lookup.unreflectSetter}.
      * In any of these cases, a {@code ReflectiveOperationException} will be
      * thrown from the attempted lookup.  The exact class will be one of
      * the following:
@@ -1436,6 +1440,7 @@
          * @return a method handle which can store values into the field
          * @throws NoSuchFieldException if the field does not exist
          * @throws IllegalAccessException if access checking fails, or if the field is {@code static}
+         *                                or {@code final}
          * @exception SecurityException if a security manager is present and it
          *                              <a href="MethodHandles.Lookup.html#secmgr">refuses access</a>
          * @throws NullPointerException if any argument is null
@@ -1559,6 +1564,7 @@
          * @return a method handle which can store values into the field
          * @throws NoSuchFieldException if the field does not exist
          * @throws IllegalAccessException if access checking fails, or if the field is not {@code static}
+         *                                or is {@code final}
          * @exception SecurityException if a security manager is present and it
          *                              <a href="MethodHandles.Lookup.html#secmgr">refuses access</a>
          * @throws NullPointerException if any argument is null
@@ -1868,19 +1874,29 @@
         /**
          * Produces a method handle giving write access to a reflected field.
          * The type of the method handle will have a void return type.
-         * If the field is static, the method handle will take a single
+         * If the field is {@code static}, the method handle will take a single
          * argument, of the field's value type, the value to be stored.
          * Otherwise, the two arguments will be the instance containing
          * the field, and the value to be stored.
-         * If the field's {@code accessible} flag is not set,
+         * If the {@code Field} object's {@code accessible} flag is not set,
          * access checking is performed immediately on behalf of the lookup class.
          * <p>
-         * If the field is static, and
+         * If the field is {@code final}, write access will not be
+         * allowed and access checking will fail, except under certain
+         * narrow circumstances documented for {@link Field#set Field.set}.
+         * A method handle is returned only if a corresponding call to
+         * the {@code Field} object's {@code set} method could return
+         * normally.  In particular, fields which are both {@code static}
+         * and {@code final} may never be set.
+         * <p>
+         * If the field is {@code static}, and
          * if the returned method handle is invoked, the field's class will
          * be initialized, if it has not already been initialized.
          * @param f the reflected field
          * @return a method handle which can store values into the reflected field
-         * @throws IllegalAccessException if access checking fails
+         * @throws IllegalAccessException if access checking fails,
+         *         or if the field is {@code final} and write access
+         *         is not enabled on the {@code Field} object
          * @throws NullPointerException if the argument is null
          */
         public MethodHandle unreflectSetter(Field f) throws IllegalAccessException {