Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Jun 7 10:24:00 PDT 2013
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
>>>>>>>>>>>>>>>
More information about the compiler-dev
mailing list