JDK-8262003: Class.arrayType should not throw IllegalArgumentException

[Johannes Kuhn]

I want to learn about writing a CSR, and need a sponsor teaching me the
ropes.

----

Bug: https://bugs.openjdk.java.net/browse/JDK-8262003

Currently, Class.arrayType() will throw an IllegalArgumentException if
the maximum number of dimensions will be exceeded.

Throwing an IllegalArgumentException from a method that doesn't take an
argument is, at least, strange.

Therefore I would like to update the specification to allow this method
to throw an IllegalStateException, similar to what ClassDesc.arrayType()
does.

----

The current plan is:

* Create a CSR detailing the spec changes:
https://bugs.openjdk.java.net/browse/JDK-8262211
* Surround the code with a try-catch block. Checking for the error case
is hard, and somewhat rare, so I think this is appropriate.
* Copy the @throws javadoc line from ClassDesc.arrayType to Class.arrayType

But there are a few questions I would love to get the answer on:

* Both Class.arrayType() and ClassDesc.arrayType() are specified by
TypeDescriptor.OfField. Should the specification of
TypeDescriptor.OfField.arrayType() changed as well?
* Is the draft of my CSR fine? Should I add some details, or omit
things? Rephrase things?

In advance, thanks for any support and feedback on this.

- Johannes

[Mandy Chung]

Hi Johannes,

Changing Class::arrayType to throw ISA makes sense to me.   I think
`TypeDescriptor.ofField::arrayType` spec should also be updated to throw
ISA to follow what JVM checks for array dimension because it's a static
constraint check [1] (rather than a resolution error) for `anewarray`
instruction to create an array no more than 255 dimensions.

The CSR looks okay.  I agree that the compatibility risk is low.

I'd like to review PR along with CSR together.

Mandy
[1]
https://docs.oracle.com/javase/specs/jvms/se15/html/jvms-4.html#jvms-4.9.1

On 2/23/21 6:38 AM, Johannes Kuhn wrote:

> I want to learn about writing a CSR, and need a sponsor teaching me
> the ropes.
>
> ----
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8262003
>
> Currently, Class.arrayType() will throw an IllegalArgumentException if
> the maximum number of dimensions will be exceeded.
>
> Throwing an IllegalArgumentException from a method that doesn't take
> an argument is, at least, strange.
>
> Therefore I would like to update the specification to allow this
> method to throw an IllegalStateException, similar to what
> ClassDesc.arrayType() does.
>
> ----
>
> The current plan is:
>
> * Create a CSR detailing the spec changes:
> https://bugs.openjdk.java.net/browse/JDK-8262211
> * Surround the code with a try-catch block. Checking for the error
> case is hard, and somewhat rare, so I think this is appropriate.
> * Copy the @throws javadoc line from ClassDesc.arrayType to
> Class.arrayType
>
> But there are a few questions I would love to get the answer on:
>
> * Both Class.arrayType() and ClassDesc.arrayType() are specified by
> TypeDescriptor.OfField. Should the specification of
> TypeDescriptor.OfField.arrayType() changed as well?
> * Is the draft of my CSR fine? Should I add some details, or omit
> things? Rephrase things?
>
> In advance, thanks for any support and feedback on this.
>
> - Johannes

[Johannes Kuhn]

Thanks Mandy.

Will amend the CSR to cover TypeDescriptor.OfField::arrayType as well,
and create a patch.

Where should I put the tests?
Didn't find any tests that exercise Class::arrayType in test/jdk/java/lang/.
Any wishes for tests that I should add w.r.t. Class::arrayType?

- Johannes

On 24-Feb-21 20:54, Mandy Chung wrote:

> Hi Johannes,
>
> Changing Class::arrayType to throw ISA makes sense to me.   I think
> `TypeDescriptor.ofField::arrayType` spec should also be updated to throw
> ISA to follow what JVM checks for array dimension because it's a static
> constraint check [1] (rather than a resolution error) for `anewarray`
> instruction to create an array no more than 255 dimensions.
>
> The CSR looks okay.  I agree that the compatibility risk is low.
>
> I'd like to review PR along with CSR together.
>
> Mandy
> [1]
> https://docs.oracle.com/javase/specs/jvms/se15/html/jvms-4.html#jvms-4.9.1
>
> On 2/23/21 6:38 AM, Johannes Kuhn wrote:
>> I want to learn about writing a CSR, and need a sponsor teaching me
>> the ropes.
>>
>> ----
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8262003
>>
>> Currently, Class.arrayType() will throw an IllegalArgumentException if
>> the maximum number of dimensions will be exceeded.
>>
>> Throwing an IllegalArgumentException from a method that doesn't take
>> an argument is, at least, strange.
>>
>> Therefore I would like to update the specification to allow this
>> method to throw an IllegalStateException, similar to what
>> ClassDesc.arrayType() does.
>>
>> ----
>>
>> The current plan is:
>>
>> * Create a CSR detailing the spec changes:
>> https://bugs.openjdk.java.net/browse/JDK-8262211
>> * Surround the code with a try-catch block. Checking for the error
>> case is hard, and somewhat rare, so I think this is appropriate.
>> * Copy the @throws javadoc line from ClassDesc.arrayType to
>> Class.arrayType
>>
>> But there are a few questions I would love to get the answer on:
>>
>> * Both Class.arrayType() and ClassDesc.arrayType() are specified by
>> TypeDescriptor.OfField. Should the specification of
>> TypeDescriptor.OfField.arrayType() changed as well?
>> * Is the draft of my CSR fine? Should I add some details, or omit
>> things? Rephrase things?
>>
>> In advance, thanks for any support and feedback on this.
>>
>> - Johannes
>

[Mandy Chung]

On 2/25/21 6:52 AM, Johannes Kuhn wrote:

> Thanks Mandy.
>
> Will amend the CSR to cover TypeDescriptor.OfField::arrayType as well,
> and create a patch.
>
> Where should I put the tests?
> Didn't find any tests that exercise Class::arrayType in
> test/jdk/java/lang/.
> Any wishes for tests that I should add w.r.t. Class::arrayType?
>

Yes, I suggest to put it under test/jdk/java/lang/Class/arrayType.

Mandy

> - Johannes
>
> On 24-Feb-21 20:54, Mandy Chung wrote:
>> Hi Johannes,
>>
>> Changing Class::arrayType to throw ISA makes sense to me.   I think
>> `TypeDescriptor.ofField::arrayType` spec should also be updated to
>> throw ISA to follow what JVM checks for array dimension because it's
>> a static constraint check [1] (rather than a resolution error) for
>> `anewarray` instruction to create an array no more than 255 dimensions.
>>
>> The CSR looks okay.  I agree that the compatibility risk is low.
>>
>> I'd like to review PR along with CSR together.
>>
>> Mandy
>> [1]
>> https://docs.oracle.com/javase/specs/jvms/se15/html/jvms-4.html#jvms-4.9.1
>>
>> On 2/23/21 6:38 AM, Johannes Kuhn wrote:
>>> I want to learn about writing a CSR, and need a sponsor teaching me
>>> the ropes.
>>>
>>> ----
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8262003
>>>
>>> Currently, Class.arrayType() will throw an IllegalArgumentException
>>> if the maximum number of dimensions will be exceeded.
>>>
>>> Throwing an IllegalArgumentException from a method that doesn't take
>>> an argument is, at least, strange.
>>>
>>> Therefore I would like to update the specification to allow this
>>> method to throw an IllegalStateException, similar to what
>>> ClassDesc.arrayType() does.
>>>
>>> ----
>>>
>>> The current plan is:
>>>
>>> * Create a CSR detailing the spec changes:
>>> https://bugs.openjdk.java.net/browse/JDK-8262211
>>> * Surround the code with a try-catch block. Checking for the error
>>> case is hard, and somewhat rare, so I think this is appropriate.
>>> * Copy the @throws javadoc line from ClassDesc.arrayType to
>>> Class.arrayType
>>>
>>> But there are a few questions I would love to get the answer on:
>>>
>>> * Both Class.arrayType() and ClassDesc.arrayType() are specified by
>>> TypeDescriptor.OfField. Should the specification of
>>> TypeDescriptor.OfField.arrayType() changed as well?
>>> * Is the draft of my CSR fine? Should I add some details, or omit
>>> things? Rephrase things?
>>>
>>> In advance, thanks for any support and feedback on this.
>>>
>>> - Johannes
>>