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

[Peter Levart]

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?
>
> 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:

      * <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>

Regards, Peter

> Regards, Peter
>
> On 7/30/19 4:47 PM, Mandy Chung wrote:
>>
>> Think about aMethod is a protected method inherited from its 
>> superclass T.  To invoke aMethod, the receiver must be an instance of 
>> T or a subclass of T.
>>
>> Mandy
>>
>> On 7/30/19 3:22 AM, Daniel Fuchs wrote:
>>> Hi Mandy,
>>>
>>>  380      *     <th scope="row">{@link 
>>> java.lang.invoke.MethodHandles.Lookup#unreflectSpecial 
>>> lookup.unreflectSpecial(aMethod,this.class)}</th>
>>>  381      *     <td>{@code T m(A*);}</td><td>{@code (T) 
>>> aMethod.invoke(this, arg*);}</td>
>>>
>>> Is this exactly true? I mean - if `this` is an instance of
>>> a subclass of `aMethod.getDeclaringClass()`, and if that
>>> subclass overrides `aMethod`, then I would expect
>>> `aMethod.invoke(this, arg*)` to execute the bytecode
>>> of the method defined in the subclass.
>>>
>>> If I'm not mistaken, the test does expect that the
>>> bytecode in the super class is executed instead.
>>> I suspect that `unreflectSpecial` can only be specified
>>> in terms of `findSpecial`. But maybe I'm missing something.
>>> I'm not too familiar with the intricacies of MethodHandle.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 26/07/2019 18:41, Mandy Chung wrote:
>>>> Daniel noticed that `unreflectSpecial` is missing in the "Lookup 
>>>> Factory Methods" section in the class spec.  In fact there are a 
>>>> duplicated `lookup.unreflect(aMethod)` row that might originally be 
>>>> for `unreflectSpecial`.   I fix the javadoc in this patch:
>>>>
>>>> http://cr.openjdk.java.net/~mchung/jdk14/8209005/webrev.01/
>>>>
>>>> Mandy
>>>>
>>>> On 7/25/19 1:12 PM, Mandy Chung wrote:
>>>>> This patch fixes Lookup.unreflectSpecial to pass the declaring 
>>>>> class of Method being unreflected (rather than null) so that it 
>>>>> can accurately check if the special caller class is either the 
>>>>> lookup class or a superinterface of the declaring class.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~mchung/jdk14/8209005/webrev.00/index.html
>>>>>
>>>>> The test runs in both unnamed module and named module to cover 
>>>>> JDK-8209078 which has been resolved by JDK-8173978.
>>>>>
>>>>> thanks
>>>>> Mandy
>>>>
>>>
>>
>