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

[Maurizio Cimadamore]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130604/2f63c604/attachment-0001.html