On Tue, 3 Sep 2024 18:27:48 GMT, ExE Boss <d...@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
