RFR: 8308753: Class-File API transition to Preview [v3]

Mon Oct 2 14:01:36 UTC 2023

On Mon, 2 Oct 2023 08:34:19 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> Classfile API is an internal library under package `jdk.internal.classfile` in JDK 21.
>> This pull request turns the Classfile API into a preview feature and moves it into `java.lang.classfile`.
>> It repackages all uses across JDK and tests and adds lots of missing Javadoc.
>> 
>> This PR goes in sync with [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API (Preview) (CSR)
>> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File API (Preview) (JEP).
>> 
>> Online javadoc is available at: 
>> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html
>> 
>> In addition to the primary transition to preview, this pull request also includes:
>> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion).
>> - A new preview feature, `CLASSFILE_API`, has been added.
>> - Buildsystem tool required a little patch to support annotated modules.
>> - All JDK modules using the Classfile API are newly participating in the preview.
>> - All tests that use the Classfile API now have preview enabled.
>> - A few Javac tests not allowing preview have been partially reverted; their conversion can be re-applied when the Classfile API leaves preview mode.
>> 
>> Despite the number of affected files, this pull request is relatively straight-forward. The preview version of the Classfile API is based on the internal version of the library from the JDK master branch, and there are no API features added.
>> 
>> Please review this pull request to help the Classfile API turn into a preview.
>> 
>> Any comments are welcome.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed javadoc typo

src/java.base/share/classes/java/lang/classfile/constantpool/ConstantPoolBuilder.java line 158:

> 156:      */
> 157:     default ClassEntry classEntry(ClassDesc classDesc) {
> 158:         if (requireNonNull(classDesc).isPrimitive()) {

It is not clear to me as to whether all the methods in the API deal with null correctly. What we did for FFM, assuming that the classfile API is also null-hostile by default, was to add a single test which would try to call all API methods and looking for NPEs:

https://github.com/openjdk/jdk/blob/2637e8ddc4ffe102418139f501fc0be8e9c5317b/test/jdk/java/foreign/TestNulls.java#L80

This test has saved us many times when adding/changing API methods.

src/java.base/share/classes/java/lang/classfile/instruction/CharacterRange.java line 77:

> 75:      * A flags word, indicating the kind of range.  Multiple flag bits
> 76:      * may be set.  Valid flags include
> 77:      * {@link java.lang.classfile.ClassFile#CRT_STATEMENT},

Maybe it would be better to organize the links in a bullet list?

src/java.base/share/classes/java/lang/classfile/instruction/InvokeInstruction.java line 67:

> 65: 
> 66:     /**
> 67:      * {@return for an {@code invokeinterface}, the {@code count} value, as defined in {@jvms 6.5}}

Suggestion:

     * {@return the {@code count} value of an {@code invokeinterface} instruction, as defined in {@jvms 6.5}}

src/java.base/share/classes/java/lang/classfile/package-info.java line 254:

> 252:  * No consistency checks are performed while building or transforming classfiles
> 253:  * (except for null arguments checks). All builders and classfile elements factory
> 254:  * methods accepts provided information without implicit validation.

Suggestion:

 * methods accepts the provided information without implicit validation.

src/java.base/share/classes/java/lang/classfile/package-info.java line 255:

> 253:  * (except for null arguments checks). All builders and classfile elements factory
> 254:  * methods accepts provided information without implicit validation.
> 255:  * However fatal inconsistencies (like for example invalid code sequence or

Suggestion:

 * However, fatal inconsistencies (like for example invalid code sequence or

src/java.base/share/classes/java/lang/classfile/package-info.java line 259:

> 257:  * the classfile building process.
> 258:  * <p>
> 259:  * Basic syntax control can be achieved using symbolic descriptors. For example

What does "syntax control" mean? The concept is used, but not defined, in the javadoc. (I presume we mean eg.g. turning String into `Ljava/lang/String;`)

src/java.base/share/classes/java/lang/classfile/package-info.java line 266:

> 264:  * On the other hand it is possible to use builders methods and factories accepting
> 265:  * constant pool entries to avoid any form of syntax control. Constant pool entries
> 266:  * can be constructed from raw values, where no syntax control is in charge.

Suggestion:

 * can be constructed from raw values, with no additional syntactic checks.

src/java.base/share/classes/java/lang/classfile/package-info.java line 273:

> 271:  * <p>
> 272:  * More complex verification of a classfile can be achieved by explicit invocation
> 273:  * of {@link java.lang.classfile.ClassModel#verify}.

Aren't part of verification also ran as part of generating stackmaps (unless stackmap inference is disabled) ? Should it be mentioned here?

src/java.base/share/classes/java/lang/classfile/package-info.java line 386:

> 384:  * resulting in many unreferenced constant pool entries.
> 385:  *
> 386:  * <h3>Transformation handling of unknown classfile elements from a future</h3>

>From a future... ? JDK?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342714653
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342716987
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342720961
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342724684
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342725264
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342726203
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342728341
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342730435
PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1342730871