[lworld] RFR: 8361496: [lworld] Treating interfaces as isValue() leads to failed assert [v2]

[Roger Riggs]

On Tue, 8 Jul 2025 14:34:50 GMT, Chen Liang <liach at openjdk.org> wrote:

>> Reviewed a few sites of isValue() usages that cause immediate trouble with bootstrap and basic tests. Factor out `ValueClass::isValueObjectCompatible` and `isValueObjectInstance` to represent the two states of value classes: field types that can hold value objects, and field types that definitely hold value objects.
>> 
>> I think we still need a more throughout review for all `isValue` usages, such as those in MethodHandle or VarHandle. Preliminary testing hasn't revealed anything yet.
>> 
>> Testing: tier 1-3, seems to have no new failures
>
> Chen Liang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Test both preview and non-preview for ValueClassTest

src/java.base/share/classes/jdk/internal/value/ValueClass.java line 45:

> 43:     /// This excludes primitives and includes Object.
> 44:     public static boolean isValueObjectCompatible(Class<?> fieldType) {
> 45:         return PreviewFeatures.isEnabled() && !fieldType.isPrimitive() && (!fieldType.isIdentity() || fieldType == Object.class);

The `fieldType == Object.class` would always be true regardless of enable-preview.
That does not seem to be accurate and might be mis-interpreted by a caller.

src/java.base/share/classes/jdk/internal/value/ValueClass.java line 50:

> 48:     /// {@return whether an object of this exact class is a value object}
> 49:     /// This excludes abstract value classes and primitives.
> 50:     public static boolean isValueObjectInstance(Class<?> clazz) {

The method name is a bit misleading, it seems to ask if the parameter is an instance, but it is a class.
Perhaps `isConcreteValueObject()` or `isConcreteValueClass()`.

test/jdk/valhalla/valuetypes/ValueClassTest.java line 60:

> 58:         assertFalse(isValueObjectCompatible(ArrayList[].class), "array class");
> 59:         assertFalse(isValueObjectCompatible(String[].class), "array class");
> 60:         assertFalse(isValueObjectCompatible(Comparable[].class), "array class");

If these ever fail, the error messages are not going to be very information  like`expected tru, actual = false Object`;
no mention of preview mode being tested or the method under test.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1504#discussion_r2205474466
PR Review Comment: https://git.openjdk.org/valhalla/pull/1504#discussion_r2205467836
PR Review Comment: https://git.openjdk.org/valhalla/pull/1504#discussion_r2205460154