Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Tue Jun 11 06:10:55 PDT 2013
Good suggestion.
Something else occurred to me: the scope of this test has grown beyond
that of a simple regression test. It should probably be moved to a
subdirectory of test/tools/javac purposed for more exhaustive tests.
Jon or Maurizio, do you have an suggestions?
On 06/10/13 21:15, Alex Buckley wrote:
> 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
>>>>>>>>>>>>>>>>>>>
>>>>
-------------- 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/20130611/9bcd0acb/eric_mccorkle-0001.vcf
More information about the compiler-dev
mailing list