[lworld] RFR: 8281463: [lworld] VALUE / PRIMITIVE modifiers should be supported by reflection [v3]

[Mandy Chung]

On Tue, 7 Jun 2022 14:26:11 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Add support to java.lang.reflect.AccessFlag ACC_IDENTITY, ACC_VALUE, and ACC_PRIMITIVE.
>> Added corresponding flags to j.l.reflect.Modifier so the "source" of the AccessFlag is the Modifer mask.
>> Remove PERMITS_VALUE; moving the definition used by HotSpot to HotSpot code.
>> 
>> Add comments to java.l.reflect.Modifier suggesting use of AccessFlag instead and limitations.
>
> Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'lworld' into 8281463-value-modifiers
>  - Add Class.is(AccessFlag) to test for a single flag.
>  - Restore AccessFlag SUPER
>  - 8281463: [lworld] VALUE / PRIMITIVE modifiers should be supported by reflection
>    Use java.lang.reflect.AccessFlag for equivalent functions for value objects.
>    Corrected ModuleDescriptor.STATIC flags to be STATIC_PHASE
>    Added flags to j.l.reflect.Modifier.

The recent commit changes `AccessFlag.IDENTITY`, `AccessFlag.VALUE`, `AccessFlag.PRIMITIVE` not a source modifier but they are.  So I expect `sourceModifier()` returns true for these new constants.    Whether these new modifiers should be defined in `Modifier` API requires discussion.   `ACC_IDENTITY` may be set even if the source does not declare `identity`.   `Modifier::toString` would not return a string that matches the source declaration.    So this version not defining new constants in Modifier API looks reasonable.

src/java.base/share/classes/java/lang/Class.java line 1511:

> 1509:      * @param flag an {@link AccessFlag}
> 1510:      */
> 1511:     public boolean is(AccessFlag flag) {

Is this API needed?   We can do `Class.accessFlags().contains(flag)`.   This discussion should belong to PR 7445 (see my comment in the `Modifier`).

src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 125:

> 123:     /**
> 124:      * The access flag {@code ACC_IDENTITY} corresponding to the
> 125:      * source modifier {@link Modifier#VALUE value} with a mask

typo: `{@link Modifier#IDENTITY identity}`

src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 134:

> 132:      * Now the flag has been repurposed as ACC_IDENTITY.
> 133:      */
> 134: //    SUPER(0x0000_0020, false, Set.of(Location.CLASS)),

I expect the test should be updated instead of commenting this out.

src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 139:

> 137:      * The access flag {@code ACC_IDENTITY} with a mask value of {@code 0x0020}.
> 138:      */
> 139:     IDENTITY(0x0000_0020, false, Set.of(Location.CLASS)),

It can also be in `Location.INNER_CLASS`.   Same for value and primitive.

src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 428:

> 426:                                        ABSTRACT, STRICT, SYNTHETIC)),
> 427:                           entry(Location.INNER_CLASS,
> 428:                                 Set.of(PUBLIC, PRIVATE, PROTECTED, IDENTITY, VALUE, PRIMITIVE,

I also caught this issue in the previous version.  Glad that it's fixed in this version but the locations of these new constant should include INNER_CLASS as well.

src/java.base/share/classes/java/lang/reflect/Modifier.java line 46:

> 44:  * The {@link AccessFlag} class provides a model that distinguishes between access flags
> 45:  * for classes, methods, and field. The {@link Class#accessFlags()} and {@link Member#accessFlags()}
> 46:  * methods provide the access flags for the respective class, method, or field.

The discussion of this proposed spec change belongs in [PR 7445](https://github.com/openjdk/jdk/pull/7445).    I would prefer to keep the version in Valhalla sync'ed with PR 7445 except the spec change to support the new `identity`, `value`, `primitive` modifiers.   Other changes not relevant to Valhalla should be done in the PR target to the main line.

src/java.base/share/classes/jdk/internal/org/objectweb/asm/Type.java line 686:

> 684: 
> 685:     static boolean isPrimitiveClass(Class<?> clazz) {
> 686:         return clazz.is(AccessFlag.PRIMITIVE);

Suggestion:

        return clazz.accessFlags().contains(AccessFlag.PRIMITIVE);

test/langtools/tools/javac/valhalla/lworld-values/InnerClassAttributeValuenessTest.java line 55:

> 53:         System.out.println("accessFlags: " + flags);
> 54: 
> 55:         if (!Inner.class.is(AccessFlag.VALUE))

Suggestion:

        if (!flags.contains(AccessFlag.VALUE))

test/langtools/tools/javac/valhalla/lworld-values/InnerClassAttributeValuenessTest.java line 57:

> 55:         if (!Inner.class.is(AccessFlag.VALUE))
> 56:             throw new AssertionError("Value flag missing");
> 57:         if (!Inner.class.is(AccessFlag.PRIMITIVE))

Suggestion:

        if (!flags.contains(AccessFlag.PRIMITIVE))

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

PR: https://git.openjdk.java.net/valhalla/pull/698