Review request for JDK-8143896: java.lang.Long is implicitly converted to double

[Hannes Wallnoefer]

Thanks, Attila! Answers inline.

Am 2016-01-12 um 14:21 schrieb Attila Szegedi:
> +1; looks good overall. I’m fine with this being committed as it is.
>
> Few somewhat unrelated thoughts that came to me as I was reviewing this:
>
> - do we need toUint32 to return long anymore? Could it just return double? There’s a place in the patch where we’re casting it.

It could return double, but most of its usage is for array indices which 
still use int/long internally, so keeping the long return type seems 
more natural.

> - NashornPrimitiveLinker.canLinkTypeStatic: what about ES6 symbols? They’re primitive values too, right? A primitive symbol value can have e.g. toString() invoked on it (or in a more absurd case, any additional method added to Symbol.prototype). Would that work similarly to how we can invoke methods on other primitive values?

You're right, good catch! I'm filing a bug for this.

hannes

> Attila.
>
>
>> On Jan 12, 2016, at 10:33 AM, Hannes Wallnoefer <hannes.wallnoefer at oracle.com> wrote:
>>
>> I uploaded a new webrev without the changes to the parser API.
>>
>> http://cr.openjdk.java.net/~hannesw/8143896/webrev.03/
>>
>> Note that parserapi.js.EXPECTED changes because of the changes in parserapi.js, which is itself included in the files it parses.
>>
>> Please review.
>>
>> Hannes
>>
>> Am 2016-01-11 um 16:57 schrieb Sundararajan Athijegannathan:
>>> As discussed offline, please leave Nashorn Parser API changes for a separate issue.
>>>
>>> -Sundar
>>>
>>> On 1/11/2016 8:07 PM, Hannes Wallnoefer wrote:
>>>> I fixed a bug with converstion to number for the strict equality operator, which also revealed some left over usage of long in Nashorn internals. New webrev is here:
>>>>
>>>> http://cr.openjdk.java.net/~hannesw/8143896/webrev.02/
>>>>
>>>> Hannes
>>>>
>>>> Am 2016-01-11 um 13:48 schrieb Hannes Wallnoefer:
>>>>> You are right of course, there needs to be consistency between typeof operator and treatment as JS numbers.
>>>>>
>>>>> This is in fact an unpleasant problem to solve. I've struggled trying to fix this without breaking any existing code, but I've come to the conclusion that it is not possible. Since we can't treat all Java longs/Longs as JS numbers, we'd have to differentiate depending on whether the value can be represented as double without losing precision.
>>>>>
>>>>> In a way we already do this with optimistic types, but I consider it more a bug than a feature. It's weird (and error prone) if the return value for a Java method returning long is reported as number or object depending on the actual value.
>>>>>
>>>>> So I think the right thing to do is draw a clear line between which Java primitive/wrapper types represent JS numbers and which don't. I've uploaded a new webrev that implements this:
>>>>>
>>>>> http://cr.openjdk.java.net/~hannesw/8143896/webrev.01/
>>>>>
>>>>> Note that the only types to be treated as JS numbers are the direct wrapper classes for Java primitives that can be fully represented as doubles. This means also things like AtomicInteger and DoubleAdder will be reported and treated as objects. I think that's the correct thing to do as they are not primitive numbers in the first place. They are still converted to numbers when used in such a context in JS. So I think the only place where this change is a actually painful/surprising is longs.
>>>>>
>>>>> Unfortunately the check for number type in JSType.isNumber gets a bit long as we have to individually check for all primitive wrapper classes. I've done extensive benchmarking and I don't think it has an impact on performance. In any way, I wouldn't know how to handle this differently.
>>>>>
>>>>> Let me know what you think.
>>>>>
>>>>> Hannes
>>>>>
>>>>> Am 2016-01-04 um 05:00 schrieb Sundararajan Athijegannathan:
>>>>>> I think I already commented on this webrev -- that we need to cover tests for BigInteger, BigDecimal.
>>>>>>
>>>>>> Also, I'm not sure linking Double and Int by nashorn primitive linkers is the right solution. AtomicInteger, DoubleAdder etc. are all Number subtypes. We return "number" when typeof is used on any Number subtype.
>>>>>> Now, that means JS code will see these as 'number' type objects -- yet Number.prototype methods won't work on those!! I know this is hard problem -- we also have another (somewhat related) BigDecimal, BigInteger toString / String conversion issue. We need to discuss this.
>>>>>>
>>>>>> -Sundar
>>>>>>
>>>>>> On 1/2/2016 8:29 PM, Attila Szegedi wrote:
>>>>>>> +1
>>>>>>>
>>>>>>>> On Dec 18, 2015, at 3:54 PM, Hannes Wallnoefer <hannes.wallnoefer at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Please review JDK-8143896: java.lang.Long is implicitly converted to double
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~hannesw/8143896/webrev/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Hannes