Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Mon Jun 10 14:47:31 PDT 2013
Link here:
http://cr.openjdk.java.net/~emc/8013357/webrev.05/
On 06/10/13 17:40, Eric McCorkle wrote:
> Okay, I implemented a better test which iterates all (24^2)
> possibilities. This actually did expose some additional problem cases.
>
> Please look at the tests, and ensure that they are JLS-compliant.
>
> On 06/07/13 13:24, Maurizio Cimadamore wrote:
>> On 07/06/13 18:17, Eric McCorkle wrote:
>>> 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.
>> Yes and no - there are still at least 9 * 9 combinations (all primitives
>> + Object vs. all primitives plus Object)
>>
>> If you throw a bunch of reference type in the picture to check the
>> castable thing, the number of possibilities is even higher.
>>>
>>> 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.
>> You can, in addition, write a simple source-based test (replica of
>> what's in JIRA) just to check that error message is generated correctly.
>>
>> Maurizio
>>>
>>> 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/20130610/0ecbdef8/eric_mccorkle-0001.vcf
More information about the compiler-dev
mailing list