RFR: 8062389, 8029459, 8061950: Class.getMethod() is inconsistent with Class.getMethods() results + more

[Peter Levart]

Hi Paul, Alan,

I incorporated Paul's suggestions into new webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.05/

This iteration also contains a nearly-exhaustive test of 
Class.getMethods(): PublicMethodsTest. It is actually a test generator. 
Given a Case template, it generates all variants of methods for each of 
the types in the case. Case1 contains 4 interface method variants ^ 3 
interfaces * 4 class method variants ^ 3 classes = 4^6 = 4096 different 
sub-cases of which only 1379 compile. The results of those 1379 
sub-cases are persisted in the Case1.results file. Running the test 
compares the persisted results with actual result of executing each 
sub-case. When running this test on plain JDK 9 (without patch), the 
test finds 218 sub-cases where results differ:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/PublicMethodsTest.jtr

Looking at those differences gives the impression of the effects of the 
patch.

Regards, Peter

On 10/11/2016 11:35 PM, Paul Sandoz wrote:
> Hi Peter,
>
> Nice work here.
>
> PublicMethods
> —
>
> I would be inclined to change PublicMethods to encapsulate an instance 
> of LinkedHashMap, since it’s only the consolidate/toArray methods that 
> really matter, and no need for the key/value types to be exposed, then 
> no need to declare serialVersionUID, and also it can be final. The 
> JavaDoc on consolidate would need to be tweaked.
>
>    29      * existing methods with same signature if they are less specific then added
> s/then/than
>
>    89             if (o == null || getClass() != o.getClass()) return false;
>
> Could be simplified to
>
>   if (!(o instanceof Key)) return false
>
> Paul.
>
>> On 4 Oct 2016, at 06:40, Peter Levart <peter.levart at gmail.com 
>> <mailto:peter.levart at gmail.com>> wrote:
>>
>> Hi,
>>
>> I have a fix for conformance (P2) bug (8062389 
>> <https://bugs.openjdk.java.net/browse/JDK-8062389>) that also fixes a 
>> conformance (P3) bug (8029459 
>> <https://bugs.openjdk.java.net/browse/JDK-8029459>) and a performance 
>> issue (8061950 <https://bugs.openjdk.java.net/browse/JDK-8061950>):
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.04/ 
>> <http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/Class.getMethods.new/webrev.04/>
>>
>> As Daniel Smith writes in 8029459 
>> <https://bugs.openjdk.java.net/browse/JDK-8029459>, the following 
>> situation is as expected:
>>
>> interface I { void m(); }
>> interface J extends I { void m(); }
>> interface K extends J {}
>> K.class.getMethods() = [ J.m ]
>>
>> But the following has a different result although it should probably 
>> have the same:
>>
>> interface I { void m(); }
>> interface J extends I { void m(); }
>> interface K extends I, J {}
>> K.class.getMethods() = [ I.m, J.m ]
>>
>> He then suggests the following algorithm:
>>
>> An implementation of getMethods consistent with JLS 8 would include 
>> the following (see Lambda Spec, Part H, 9.4.1 and 8.4.8):
>> 1) The class's/interface's declared (public) methods
>> 2) The getMethods() of the superclass (if this is a class), minus any 
>> that have a match in (1)
>> 3) The getMethods() of each superinterface, minus any that have a 
>> match in (1) or a _concrete_ match in (2) or a match from a 
>> more-specific class/interface in (2) or (3)
>>
>> But even that is not sufficient for the following situation:
>>
>> interface E { void m(); }
>> interface F extends E { void m(); }
>> abstract class G implements E {}
>> abstract class H extends G implements F {}
>> H.class.getMethods() = [ E.m, F.m ]
>>
>> The desired result of H.class.getMethods() = [ F.m ]
>>
>> So a more precise definition is required which is implemented in the fix.
>>
>> The getMethods() implementation partitions the union of the following 
>> methods:
>>
>> 1) The class's/interface's declared public methods
>> 2) The getMethods() of the superclass (if this is a class)
>> 3) The non-static getMethods() of each direct superinterface
>>
>> ...into equivalence classes (sets) of methods with same signature 
>> (return type, name, parameter types). Within each such set, only the 
>> "most specific" methods are kept and others are discarded. The union 
>> of the kept methods is the result.
>>
>> The "more specific" is defined as a partial order within a set of 
>> methods of same signature:
>>
>> Method A is more specific than method B if:
>> - A is declared by a class and B is declared by an interface; or
>> - A is declared by the same type as or a subtype of B's declaring 
>> type and both are either declared by classes or both by interfaces 
>> (clearly if A and B are declared by the same type and have the same 
>> signature, they are the same method)
>>
>> If A and B are distinct elements from the set of methods with same 
>> signature, then either:
>> - A is more specific than B; or
>> - B is more specific than A; or
>> - A and B are incomparable
>>
>> A sub-set of "most specific" methods are the methods from the set 
>> where for each such method M, there is no method N != M such that N 
>> is "more specific" than M.
>>
>> There can be more than one "most specific" method for a particular 
>> signature as they can be inherited from multiple unrelated 
>> interfaces, but:
>> - javac prevents compilation when among multiply inherited methods 
>> with same signature there is at least one default method, so in 
>> practice, getMethods() will only return multiple methods with same 
>> signature if they are abstract interface methods. With one exception: 
>> bridge methods that are created by javac for co-variant overrides are 
>> default methods in interfaces. For example:
>>
>>    interface I { Object m(); }
>>    interface J1 extends I { Map m(); }
>>    interface J2 extends I { HashMap m(); }
>>    interface K extends J1, J2 {}
>>
>> K.class.getMethods() = [ default Object j1.m, abstract Map j1.m, 
>> default Object j2.m, abstract HashMap j2.m ]
>>
>> But this is an extreme case where one can still expect multiple 
>> non-abstract methods with same signature, but different declaring 
>> type, returned from getMethods().
>>
>> In order to also fix 8062389 
>> <https://bugs.openjdk.java.net/browse/JDK-8062389>, getMethod() and 
>> getMethods() share the same consolidation logic that results in a set 
>> of "most specific" methods. The partitioning in getMethods() is 
>> partially performed by collecting Methods into a HashMap where the 
>> keys are (name, parameter types) tuples and values are linked lists 
>> of Method objects with possibly varying return and declaring types. 
>> The consolidation, i.e. keeping only the set of most specific methods 
>> as new methods are encountered, is performed only among methods in 
>> the list that share same return type and also removes duplicates. 
>> getMethod() uses only one such list, consolidates methods that match 
>> given name and parameter types and returns the 1st method from the 
>> resulting list that has the most specific return type.
>>
>> That's it for algorithms used. As partitioning partially happens 
>> using HashMap with (name, parameter types) keys, lists of methods 
>> that form values are typically kept short with most of them 
>> containing only a single method, so this fix also fixes performance 
>> issue 8061950 <https://bugs.openjdk.java.net/browse/JDK-8061950>.
>>
>> The patch also contains some optimizations around redundant copying 
>> of arrays and reflective objects.
>>
>> The FilterNotMostSpecific jtreg test has been updated to accommodate 
>> for changed behavior. Both of the above extreme cases have been added 
>> to the test.
>>
>> So, comments, please.
>>
>> Regards, Peter
>>
>