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... Changes requested by exe-b...@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); Module version is `null`able by Module spec. Suggestion: src/java.base/share/classes/java/lang/classfile/attribute/ModuleAttribute.java line 240: > 238: requireNonNull(module); > 239: requireNonNull(requiresFlags); > 240: requireNonNull(version); Module version is `null`able by Module spec. Suggestion: src/java.base/share/classes/java/lang/classfile/attribute/ModuleRequireInfo.java line 93: > 91: requireNonNull(requires); > 92: //todo: uncomment later after CorpusTest is fixed > 93: // requireNonNull(requiresVersion); Module version is `null`able by Module spec. Suggestion: src/java.base/share/classes/java/lang/classfile/attribute/ModuleRequireInfo.java line 104: > 102: */ > 103: static ModuleRequireInfo of(ModuleEntry requires, > Collection<AccessFlag> requiresFlags, Utf8Entry requiresVersion) { > 104: requireNonNull(requiresVersion); Module version is `null`able by Module spec. Suggestion: src/java.base/share/classes/java/lang/classfile/attribute/ModuleRequireInfo.java line 106: > 104: requireNonNull(requiresVersion); > 105: requireNonNull(requiresFlags); > 106: requireNonNull(requiresVersion); Module version is `null`able by Module spec. Suggestion: src/java.base/share/classes/java/lang/classfile/attribute/ModuleRequireInfo.java line 118: > 116: static ModuleRequireInfo of(ModuleDesc requires, int requiresFlags, > String requiresVersion) { > 117: requireNonNull(requires); > 118: requireNonNull(requiresVersion); Module version is `null`able by Module spec. Suggestion: 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) src/java.base/share/classes/jdk/internal/classfile/impl/ClassFileImpl.java line 78: > 76: public ClassFileImpl withOptions(Option... options) { > 77: requireNonNull(options); > 78: for (var o : options){ Suggestion: for (var o : options) { // implicit null-check src/java.base/share/classes/jdk/internal/classfile/impl/ClassRemapperImpl.java line 101: > 99: requireNonNull(clb); > 100: requireNonNull(cle); > 101: switch (cle) { Suggestion: switch (cle) { // implicit null-check src/java.base/share/classes/jdk/internal/classfile/impl/ClassRemapperImpl.java line 280: > 278: @Override > 279: public ClassDesc map(ClassDesc desc) { > 280: requireNonNull(desc); Allowed below: Suggestion: src/java.base/share/classes/jdk/internal/classfile/impl/CodeLocalsShifterImpl.java line 54: > 52: Objects.requireNonNull(cob); > 53: Objects.requireNonNull(coe); > 54: switch (coe) { Suggestion: switch (coe) { // implicit null-check src/java.base/share/classes/jdk/internal/classfile/impl/CodeRelabelerImpl.java line 55: > 53: requireNonNull(cob); > 54: requireNonNull(coe); > 55: switch (coe) { Suggestion: switch (coe) { // implicit null-check src/java.base/share/classes/jdk/internal/classfile/impl/ModuleAttributeBuilderImpl.java line 83: > 81: @Override > 82: public ModuleAttributeBuilder moduleVersion(String version) { > 83: requireNonNull(version); Module version is `null`able by Module spec. Suggestion: src/java.base/share/classes/jdk/internal/classfile/impl/ModuleAttributeBuilderImpl.java line 92: > 90: requireNonNull(module); > 91: // todo this crashes corpus test?? > 92: // requireNonNull(version); Module version is `null`able by Module spec. Suggestion: ------------- PR Review: https://git.openjdk.org/jdk/pull/20556#pullrequestreview-2278167656 PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2288819265 PR Comment: https://git.openjdk.org/jdk/pull/20556#issuecomment-2325306146 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742497978 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742496132 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742500730 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742505667 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742512822 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742505172 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742523836 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742523525 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742523581 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1739220025 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742526544 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742527119 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742528052 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742528767 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742529154 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742510632 PR Review Comment: https://git.openjdk.org/jdk/pull/20556#discussion_r1742510377
