Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Fri Jun 7 10:17:10 PDT 2013
That's a good testing library, but I have some reservations about using
it here.
First, isEqualityComparable represents a subset of the work that should
go into deciding whether a == b is a valid comparison (regretfully so,
and we should probably look at cleaning up this part of the compiler a
bit). Maybe it would be good to make an enhancement request to build a
similar library which tests operators.
Second, the bug was about a specific program that should have caused a
compiler error but didn't. So it seems better to test it with
source-based tests.
On 06/07/13 11:41, Maurizio Cimadamore wrote:
> 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 --------------
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/ade81696/eric_mccorkle-0001.vcf
More information about the compiler-dev
mailing list