RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.

[David Holmes]

Hi Goetz,

Very quick reply  to one point ...

On 23/02/2018 11:25 PM, Lindenmaier, Goetz wrote:
> Hi David,
> 
> thanks for looking at the change.
> 
> I'll implement a new test case using your example.  I also think your
> wording is better. Except the word 'Receiver', I think not all Java
> developers will be familiar with that wording. I would prefer the
> word 'Class' here, what do you think?

I'd be surprised anyone doing object-oriented programming is unfamiliar 
with the term "receiver"! What do they teach people these days? ;-)

How about "target" or "Class of the target object" - something like that.

More on Monday.

Thanks,
David


> But see also my comments inline.
> 
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Freitag, 23. Februar 2018 04:22
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(L): 8197405: Improve messages of AbstractMethodErrors
>> and IncompatibleClassChangeErrors.
>>
>> Hi Goetz,
>>
>> I couldn't work through all of this but it touches on some areas I'm
>> working on with JEP 181 (Nestmates).
>>
>> I think this is well intentioned but I think there are far more complex
>> possibilities that make your improved messages inaccurate by trying to
>> be too specific.
>>
>> Also at least for ICCE there may be existing tests that check the ICCE
>> error message and which will now need updating.
> I ran all jdk and hotspot tests on all the platforms we support. None fail due to my change.
> There are the selection_resolution tests which run into timeouts sporadically.
> They test ICCE etc., but I could not find a place that checks
> for the exception message.
> 
>>
>> More below ...
>>
>> On 10/02/2018 12:01 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> I changed the test to use jasm.
>>> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.02/
>>
>> src/hotspot/share/interpreter/linkResolver.cpp
>>
>> 1774 void LinkResolver::throw_abstract_method_error(methodHandle&
>> method, Klass *abstract_klass,
>>
>> The Klass* pointer is the resolved method class - yes? The name
>> "abstract_klass" is misleading as it need not be abstract.
> Yes, abstract class is the superclass/interface of the klass of the
> concrete object the method is called on.
> I would like to simplify the code by using method->method_holder()
> instead.
> 
>> Relatedly
>> your error messages may not be accurate. Suppose you have:
>>
>> class A {
>>     public void m() {}
>>
>>     void test(A a) {
>>       a.m();
>>     }
>> }
>>
>> abstract class B extends A {
>>     public abstract void m();
>> }
>>
> class C extends B {}    // as .jasm
>   
>> If you pass test() a receiver that is a subclass of B and which doesn't
>> define an implementation of m, then IIUC the resolved method class is A,
>> which is not abstract - but your error message would say "abstract class A".
> I implemented it. It says
> "Class C does not implement the requested method m()V inherited from abstract class B"
> 
>>
>> I think if you want to report whether a class is abstract then you need
>> to actually query is_abstract(). Or simply report things more directly:
> I'll implement this. It's a good point.
>   
>> "Receiver class R does not define or inherit an implementation of the
>> resolved method X.y"
> I'll change the wording accordingly. See also my comment above on 'Receiver'.
> 
>> It may also be useful to report the access modifier of the resolved
>> method (and the selected method that was found to be abstract) as that
>> can also affect selection.
> Don't get this.What's the difference between resolved and selected here?
> If I understand your comment below correctly, there is no resolved method
> as the exception must be thrown.
> I'll print the access modifier of the method at hand :)
> 
>> 1780   // If method is null, use void signature.
>>
>> How can method be null? Surely it is either the selected method (which
>> is abstract) or it's the resolved method (for which no selected method
>> could be found).
> We had to add this for coverity. The code changed over the years,
> especially with the invokeinterface fix. So it might be superfluous now.
> 
>> 1785   if(recv_klass) {
>>
>> Style: if (recv_klass != NULL) {
>>
>> But how can the receiver class be null here ?
> Yes, nonsense. will remove.  (also, the code messes up recv and abstract class arguments ...)
> 
> Best regards,
>    Goetz.
> 
> 
> 
> 
>>
>>
>> Cheers,
>> David
>> -----
>>
>>> Also, I fixed a bug and disabled the test on aarch/arm.
>>>
>>> I started to port the simple stuff to aarch/arm.
>>> I'm happy to include a further patch for these, but if I
>>> have time I will also try to implement the rest, can't test
>>> it though.
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Donnerstag, 8. Februar 2018 23:54
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR(L): 8197405: Improve messages of AbstractMethodErrors
>>>> and IncompatibleClassChangeErrors.
>>>>
>>>> Hi Goetz,
>>>>
>>>> Binary class files can not be checked-in to the repo. You'll need to use
>>>> jasm or jcod files that then get compiled as part of the test.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 9/02/2018 12:41 AM, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> This change improves the messages of AbstractMethodErrors and
>>>> IncompatibleClassChangeErrors.
>>>>> Please review.
>>>>> http://cr.openjdk.java.net/~goetz/wr18/8197405-
>> ameExMsg/webrev.01/
>>>>>
>>>>> It adds tests that check some of the improved messages.
>>>>> The tests check the following improvements:
>>>>>
>>>>> AbstractMethodErrors:
>>>>>
>>>>> test_ameInt, test case 1:
>>>>> before: no message / null
>>>>> after:  Missing implementation of interface method
>>>> MyAbstractInt.anAbstractMethod()Ljava/lang/String;
>>>>>
>>>>> test_ameInt, test case 2:
>>>>> before: no message / null
>>>>> after:  Class ImplementsSomeFunctionsInt does not implement the
>>>> requested method
>>>>>            aFunctionOfMyInterface()Ljava/lang/String; inherited from
>> interface
>>>> MyInterfaceInt1
>>>>>
>>>>> test_ame, interpreted:
>>>>> before: no message / null
>>>>> after:  Missing implementation of interface method
>>>> MyInterface.aFunctionOfMyInterface()V
>>>>>
>>>>> test_ame, compiled:
>>>>> before: MyAbstract.aFunctionOfMyInterface()V
>>>>> after:  Class ImplementsSomeFunctions does not implement the
>> requested
>>>> method
>>>>>            aFunctionOfMyInterface()V inherited from abstract class
>> MyAbstract
>>>>>
>>>>> IncompatibleClassChangeErrors:
>>>>>
>>>>> test_icc_compiled_itable_stub
>>>>> before: vtable stub
>>>>> after:  Class ICC_B does not implement the requested interface ICC_iB
>>>>>
>>>>> Best regards,
>>>>>      Goetz.
>>>>>