Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Alex Buckley
alex.buckley at oracle.com
Mon Jun 10 18:15:12 PDT 2013
The only reasonable way to check correctness of truthtab entries for,
say, Object, is to know that "the first six are comparisons with
non-numeric reference types, the second six with primitive types, the
third six are captured numeric reference types", etc. Well, things
aren't so easy, as CompareType splits the seven captured types across
two groups of six. I guess a single master list of types which is
compared with itself is good for completeness, but please order the
CompareType constants in a saner way. Please see JLS7 5.5 for the order
in which I presented primitive and reference types in tables 5.1 and 5.2
- don't forget byte, and stick boolean/Boolean at the end.
Watch out on the comments: long's comment doesn't say it's comparable to
itself, and "// Number, any boxed form of a numeric primitive, and any
captures." ignores that Number is comparable to Object.
Since deciding 'true' in truthtab means deciding whether JLS 15.21.1,
.2, or .3 is involved, I think the section number should be reified in
truthtab. You could replace false and true with 0 and 1/2/3. If you get
the order of the master list right, the patterns of 1/2/3 should be
rather predictable.
Alex
On 6/10/2013 2:47 PM, Eric McCorkle wrote:
> 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
>>>>>>>>>>>>>>>>>>
>>>
More information about the compiler-dev
mailing list