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

Problem in Temporal.PlainDate.prototype.since/until #1864

Closed
FrankYFTang opened this issue Oct 8, 2021 · 1 comment · Fixed by #1955
Closed

Problem in Temporal.PlainDate.prototype.since/until #1864

FrankYFTang opened this issue Oct 8, 2021 · 1 comment · Fixed by #1955
Assignees

Comments

@FrankYFTang
Copy link
Contributor

While I debug my v8 implementation test breakage I think I found a spec bug. Not 100% sure.
The test is built-ins/Temporal/PlainDate/prototype/since/roundingmode-undefined.js

const earlier = new Temporal.PlainDate(2000, 1, 1);

const later1 = new Temporal.PlainDate(2005, 2, 20);
const explicit1 = later1.since(earlier, { smallestUnit: "year", roundingMode: undefined });

I got a throw in the since line

look at
https://tc39.es/proposal-temporal/#sec-temporal.plaindate.prototype.since

6. Let disallowedUnits be « "hour", "minute", "second", "millisecond", "microsecond", "nanosecond" ».
7. Let smallestUnit be ? ToSmallestTemporalUnit(options, disallowedUnits, "day").
8. Let largestUnit be ? ToLargestTemporalUnit(options, disallowedUnits, "auto", "day").
9. Perform ? ValidateTemporalUnitRange(largestUnit, smallestUnit).

After step 7 smallestUnit will be "year".
After step 8 largestUnit will be "day"
And inside step 9 it will throw.

The spec look similar in until

should we change step 8 to

  1. Let largestUnit be ? ToLargestTemporalUnit(options, disallowedUnits, "auto", smallestUnit).
    instead?

@ptomato @ljharb @Ms2ger @jugglinmike @ryzokuken

@ptomato
Copy link
Collaborator

ptomato commented Oct 8, 2021

At first glance it looks like PlainDate is missing something like

Let defaultLargestUnit be ! LargerOfTwoTemporalUnits("day", smallestUnit).

(we have this in other types such as PlainDateTime, I believe it was intended to have it everywhere as per #827 (comment))

@ptomato ptomato self-assigned this Dec 3, 2021
ptomato added a commit that referenced this issue Dec 3, 2021
To bring the default for largestUnit in PlainDate's since() and until()
methods in line with other types' since() and until() methods, we have to
add an algorithm step for LargerOfTwoTemporalUnits.

Without this, code such as
date1.until(date2, { smallestUnit: 'months' })
would throw because the default largestUnit is days. As per
#827 (comment)
this was not intended.

PlainDate seems to be the only place where this was not working as
intended. ZonedDateTime, Instant, PlainTime, PlainYearMonth, and
PlainDateTime either already have this step or their default largestUnit
is already the largest one so they wouldn't have this problem.

The reference polyfill code is already correct in this regard.

Closes: #1864
ptomato added a commit that referenced this issue Dec 17, 2021
To bring the default for largestUnit in PlainDate's since() and until()
methods in line with other types' since() and until() methods, we have to
add an algorithm step for LargerOfTwoTemporalUnits.

Without this, code such as
date1.until(date2, { smallestUnit: 'months' })
would throw because the default largestUnit is days. As per
#827 (comment)
this was not intended.

PlainDate seems to be the only place where this was not working as
intended. ZonedDateTime, Instant, PlainTime, PlainYearMonth, and
PlainDateTime either already have this step or their default largestUnit
is already the largest one so they wouldn't have this problem.

The reference polyfill code is already correct in this regard.

Closes: #1864
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.

2 participants