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

[Chen Liang]

On Tue, 3 Sep 2024 18:27:48 GMT, ExE Boss <duke 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...
>
> src/java.base/share/classes/java/lang/classfile/ClassHierarchyResolver.java line 226:
> 
>> 224:     static ClassHierarchyResolver ofClassLoading(ClassLoader loader) {
>> 225:         //null check here breaks WithSecurityManagerTest
>> 226: //        requireNonNull(loader);
> 
> The `null` `ClassLoader` is valid and refers to either the system or bootstrap class loader (depending on the API).
> 
> In the case of [`Class​::forName(…)`], it’s the bootstrap class loader (which is represented by `null` (`Object.class.getClassLoader()` returns `null`!))
> Suggestion:
> 
> 
> 
> [`Class​::forName(…)`]: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Class.html#forName(java.lang.String,boolean,java.lang.ClassLoader)

We intend to avoid using `null` to represent bootstrap class loader here, like in `MethodType`: https://github.com/openjdk/jdk/blob/bbb516163d400a9c7e923e423fe2a60091b59322/src/java.base/share/classes/java/lang/invoke/MethodType.java#L1197

> src/java.base/share/classes/java/lang/classfile/CodeBuilder.java line 622:
> 
>> 620:     default CodeBuilder loadConstant(ConstantDesc value) {
>> 621: //        adding null check here causes error
>> 622: //        requireNonNull(value);
> 
> This method is specified as allowing `null` below.
> Suggestion:

We are thinking about whether to allow nullable or enforce `Optional` across the ClassFile API. So this may be changed to accept an `Optional` in the future.

> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java line 385:
> 
>> 383:         @Override
>> 384:         public boolean equalsString(String s) {
>> 385:             Objects.requireNonNull(s);
> 
> I’d prefer if this method returned `false` on `null`, just like how [`Object​::equals​(Object)`] and [`String​::equalsIgnoreCase​(String)`] return `false` on `null`.
> 
> [`Object​::equals​(Object)`]: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Object.html#equals(java.lang.Object)
> [`String​::equalsIgnoreCase​(String)`]: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/String.html#equalsIgnoreCase(java.lang.String)

We should have noted clearly that the intended use case is for a read utf8 to compare to an existing string, like a method name, so we can minimally inflate a utf8 to perform a comparison. This means that there is little purpose of using `null` here, just like for any other Class-File API endpoints.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742619721
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742622251
PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1739266003