Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Mon Jun 10 14:40:42 PDT 2013
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/ae81eb01/eric_mccorkle.vcf
More information about the compiler-dev
mailing list