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

[Maurizio Cimadamore]

Ok - thx for the clarification!

Maurizio

On 06/06/13 16:06, Eric McCorkle wrote:
> That's what I meant when I said it didn't work, sorry if I wasn't clear
> about it.  The code in webrev.01 does work, though.  So does the
> following code:
>
>      public boolean isEqualityComparable(Type s, Type t, Warner warn) {
>          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;
>      }
>
> I haven't posted a new webrev yet, though, as I want to understand *why*
> it works, and why the code with unboxedTypeOrType doesn't work.
>
> On 06/05/13 05:16, Maurizio Cimadamore wrote:
>> 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
>>>>>>>>>