264727 - JavaScript Test Library - TimeWithinDay() implementation doesn't conform to ECMA-262 3rd ed standard

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Leonard]

User-Agent:       Mozilla/5.0 (X11; U; OpenBSD i386; en-US; rv:1.6) Gecko/20040324 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; OpenBSD i386; en-US; rv:1.6) Gecko/20040324 Firefox/0.8

This problem was found when investigating a bug discrepancy when running the
mozilla js test suite against an alternate ecmascript implementation. In this
case, ecma/Date/15.9.5.34-1.js gave me:

TDATE = new Date(-2208988800000); (TDATE).setMonth(11,31); TDATE.getTime
() = -2177539200000 FAILED! expected: -2177452800000

This test is taking the known time for '1 Jan 1900 00:00 UTC' and finding the
time for the last day of that year (31 Dec 1900 00:00 UTC). 1900 is a leap year,
so the right answer is -2208988800000 + 364 * 86400000 (= -2177539200000, what
my interpreter gives, and NOT what the test scripts calculate).

After investigation, this is not a bug in any leap year calculation. It is a
problem with the test script's implementation of TimeWithinDay().

The standard (ECMA-262 3rd edition 1999) says this about TimeWithinDay():

   15.9.1.2 Day Number and Time within Day
    A given time value t belongs to day number
        Day(t) = floor(t / msPerDay)
    where the number of milliseconds per day is
        msPerDay = 86400000
    The remainer is called the time within the day:
        TimeWithinDay(t) = t modulo msPerDay

The aternate interpreter's implementation is effectively fmod(t, msPerDay), but
the test fragment ecma/shell.js implements it as:

        function TimeWithinDay( t )     {
                if ( t < 0 ) {
                        return ( (t     % msPerDay)     + msPerDay );
                } else {
                        return ( t % msPerDay );
                }
        }

This seems to be a misunderstanding of modulo, which is defined over negative
integers just fine. For negative times, negative remainders make perfect sense.
There is no need to sign-correct the result of TimeWithinDay: all the maths in
teh standard work fine without such corrections.

Reproducible: Always
Steps to Reproduce:
1. load ecma/shell.js
2. compute TimeWithinDay(TIME_1900)

Actual Results:  
86400000

Comment 1

[David Leonard]

The expected result of TimeWithinDay(TIME_1900) is 0.

(I had typed '0' as the content of the expected field, but bugzilla seems to
have treated it as missing. bug in bugzilla?)

Comment 2

[Igor Bukanov]

Attachment: Patch to remove NativeFunction.(version|argNames|argCount)

The patch replaces version|argNames|argCount fields in NativeFunction by
getLanguageVersion()|getParamCount()|getParamAndVarCount()|getParamOrVarName(int
index) abstract methods.

For the interpreted mode the methods are then trivially implemented in
InterpretedFunction using information stored in InterpreterData. 

For the class compilation mode patch adds code to generate methods'
implementations using data embedded in the generated class file. In this way
for the class compiler the patch not only removes 3 fields from each function
instance but also avoids the construction of argNames array replacing the
bytecode to initialize it by the bytecode to do the switch over var index. 

The patch also replaces extending from NativeFunction in
NativeScript|NativeRegExpCtor classes by the proper extending from BaseFunction
which was a oversight left from pre-1.5R5 reorganization of code generation.

Comment 3

[Igor Bukanov]

Comment on attachment 162379 [details] [diff] [review]
Patch to remove NativeFunction.(version|argNames|argCount)

Please ignore, this an attchment fro the wrong bug!

Comment 4

[Igor Bukanov]

Comment on attachment 162379 [details] [diff] [review]
Patch to remove NativeFunction.(version|argNames|argCount)

Please ignore, this an attchment for the wrong bug!

Comment 5

[Igor Bukanov]

Reassigning to [e-mail] pending resolution of bug 288433

Comment 6

[Attila Szegedi]

Could you please elaborate on how exactly is this a Rhino bug?

Comment 7

[David Leonard]

(In reply to comment #6)
> Could you please elaborate on how exactly is this a Rhino bug?

I'm guessing it's a bug in Rhino's test suite. Maybe I'm wrong.

Comment 8

[Bob Clary [:bc] (inactive)]

mine. If you have future bugs on the test library, please file them in the js engine component and we will take care of them hopefully.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: patch

(In reply to comment #0)

> 
> This test is taking the known time for '1 Jan 1900 00:00 UTC' and finding the
> time for the last day of that year (31 Dec 1900 00:00 UTC). 1900 is a leap
> year,

1900 is not a leap year since it is divisible by 100 but not 400.
...
> the test fragment ecma/shell.js implements it as:
> 
>         function TimeWithinDay( t )     {
>                 if ( t < 0 ) {
>                         return ( (t     % msPerDay)     + msPerDay );
>                 } else {
>                         return ( t % msPerDay );
>                 }
>         }
> 
> This seems to be a misunderstanding of modulo, which is defined over negative
> integers just fine. For negative times, negative remainders make perfect sense.
> There is no need to sign-correct the result of TimeWithinDay: all the maths in
> teh standard work fine without such corrections.
> 

however, ecma 262-3 says:

<quote> 
The notation “x modulo y” (y must be finite and
nonzero) computes a value k of the same sign as y (or zero) such
that abs(k) < abs(y) and x−k = q × y for some integer q.
</quote>

So the result of modulo must be the same sign as y which is why
the test scripts perform the adjustment. However, they check the
sign of x to determine if the adjustment to make the result
positive. This over corrects in the case where the result would 
have been zero anyway. TimeWithinDay appears to be the only case 
where this mistake was made.

Igor, think it is worth adding a TimeWithinDay(TIME_1900) === 0 test?

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Igor, think it is worth adding a TimeWithinDay(TIME_1900) === 0 test?

Yes, to capture tests assumptions.

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/Date/15.9.1.2-01.js

Comment 12

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma/shell.js,v  <--  shell.js
new revision: 1.23; previous revision: 1.22

/cvsroot/mozilla/js/tests/ecma_3/Date/shell.js,v  <--  shell.js
new revision: 1.11; previous revision: 1.10

/cvsroot/mozilla/js/tests/ecma_3/Date/15.9.1.2-01.js,v  <--  15.9.1.2-01.js
initial revision: 1.1

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.0