Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Jun 7 08:41:02 PDT 2013
Nice - now you 'only' have to fix the test by using the Type harness:
http://hg.openjdk.java.net/jdk8/tl/langtools/file/5b039297151e/test/tools/javac/types/PrimitiveConversionTest.java
Maurizio
On 07/06/13 16:27, Eric McCorkle wrote:
> 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 --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130607/75693649/attachment.html
More information about the compiler-dev
mailing list