Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MakeDay should be more explicit how unreasonably large numbers are handled #1087

Open
anba opened this issue Feb 1, 2018 · 8 comments
Open

Comments

@anba
Copy link
Contributor

anba commented Feb 1, 2018

20.3.1.12 MakeDay should use a more precise language than "[...] but if this is not possible (because some argument is out of range), return NaN.". For example 20.3.1.11 MakeTime explicitly requires to use IEEE 754-2008 semantics for its operations.

This test prints different results in all four engines (SpiderMonkey and ChakraCore are almost the same, except Chakra prints the year as "-0000" for the second date, but that's probably a different issue) and I blame it one the imprecise language. 😛

d = new Date(0);
d.setUTCFullYear(Number.MAX_VALUE, Number.MAX_VALUE);
print(d.toUTCString());

d = new Date(0);
d.setUTCFullYear(Number.MAX_VALUE / 12 + 100, -Number.MAX_VALUE);
print(d.toUTCString());
@littledan
Copy link
Member

cc @jungshik @rwaldron

@ljharb
Copy link
Member

ljharb commented Apr 26, 2019

cc @gibson042

@gibson042
Copy link
Contributor

gibson042 commented Apr 26, 2019

Engine inconsistency confirmed; I'm willing to categorize it into three behaviors (V8/XS vs. SM/Chakra vs. JSC):

$ eshost -is script.js
## Source
d = new Date(0);
d.setUTCFullYear(Number.MAX_VALUE, Number.MAX_VALUE);
print(d.toUTCString());

d = new Date(0);
d.setUTCFullYear(Number.MAX_VALUE / 12 + 100, -Number.MAX_VALUE);
print(d.toUTCString());


#### Chakra
Invalid Date
Mon, 01 May -0000 00:00:00 GMT

#### JavaScriptCore
Sat, 01 Jan 0000 00:00:00 GMT
Sat, 01 Jan 0000 00:00:00 GMT

#### SpiderMonkey
Invalid Date
Mon, 01 May 0000 00:00:00 GMT

#### V8, XS
Invalid Date
Invalid Date

As for the spec, MakeDay( year, month, date ) step 7 relies on two mathematical integers derived from combining integer Number interpretations of year and monthym (a whole number of years after 1 BCE) and mn (a positive number of months in the year corresponding with ym)—and calls for returning NaN unless a corresponding value (time value?) t exists. Time values are bounded to be safe integers in the inclusive range ±8.64e15 ("approximately -285,426 to 285,426 -273,790 to 273,790 years relative to [1970]"), so there is definitely no time value t corresponding to a year greater than even 275,760. A strict reading of the spec therefore requires "Invalid Date" for the first case (i.e., JSC is nonconforming). For the second case, though, year and month mostly cancel out and there is a time value corresponding to +000100-05-01T00:00Z (i.e., every engine is nonconforming).

I like the spec result for the second case, but to be honest, I don't like the bounds clipping implicit in the calls of step 7 because it doesn't incorporate the date argument (resulting in strange behavior for e.g. setUTCFullYear(275761, 0, -365)). I'd think we should move that as far out as possible, and instead spec the inner part of MakeDay and the other Date-related abstract operations to work in the purely mathematical space.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2019

Would you be willing to prepare a PR, and present it at an upcoming meeting for consensus? 🙏

@littledan
Copy link
Member

littledan commented Apr 26, 2019

Great work, thanks for this analysis.

I'd rather avoid using mathematical values in the ECMAScript specification as much as possible, and move towards using Number internally or, in the future, BigInt, when possible. This is the direction of #1135 . I don't know of anything which could produce a valid Date which would rely on calculations outside of Number range. What are the downsides of standardizing the XS/V8 semantics?

@gibson042
Copy link
Contributor

gibson042 commented Apr 27, 2019

I don't know of anything which could produce a valid Date which would rely on calculations outside of Number range.

setUTCFullYear(Number.MAX_SAFE_INTEGER, 24, -365.2425 * (Number.MAX_SAFE_INTEGER - 2019))

I'd rather avoid using mathematical values in the ECMAScript specification as much as possible, and move towards using Number internally or, in the future, BigInt

In this case, a mathematical treatment can be defined in terms of BigInt.

What are the downsides of standardizing the XS/V8 semantics?

I'm not sure what those semantics are.

@gibson042
Copy link
Contributor

gibson042 commented May 28, 2019

It turns out that new test cases and/or spec text are also needed for existing MakeTime IEEE 754-2008 binary64 semantics:

Input that cancels out (JSC fails)

$ eshost -si cancel.js
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;

const h = Number.MAX_VALUE / msPerHour;
const m = -Number.MAX_VALUE / msPerMinute;
const s = 0;
const ms = 0;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,1, h,m,s,ms)}`);

#### Chakra, SpiderMonkey, V8, XS
Number 0, time value 0

#### JavaScriptCore
Number 0, time value NaN

Input that cancels out with rounding (JSC and XS fail)

$ eshost -si round_cancel.js 
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;

const h = Number.MAX_VALUE / msPerHour;
const m = +(2**(1023-54)) / msPerMinute;
const s = 0;
const ms = -Number.MAX_VALUE;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,1, h,m,s,ms)}`);

#### Chakra, SpiderMonkey, V8
Number 0, time value 0

#### JavaScriptCore, XS
Number 0, time value NaN

Input that cancels out and then comes in at the time value limit (JSC fails)

$ eshost -si max.js
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;

const h = Number.MAX_VALUE / msPerHour;
const m = -Number.MAX_VALUE / msPerMinute;
const s = 0;
const ms = 8.64e15;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,1, h,m,s,ms)}`);

#### Chakra, SpiderMonkey, V8, XS
Number 8640000000000000, time value 8640000000000000

#### JavaScriptCore
Number 8640000000000000, time value NaN
$ eshost -si min.js 
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;

const h = Number.MAX_VALUE / msPerHour;
const m = -Number.MAX_VALUE / msPerMinute;
const s = 0;
const ms = -8.64e15;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,1, h,m,s,ms)}`);

#### Chakra, SpiderMonkey, V8, XS
Number -8640000000000000, time value -8640000000000000

#### JavaScriptCore
Number -8640000000000000, time value NaN

Input that cancels out and then exceeds the time value limit (all pass)

$ eshost -si over.js
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;

const h = Number.MAX_VALUE / msPerHour;
const m = -Number.MAX_VALUE / msPerMinute;
const s = 0;
const ms = 8.64e15 + 1;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,1, h,m,s,ms)}`);

#### Chakra, JavaScriptCore, SpiderMonkey, V8, XS
Number 8640000000000001, time value NaN

Input that exceeds the Number range and unsuccessfully attempts to cancel out (all pass)

$ eshost -si round_infinity.js
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;

const h = Number.MAX_VALUE / msPerHour;
const m = +(2**(1023-52)) / msPerMinute;
const s = -(2**(1023-52)) / msPerSecond;
const ms = -Number.MAX_VALUE;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,1, h,m,s,ms)}`);

#### Chakra, JavaScriptCore, SpiderMonkey, V8, XS
Number Infinity, time value NaN

Input where MakeTime exceeds the Number range only when considered independently from MakeDay (all pass)

$ eshost -si day_time_cancel.js
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;
const msPerDay = msPerHour * 24;

const d = -Number.MAX_VALUE / msPerDay;
const h = Number.MAX_VALUE / msPerHour;
const m = +(2**(1023-52)) / msPerMinute;
const s = -(2**(1023-52)) / msPerSecond;
const ms = 0;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(d) * msPerDay +
    ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,d, h,m,s,ms)}`);

#### Chakra, JavaScriptCore, SpiderMonkey, V8, XS
Number 0, time value NaN

Input where MakeDay and MakeTime cancel out (JSC and XS fail)

$ eshost -si day_time.js 
## Source
const msPerSecond = 1000, msPerMinute = msPerSecond * 60, msPerHour = msPerMinute * 60;
const msPerDay = msPerHour * 24;

const d = -Number.MAX_VALUE / msPerDay;
const h = Number.MAX_VALUE / msPerHour;
const m = 0;
const s = 0;
const ms = 0;

const ToInteger = number => Math.sign(number) * Math.floor(Math.abs(number));
const n = ToInteger(d) * msPerDay +
    ToInteger(h) * msPerHour + ToInteger(m) * msPerMinute +
    ToInteger(s) * msPerSecond + ms;
print(`Number ${n}, time value ${Date.UTC(1970,0,d, h,m,s,ms)}`);

#### Chakra, SpiderMonkey, V8
Number 0, time value 0

#### JavaScriptCore, XS
Number 0, time value NaN

@ptomato
Copy link
Contributor

ptomato commented May 7, 2022

This is relevant even when not using out-of-range Number.MAX_VALUE arguments, as well. MakeDay always runs into trouble around the earlier limit of time values, because it specifies finding a time value corresponding to the first day of the month. The earliest valid date is -271821-04-20, so not the first of a month.

For example, Date.UTC(-271821, 3, 20) should return the earliest possible time value (-86400×10^11) but in step 10 of Date.UTC() we call MakeDay(-271821, 3, 20). (I'm omitting the F subscripts for brevity)

Step 8 of MakeDay requires finding a finite time value t such that YearFromTime(t) = -271821, MonthFromTime(t) = 3, and DateFromTime(t) = 1. No such time value exists. Its Number counterpart would be -8640001641600000, in other words the earliest time value minus 19 days.

I think #1564 would solve this, but at the very least MakeDay shoudn't return a time value.

ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 1, 2024
We must avoid calling GetUTCEpochNanoseconds with dates that are too large
because it is ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). We had a condition in
ToTemporalInstant that was supposed to prevent this, but was incorrect
because subtracting the UTC offset could push the date out of range.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.

See: #2985
ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 1, 2024
Similarly to the previous commit, we must avoid calling
GetUTCEpochNanoseconds with dates that are too large because it is
ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). Previously to PR #2925, that
could not happen because GetPossibleInstantsFor took a PlainDateTime
object, but now it takes an ISO Date-Time Record.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.
ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 2, 2024
We must avoid calling GetUTCEpochNanoseconds with dates that are too large
because it is ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). We had a condition in
ToTemporalInstant that was supposed to prevent this, but was incorrect
because subtracting the UTC offset could push the date out of range.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.

See: #2985
ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 2, 2024
Similarly to the previous commit, we must avoid calling
GetUTCEpochNanoseconds with dates that are too large because it is
ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). Previously to PR #2925, that
could not happen because GetPossibleInstantsFor took a PlainDateTime
object, but now it takes an ISO Date-Time Record.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.
ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 2, 2024
Similarly to the previous commit, we must avoid calling
GetUTCEpochNanoseconds with dates that are too large because it is
ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). Previously to PR #2925, that
could not happen because we used a PlainDateTime object in
InterpretISODateTimeOffset, but now we use an ISO Date-Time Record.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.
Ms2ger pushed a commit to tc39/proposal-temporal that referenced this issue Oct 3, 2024
We must avoid calling GetUTCEpochNanoseconds with dates that are too large
because it is ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). We had a condition in
ToTemporalInstant that was supposed to prevent this, but was incorrect
because subtracting the UTC offset could push the date out of range.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.

See: #2985
Ms2ger pushed a commit to tc39/proposal-temporal that referenced this issue Oct 3, 2024
Similarly to the previous commit, we must avoid calling
GetUTCEpochNanoseconds with dates that are too large because it is
ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). Previously to PR #2925, that
could not happen because GetPossibleInstantsFor took a PlainDateTime
object, but now it takes an ISO Date-Time Record.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.
Ms2ger pushed a commit to tc39/proposal-temporal that referenced this issue Oct 3, 2024
Similarly to the previous commit, we must avoid calling
GetUTCEpochNanoseconds with dates that are too large because it is
ill-defined in ECMA-262 what is supposed to happen. (See
tc39/ecma262#1087). Previously to PR #2925, that
could not happen because we used a PlainDateTime object in
InterpretISODateTimeOffset, but now we use an ISO Date-Time Record.

Note that this is editorial, because IsValidEpochNanoseconds would throw
anyway in this case even if GetUTCEpochNanoseconds was fully defined for
large inputs.
ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 8, 2024
This pattern is there because of #2729 and #2985. Abstract it into its own
operation, for documentation purposes and because it can disappear after
tc39/ecma262#1087 is fixed.

See: #3005
ptomato added a commit to tc39/proposal-temporal that referenced this issue Oct 9, 2024
This pattern is there because of #2729 and #2985. Abstract it into its own
operation, for documentation purposes and because it can disappear after
tc39/ecma262#1087 is fixed.

See: #3005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants