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

**Edit: the entire list has now been checked**

I now have some tests that pick up on the bug in `Utf8Entry::equalsString`.
I have added `Objects::requireNonNull` in a few places, will do an other scan when I'm done to remove a few that may be redundant, as the null check is sometimes implicit.
Classes/Interfaces that I haven't tested yet are the following 

AttributeMapper
AttributeMapper.AttributeStability
BufWriter
ClassBuilder
ClassFileElement
ClassFileTransform
ClassHierarchyResolver.ClassHierarchyInfo
ClassModel
ClassReader
ClassSignature
ClassTransform
CodeBuilder
CodeBuilder.CatchBuilder
CodeBuilder.BlockCodeBuilder
CodeElement
CodeTransform
CompoundElement
FieldBuilder
FieldTransform
MethodBuilder
MethodElement
MethodSignature
MethodTransform
Opcode
Opcode.Kind
Signature.ArrayTypeSig
Signature.BaseTypeSig
Signature.TypeArg
Signature.TypeArg.Bounded
Signature.TypeArg.Bounded.WildcardIndicator
Signature.TypeArg.Unbounded
Signature.ClassTypeSig
Signature.ThrowableSig
Signature.TypeParam
Signature.TypeVarSig
Signature.RefTypeSig
TypeAnnotation.TargetInfo
TypeAnnotation.TypePathComponent
TypeAnnotation.TypePathComponent.Kind
TypeAnnotation.TypeArgumentTarget
TypeAnnotation.OffsetTarget
TypeAnnotation.CatchTarget
TypeAnnotation.LocalVarTargetInfo
TypeAnnotation.LocalVarTarget
TypeAnnotation.ThrowsTarget
TypeAnnotation.FormalParameterTarget
TypeAnnotation.EmptyTarget
TypeAnnotation.TypeParameterBoundTarget
TypeAnnotation.SupertypeTarget
TypeAnnotation.TypeParameterTarget
TypeAnnotation.TargetType
AnnotationDefaultAttribute
BootstrapMethodsAttribute
ModuleAttribute.ModuleAttributeBuilder
ModuleExportInfo
ModuleHashInfo
ModuleHashesAttribute
ModuleMainClassAttribute
ModuleOpenInfo
ModulePackagesAttribute
ModuleProvideInfo
ModuleRequireInfo
ModuleResolutionAttribute
ModuleTargetAttribute
ClassPrinter.MapNode
ClassPrinter.ListNode
ClassPrinter.LeafNode
ClassPrinter.Node
ClassRemapper
CodeLocalsShifter
CodeRelabeler

Thanks for keeping track of this PR, I will do one more push tomorrow and open it.

I shaved 500 lines from the test I was using, I can probably make it more concise with more time.

Adding null checks breaks some tests (sometimes even the build), some are method handles related but the rest are limited to CF. 

I have filed [JDK-8339344](https://bugs.openjdk.org/browse/JDK-8339344) but more Bugs/RFEs will need to be filed.

I will modify the PR's body with some questions on which methods can be added to `TestNullHostile##EXCLUDE_LIST` and which need to be updated.

Also, I noticed a recent push (#20779) removing `CodeBuilder.loadConstant(Opcode, ConstantDesc)` which had been a major source of pain today. Will merge in a bit.

An other want I want to discuss is CodeBuilder.loadConstant(ConstantDesc value).

It [handles nulls ](https://github.com/openjdk/jdk/blob/ad40a122d632d65052b71125c0dfd58c54e3a521/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java#L614), and adding a null check causes a `BootstrapMethodError` during the build

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

PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2321858976
PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2325417395
PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2326579191
PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2326627971