RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Aleksey Shipilev]

On 08/21/2017 08:17 PM, Aleksey Shipilev wrote:
> On 08/21/2017 08:06 PM, Paul Sandoz wrote:
>>> On 21 Aug 2017, at 07:48, Claes Redestad <claes.redestad at oracle.com> wrote:
>>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants fails
>>> when providing an Integer as a constant, which appears to be due to failure to
>>> coerce boxed types to the corresponding primitive types when looking up various
>>> methods in StringConcatHelper:
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500
>>>
>>> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
>>> counterpart, and is a (semantical) no-op for other types, e.g., String.
>>
>> Looks good. Perhaps a token test would be useful (maybe hard to test all code paths here without some combinator test).
> 
> Please add tests. We have to at least assert that passing "null" works fine there -- I am not sure
> it does right now. Since we are talking non-String constants, passing java/lang/Object is a good
> test too.

I remembered why we haven't covered this before:

     *   <li><em>\2 (Unicode point 0002):</em> a constant. This input passed
     *   through static bootstrap argument. This constant can be any value
     *   representable in constant pool. If necessary, the factory would call
     *   {@code toString} to perform a one-time String conversion.</li>

Neither Integer, nor Object are representable in constant pool, and so this bug cannot surface via
JDK 9 bytecode, but only via direct SCF call. So, from some point of view, SCF may exhibit undefined
behavior, including throwing up on non-CP-representable value.

Using "should be the value representable in constant pool" instead of "can..." would make it
stronger, but alas. Once other constant flavors are added (/me looks at Paul, Brian and John), it
would become a possibility. This is why I think we need to add Integer and Object constant tests for
future proofing SCF, while this is still a gray area.

...and maybe add {Class, MethodHandle, MethodType} as the tested arguments too, as per:
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4

Thanks for taking care of this!

-Aleksey