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

invalid Assertion in BalanceISODate #2396

Closed
FrankYFTang opened this issue Aug 31, 2022 · 7 comments
Closed

invalid Assertion in BalanceISODate #2396

FrankYFTang opened this issue Aug 31, 2022 · 7 comments

Comments

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Aug 31, 2022

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
2. Assert: epochDays is finite.

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

@FrankYFTang
Copy link
Contributor Author

@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
Copy link
Contributor

anba commented Aug 31, 2022

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
Copy link
Contributor Author

FrankYFTang commented Aug 31, 2022

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
Copy link
Contributor Author

FrankYFTang commented Aug 31, 2022

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
Copy link
Contributor

anba commented Sep 1, 2022

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

@anba
Copy link
Contributor

anba commented Sep 1, 2022

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

Yes, exactly that.

@ptomato
Copy link
Collaborator

ptomato commented Sep 8, 2022

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

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

No branches or pull requests

3 participants