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

[Eric McCorkle]

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 --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 314 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130601/c896af5a/eric_mccorkle.vcf