tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.35k stars 153 forks source link

invalid Assertion in BalanceISODate #2396

Closed FrankYFTang closed 2 years ago

FrankYFTang commented 2 years ago

https://tc39.es/proposal-temporal/#sec-temporal-balanceisodate has the following code

  1. Let epochDays be MakeDay(𝔽(year), 𝔽(month - 1), 𝔽(day)).
  2. Assert: epochDays is finite. ...

From https://github.com/tc39/test262/blob/main/test/built-ins/Temporal/Calendar/prototype/dateAdd/argument-duration-years-and-months-number-max-value.js Let's has

var cal = new Temporal.Calendar("iso8601");
var date = new Temporal.PlainDate(1970, 1, 1);
var maxValue = new Temporal.Duration(Number.MAX_VALUE, Number.MAX_VALUE);
cal.dateAdd(date, maxValue)

Now, we will get into AddISODate ( year, month, day, years, months, weeks, days, overflow )

as AddISODate ( 1970, 1, 1, Number.MAX_VALUE, Number.MAX_VALUE, 0, 0, overflow )

and get into BalanceISODate() with a not finite years and therefore MakeDay() will return a NaN

But then we will have this assertion violation

  1. Assert: epochDays is finite.

Or did I missed something? @ptomato @anba @ljharb @gibson042 @sffc @ryzokuken

FrankYFTang commented 2 years ago

@anba

could you tell me where is the throw of the RangeError in https://github.com/tc39/test262/blob/main/test/built-ins/Temporal/Calendar/prototype/dateAdd/argument-duration-years-and-months-number-max-value.js

"! BalanceISOYearMonth(....)" won't throw, right?

anba commented 2 years ago

Yes, MakeDay callers currently don't handle too large values → #2315.

could you tell me where is the throw of the RangeError in

It should probably throw in BalanceISOYearMonth or RegulateISODate (*), but it'll definitely throw in CreateTemporalDate.

(*) RegulateISODate can't handle infinities, because it calls ISODaysInMonth, which in turn calls InLeapYear, which in turn calls DaysInYear, and DaysInYear applies on its input and ℝ(Infinity) isn't defined. Infinity is computed earlier when evaluating TimeFromYear(𝔽(year)) with year = Number.MAX_VALUE.

FrankYFTang commented 2 years ago

ok, I think I misread. your comment earlier on. so you mean that should be throw earlier on in BalanceISOYearMonth but the spec text currently does not throw, right?

FrankYFTang commented 2 years ago

It should probably throw in BalanceISOYearMonth or RegulateISODate

Is that enough? What if in the input weeks is Number.MAX_VALUE or days is Number.MAX_VALUE ? that will make BalanceISOYearMonth or RegulateISODate not throwing (since neither weeks or days are used for that two calls) but will/may reach BalanceISODate with a date = Infinity right?

anba commented 2 years ago

Yes, when either weeks or days is too large, additional error handling is needed, but that can probably be handled through #2315.

anba commented 2 years ago

so you mean that should be throw earlier on in BalanceISOYearMonth but the spec text currently does not throw, right?

Yes, exactly that.

ptomato commented 2 years ago

I think this is covered by #2315. Thanks for the code example, I'll copy it over there.