On Mon, 20 Mar 2023 16:18:27 GMT, Chen Liang <li...@openjdk.org> wrote:

>> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with 
>> proper privileges, with tests.
>> 
>> This addition is useful in case classes at runtime are not loaded from the 
>> system class loader, such as Proxy. This is also useful to APIs that 
>> generate bytecode with a `Lookup` object, such as a custom 
>> single-abstract-method class implementations from a method handle.
>> 
>> See 
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html 
>> as well.
>> 
>> Current questions, which I wish to discuss with @asotona:
>> 1. Should the resolver fail fast on `IllegalAccessException` from the 
>> lookup? This usually indicates the hierarchy resolver is set up improperly, 
>> and proceeding may simply yield verification errors in class loading that 
>> are hard to track. For bytecode-generating APIs, throwing access errors for 
>> the Lookup eagerly is also more preferable than later silent generation 
>> failure.
>> 2. Whether the default resolver should be reading from jrt alone, reflection 
>> alone, or jrt then reflection. I personally believe reflection alone is more 
>> reliable, for classes may redefined with instrumentation or jfr, which may 
>> not be reflected in the system resources.
>> 3. In addition, I don't think chaining system class loader reflection after 
>> system resource retrieval is really meaningful: is there any case where 
>> reflection always works while the system resource retrieval always fails?
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Make lookup based resolver throw on illegal access eagerly

src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java 
line 187:

> 185:     }
> 186: 
> 187:     public static abstract class ReflectionClassHierarchyResolver 
> implements ClassHierarchyResolver {

let's use blessed modifiers order:
Suggestion:

    public abstract static class ReflectionClassHierarchyResolver implements 
ClassHierarchyResolver {

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1142445278

Reply via email to