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

[Peter Levart]

Hi Alan,


On 10/14/2016 02:16 PM, Alan Bateman wrote:
> On 13/10/2016 18:30, Peter Levart wrote:
>
>> 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.
> Overall I think this looks very good. I mostly focused on the 
> PublicMethods implementation to satisfy myself that it does selects 
> the most specific methods. In passing I wonder if "combine" or "merge" 
> might be better than "consolidate" for method name and terminology, a 
> minor point of course.

What about "coalesce" ?

>
> Given the behavior change then I think we'll need to capture it in a 
> release notes. I can't think of any libraries or frameworks that might 
> have see but something might come out of the woodwork and would be 
> nice to be able to point to a summary.

What do you think of specifying new behavior in javadoc(s) of 
getMethod() and getMethods() themselves? The description in those docs 
was never very precise. It is composed of a few seemingly unconnected 
statements which don't give a complete picture of what those methods 
return. It is now plainly wrong in getMethod() and does not give any 
clew about what it means to "inherit" a method from supertype in 
getMethods(). I have tried to capture the precise behavior in the 
changed javadocs that I present here:

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

Those javadocs refer to definitions of two other methods: 
Class.getDeclaredMethods() and Class.isAssignableFrom(). Their 
definitions are leveraged so javadocs of getMethod() and getMethods() 
themselves can be simpler - I removed the redundant statements that are 
a direct consequence of the algorithm description.

We could then perhaps point from release notes to the new javadocs or 
even include them.

>
> I assume that copyright headers will be added before this is pushed.

Added in above webrev.

> Also there is a @SuppressWarnings arguments that I don't recognize 
> (IDE specific)? It would good to trim back some of the really long 
> times in the test too (there are some >150 char lines that will be a 
> pain to review side-by-side when there are future changes).

The trimming was done. I have missed the @SuppressWarnings. Will do it 
in next iteration.

I have updated the test which now includes Class.getMethod() testing 
too. It also presents results (differences from expected) in a more 
pleasing way - just those results that differ are presented. Here's a 
result of running the test on unpatched JDK 9:

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

There are some more differences now that Class.getMethod() is also part 
of the test. Some interesting consequences of the patch (expected=new 
behavior, actual=old behavior):

interface I { void m(); }
interface J {  }
interface K extends I, J { default void m() {} }
abstract class C {  }
abstract class D extends C implements I {  }
abstract class E extends D implements J, K {  }

expected: E.gM: K.m
   actual: E.gM: I.m

...here E.class.getMethods() has the same result between unpatched and 
patched JDK 9, just getMethod() returns different method. Which means 
that in present JDK, getMethod() returns a method that is not included 
in the getMethods() result.


interface I { void m(); }
interface J { void m(); }
interface K extends I, J { void m(); }
abstract class C { public abstract void m(); }
abstract class D extends C implements I {  }
abstract class E extends D implements J, K {  }

expected: E.gMs: [C.m]
   actual: E.gMs: [C.m, I.m, J.m, K.m]

expected: D.gMs: [C.m]
   actual: D.gMs: [C.m, I.m]

...huge difference, huh.


interface I { default void m() {} }
interface J { void m(); }
interface K extends I, J { void m(); }
abstract class C {  }
abstract class D extends C implements I {  }
abstract class E extends D implements J, K {  }

expected: E.gMs: [K.m]
   actual: E.gMs: [I.m, J.m, K.m]

expected: E.gM: K.m
   actual: E.gM: I.m

...difference in both getMethod() and getMethods().


Regards, Peter