Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Fri Jun 7 08:27:49 PDT 2013
Ok, I got it now...
That code works because it never sees Boxed(T) == T comparisons. Back
in Attr, when it decides which opcode to use, it does the boxing and
unboxing. So say, Integer == int will get if_icmpeq, whereas object
comparisons will get if_acmpeq. It only calls isEqualityComparable when
the comparison is for if_acmpeq, so isEqualityComparable never sees
primitive == primitive or Boxed(T) == T comparisons at all.
I posted a new webrev with isEqualityComparable slimmed down to not do
primitive == primitive comparisons, expanded comments, and some extra tests.
On 06/07/13 06:29, Maurizio Cimadamore wrote:
> 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
>>>>>>>>>>>>
>
-------------- 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/20130607/10651fa6/eric_mccorkle-0001.vcf
More information about the compiler-dev
mailing list