Method Parameter Reflection spec

[Alex Buckley]

On 9/9/2013 12:02 PM, Eric McCorkle wrote:
> On 09/05/13 00:24, Joe Darcy wrote:
>> I agree the sort of cases below due to corrupt / inconsistent class
>> files are closer in flavor to error conditions than to exceptional ones.
>> An end user program "shouldn't" have to worry about recovering from
>> corrupt class files.
>
> Then it sounds like there's an agreement that the exception(s) should be
> thrown from Executable.getParameters()  Good.
>
> What remains at this point is to determine what exceptions should be
> thrown for what cases.  I don't really have a strong opinion on the
> matter; I'll happily leave that up to someone else.

First, we should stop using ReflectiveOperationException for 
reflection-time detection of malformed MethodParameters. While ROE 
itself is new, it represents the legacy checked exceptions from 
j.l.Class. For j.l.r.Parameter, it would be better to throw unchecked 
exceptions, in line with j.l.r.Field/Executable.

Second, we all agree that the unchecked exceptions should be runtime 
exceptions rather than Errors. (When Joe said "closer in flavor to error 
conditions than to exceptional ones", I don't believe he intended "error 
conditions" to mean java.lang.Error. I think he meant "closer in flavor 
to problematic conditions than to truly exceptional, program-stopping 
ones.")

I suggest MalformedParametersException, a subclass of RuntimeException, 
for cases (ii)-(vi). (Inspired by MalformedParameterizedTypeException.)

Alex

>>> Here's a list of all the ways MethodParameters attributes could go wrong:
>>>
>>> (i) Actual length does not match attribute_length
>>> (ii) The number of parameters (parameter_count) is wrong for the method
>>> (iii) A constant pool index is out of bounds.
>>> (iv) A constant pool index does not refer to a UTF-8 entry
>>> (v) A parameter's name is "", or contains an illegal character [0]
>>> (vi) The flags field contains an illegal flag (something other than
>>> FINAL, SYNTHETIC, or MANDATED)
>>>
>>> (i) causes a ClassFileFormatError when parsing the class file (as it
>>> should).  Right now, (iii) and (iv) cause an IllegalArgumentException to
>>> be thrown.
>>>
>>> I am working on a patch to correct the behavior for (ii)-(vi), so I can
>>> have Executable.getParameters() throw whatever exception you choose (ie.
>>> this is one of those rare moments of freedom).
>>>
>>>
>>> [0]: I seem to recall there being some discussion about backspaces, but
>>> the current version doesn't seem to rule them out.  Has this been
>>> changed?
>>>
>>> On 08/20/13 18:10, Alex Buckley wrote:
>>>> Sounds good. I'd wait a day in case Joe has a comment, then do it.
>>>> Please send a summary to this list of which exceptions are thrown by
>>>> Executable#getParameters and for what reasons. This will let me record
>>>> some more color in section 2.3 of 8misc.pdf, above and beyond the change
>>>> from:
>>>>
>>>> Methods of class java.lang.reflect.Parameter must throw
>>>> ReflectiveOperationException if ...
>>>>
>>>> to:
>>>>
>>>> java.lang.reflect.Executable#getParameters must throw
>>>> ReflectiveOperationException if ...
>>>>
>>>> Alex
>>>>
>>>> On 8/20/2013 1:41 PM, Eric McCorkle wrote:
>>>>> getAnnotations for each of these calls out to AnnotationParser, which
>>>>> does indeed throw an AnnotationFormatError in a variety of cases
>>>>> (duplicated annotations, malformed or incomplete annotations, etc).  In
>>>>> other cases (ie type mismatches), it delays throwing of exceptions.
>>>>> Exceptions coming from the VM (illegal access, bad constant pool
>>>>> indexes, parameter mismatches, class casts, and the like) are caught
>>>>> and
>>>>> rethrown as AnnotationFormatErrors.
>>>>>
>>>>> In light of this, I'm going to argue the precedent's there.  General
>>>>> formatting issues (which I'll argue covers bad names and wrong numbers
>>>>> of parameters) cause errors to be thrown immediately, and the
>>>>> completely
>>>>> analogous case of a bad constant pool index does cause an error to be
>>>>> thrown immediately.
>>>>>
>>>>> I'll also put forward that parameters are a bit different, and favor
>>>>> the
>>>>> approach of having Executable.getParameters() be able to throw
>>>>> exceptions for bad formatting.  Annotations are generally complex, with
>>>>> far more complex correctness conditions, and far higher cost to
>>>>> transform them from their storage format to reflective objects.  It
>>>>> makes sense to defer some of this processing.  MethodParameters, on the
>>>>> other hand, are very simple, and the main use case is to get the names
>>>>> of parameters.  Therefore, the spec ought to imply, or at least allow
>>>>> for an implementation that makes a single trip to the VM per method,
>>>>> after which the names are immediately available.  (Also, there are
>>>>> several practical advantages to allowing Parameter to be an immutable
>>>>> object that cannot be subclassed...)
>>>>>
>>>>> On 08/20/13 15:38, Alex Buckley wrote:
>>>>>> The precedent is AnnotatedElement#getAnnotations as found in Class,
>>>>>> Field, Executable, etc. Do those methods validate the
>>>>>> RuntimeVisibleAnnotations attribute before returning an array of
>>>>>> Annotation objects? I think they do. Can you describe the validation
>>>>>> they do which may cause an AnnotationTypeMismatchException or
>>>>>> IncompleteAnnotationException?
>>>>>>
>>>>>> If their validation is "equivalent" to what Executable#getParameters
>>>>>> would do, then it would be acceptable to change the spec and
>>>>>> implementation of Executable#getParameters and
>>>>>> Parameter#getName/Type/etc at this late stage. Please comment ASAP.
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>> On 8/20/2013 12:10 PM, Eric McCorkle wrote:
>>>>>>> Okay, strike #2 from my requests.  What about having
>>>>>>> Executable.getParameters() throw ReflectiveOperationException
>>>>>>> instead of
>>>>>>> methods in Parameter, for the reasons I discuss?
>>>>>>>
>>>>>>> On 08/19/13 18:12, Alex Buckley wrote:
>>>>>>>> The 8misc.pdf file doesn't say anything about bad constant pool
>>>>>>>> indexes.
>>>>>>>>
>>>>>>>> If MethodParameters was an attribute that the JVM is required to
>>>>>>>> recognize and correctly read, then the JVM would be required to
>>>>>>>> throw
>>>>>>>> ClassFormatError for bad constant pool indexes. But since
>>>>>>>> MethodParameters is an attribute that the class libraries of the
>>>>>>>> Java SE
>>>>>>>> platform - as opposed to the JVM - are required to recognize and
>>>>>>>> correctly read, it's the responsibility of the class libraries to
>>>>>>>> throw
>>>>>>>> something which is a) useful and b) consistent with bad constant
>>>>>>>> pool
>>>>>>>> indexes for similar situations such as
>>>>>>>> AnnotatedElement#getAnnotations
>>>>>>>> on a RuntimeVisibleAnnotations attribute.
>>>>>>>>
>>>>>>>> How the class libraries interact with a JVM implementation to
>>>>>>>> determine
>>>>>>>> good or bad constant pool entries (e.g. private functions of the JVM
>>>>>>>> implementation throw IllegalArgumentException, which
>>>>>>>> AnnotatedElement#getAnnotations then wraps as a FooBarException) is
>>>>>>>> not
>>>>>>>> a matter for the SE API specification. The only thing that
>>>>>>>> matters for
>>>>>>>> the SE API specification is what something like
>>>>>>>> AnnotatedElement#getAnnotations is specified to throw today.
>>>>>>>>
>>>>>>>> Alex
>>>>>>>>
>>>>>>>> On 8/19/2013 2:57 PM, Eric McCorkle wrote:
>>>>>>>>> The current version of the spec indicates that methods in
>>>>>>>>> java.lang.reflect.Parameter should throw
>>>>>>>>> ReflectiveOperationException
>>>>>>>>> for several cases (wrong number of parameters, bad parameter name,
>>>>>>>>> bad
>>>>>>>>> constant pool index).
>>>>>>>>>
>>>>>>>>> I'd like to suggest two (separate) minor revisions:
>>>>>>>>>
>>>>>>>>> 1) Executable.getParameters() should throw the
>>>>>>>>> ReflectiveOperationException in the case of an invalid name or the
>>>>>>>>> wrong
>>>>>>>>> number of parameters.  The rationale is as follows:
>>>>>>>>>
>>>>>>>>>       * Doing it this way makes it straightforward to implement
>>>>>>>>> j.l.r.Parameter as an immutable object.  Having methods in
>>>>>>>>> Parameter
>>>>>>>>> throw the exception means Parameter objects will have to remember
>>>>>>>>> whether or not they have verified their name (or else verify the
>>>>>>>>> name
>>>>>>>>> every time they are called).
>>>>>>>>>
>>>>>>>>>       * Having every single method in Parameter possibly check
>>>>>>>>> whether the
>>>>>>>>> number of parameters that were returned from the VM is correct
>>>>>>>>> seems
>>>>>>>>> rather awkward, as opposed to having a single check when Executable
>>>>>>>>> obtains the Parameter array back from the VM.
>>>>>>>>>
>>>>>>>>> 2) In the event of a bad constant pool index, an
>>>>>>>>> IllegalArgumentException.
>>>>>>>>>
>>>>>>>>>       * The current behavior of hotspot whenever it expects a UTF-8
>>>>>>>>> constant
>>>>>>>>> pool entry, but sees either an out-of-bounds index or something
>>>>>>>>> other
>>>>>>>>> than a UTF-8 is to throw an IllegalArgumentException.
>>>>>>>>>
>>