On Mon, 12 Aug 2024 17:23:15 GMT, Nizar Benalla <nbena...@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
