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

[Adam Sotona]

On Thu, 5 Sep 2024 08:49:14 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...
>
> Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision:
> 
>   convert TestNullHostile to use JUnit Jupiter API

JDK-8317356 subject is "Test ClassFile API if it deals with nulls correctly across the whole API". 

Unfortunately this pull request blindly covers the whole API with a layer of null check, which are redundant in many cases or can be placed better to avoid double-checks, save the bytecode footprint and performance.

With such massive code addition I would also expect performance impact evaluation. 

Below are just few examples of redundant checks, I did not went through the whole code.

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

> 120:     static Annotation of(ClassDesc annotationClass,
> 121:                          List<AnnotationElement> elements) {
> 122:         requireNonNull(elements);

Isn't this covered by the method above, where the null check is also redundant?

src/java.base/share/classes/java/lang/classfile/AnnotationValue.java line 681:

> 679:      */
> 680:     static AnnotationValue of(Object value) {
> 681:         requireNonNull(value);

Below is a null test throwing IAE.

src/java.base/share/classes/java/lang/classfile/ClassFile.java line 76:

> 74:         for (var o : options){
> 75:             requireNonNull(o);
> 76:         }

Is this really necessary? I would expect NPE from ClassFile::withOptions .

src/java.base/share/classes/java/lang/classfile/ClassHierarchyResolver.java line 213:

> 211:             requireNonNull(i);
> 212:         }
> 213:         requireNonNull(classToSuperClass);

Obsolete checks.

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

Changes requested by asotona (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20556#pullrequestreview-2285569207
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1746787611
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1746791566
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1746784544
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1746796891