RFR: 8317356: Test ClassFile API if it deals with nulls correctly across the whole API

[Chen Liang]

On Mon, 12 Aug 2024 17:23:15 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

> The test is inspired from [FFM API's TestNulls](https://github.com/openjdk/jdk/blob/master/test/jdk/java/foreign/TestNulls.java), I customized their Null checking framework it to work with ClassFile API.
> 
> The framework for for testing an API in bulk, so that all methods are reflectively called with some values replaced with nulls, so that all combinations are tried.
> 
> This test reveals some inconsistencies in the ClassFile API, whether it's a method with nullable arguments`[1]`, nulls are passed to`[2]` or its implementation handles nulls `[3]` `[4]`.
> 
> 
> `[1]:` [ModuleRequireInfo#of](https://github.com/openjdk/jdk/blob/25e03b52094f46f89f2fe8f20e7e5622928add5f/src/java.base/share/classes/java/lang/classfile/attribute/ModuleRequireInfo.java#L89)
> `[2]:` [Signature$ClassTypeSig#of](https://github.com/openjdk/jdk/blob/7ad61605f1669f51a97f4f263a7afaa9ab7706be/src/java.base/share/classes/java/lang/classfile/Signature.java#L150)
> `[3]:`  [CatchBuilderImpl#catching](https://github.com/openjdk/jdk/blob/7ad61605f1669f51a97f4f263a7afaa9ab7706be/src/java.base/share/classes/jdk/internal/classfile/impl/CatchBuilderImpl.java#L55)
> `[4]:` [CodeBuilder#loadConstant](https://github.com/openjdk/jdk/blob/7ad61605f1669f51a97f4f263a7afaa9ab7706be/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java#L615)
> 
> 
> 
> `test/jdk/jdk/classfile/TestNullHostile.java#EXCLUDE_LIST` has the list of methods that are still not null hostile, they are ignored by the test, as making them null hostile either breaks some tests (listed below) or breaks the build with a `NullPointerException` or `BootstapMethodError`. I will paste the relevant methods from that list here.
> 
> Tests broken by these methods are :
> 
> 
> jdk/classfile/VerifierSelfTest.java
> jdk/classfile/TestRecordComponent.java
> jdk/classfile/TestNullHostile.java	
> jdk/classfile/OpcodesValidationTest.java
> jdk/classfile/ClassPrinterTest.java
> java/lang/invoke/MethodHandlesGeneralTest.java	
> java/lang/invoke/MethodHandleProxies/WrapperHiddenClassTest.java
> java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java	
> java/lang/invoke/MethodHandleProxies/BasicTest.java
> 
> 
> Full list of methods
> 
> 
> //the implementation of this method in CatchBuilderImpl handles nulls, is this fine?
> "java.lang.classfile.CodeBuilder$CatchBuilder/catching(java.lang.constant.ClassDesc,java.util.function.Consumer)/0/0",
> 
> // making this method null-hostile causes a BootstapMethodError during the the build
> //        java.lang.BootstrapMethodError: bootstrap method initiali...

👍 Reveals a lot of inconsistencies in the API. Many are remarks for future RFEs.

I doubt this is more of a conformance instead of a functional test - and this test cannot effectively test the cases where some object states will have code paths that do not throw NPE.

Indeed, `ConstantInstruction.ofArgument` needs a null check now. The other APIs should be fine as-is.

src/java.base/share/classes/java/lang/classfile/Annotation.java line 100:

> 98:                          List<AnnotationElement> elements) {
> 99:         requireNonNull(annotationClass);
> 100:         requireNonNull(elements);

How did you test these? Shouldn't null elements result in an NPE in `List.copyOf`?

src/java.base/share/classes/java/lang/classfile/ClassFileTransform.java line 1:

> 1: /*

Methods here technically shouldn't be called by users; they only exist for the API to call, and for chained transforms, API don't call these and they throw UOE. We should look for another way to handle this.

src/java.base/share/classes/java/lang/classfile/Signature.java line 162:

> 160:          * @param typeArgs signatures of the type arguments
> 161:          */
> 162:         public static ClassTypeSig of(ClassTypeSig outerType, ClassDesc className, TypeArg... typeArgs) {

Another instance of nullable argument... I thought @asotona expressed that we wish to convert nullable args to `Optional`. Need to make such parameters consistent across the ClassFile API.

test/jdk/jdk/classfile/testdata/TestClass.java line 1:

> 1: package testdata;

File seems redundant? Or did you forget to upload the test driver?

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

PR Review: https://git.openjdk.org/jdk/pull/20556#pullrequestreview-2276280243
PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2288755193
PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2326604717
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1741341522
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1741340877
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1741342557
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1741343202