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

Wed Sep 4 10:36:53 UTC 2024

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...

Changes requested by ExE-Boss at github.com (no known OpenJDK username).

> And this test cannot effectively test the cases where some object states will have code paths that do not throw NPE.

**Ref:** [JDK‑8336430](https://bugs.openjdk.org/browse/JDK-8336430)

Since this now makes the NPE behaviour of [`Utf8Entry::equalsString(String)`] consistent, this PR can be marked as fixing [JDK‑8336430].

[`Utf8Entry::equalsString(String)`]: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/classfile/constantpool/Utf8Entry.html#equalsString(java.lang.String)
[JDK‑8336430]: https://bugs.openjdk.org/browse/JDK-8336430

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

> 86:             //todo: uncomment later
> 87:             // this causes CorpusTest to fail
> 88:             // requireNonNull(superClass);

This is valid, as the super class descriptor of `java.lang.Object` is `null` (it’s the only one allowed and required to extend `null`).
Suggestion:

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)

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:

src/java.base/share/classes/java/lang/classfile/attribute/ModuleAttribute.java line 144:

> 142:         requireNonNull(moduleName);
> 143:         //todo should moduleVersion be Nullable?
> 144: //        requireNonNull(moduleVersion);