8321248: ClassFile API ClassModel::verify is inconsistent with the rest of the API

Reviewed-by: jlahoda, mcimadamore

src/java.base/share/classes/java/lang/classfile/ClassFile.java

@@ -43,6 +43,7 @@ import java.lang.classfile.attribute.CharacterRangeInfo;
 import java.lang.classfile.attribute.LocalVariableInfo;
 import java.lang.classfile.attribute.LocalVariableTypeInfo;
 import java.lang.classfile.instruction.ExceptionCatch;
+import java.util.List;
 import static java.util.Objects.requireNonNull;
 import jdk.internal.javac.PreviewFeature;
 
@@ -481,6 +482,33 @@ public sealed interface ClassFile
      */
     byte[] transform(ClassModel model, ClassEntry newClassName, ClassTransform transform);
 
+    /**
+     * Verify a classfile.  Any verification errors found will be returned.
+     * @param model the class model to verify
+     * @return a list of verification errors, or an empty list if no errors are
+     * found
+     */
+    List<VerifyError> verify(ClassModel model);
+
+    /**
+     * Verify a classfile.  Any verification errors found will be returned.
+     * @param bytes the classfile bytes to verify
+     * @return a list of verification errors, or an empty list if no errors are
+     * found
+     */
+    List<VerifyError> verify(byte[] bytes);
+
+    /**
+     * Verify a classfile.  Any verification errors found will be returned.
+     * @param path the classfile path to verify
+     * @return a list of verification errors, or an empty list if no errors are
+     * found
+     * @throws java.io.IOException if an I/O error occurs
+     */
+    default List<VerifyError> verify(Path path) throws IOException {
+        return verify(Files.readAllBytes(path));
+    }
+
     /** 0xCAFEBABE */
     int MAGIC_NUMBER = 0xCAFEBABE;
 

src/java.base/share/classes/java/lang/classfile/ClassModel.java

@@ -78,29 +78,4 @@ public sealed interface ClassModel
 
     /** {@return whether this class is a module descriptor} */
     boolean isModuleInfo();
-
-    /**
-     * Verify this classfile.  Any verification errors found will be returned.
-     *
-     * @param debugOutput handler to receive debug information
-     * @return a list of verification errors, or an empty list if no errors are
-     * found
-     */
-    default List<VerifyError> verify(Consumer<String> debugOutput) {
-        return VerifierImpl.verify(this, debugOutput);
-    }
-
-    /**
-     * Verify this classfile.  Any verification errors found will be returned.
-     *
-     * @param debugOutput handler to receive debug information
-     * @param classHierarchyResolver class hierarchy resolver to provide
-     *                               additional information about the class hierarchy
-     * @return a list of verification errors, or an empty list if no errors are
-     * found
-     */
-    default List<VerifyError> verify(ClassHierarchyResolver classHierarchyResolver,
-                                     Consumer<String> debugOutput) {
-        return VerifierImpl.verify(this, classHierarchyResolver, debugOutput);
-    }
 }

src/java.base/share/classes/java/lang/classfile/package-info.java

@@ -277,8 +277,8 @@
  *                             constantPoolBuilder.utf8Entry("mypackage.MyClass"));
  * }
  * <p>
- * More complex verification of a classfile can be achieved by explicit invocation
+ * More complex verification of a classfile can be achieved by invocation of
- * of {@link java.lang.classfile.ClassModel#verify}.
+ * {@link java.lang.classfile.ClassFile#verify}.
  *
  * <h2>Transforming classfiles</h2>
  * ClassFile Processing APIs are most frequently used to combine reading and

src/java.base/share/classes/jdk/internal/classfile/impl/ClassFileImpl.java

@@ -39,6 +39,7 @@ import java.lang.classfile.ClassTransform;
 import java.lang.classfile.constantpool.ClassEntry;
 import java.lang.classfile.constantpool.ConstantPoolBuilder;
 import java.lang.classfile.constantpool.Utf8Entry;
+import jdk.internal.classfile.impl.verifier.VerifierImpl;
 
 public record ClassFileImpl(StackMapsOption stackMapsOption,
                             DebugElementsOption debugElementsOption,
@@ -128,6 +129,21 @@ public record ClassFileImpl(StackMapsOption stackMapsOption,
                     }
                 });
     }
+
+    @Override
+    public List<VerifyError> verify(ClassModel model) {
+        return VerifierImpl.verify(model, classHierarchyResolverOption().classHierarchyResolver(), null);
+    }
+
+    @Override
+    public List<VerifyError> verify(byte[] bytes) {
+        try {
+            return verify(parse(bytes));
+        } catch (IllegalArgumentException parsingError) {
+            return List.of(new VerifyError(parsingError.getMessage()));
+        }
+    }
+
     public record AttributeMapperOptionImpl(Function<Utf8Entry, AttributeMapper<?>> attributeMapper)
             implements AttributeMapperOption {
     }

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java

@@ -212,7 +212,7 @@ public class BindingSpecializer {
         }
 
         if (PERFORM_VERIFICATION) {
-            List<VerifyError> errors = ClassFile.of().parse(bytes).verify(null);
+            List<VerifyError> errors = ClassFile.of().verify(bytes);
             if (!errors.isEmpty()) {
                 errors.forEach(System.err::println);
                 throw new IllegalStateException("Verification error(s)");

test/jdk/jdk/classfile/AdvancedTransformationsTest.java

@@ -75,7 +75,7 @@ class AdvancedTransformationsTest {
         try (var in = StackMapGenerator.class.getResourceAsStream("StackMapGenerator.class")) {
             var cc = ClassFile.of();
             var clm = cc.parse(in.readAllBytes());
-            var remapped = cc.parse(cc.transform(clm, (clb, cle) -> {
+            cc.verify(cc.transform(clm, (clb, cle) -> {
                 if (cle instanceof MethodModel mm) {
                     clb.transformMethod(mm, (mb, me) -> {
                         if (me instanceof CodeModel com) {
@@ -99,7 +99,6 @@ class AdvancedTransformationsTest {
                 else
                     clb.with(cle);
             }));
-            remapped.verify(null);
         }
     }
 
@@ -115,12 +114,12 @@ class AdvancedTransformationsTest {
             var cc = ClassFile.of();
             var clm = cc.parse(in.readAllBytes());
             var remapped = cc.parse(ClassRemapper.of(map).remapClass(cc, clm));
-            assertEmpty(remapped.verify(
+            assertEmpty(ClassFile.of(ClassFile.ClassHierarchyResolverOption.of(
                     ClassHierarchyResolver.of(Set.of(ClassDesc.of("remapped.List")), Map.of(
                             ClassDesc.of("remapped.RemappedBytecode"), ConstantDescs.CD_Object,
                             ClassDesc.ofDescriptor(RawBytecodeHelper.class.descriptorString()), ClassDesc.of("remapped.RemappedBytecode")))
                                           .orElse(ClassHierarchyResolver.defaultResolver())
-                    , null)); //System.out::print));
+                    )).verify(remapped));
             remapped.fields().forEach(f -> f.findAttribute(Attributes.SIGNATURE).ifPresent(sa ->
                     verifySignature(f.fieldTypeSymbol(), sa.asTypeSignature())));
             remapped.methods().forEach(m -> m.findAttribute(Attributes.SIGNATURE).ifPresent(sa -> {
@@ -239,7 +238,7 @@ class AdvancedTransformationsTest {
         var instrumentor = cc.parse(AdvancedTransformationsTest.class.getResourceAsStream("AdvancedTransformationsTest$InstrumentorClass.class").readAllBytes());
         var target = cc.parse(AdvancedTransformationsTest.class.getResourceAsStream("AdvancedTransformationsTest$TargetClass.class").readAllBytes());
         var instrumentedBytes = instrument(target, instrumentor, mm -> mm.methodName().stringValue().equals("instrumentedMethod"));
-        assertEmpty(cc.parse(instrumentedBytes).verify(null)); //System.out::print));
+        assertEmpty(cc.verify(instrumentedBytes));
         var targetClass = new ByteArrayClassLoader(AdvancedTransformationsTest.class.getClassLoader(), "AdvancedTransformationsTest$TargetClass", instrumentedBytes).loadClass("AdvancedTransformationsTest$TargetClass");
         assertEquals(targetClass.getDeclaredMethod("instrumentedMethod", Boolean.class).invoke(targetClass.getDeclaredConstructor().newInstance(), false), 34);
     }

test/jdk/jdk/classfile/ClassHierarchyInfoTest.java

@@ -136,7 +136,7 @@ class ClassHierarchyInfoTest {
                     else
                         clb.with(cle);
                 });
-        var errors = ClassFile.of().parse(newBytes).verify(null);
+        var errors = ClassFile.of().verify(newBytes);
         if (!errors.isEmpty()) {
             var itr = errors.iterator();
             var thrown = itr.next();

test/jdk/jdk/classfile/CorpusTest.java

@@ -208,7 +208,7 @@ class CorpusTest {
                 ClassRecord.ofClassModel(classModel, CompatibilityFilter.By_ClassBuilder),
                 "ClassModel[%s] transformed by ClassBuilder (actual) vs ClassModel before transformation (expected)".formatted(path));
 
-        assertEmpty(newModel.verify(null));
+        assertEmpty(cc.verify(newModel));
 
         //testing maxStack and maxLocals are calculated identically by StackMapGenerator and StackCounter
         byte[] noStackMaps = ClassFile.of(ClassFile.StackMapsOption.DROP_STACK_MAPS)

test/jdk/jdk/classfile/StackMapsTest.java

@@ -229,11 +229,10 @@ class StackMapsTest {
                                 .walk().anyMatch(n -> n.name().equals("stack map frames")));
 
         //test transformation to class version 50 with re-generation of StackMapTable attributes
-         assertEmpty(cc.parse(cc.transform(
+         assertEmpty(cc.verify(cc.transform(
                                     version49,
                                     ClassTransform.transformingMethodBodies(CodeTransform.ACCEPT_ALL)
-                                                  .andThen(ClassTransform.endHandler(clb -> clb.withVersion(50, 0)))))
+                                                  .andThen(ClassTransform.endHandler(clb -> clb.withVersion(50, 0))))));
-                              .verify(null));
     }
 
     @Test
@@ -271,7 +270,7 @@ class StackMapsTest {
                                                });
 
         //then verify transformed bytecode
-        assertEmpty(cc.parse(transformedBytes).verify(null));
+        assertEmpty(cc.verify(transformedBytes));
     }
 
     @Test

test/jdk/jdk/classfile/VerifierSelfTest.java

@@ -51,7 +51,7 @@ class VerifierSelfTest {
                     .flatMap(p -> p)
                     .filter(p -> Files.isRegularFile(p) && p.toString().endsWith(".class")).forEach(path -> {
                         try {
-                            ClassFile.of().parse(path).verify(null);
+                            ClassFile.of().verify(path);
                         } catch (IOException e) {
                             throw new AssertionError(e);
                         }
@@ -59,7 +59,7 @@ class VerifierSelfTest {
     }
 
     @Test
-    void testFailedDump() throws IOException {
+    void testFailed() throws IOException {
         Path path = FileSystems.getFileSystem(URI.create("jrt:/")).getPath("modules/java.base/java/util/HashMap.class");
         var cc = ClassFile.of(ClassFile.ClassHierarchyResolverOption.of(
                 className -> ClassHierarchyResolver.ClassHierarchyInfo.ofClass(null)));
@@ -79,13 +79,8 @@ class VerifierSelfTest {
                         clb.with(cle);
                 });
         StringBuilder sb = new StringBuilder();
-        if (ClassFile.of().parse(brokenClassBytes).verify(sb::append).isEmpty()) {
+        if (ClassFile.of().verify(brokenClassBytes).isEmpty()) {
             throw new AssertionError("expected verification failure");
         }
-        String output = sb.toString();
-        if (!output.contains("- method name: ")) {
-            System.out.println(output);
-            throw new AssertionError("failed method not dumped to output");
-        }
     }
 }

test/jdk/tools/lib/tests/JImageValidator.java

@@ -223,10 +223,10 @@ public class JImageValidator {
     }
 
     public static void readClass(byte[] clazz) throws IOException{
-        var errors = ClassFile.of().parse(clazz).verify(
+        var errors = ClassFile.of(
                 //resolution of all classes as interfaces cancels assignability verification
-                cls -> ClassHierarchyResolver.ClassHierarchyInfo.ofInterface(),
+                ClassFile.ClassHierarchyResolverOption.of(cls -> ClassHierarchyResolver.ClassHierarchyInfo.ofInterface()))
-                null);
+                .verify(clazz);
         if (!errors.isEmpty()) {
             var itr = errors.iterator();
             var thrown = itr.next();