RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]

[liach]

On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy <darcy at openjdk.org> wrote:

>> This is an early review of changes to better model JVM access flags, that is "modifiers" like public, protected, etc. but explicitly at a VM level.
>> 
>> Language level modifiers and JVM level access flags are closely related, but distinct. There are concepts that overlap in the two domains (public, private, etc.), others that only have a language-level modifier (sealed), and still others that only have an access flag (synthetic).
>> 
>> The existing java.lang.reflect.Modifier class is inadequate to model these subtleties. For example, the bit positions used by access flags on different kinds of elements overlap (such as "volatile" for fields and "bridge" for methods. Just having a raw integer does not provide sufficient context to decode the corresponding language-level string. Methods like Modifier.methodModifiers() were introduced to cope with this situation.
>> 
>> With additional modifiers and flags on the horizon with projects like Valhalla, addressing the existent modeling deficiency now ahead of time is reasonable before further strain is introduced.
>> 
>> This PR in its current form is meant to give the overall shape of the API. It is missing implementations to map from, say, method modifiers to access flags, taking into account overlaps in bit positions.
>> 
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in once the API is further along.
>
> Joe Darcy 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 26 additional commits since the last revision:
> 
>  - Respond to review feedback.
>  - Merge branch 'master' into JDK-8266670
>  - Make workding changes suggested in review feedback.
>  - Merge branch 'master' into JDK-8266670
>  - Typo fix; add implSpec to Executable.
>  - Appease jcheck.
>  - Fix some bugs found by inspection, docs cleanup.
>  - Merge branch 'master' into JDK-8266670
>  - Initial support for accessFlags methods
>  - Add mask to access flag functionality.
>  - ... and 16 more: https://git.openjdk.java.net/jdk/compare/0053820b...14980605

Here, I argue for the case of splitting different types of access flags into distinct enums, such as `MemberAccessFlag`, `ModuleRequiresAccessFlag`, `ModuleExportsAccessFlag` implementing a sealed interface `AccessFlag` that keeps all its existing methods, since most of those access flags will never be returned in the same collection. The `accessFlags()` method should return a `Set<? extends AccessFlag>`, which is 1. safe as the set is read-only; 2. can be overridden with `Set<? extends MemberAccessFlag>` (For forward compability, we can make `MemberAccessFlag` an interface with static final fields, implemented by some package-private enum in case we want to split it into more specific types down the road).

But you may ask, what about `SYNTHETIC`, `MANDATED`, `FINAL`? Aren't they shared by almost all of the locations? What if users test with incorrect enum instance, such as looking for the `MemberAccessFlag.SYNTHETIC` in `moduleDescriptor.accessFlags()`?

This problem can be prevented:
1. by making `ModuleDescriptor.accessFlags()` return `Set<? extends ModuleDescriptorAccessFlag>`, thus the IDE can easily detect misuse when they insert the access flag of a different type;
2. In the current hodgepodge `AccessFlag`, we have `STATIC` and `STATIC_PHASE`, and the incorrect `ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC)` call is much more subtle, especially to new users of this class. Arguably, this misuse would be way worse than that in the distinct enum case.

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

PR: https://git.openjdk.java.net/jdk/pull/7445