Review request for JDK-8013357: Javac accepts erroneous binary comparison operations

[Maurizio Cimadamore]

All versions I've seen so far are wrong in a way or another, so I would 
not go back to a previous version. Have you tried the code I pasted in 
my last email?

Maurizio

On 04/06/13 23:03, Eric McCorkle wrote:
> Actually that code doesn't work (it won't even get through a build).
> Any kind of null == Object comparison fails.  Strangely, the code in
> webrev.02 seems to pass all the existing tests (including my new ones),
> which seems strange to me.
>
> I'll backpedal to webrev.01 and simplify it.  But perhaps we ought to
> flag this part of the type checker for refactoring as well.
>
> On 06/04/13 06:08, Maurizio Cimadamore wrote:
>> On 03/06/13 20:38, Eric McCorkle wrote:
>>> Good suggestion, I've made the change (no new webrev, as it just
>>> reorders defs).
>>>
>>> Are there any more suggestions from anyone, or is this one good to go?
>> The code is wrong - I didn't realize when writing it on the review. You
>> only do unboxing _after_ having checked that both types are primitives -
>> so nothing can possibly happen. I have to say I'm a bit worried that the
>> test you wrote didn't catch this (as well as the symmetry problems with
>> the previous implementation). I'd focus on writing a better test first
>> and then tweaking the compiler code accordingly. Note that the test can
>> be made much simpler by using the infrastructure for testing types
>> available in test/tools/javac/types (which will save you the hassle of
>> creating mini test cases when the only thing you are interested in is
>> whether a type is 'equality comparable' to another type).
>>
>> The right code for the test should be this:
>>
>> if (allowBoxing) {
>>     s = unboxedTypeOrType(s);
>>     t = unboxedTypeOrType(t);
>> }
>>
>> boolean tPrimitive = t.isPrimitive();
>> boolean sPrimitive = s.isPrimitive();
>>
>> if (tPrimitive && sPrimitive) {
>>     return isSubtype(s, t) || isSubtype(t, s);
>> } else if (!tPrimitive && !sPrimitive) {
>>     return isCastable(s, t, warn) || isCastable(t, s, warn);
>> } else {
>>      return false;
>> }
>>
>> Maurizio
>>> On 06/03/13 14:22, Alex Buckley wrote:
>>>> In ObjectZeroCompare, I would just suggest having the *_name and
>>>> *_contents variables listed in the same order as the assert_compile_*
>>>> invocations in run(). I think the most readable order is: Object,
>>>> Number, IntegerSuper, Uncastable, [Flips from failure to success here]
>>>> Castable, Integer, etc [as you have it already].
>>>>
>>>> Alex
>>>>
>>>> On 6/3/2013 11:11 AM, Eric McCorkle wrote:
>>>>> Forgot the URL on this.  It's here:
>>>>> http://cr.openjdk.java.net/~emc/8013357/webrev.02/
>>>>>
>>>>> On 06/01/13 08:28, Eric McCorkle wrote:
>>>>>> New webrev is up.  I rolled back allowing Number == primitive
>>>>>> comparisons (I thought I'd rolled that back in the previous), and I
>>>>>> implemented maurizio's suggested simpler comparison.  I also added a few
>>>>>> more test cases.
>>>>>>
>>>>>> JPRT's been a bit unreliable, but I'm getting clean runs.
>>>>>>
>>>>>> Please review and comment.
>>>>>>
>>>>>> On 05/30/13 05:03, Maurizio Cimadamore wrote:
>>>>>>> On 29/05/13 20:53, Eric McCorkle wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review my latest patch for this problem.  Since my earlier
>>>>>>>> review
>>>>>>>> request, several things have happened:
>>>>>>>>
>>>>>>>> 1) A CCC has been submitted, reviewed (at length) and approved.
>>>>>>>>
>>>>>>>> 2) I've confirmed the correct behavior wrt object-primitive
>>>>>>>> comparisons.
>>>>>>>>
>>>>>>>> 3) I've fixed some errors in our tests, which would not compile when
>>>>>>>> javac enforces the correct type rules.
>>>>>>>>
>>>>>>>> 4) This change has been flagged for a release note.
>>>>>>>>
>>>>>>>> The webrev is here:
>>>>>>>> http://cr.openjdk.java.net/~emc/8013357/webrev.01/
>>>>>>> Hi,
>>>>>>> couple of comments:
>>>>>>>
>>>>>>> *) I would be wary of adding Number to the symtab which will load
>>>>>>> symbols eagerly; j.l.Number is not available on all platforms, so there
>>>>>>> will be issues when using javac in cross compilation environment,
>>>>>>> compiling against a J2ME rt.jar; the usual trick is to call Symbtab.
>>>>>>> synthesizeEmptyInterfaceIfMissing (see for other code in Symtab that
>>>>>>> does that for a reference).
>>>>>>>
>>>>>>> *) The logic for checking equality seems questionable/overly complex
>>>>>>> (expanded in meta code below):
>>>>>>>
>>>>>>> boolean isEqualityComparable (Type s, Type t)
>>>>>>> if (<both primitives>) {
>>>>>>>       return s <: t //1
>>>>>>> } else if (<both references>) {
>>>>>>>       return s castable to t //2
>>>>>>> } else if (source >= 1.5) {
>>>>>>>       if (<t is primitive>) {
>>>>>>>           return (<s is j.l.Number>) ? <t.isNumeric() : (unbox(s) <:
>>>>>>> t) //3
>>>>>>>       } else {
>>>>>>>           return (<t is j.l.Number>) ? <s.isNumeric() : (unbox(t) <:
>>>>>>> s) //4
>>>>>>>       }
>>>>>>> }
>>>>>>>
>>>>>>> The first thing that strikes me is that in places (1, 2, 3, 4) we are
>>>>>>> not being symmetric; i.e.
>>>>>>>
>>>>>>> isEqualityComparable(int, long) = true
>>>>>>> isEqualityComparable(long, int) = false
>>>>>>>
>>>>>>> The second thing that worries me is that the spec says something like:
>>>>>>>
>>>>>>> *) if both are numeric (i.e. primitives) and one is convertible to
>>>>>>> numeric (i.e. unboxable), then a numeric comparison is performed
>>>>>>>
>>>>>>> I think this means that the code should do some check to see whether
>>>>>>> numeric comparison is applicable, (i.e. eventually unboxing) and then
>>>>>>> calling appropriate routine to do the comparison test.
>>>>>>>
>>>>>>> Also, restating a point raised by Alex, Number == primitive is not
>>>>>>> allowed as Number is not convertible to a numeric type; I think the
>>>>>>> test
>>>>>>> should be something like:
>>>>>>>
>>>>>>> boolean isEqualityComparable(Type s, Type t)
>>>>>>> if (<both primitives>) {
>>>>>>>       if (source >= 1.5) {
>>>>>>>          s = types.unboxedTypeOrType(s);
>>>>>>>          t = types.unboxedTypeOrType(t);
>>>>>>>       }
>>>>>>>       return s <: t || t <: s;
>>>>>>> } else if (<both references>) {
>>>>>>>       return (s castable to t) || (t castable to s);
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> Maurizio
>>>>>>>