On 12/01/2019 10:13 am, Mandy Chung wrote:
On 1/11/19 3:03 PM, David Holmes wrote:
On 12/01/2019 8:47 am, Mandy Chung wrote:
On 1/11/19 2:38 PM, David Holmes wrote:
There seem to be a number of spec issues around this. Shouldn't
findStaticSetter say something about what happens when the field is
final? Same for findSetter? This issue seems to be much bigger than
just a simple bug fix.
I don't see any issue with the spec for findSetter and
findStaticSetter. findSetter returns a method handle equivalent to
`putField` bytecode and it throws IAE when access checking is
performed since it's final and not writeable. Similiarly for
findStaticSetter.
Well that is what it does, but it doesn't explicitly mention final
fields and the @throws information for IAE:
IllegalAccessException - if access checking fails, or if the field
is static
doesn't convey anything about getting IAE for a final field. Perhaps
it is implicit in the "this behaves like bytecode" but it is very
obscure.
I assume you agree that there is no spec issue w.r.t. findSetter or
findStaticSetter while you may find it obscure.
I think the implicit reliance on bytecode behaviour is a problem when it
involves final fields - see below.
A CSR request will need to be filed.
Of course, as this is a spec change.
I'm unclear what spec is actually to be changed here and in what way?
I expect Adam will send a revised webrev to include the proposed spec
change.
If these methods produce MH that obey bytecode semantics then the fact
the Field had setAccessible(true) called on it is not relevant - the
JVMS knows nothing about setAccessible. The only issue I see is the
implementation not throwing IAE when the Field represents a final
field, because it examines the "accessibility" state. (And that should
apply whether the field is static or not.)
This is the current implementation matching the specification:
If the field's accessible flag is not set, access checking is
performed immediately on behalf of the lookup class.
I think it's intended to match Field::setAccessible behavior but
Field::setAccessible has no effect in suppressing the language access
check in static final field. Hence I agree with Adam's fix changes that
the access check should be performed for static final field even its
accessible flag is set.
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. 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.
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. Which means we can't defer to bytecode
semantics here but rather have to very clearly define how MH interact
with final fields.
David
Mandy
