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.

Sign up for GitHub

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

Jump to bottom

option of Calendar.dateAdd is not undefined but null #3262

Open
FrankYFTang opened this issue Oct 9, 2021 · 6 comments
Open

option of Calendar.dateAdd is not undefined but null #3262

FrankYFTang opened this issue Oct 9, 2021 · 6 comments

Comments

@FrankYFTang
Copy link
Contributor

This test is wrong I think
Temporal/Duration/prototype/add/calendar-dateadd-called-with-options-undefined.js

const calendar = TemporalHelpers.calendarDateAddUndefinedOptions();
const timeZone = TemporalHelpers.oneShiftTimeZone(new Temporal.Instant(0n), 3600e9);
const instance = new Temporal.Duration(1, 1, 1, 1);
instance.add(instance, { relativeTo: new Temporal.ZonedDateTime(0n, timeZone, calendar) });

The problem is in step6 of 7.3.18 Temporal.Duration.prototype.add ( other [ , options ] )
https://tc39.es/proposal-temporal/#sec-temporal-addzoneddatetime

6. Let result be ? AddDuration(duration.[[Years]], duration.[[Months]], duration.[[Weeks]], duration.[[Days]], duration.[[Hours]], duration.[[Minutes]], duration.[[Seconds]], duration.[[Milliseconds]], duration.[[Microseconds]], duration.[[Nanoseconds]], other.[[Years]], other.[[Months]], other.[[Weeks]], other.[[Days]], other.[[Hours]], other.[[Minutes]], other.[[Seconds]], other.[[Milliseconds]], other.[[Microseconds]], other.[[Nanoseconds]], relativeTo).

it will call AddDuration. then
AddDuration will call AddZonedDateTime in step 7d
https://tc39.es/proposal-temporal/#sec-temporal-addduration

d. Let intermediateNs be ? AddZonedDateTime(relativeTo.[[Nanoseconds]], timeZone, calendar, y1, mon1, w1, d1, h1, min1, s1, ms1, mus1, ns1).

and then in AddZonedDateTime
https://tc39.es/proposal-temporal/#sec-temporal-addzoneddatetime

1. If options is not present, set options to ! OrdinaryObjectCreate(null).

options will be a null object (not undefined)
then

7. Let addedDate be ? CalendarDateAdd(calendar, datePart, dateDuration, options).

so what the Calendar.dateAdd will get in this case is a empty object, not undefined.
@FrankYFTang
Copy link
Contributor Author

@ptomato @Ms2ger @jugglinmike @ryzokuken

@FrankYFTang
Copy link
Contributor Author

I think the following tests has the same issue

'built-ins/Temporal/Duration/prototype/add/calendar-dateadd-called-with-options-undefined': [FAIL],
'built-ins/Temporal/Duration/prototype/subtract/calendar-dateadd-called-with-options-undefined': [FAIL],
'built-ins/Temporal/PlainDate/prototype/toZonedDateTime/calendar-dateadd-called-with-options-undefined': [SKIP],
'built-ins/Temporal/PlainDateTime/prototype/toZonedDateTime/calendar-dateadd-called-with-options-undefined': [SKIP],
'built-ins/Temporal/PlainTime/prototype/toZonedDateTime/calendar-dateadd-called-with-options-undefined': [FAIL],
'built-ins/Temporal/TimeZone/prototype/getInstantFor/calendar-dateadd-called-with-options-undefined': [SKIP],
'built-ins/Temporal/ZonedDateTime/prototype/round/calendar-dateadd-called-with-options-undefined': [SKIP],
'built-ins/Temporal/ZonedDateTime/prototype/startOfDay/calendar-dateadd-called-with-options-undefined': [SKIP],
'built-ins/Temporal/ZonedDateTime/prototype/withPlainDate/calendar-dateadd-called-with-options-undefined': [SKIP],
'built-ins/Temporal/ZonedDateTime/prototype/withPlainTime/calendar-dateadd-called-with-options-undefined': [SKIP],

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 11, 2021

Ref tc39/proposal-temporal#1685

@ptomato
Copy link
Contributor

ptomato commented Oct 15, 2021

Yes, this was a discrepancy with the spec text. I'm hoping to present that normative change at the December TC39 meeting. I guess it's up to the maintainers whether to remove the tests affected by it in the meantime.

@FrankYFTang
Copy link
Contributor Author

Yes, this was a discrepancy with the spec text. I'm hoping to present that normative change at the December TC39 meeting. I guess it's up to the maintainers whether to remove the tests affected by it in the meantime.

I suggest we wait till the conclusion from Decemeber TC39 before changing anything.

@ptomato
Copy link
Contributor

ptomato commented Jan 20, 2022

I didn't get a chance to finish this for December and I'm hoping to present it at the March TC39 meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Ms2ger @ptomato @FrankYFTang