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: Fix Instant rounding modes #2210

Merged
merged 1 commit into from Jun 14, 2022
Merged

Normative: Fix Instant rounding modes #2210

merged 1 commit into from Jun 14, 2022

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented May 19, 2022

As described in the documentation, the proposal champions' intention was
for rounding of exact times to consider the Big Bang as the "zero" point,
rather than the arbitrary zero point of the Unix epoch. The spec text
didn't reflect this correctly.

Closes: #2191

@ptomato ptomato requested a review from Ms2ger May 19, 2022 19:17
@ptomato ptomato marked this pull request as draft May 19, 2022 19:17
@ptomato
Copy link
Collaborator Author

ptomato commented May 19, 2022

Draft until presented at TC39.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #2210 (aa6a5c2) into main (8f90398) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2210   +/-   ##
=======================================
  Coverage   89.95%   89.95%           
=======================================
  Files          19       19           
  Lines       10901    10901           
  Branches     1683     1683           
=======================================
  Hits         9806     9806           
  Misses       1081     1081           
  Partials       14       14           
Flag Coverage Δ
test262 83.47% <ø> (ø)
tests 80.46% <ø> (ø)

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 8f90398...aa6a5c2. Read the comment docs.

spec/abstractops.html Outdated Show resolved Hide resolved
As described in the documentation, the proposal champions' intention was
for rounding of exact times to consider the Big Bang as the "zero" point,
rather than the arbitrary zero point of the Unix epoch. The spec text
didn't reflect this correctly.
@rkirsling
Copy link
Member

Thanks, makes sense to me!

@ptomato ptomato marked this pull request as ready for review June 14, 2022 14:35
@ptomato
Copy link
Collaborator Author

ptomato commented Jun 14, 2022

This reached consensus at the July 2022 TC39 plenary meeting.

@ptomato ptomato merged commit 0993b75 into main Jun 14, 2022
@ptomato ptomato deleted the 2191-round-big-bang branch June 14, 2022 14:36
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Jun 15, 2022
… as zero

https://bugs.webkit.org/show_bug.cgi?id=241622

Reviewed by Yusuke Suzuki.

Implement the spec fix of tc39/proposal-temporal#2210.
This change is quite simple, as we can just update the Int128 version of roundNumberToIncrement.

* JSTests/test262/expectations.yaml:
Mark four test cases as passing.

* Source/JavaScriptCore/runtime/TemporalObject.cpp:
(JSC::roundNumberToIncrement):
(JSC::abs): Deleted.

Canonical link: https://commits.webkit.org/251556@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295551 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@anba
Copy link
Contributor

anba commented Jun 15, 2022

This change invalidated some other tests, for example https://github.com/tc39/test262/blob/main/test/built-ins/Temporal/ZonedDateTime/prototype/since/roundingincrement-non-integer.js:

DifferenceTemporalZonedDateTime calls DifferenceInstant with ns1 = 1_000_000_000_000_000_005n and ns2 = 1_000_000_000_000_000_000n. DifferenceInstant then calls RoundTemporalInstant with ns2 - ns1 = -5n and unit = "nanosecond". RoundNumberToIncrementAsIfPositive is then called with x = -5, increment = 2, and roundingMode = "trunc".

RoundNumberToIncrementAsIfPositive computes:

  1. Let quotient be x / increment.
  2. Let unsignedRoundingMode be GetUnsignedRoundingMode(roundingMode, false).
  3. Let r1 be the largest integer such that r1 ≤ quotient.
  4. Let r2 be the smallest integer such that r2 > quotient.
  5. Let rounded be ApplyUnsignedRoundingMode(quotient, r1, r2, unsignedRoundingMode).
  6. Return rounded × increment.

  1. quotient = -5 / 2 = -2.5
  2. unsignedRoundingMode = zero
  3. r1 = floor(quotient) = -3
  4. r2 = floor(quotient) + 1 = -2
  5. rounded = r1 = -3
  6. return = -3 * 2 = -6

With ApplyUnsignedRoundingMode:

  1. If x is equal to r1, return r1.
  2. Assert: r1 < x < r2.
  3. Assert: unsignedRoundingMode is not undefined.
  4. If unsignedRoundingMode is zero, return r1.

with x = -2.5, r1 = -3, r2 = -2

  1. Not taken
  2. ...
  3. ...
  4. return r1

So the test needs to be changed as follows:

- TemporalHelpers.assertDuration(result, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, "roundingIncrement 2.5 floors to 2");
+ TemporalHelpers.assertDuration(result, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, "roundingIncrement 2.5 floors to 2");

But I'm not sure this change was intentional by this PR.

@ptomato
Copy link
Collaborator Author

ptomato commented Jun 20, 2022

Whoops. I don't think it was ever correct to call RoundTemporalInstant on the difference between two instants, which should be a duration. I'll open a new issue for this.

@FrankYFTang
Copy link
Contributor

After I try to change my v8 code to sycn with 2210 my build break the following tests, I am not 100% sure that is because the same issue @anba mentioned or I have some bugs in my patch or some other pre-existing issues but I would like to mention these tests here so you will consider them when you work on #2320

  'built-ins/Temporal/Instant/prototype/since/roundingincrement-non-integer': [FAIL],
  'built-ins/Temporal/Instant/prototype/since/roundingincrement-wrong-type': [FAIL],
  'built-ins/Temporal/Instant/prototype/since/roundingmode-undefined': [FAIL],
  'built-ins/Temporal/Instant/prototype/since/roundingmode-wrong-type': [FAIL],
  'built-ins/Temporal/Instant/prototype/since/smallestunit-wrong-type': [FAIL],
  'built-ins/Temporal/ZonedDateTime/prototype/since/roundingincrement-non-integer': [FAIL],
  'built-ins/Temporal/ZonedDateTime/prototype/since/roundingincrement-wrong-type': [FAIL],
  'built-ins/Temporal/ZonedDateTime/prototype/since/roundingmode-undefined': [FAIL],
  'built-ins/Temporal/ZonedDateTime/prototype/since/roundingmode-wrong-type': [FAIL],
  'built-ins/Temporal/ZonedDateTime/prototype/since/smallestunit-wrong-type': [FAIL],
  'staging/Temporal/Instant/old/since': [FAIL],
  'staging/Temporal/Instant/old/until': [FAIL],

@FrankYFTang
Copy link
Contributor

@anba - do you see the same set of breakage or different?

@anba
Copy link
Contributor

anba commented Aug 29, 2022

@anba - do you see the same set of breakage or different?

Yes, I see exactly the same failures.

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 this pull request may close these issues.

Temporal.Instant.p.round is spec'ed to round signed nanoseconds, contrary to tests
7 participants