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

Normative: Disallow negative day lengths #2261

Merged
merged 1 commit into from
Jul 21, 2022
Merged

Conversation

anba
Copy link
Contributor

@anba anba commented Jun 8, 2022

User-defined time zones can produce negative day lengths.

class TimeZone extends Temporal.TimeZone {
  #count = 0;
  #nanoseconds;

  constructor(nanoseconds) {
    super("UTC");
    this.#nanoseconds = nanoseconds;
  }
  getPossibleInstantsFor(dateTime) {
    if (++this.#count === 2) {
      return [new Temporal.Instant(this.#nanoseconds)];
    }
    return super.getPossibleInstantsFor(dateTime);
  }
}

function test(epochNanoseconds, tomorrowEpochNanoseconds, testCases) {
  for (let [roundingMode, expected] of Object.entries(testCases)) {
    let timeZone = new TimeZone(tomorrowEpochNanoseconds);
    let zoned = new Temporal.ZonedDateTime(epochNanoseconds, timeZone);
    let result = zoned.round({ smallestUnit: "days", roundingMode });
    console.log(result.epochNanoseconds, expected);
  }
}

const oneDay = 24n * 60n * 60n * 1000n * 1000n * 1000n;

test(3n, -10n, {
  ceil: 0n,
  floor: -oneDay,
  trunc: 0n,
  halfExpand: 0n,
});
test(-3n, -10n, {
  ceil: oneDay,
  floor: 0n,
  trunc: 0n,
  halfExpand: 0n,
});

User-defined time zones can produce negative day lengths.

```js
class TimeZone extends Temporal.TimeZone {
  #count = 0;
  #nanoseconds;

  constructor(nanoseconds) {
    super("UTC");
    this.#nanoseconds = nanoseconds;
  }
  getPossibleInstantsFor(dateTime) {
    if (++this.#count === 2) {
      return [new Temporal.Instant(this.#nanoseconds)];
    }
    return super.getPossibleInstantsFor(dateTime);
  }
}

function test(epochNanoseconds, tomorrowEpochNanoseconds, testCases) {
  for (let [roundingMode, expected] of Object.entries(testCases)) {
    let timeZone = new TimeZone(tomorrowEpochNanoseconds);
    let zoned = new Temporal.ZonedDateTime(epochNanoseconds, timeZone);
    let result = zoned.round({ smallestUnit: "days", roundingMode });
    console.log(result.epochNanoseconds, expected);
  }
}

const oneDay = 24n * 60n * 60n * 1000n * 1000n * 1000n;

test(3n, -10n, {
  ceil: 0n,
  floor: -oneDay,
  trunc: 0n,
  halfExpand: 0n,
});
test(-3n, -10n, {
  ceil: oneDay,
  floor: 0n,
  trunc: 0n,
  halfExpand: 0n,
});
```
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #2261 (47a819a) into main (7fe29eb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2261   +/-   ##
=======================================
  Coverage   91.16%   91.16%           
=======================================
  Files          19       19           
  Lines       10524    10524           
  Branches     1688     1688           
=======================================
  Hits         9594     9594           
  Misses        918      918           
  Partials       12       12           
Flag Coverage Δ
test262 83.19% <ø> (ø)
tests 82.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fe29eb...47a819a. Read the comment docs.

@anba anba mentioned this pull request Jun 17, 2022
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. I'll convert this to draft in advance of presenting it to TC39.

@ptomato ptomato added spec-text Specification text involved normative Would be a normative change to the proposal labels Jul 4, 2022
@ptomato ptomato marked this pull request as draft July 4, 2022 21:27
@ptomato
Copy link
Collaborator

ptomato commented Jul 21, 2022

This change reached consensus in the July 2022 TC39 plenary meeting. Tests are in tc39/test262#3610.

@ptomato ptomato marked this pull request as ready for review July 21, 2022 21:14
@ptomato ptomato merged commit 6f04074 into tc39:main Jul 21, 2022
@ptomato
Copy link
Collaborator

ptomato commented Jul 25, 2022

Whoops, I merged the wrong PR in reference to the above test262 pull request. This change does not yet have tests.

anba added a commit to anba/test262 that referenced this pull request Jul 27, 2022
Negative day lengths are no longer valid after
<tc39/proposal-temporal#2261>.
ptomato pushed a commit to tc39/test262 that referenced this pull request Jul 27, 2022
Negative day lengths are no longer valid after
<tc39/proposal-temporal#2261>.
@ptomato
Copy link
Collaborator

ptomato commented Aug 2, 2022

(To close the loop, now it has tests.)

ptomato added a commit that referenced this pull request Aug 2, 2022
This is the corresponding polyfill change to the spec change made in
#2261
Ms2ger pushed a commit that referenced this pull request Aug 2, 2022
This is the corresponding polyfill change to the spec change made in
#2261
pull bot pushed a commit to jamlee-t/v8 that referenced this pull request Aug 30, 2022
Disallow negative day lengths as round result

PR tc39/proposal-temporal#2261

Also fix the missing extraValues=<"day"> to GetTemporalUnit

Spec Text: https://tc39.es/proposal-temporal/#sec-temporal.zoneddatetime.prototype.round

Bug: v8:11544
Change-Id: Ibc963d5d93dde30f29df707ef3b3ecea99cd4a60
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3855704
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82798}
12wrigja pushed a commit to 12wrigja/temporal-polyfill that referenced this pull request Jan 14, 2023
This is the corresponding polyfill change to the spec change made in
tc39/proposal-temporal#2261

UPSTREAM_COMMIT=a1314bd0a47ecf2f41b85d5888a233d15e8a62a8
12wrigja pushed a commit to 12wrigja/temporal-polyfill that referenced this pull request Jan 18, 2023
This is the corresponding polyfill change to the spec change made in
tc39/proposal-temporal#2261

UPSTREAM_COMMIT=a1314bd0a47ecf2f41b85d5888a233d15e8a62a8
12wrigja pushed a commit to 12wrigja/temporal-polyfill that referenced this pull request Jan 18, 2023
This is the corresponding polyfill change to the spec change made in
tc39/proposal-temporal#2261

UPSTREAM_COMMIT=a1314bd0a47ecf2f41b85d5888a233d15e8a62a8
12wrigja pushed a commit to 12wrigja/temporal-polyfill that referenced this pull request Jan 19, 2023
This is the corresponding polyfill change to the spec change made in
tc39/proposal-temporal#2261

UPSTREAM_COMMIT=a1314bd0a47ecf2f41b85d5888a233d15e8a62a8
12wrigja pushed a commit to js-temporal/temporal-polyfill that referenced this pull request Jan 25, 2023
This is the corresponding polyfill change to the spec change made in
tc39/proposal-temporal#2261

UPSTREAM_COMMIT=a1314bd0a47ecf2f41b85d5888a233d15e8a62a8
@anba anba deleted the negative-day-length branch August 4, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants