Review Request: JDK-8209005: Lookup.unreflectSpecial fails for default methods when Lookup.findSpecial works

[Mandy Chung]

Hi Peter, Daniel

On 7/31/19 1:44 AM, Peter Levart wrote:
>
>
> On 7/31/19 9:59 AM, Peter Levart wrote:
>> Hi,
>>
>> I think Daniel is talking about the "dispatch" semantics of 
>> unreflectSpecial here, right Daniel?
>>

Thanks for clarifying this.

>> The findSpecial / unreflectSpecial is a MethodHandle equivalent for 
>> bytecode instruction invokespecial (sans actual invoking). 
>> invokespecial is typically used for implementing the 
>> super.method(args) Java invocations. In that case, the superclass 
>> method is targeted - this is not a virtual method dispatch like 
>> aMethod.invoke(this, arg*) - i.e. the reflective invocation is always 
>> a virtual invocation (for non-private methods). Likewise findSpecial 
>> and unreflectSpecial produce a MethodHandle that dispatches to the 
>> method in the superclass (the aMethod.getDeclatingClass() in case of 
>> unreflectSpecial) regardless of whether that method is overridden in 
>> the subclass or not.
>>
>
> Expanding on this a little. The javadocs of MethodHandles.Lookup 
> starts talking about the Lookup factory methods methods and their 
> equivalence to bytecode instructions, but then present the equivalence 
> between find* and Java source code (which is OK given that translation 
> to bytecode is known) followed by equivalence between unreflect* and 
> reflective invocations. Public reflection API does not implement the 
> equivalent behavior to unreflectSpecial. So perhaps, this line only 
> could present the equivalence in terms of Java code like findSpecial 
> does with a comment stating that there's no equivalence with 
> reflective invocation API. For example:
>
>      * <tr>
>      *     <th scope="col"><a id="equiv"></a>lookup expression</th>
>      *     <th scope="col">member</th>
>      *     <th scope="col">bytecode / reflection behavior</th>
>
> Added "/ reflection" above; and:
>

I prefer to keep the original header and revisit this separately. It 
intends to talk about bytecode behavior and perhaps handle this 
unreflect* exception differently.

>      * <tr>
>      *     <th scope="row">{@link 
> java.lang.invoke.MethodHandles.Lookup#unreflectSpecial 
> lookup.unreflectSpecial(aMethod,this.class)}</th>
>      *     <td>{@code T m(A*);}</td><td>{@code (T) super.m(arg*); // 
> no equivalent reflective invocation}</td>
>      * </tr>
>

This is correct when this class is a subclass of C.   A slight 
clarification to indicate that aMethod's declaring class is C might 
help.  I also fixed another typo:

--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
@@ -334,7 +334,7 @@
       * </tr>
       * <tr>
       *     <th scope="row">{@link java.lang.invoke.MethodHandles.Lookup#findStaticGetter lookup.findStaticGetter(C.class,"f",FT.class)}</th>
-     *     <td>{@code static}<br>{@code FT f;}</td><td>{@code (T) C.f;}</td>
+     *     <td>{@code static}<br>{@code FT f;}</td><td>{@code (FT) C.f;}</td>
       * </tr>
       * <tr>
       *     <th scope="row">{@link java.lang.invoke.MethodHandles.Lookup#findSetter lookup.findSetter(C.class,"f",FT.class)}</th>
@@ -377,8 +377,8 @@
       *     <td>{@code C(A*);}</td><td>{@code (C) aConstructor.newInstance(arg*);}</td>
       * </tr>
       * <tr>
-     *     <th scope="row">{@link java.lang.invoke.MethodHandles.Lookup#unreflect lookup.unreflect(aMethod)}</th>
-     *     <td>({@code static})?<br>{@code T m(A*);}</td><td>{@code (T) aMethod.invoke(thisOrNull, arg*);}</td>
+     *     <th scope="row">{@link java.lang.invoke.MethodHandles.Lookup#unreflectSpecial lookup.unreflectSpecial(aMethod,this.class)}</th>
+     *     <td>{@code T m(A*);}</td><td>{@code (T) super.m(arg*);}</td>
       * </tr>
       * <tr>
       *     <th scope="row">{@link java.lang.invoke.MethodHandles.Lookup#findClass lookup.findClass("C")}</th>
@@ -403,7 +403,7 @@
       * stands for a null reference if the accessed method or field is static,
       * and {@code this} otherwise.
       * The names {@code aMethod}, {@code aField}, and {@code aConstructor} stand
-     * for reflective objects corresponding to the given members.
+     * for reflective objects corresponding to the given members declared in type {@code C}.
       * <p>
       * The bytecode behavior for a {@code findClass} operation is a load of a constant class,
       * as if by {@code ldc CONSTANT_Class}.


Thanks.
Mandy