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

[Mandy Chung]

> On Dec 19, 2016, at 1:41 AM, Peter Levart  wrote:
> 
> Hi,
> 
> This is the latest webrev of changes to Class.getMethod() and 
> Class.getMethods(), rebased to current tip of jdk9-dev, incorporating 
> comments from CCC review:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.08/ 
> 
> 
> Javadocs now include explicit text about Method(s) returned for interface and 
> array types as well as description of general algorithm. Apart from javadocs, 
> the following changed from webrev.07:
> 
> - package-private Class.getMethdOrNull() (added by resent jigsaw changes) 
> must copy the returned Method object itself since getMethod0() does not 
> return a copy any more.
> - renamed PublicMethods[.MethodList].coalesce() -> merge(). I think this is a 
> less confusing name.

I reviewed webrev.09 on the delta change from webrev.07.

Looks good.

Mandy

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

[Paul Sandoz]

> On 20 Dec 2016, at 00:56, Peter Levart  wrote:
> 
> Hi Paul,
> 
> On 12/20/2016 02:51 AM, Paul Sandoz wrote:
>> Hi Peter,
>> 
>> Very good work (that’s one heck of a test on steroids).
> 
> Which would not be possible without such nice APIs like Stream with lambdas 
> and JavaCompiler... ;-)
> 

:-)


>> 
>> Trivially on Class you could turn the “ Note that there may be …” into 
>> @apiNote.
> 
> Like this?
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.09/
> 

Yes.


>> 
>> In PublicMethodsTest can you merge Case and Case1? or did you intend the 
>> separation for future extensions?
> 
> Yes. While I can't currently imagine a situation that is not covered by the 
> test, I also can't prove that this is an exhaustive test. So if someone 
> discovers a situation that is important and not covered, it would be easy to 
> create new Case implementation to cover just this situation and not have to 
> touch Case1…
> 

Ok.


> Maybe, if the need arises, the templating, generation of combinations and 
> compilation could even be factored out into a test utility and used in some 
> other test?
> 

Yes.

+1

Paul.

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

[Peter Levart]

Hi Paul,

On 12/20/2016 02:51 AM, Paul Sandoz wrote:

Hi Peter,

Very good work (that’s one heck of a test on steroids).


Which would not be possible without such nice APIs like Stream with 
lambdas and JavaCompiler... ;-)




Trivially on Class you could turn the “ Note that there may be …” into @apiNote.


Like this?

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



In PublicMethodsTest can you merge Case and Case1? or did you intend the 
separation for future extensions?


Yes. While I can't currently imagine a situation that is not covered by 
the test, I also can't prove that this is an exhaustive test. So if 
someone discovers a situation that is important and not covered, it 
would be easy to create new Case implementation to cover just this 
situation and not have to touch Case1...


Maybe, if the need arises, the templating, generation of combinations 
and compilation could even be factored out into a test utility and used 
in some other test?


Regards, Peter



Paul.


On 19 Dec 2016, at 01:41, Peter Levart  wrote:

Hi,

This is the latest webrev of changes to Class.getMethod() and 
Class.getMethods(), rebased to current tip of jdk9-dev, incorporating comments 
from CCC review:

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

Javadocs now include explicit text about Method(s) returned for interface and 
array types as well as description of general algorithm. Apart from javadocs, 
the following changed from webrev.07:

- package-private Class.getMethdOrNull() (added by resent jigsaw changes) must 
copy the returned Method object itself since getMethod0() does not return a 
copy any more.
- renamed PublicMethods[.MethodList].coalesce() -> merge(). I think this is a 
less confusing name.

For those of you, watching the public list, changes from webrev.04 that was 
last presented there are as follows:

- PublicMethods class changed to embed, rather than extend a HashMap.
-  Added a nearly-exhaustive test of Class.getMethods() and Class.getMethod(): 
PublicMethodsTest. This 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 sub-cases where results differ:

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

Regards, Peter


On 12/18/2016 06:01 AM, joe darcy wrote:

Hello Peter,

Some comments on the spec changes proposed in this request. The new algorithm 
looks, but I don't think it is appropriate to remove explicit text like


If this |Class| object represents an array type, then the returned array has a 
|Method| object for each of the public methods inherited by the array type from 
|Object|. It does not contain a |Method| object for |clone()|.

If this |Class| object represents an interface then the returned array does not 
contain any implicitly declared methods from |Object|. Therefore, if no methods 
are explicitly declared in this interface or any of its superinterfaces then 
the returned array has length 0. (Note that a |Class| object which represents a 
class always has public methods, inherited from |Object|.)


even if it is (non-obviously) implied by the rest of the text.

I'm voting to approve the request on the condition that some explicit 
discussion is added back to describe the handling of array types and interface.

Sorry for the delays,

-Joe


On 12/12/2016 11:09 PM, joe darcy wrote:

Hi Peter,

Sorry for the delays on reviewing your request. I've been backed up on some ccc 
requests and I suspect the changes in your request are subtle enough to merit 
some time to examine.

I'm trying to clear out my queue this week ahead of the next round of JDK 9 
deadlines.

Thanks,

-Joe


On 12/8/2016 12:42 AM, Alan Bateman wrote:

On 08/12/2016 08:34, Peter Levart wrote:


Hi Mandy, Alan,

I know you're all very busy with finalization of jigsaw features before the 
freeze, but I would like to ask whether there has been any feedback on the CCC 
request for this issue.

Sorry for really really long delay on this. Joe Darcy is the chair of the CCC 
and is in his queue to review/approve. He told me yesterday that he wanted to 
get to it soon, I think he's just being pulled into too many issues at the 
moment. Joe, do you have an ETA for Peter? I think it's important that we get 
this into jdk9/dev by Dec 16 in order to make the Dec 22 promotion.

-Alan

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

[Paul Sandoz]

Hi Peter,

Very good work (that’s one heck of a test on steroids).

Trivially on Class you could turn the “ Note that there may be …” into @apiNote.

In PublicMethodsTest can you merge Case and Case1? or did you intend the 
separation for future extensions?

Paul.

> On 19 Dec 2016, at 01:41, Peter Levart  wrote:
> 
> Hi,
> 
> This is the latest webrev of changes to Class.getMethod() and 
> Class.getMethods(), rebased to current tip of jdk9-dev, incorporating 
> comments from CCC review:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.08/
> 
> Javadocs now include explicit text about Method(s) returned for interface and 
> array types as well as description of general algorithm. Apart from javadocs, 
> the following changed from webrev.07:
> 
> - package-private Class.getMethdOrNull() (added by resent jigsaw changes) 
> must copy the returned Method object itself since getMethod0() does not 
> return a copy any more.
> - renamed PublicMethods[.MethodList].coalesce() -> merge(). I think this is a 
> less confusing name.
> 
> For those of you, watching the public list, changes from webrev.04 that was 
> last presented there are as follows:
> 
> - PublicMethods class changed to embed, rather than extend a HashMap.
> -  Added a nearly-exhaustive test of Class.getMethods() and 
> Class.getMethod(): PublicMethodsTest. This 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 sub-cases where results differ:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/PublicMethodsTest.jtr
> 
> Regards, Peter
> 
> 
> On 12/18/2016 06:01 AM, joe darcy wrote:
>> 
>> Hello Peter,
>> 
>> Some comments on the spec changes proposed in this request. The new 
>> algorithm looks, but I don't think it is appropriate to remove explicit text 
>> like
>> 
>>> If this |Class| object represents an array type, then the returned array 
>>> has a |Method| object for each of the public methods inherited by the array 
>>> type from |Object|. It does not contain a |Method| object for |clone()|.
>>> 
>>> If this |Class| object represents an interface then the returned array does 
>>> not contain any implicitly declared methods from |Object|. Therefore, if no 
>>> methods are explicitly declared in this interface or any of its 
>>> superinterfaces then the returned array has length 0. (Note that a |Class| 
>>> object which represents a class always has public methods, inherited from 
>>> |Object|.)
>>> 
>> even if it is (non-obviously) implied by the rest of the text.
>> 
>> I'm voting to approve the request on the condition that some explicit 
>> discussion is added back to describe the handling of array types and 
>> interface.
>> 
>> Sorry for the delays,
>> 
>> -Joe
>> 
>> 
>> On 12/12/2016 11:09 PM, joe darcy wrote:
>>> Hi Peter,
>>> 
>>> Sorry for the delays on reviewing your request. I've been backed up on some 
>>> ccc requests and I suspect the changes in your request are subtle enough to 
>>> merit some time to examine.
>>> 
>>> I'm trying to clear out my queue this week ahead of the next round of JDK 9 
>>> deadlines.
>>> 
>>> Thanks,
>>> 
>>> -Joe
>>> 
>>> 
>>> On 12/8/2016 12:42 AM, Alan Bateman wrote:
 On 08/12/2016 08:34, Peter Levart wrote:
 
> Hi Mandy, Alan,
> 
> I know you're all very busy with finalization of jigsaw features before 
> the freeze, but I would like to ask whether there has been any feedback 
> on the CCC request for this issue.
 Sorry for really really long delay on this. Joe Darcy is the chair of the 
 CCC and is in his queue to review/approve. He told me yesterday that he 
 wanted to get to it soon, I think he's just being pulled into too many 
 issues at the moment. Joe, do you have an ETA for Peter? I think it's 
 important that we get this into jdk9/dev by Dec 16 in order to make the 
 Dec 22 promotion.
 
 -Alan
>>> 
>> 
> 

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

[joe darcy]

Hi Peter,

Revised specification looks good; thanks,

-Joe


On 12/19/2016 1:41 AM, Peter Levart wrote:

Hi,

This is the latest webrev of changes to Class.getMethod() and 
Class.getMethods(), rebased to current tip of jdk9-dev, incorporating 
comments from CCC review:


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

Javadocs now include explicit text about Method(s) returned for 
interface and array types as well as description of general algorithm. 
Apart from javadocs, the following changed from webrev.07:


- package-private Class.getMethdOrNull() (added by resent jigsaw 
changes) must copy the returned Method object itself since 
getMethod0() does not return a copy any more.
- renamed PublicMethods[.MethodList].coalesce() -> merge(). I think 
this is a less confusing name.


For those of you, watching the public list, changes from webrev.04 
that was last presented there are as follows:


- PublicMethods class changed to embed, rather than extend a HashMap.
-  Added a nearly-exhaustive test of Class.getMethods() and 
Class.getMethod(): PublicMethodsTest. This 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 sub-cases where results differ:


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

Regards, Peter


On 12/18/2016 06:01 AM, joe darcy wrote:


Hello Peter,

Some comments on the spec changes proposed in this request. The new 
algorithm looks, but I don't think it is appropriate to remove 
explicit text like


If this |Class| object represents an array type, then the returned 
array has a |Method| object for each of the public methods inherited 
by the array type from |Object|. It does not contain a |Method| 
object for |clone()|.


If this |Class| object represents an interface then the returned 
array does not contain any implicitly declared methods from 
|Object|. Therefore, if no methods are explicitly declared in this 
interface or any of its superinterfaces then the returned array has 
length 0. (Note that a |Class| object which represents a class 
always has public methods, inherited from |Object|.)



even if it is (non-obviously) implied by the rest of the text.

I'm voting to approve the request on the condition that some explicit 
discussion is added back to describe the handling of array types and 
interface.


Sorry for the delays,

-Joe


On 12/12/2016 11:09 PM, joe darcy wrote:

Hi Peter,

Sorry for the delays on reviewing your request. I've been backed up 
on some ccc requests and I suspect the changes in your request are 
subtle enough to merit some time to examine.


I'm trying to clear out my queue this week ahead of the next round 
of JDK 9 deadlines.


Thanks,

-Joe


On 12/8/2016 12:42 AM, Alan Bateman wrote:

On 08/12/2016 08:34, Peter Levart wrote:


Hi Mandy, Alan,

I know you're all very busy with finalization of jigsaw features 
before the freeze, but I would like to ask whether there has been 
any feedback on the CCC request for this issue.
Sorry for really really long delay on this. Joe Darcy is the chair 
of the CCC and is in his queue to review/approve. He told me 
yesterday that he wanted to get to it soon, I think he's just being 
pulled into too many issues at the moment. Joe, do you have an ETA 
for Peter? I think it's important that we get this into jdk9/dev by 
Dec 16 in order to make the Dec 22 promotion.


-Alan

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

[Peter Levart]

Hi,

This is the latest webrev of changes to Class.getMethod() and 
Class.getMethods(), rebased to current tip of jdk9-dev, incorporating 
comments from CCC review:


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

Javadocs now include explicit text about Method(s) returned for 
interface and array types as well as description of general algorithm. 
Apart from javadocs, the following changed from webrev.07:


- package-private Class.getMethdOrNull() (added by resent jigsaw 
changes) must copy the returned Method object itself since getMethod0() 
does not return a copy any more.
- renamed PublicMethods[.MethodList].coalesce() -> merge(). I think this 
is a less confusing name.


For those of you, watching the public list, changes from webrev.04 that 
was last presented there are as follows:


- PublicMethods class changed to embed, rather than extend a HashMap.
-  Added a nearly-exhaustive test of Class.getMethods() and 
Class.getMethod(): PublicMethodsTest. This 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 sub-cases where results differ:


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

Regards, Peter


On 12/18/2016 06:01 AM, joe darcy wrote:


Hello Peter,

Some comments on the spec changes proposed in this request. The new 
algorithm looks, but I don't think it is appropriate to remove 
explicit text like


If this |Class| object represents an array type, then the returned 
array has a |Method| object for each of the public methods inherited 
by the array type from |Object|. It does not contain a |Method| 
object for |clone()|.


If this |Class| object represents an interface then the returned 
array does not contain any implicitly declared methods from |Object|. 
Therefore, if no methods are explicitly declared in this interface or 
any of its superinterfaces then the returned array has length 0. 
(Note that a |Class| object which represents a class always has 
public methods, inherited from |Object|.)



even if it is (non-obviously) implied by the rest of the text.

I'm voting to approve the request on the condition that some explicit 
discussion is added back to describe the handling of array types and 
interface.


Sorry for the delays,

-Joe


On 12/12/2016 11:09 PM, joe darcy wrote:

Hi Peter,

Sorry for the delays on reviewing your request. I've been backed up 
on some ccc requests and I suspect the changes in your request are 
subtle enough to merit some time to examine.


I'm trying to clear out my queue this week ahead of the next round of 
JDK 9 deadlines.


Thanks,

-Joe


On 12/8/2016 12:42 AM, Alan Bateman wrote:

On 08/12/2016 08:34, Peter Levart wrote:


Hi Mandy, Alan,

I know you're all very busy with finalization of jigsaw features 
before the freeze, but I would like to ask whether there has been 
any feedback on the CCC request for this issue.
Sorry for really really long delay on this. Joe Darcy is the chair 
of the CCC and is in his queue to review/approve. He told me 
yesterday that he wanted to get to it soon, I think he's just being 
pulled into too many issues at the moment. Joe, do you have an ETA 
for Peter? I think it's important that we get this into jdk9/dev by 
Dec 16 in order to make the Dec 22 promotion.


-Alan

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

[Mandy Chung]

> On Nov 3, 2016, at 1:32 AM, Peter Levart  wrote:
> 
> 
> Here is a webrev incorporating your suggestions:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.07/


This looks really good. I went through the test output and the expected result 
looks right to me as far as I can focus (there are many many combination there).

As for the spec, it is clear and there may be some wordsmithing we could do but 
it’s fine to leave it for the future.

Nit: "sub-sets” should it be “subsets” (no hyphen)?

Mandy

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

[Peter Levart]

Hi Mandy,


On 10/18/2016 04:14 PM, Peter Levart wrote:
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/ 







On 11/02/2016 04:12 AM, Mandy Chung wrote:

1786  *(clearly, it M's and N's declaring types are the same type, 
then
1957  *(clearly, it M's and N's declaring types are the same type, 
then

typo: s/it/if

There are phrases such as "methods are kept” - what about “select” instead of 
“keep”? AFAICT, the algorithm is correct.  I feel that the spec wording could be 
improved a little but that can be done later.  I think this version is good for CCC 
submission.

When you update the source, you use ,  capital letters - our convention uses 
lowercase ,  etc.

One leftover IDE-specific suppress warnings:
  111 @SuppressWarnings("StringEquality”)

The patch looks good to me.  I will do one more pass and reply to the open.

Mandy




On 11/02/2016 04:43 AM, Mandy Chung wrote:

A few other comments for consideration:
   methods with same VM signature (return type, name, parameter types)

Would it be better to say "methods with the signature (name and parameter types) and 
with same return type"?

   declared public methods (including static)

Alternatively, you could say:
   declared public static and instance methods

   • Include the results of invoking this algorithm recursively on all direct 
superinterfaces of C, excluding any static methods.

An alternative:
   All public instance methods of all direct superinterfaces of C

Mandy



Here is a webrev incorporating your suggestions:

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


I think this could be used as a submission for CCC.

Regards, Peter

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

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

[Alan Bateman]

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.


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.


I assume that copyright headers will be added before this is pushed. 
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).


-Alan

Re: 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 > wrote:


Hi,

I have a fix for conformance (P2) bug (8062389 
) that also fixes a 
conformance (P3) bug (8029459 
) and a performance 
issue (8061950 ):


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



As Daniel Smith writes in 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 

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

[Paul Sandoz]

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  wrote:
> 
> Hi,
> 
> I have a fix for conformance (P2) bug (8062389 
> ) that also fixes a 
> conformance (P3) bug (8029459 
> ) and a performance issue 
> (8061950 ):
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.04/
> 
> As Daniel Smith writes in 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 
> , getMethod() and 
> getMethods() share the same consolidation logic 

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

[Peter Levart]

Hi David,


On 10/05/2016 04:27 AM, David Holmes wrote:

Hi Peter,

Without making any comment on what you have actually done, does this 
account for private interface methods correctly?


In short, yes. Class.getMethods() and Class.getMethod() only ever return 
public methods. The source of Method objects is in both cases the 
following invocation:


privateGetDeclaredMethods(/* publicOnly */ true)

in lines 2981 and 3087, respectively.

Regards, Peter



Thanks,
David

On 4/10/2016 11:40 PM, Peter Levart wrote:

Hi,

I have a fix for conformance (P2) bug (8062389
) that also fixes a
conformance (P3) bug (8029459
) and a performance
issue (8061950 ):

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




As Daniel Smith writes in 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
, 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 

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

[David Holmes]

Hi Peter,

Without making any comment on what you have actually done, does this 
account for private interface methods correctly?


Thanks,
David

On 4/10/2016 11:40 PM, Peter Levart wrote:

Hi,

I have a fix for conformance (P2) bug (8062389
) that also fixes a
conformance (P3) bug (8029459
) and a performance
issue (8061950 ):

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


As Daniel Smith writes in 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
, 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 

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

[Peter Levart]

Hi,

I have a fix for conformance (P2) bug (8062389 
) that also fixes a 
conformance (P3) bug (8029459 
) and a performance 
issue (8061950 ):


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

As Daniel Smith writes in 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 
, 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