Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Jun 7 03:29:08 PDT 2013
On 06/06/13 21:21, Eric McCorkle wrote:
> Is there any reason why isPrimitive() on the type representing Number
> (or any unboxable type, for that matter) would return true? Because if
> that's the case, and isSubtype handles unboxing, then it would explain
> why this code works for Boxed(T) == T style comparisons.
Nope - isPrimitive should only take the type tag and compare it with the
primitive tags. Number, Integer etc. have tag CLASS, which is not covered.
Maurizio
>
> On 06/06/13 11:41, Maurizio Cimadamore wrote:
>> 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
>>>>>>>>>>>
More information about the compiler-dev
mailing list