RFR: 8159746: (proxy) Support for default methods

[Mandy Chung]

On Wed, 7 Oct 2020 07:29:51 GMT, Peter Levart <plevart at openjdk.org> wrote:

>> Joe has given some feedback in the CSR review.
>>> should this functionality be housed in the InvocationHandler interface rather than the Proxy class? If I understand the
>>> intended usage of the new method, it should only be done in an InvocationHandler.
>> 
>> The primary usage of this new method is `InvocationHandler`.  I agree that the new API should be defined in
>> `InvocationHandler`.
>> @plevart @AlanBateman any feedback or thought on moving the new API to `InvocationHandler`?
>> 
>>> Does the current spec handle a case like interface A declares a default method m, interface B extends A and separately
>>> interface B declare a default method m.
>> 
>> Yes
>> 
>>> Would invokeDefaultMethod on A::m do-what-is-wanted by call the default method inherited from A? Indirectly, this is
>>> also a question if the operation that are binary compatible with respect to moving around default methods and
>>> behaviorally compatible with the semantics of the new method.
>> 
>> If a proxy instance implements both A and B, `invokeDefaultMethod(o, A.class.getMethod("m"), params)` will invoke
>> `A::m`.
>> If a proxy instance implements B only, `invokeDefaultMethod(o, A.class.getMethod("m"), params)` will get IAE thrown.
>> This is consistent with a concrete class implements B that can only call `B.super::m` but not `A::m`.
>> I update the javadoc to clarify this.
>> 
>>> Any special cases for hidden classes?
>> 
>> No because proxy cannot implement a hidden proxy interface as specified in `Proxy::newProxyInstance`
>> 
>> IllegalArgumentException will be thrown if any of the following restrictions is violated:
>> 
>> - All of Class objects in the given interfaces array must represent non-hidden interfaces, not classes or primitive types.
>
>> Joe has given some feedback in the CSR review.
>> 
>> >    should this functionality be housed in the InvocationHandler interface rather than the Proxy class? If I understand the
>> >    intended usage of the new method, it should only be done in an InvocationHandler.
>> 
>> The primary usage of this new method is InvocationHandler. I agree that the new API should be defined in
>> InvocationHandler.
> 
> If it is a public static method, then Proxy or InvocationHandler are just a namespace for it. From discoverability
> perspective, either one is a good place I think. One point for moving the method into the InvocationHandler is the fact
> that Proxy is a class and classes "inherit" static methods too while interfaces don't. It would be impossible to create
> a dynamic proxy for an interface with a method having the same signature as the Proxy.invokeDefaultMethod static method
> since such proxy class would need to implement an instance method having the same signature as its superclass static
> method.  If interfaces supported protected default methods, then it would make sense to be a protected default method
> in InvocationHandler. Such instance method could additionally enforce two things:
> - that it is called from code comprising InvocationHandler
> - that the proxy instance passed to that method is in fact backed by the invocation handler instance that is the target
>   of that call by checking that in the method.
> 
> But we don't have protected default methods in interfaces (yet). So a static method it must be.
> 
> I was thinking about what such public static method allows and whether that presents a problem (perhaps security
> related) and came to the conclusion that it does not allow anything that normal Java code would not allow except for
> one thing (see below). From the 1st glance it would seem that it allows any code to call super default method on any
> Proxy instance, but what does that mean exactly? The same effect can be achieved by creating a special hand-made proxy
> class implementing all the interfaces that the dynamic Proxy class implements:  class HandmadeProxy implements I1, I2,
> ... {
>     private final Proxy targetProxy;
>     HandmadeProxy(Proxy targetProxy) { this.targetProxy = targetProxy }
>     // implement all non-default methods of I1, I2, ... by delegating to targetProxy
>     // example for I1.m()
>     public void m() {
>         ((I1)targetProxy).m();
>     }
> }
> 
> ...and then using such class to invoke some default method on an instance of the Proxy:
> 
> Proxy proxy = ...
> new HandmadeProxy(proxy).someDefaultMethod();
> 
> So such apparently powerful capability does not represent a problem. One additional thing is possible though:
> 
>>> Does the current spec handle a case like interface A declares a default method m, interface B extends A and separately
>>> interface B declare a default method m.
>>
>> If a proxy instance implements both A and B, `invokeDefaultMethod(o, A.class.getMethod("m"), params)` will invoke
>> `A::m`.
> 
> This is true since lookup.findSpecial() allows such thing (and I assume equivalent bytecode does too). The Java
> language (compiler) OTOH does not allow calling A.super.m() from a class implementing both A and B when B extends A and
> both A and B declare default method m():
>     interface A {
>         default void m() { }
>     }
> 
>     interface B extends A {
>         default void m() { }
>     }
> 
>     static class C implements A, B {
>         public void m() {
>             A.super.m();
>         }
>     }
> 
> java: bad type qualifier A in default super call
>   redundant interface A is extended by B
> 
> I'm in favour of following the bytecode (method handles) behaviour here even though it allows more than Java language
> allows.

I'm thinking a static method `InvocationHandler::invokeDefaultMethod` and no new method in `Proxy` class.   Proxy class
will still have the package-private methods for `InvocationHandler::invokeDefaultMethod` implementation to use.

It may seem to be cleaner to be a default method at the use site called from InvocationHandler.  However, it's not
really a method meant to be overridden.

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

PR: https://git.openjdk.java.net/jdk/pull/313