RFR: 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes [v2]

[Mandy Chung]

On Fri, 3 Feb 2023 15:11:30 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes and this patch converts it to use Classfile API.
>> 
>> This pull request suppose to chain on the 8294982: Implementation of Classfile API https://github.com/openjdk/jdk/pull/10982
>> 
>> Please review.
>> 
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - j.l.r.ProxyGenerator fix - Classfile API moved under jdk.internal.classfile package
>  - Merge branch 'JDK-8294982' into JDK-8294961
>  - Merge branch 'JDK-8294982' into JDK-8294961
>  - 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes

This looks good in general.   I like this, easy to use, easy to read and understand and greatly simplified.

Reviewing this is one easy way to study the ClassFile API.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 390:

> 388:             uniqueList.add(ex);
> 389:         }
> 390:         return uniqueList.stream().map(ex -> ClassDesc.ofDescriptor(ex.descriptorString())).toList();

It would be useful to add a helper method to convert Class to ClassDesc:

     private static ClassDesc toClassDesc(Class<?> type) {
         return ClassDesc.ofDescriptor(type.descriptorString());
     }
     
Suggestion:

        return uniqueList.stream().map(ProxyGenerator::toClassDesc).toList();

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 423:

> 421:     private byte[] generateClassFile() {
> 422:         var localCache = new HashMap<ClassDesc, ClassHierarchyResolver.ClassHierarchyInfo>();
> 423:         return Classfile.build(classDesc, List.of(Classfile.Option.classHierarchyResolver(classDesc ->

How/when is `ClassHierarchyResolver` being called?   I didn't dig deeper.

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 664:

> 662:          */
> 663:         private void generateMethod(ClassBuilder clb, ClassDesc className) {
> 664:             MethodTypeDesc desc = MethodTypeDesc.ofDescriptor(MethodType.methodType(returnType, parameterTypes).toMethodDescriptorString());

I slightly prefer to call `MethodType::descriptorString` that `descriptorString` is called for both `Class` and `MethodType`. 


MethodTypeDesc desc = MethodTypeDesc.ofDescriptor(MethodType.methodType(returnType, parameterTypes).descriptorString());


Alternatively, use `describeConstable` instead.


MethodTypeDesc desc = MethodType.methodType(returnType, parameterTypes).describeConstable().orElseThrow();

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 665:

> 663:         private void generateMethod(ClassBuilder clb, ClassDesc className) {
> 664:             MethodTypeDesc desc = MethodTypeDesc.ofDescriptor(MethodType.methodType(returnType, parameterTypes).toMethodDescriptorString());
> 665:             int accessFlags = (method.isVarArgs()) ? ACC_VARARGS | ACC_PUBLIC | ACC_FINAL : ACC_PUBLIC | ACC_FINAL;

Nit: 
Suggestion:

            int accessFlags = (method.isVarArgs()) ? ACC_VARARGS | ACC_PUBLIC | ACC_FINAL
                                                   : ACC_PUBLIC | ACC_FINAL;

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 672:

> 670:                         .stream()
> 671:                         .map(cpb::classEntry)
> 672:                         .toList();

Suggestion:

                List<ClassEntry> exceptionClassEntries = Arrays.asList(exceptionTypes)
                        .stream()
                        .map(ProxyGenerator::toClassDesc)
                        .map(cpb::classEntry)
                        .toList();


`typeNames` streams over the given types.   This can be simplified by mapping `Class` to `ClassDesc` in place here.   I would also suggest to add  the `toClassDesc` helper method.

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

PR: https://git.openjdk.org/jdk/pull/10991