`Date` refactor #3595

raskad · 2024-01-18T01:23:08Z

This pull request refactors the Date builtin to align it with the spec and enable all tests to pass.

The date operations by chrono are removed. This is necessary, because chrono only allows a limited date range from Jan 1, 262145 BCE to Dec 31, 262143 CE. The spec allows for dates from -271821-04-20T00:00:00.000Z to +275760-09-13T00:00:00.000Z. Instead most datetime specific operations are now performed by the functions that the spec specifies. The only thing that is done by the time create is the calculation of local timzone offsets. All other usages of chrono are also removed so we can remove the dependency.

github-actions · 2024-01-18T01:32:11Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	95,960	95,960	0
Passed	76,572	76,588	+16
Ignored	18,477	18,477	0
Failed	911	895	-16
Panics	0	0	0
Conformance	79.80%	79.81%	+0.02%

Fixed tests (16):

test/built-ins/Date/parse/time-value-maximum-range.js [strict mode] (previously Failed)
test/built-ins/Date/parse/time-value-maximum-range.js (previously Failed)
test/built-ins/Date/UTC/fp-evaluation-order.js [strict mode] (previously Failed)
test/built-ins/Date/UTC/fp-evaluation-order.js (previously Failed)
test/built-ins/Date/UTC/time-clip.js [strict mode] (previously Failed)
test/built-ins/Date/UTC/time-clip.js (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-11.js [strict mode] (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-11.js (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-9.js [strict mode] (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-9.js (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-10.js [strict mode] (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-10.js (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-12.js [strict mode] (previously Failed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-12.js (previously Failed)
test/annexB/built-ins/Date/prototype/setYear/time-clip.js [strict mode] (previously Failed)
test/annexB/built-ins/Date/prototype/setYear/time-clip.js (previously Failed)

codecov · 2024-01-18T01:32:25Z

Codecov Report

Attention: 190 lines in your changes are missing coverage. Please review.

Comparison is base (e1d9a80) 47.35% compared to head (a291e8f) 47.33%.
Report is 8 commits behind head on main.

Files	Patch %	Lines
core/engine/src/builtins/date/utils.rs	65.83%	96 Missing ⚠️
core/engine/src/builtins/date/mod.rs	75.26%	92 Missing ⚠️
core/engine/src/object/builtins/jsdate.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3595      +/-   ##
==========================================
- Coverage   47.35%   47.33%   -0.03%     
==========================================
  Files         475      475              
  Lines       45919    46138     +219     
==========================================
+ Hits        21746    21839      +93     
- Misses      24173    24299     +126

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nekevss

Was taking a look at this. A lot of it looks great! Still have a bit more to review but wanted to run a question by you.

I think some of the utils in this have shared logic in the boa_temporal/utils. Would it be better to share the logic from boa_temporal/utils where possible instead of implementing it twice.

EDIT: Just checked the spec, the idea is actually to unify these but the blocker is 1087, but we seem to be treating the values in most case as the mathematical anyways. So it would probably be best to unify them where possible.

nekevss · 2024-01-18T03:17:12Z

core/engine/src/builtins/date/utils.rs

+    // 7. Let mn be 𝔽(ℝ(m) modulo 12).
+    let mn = modulo(m, 12.0) as u8;
+
+    // 8. Find a finite time value t such that YearFromTime(t) is ym, MonthFromTime(t) is mn,


I think there's an outstanding issue on MakeDay here. Idk if it would be worth tracking it.

For reference

core/engine/src/builtins/date/utils.rs

raskad · 2024-01-18T16:46:22Z

I think some of the utils in this have shared logic in the boa_temporal/utils. Would it be better to share the logic from boa_temporal/utils where possible instead of implementing it twice.

EDIT: Just checked the spec, the idea is actually to unify these but the blocker is 1087, but we seem to be treating the values in most case as the mathematical anyways. So it would probably be best to unify them where possible.

My first idea was that we may just go with the different implementations for now, until things like 1087 are resolved or the issue of how these are handeled in the spec is resolved. That way the temporal code could potentially evolve if needed and we have no cross cutting effort. But you are way more up to date on how the temporal spec, if you say the spec will probably not change considerably, I'm all for sharing the logic.

One thing to keep in mind is maybe optimizations. My feeling is that there is at least some optimization potential in the Date logic. For example there are places where functions are called multiple times that could be avoided etc. I wanted to take a detailed look later on and I'm not sure how that would change the interoperability as some Date specific optimizations may require specific logic.

nekevss · 2024-01-18T17:13:25Z

We can keep it separate for now. I think some of this logic is fairly stable, so to speak, but there are still spec changes being made. I'd be surprised if the date equations changed much more, but airing on the cautious side doesn't hurt. Especially if there may be some places for optimizations as you said.

nekevss

Looks good to me overall. Nice work! There were a couple more things I came across, but nothing blocking.

core/engine/src/builtins/date/utils.rs

nekevss · 2024-01-19T03:34:35Z

core/engine/src/builtins/date/utils.rs

+    }
+
+    // 2. Let truncated be ! ToIntegerOrInfinity(year).
+    let truncated = IntegerOrInfinity::from(year);


thought: is IntegerOrInfinity needed in this instance if we've already accounted for NaN?

IIRC, IntegerOrInfinity also rounds to an integer, so I think it's pretty much needed here.

Yeah, I think it's just truncating.

So I was just wondering whether this step could just be year.trunc() in this particular instance since the truncating here casts and then recasts back to f64.

Hmmm that is true... IntegerOrInfinity is mostly used to convert from f64 to an integer, or handle the non-finite edge cases, but here it's only used for the rounding...

It can be left as is and merged. It was just something I saw when I was reviewing.

I think this could mostly be expressed as the below.

// Return non-finite cases if !year.is_finite() { return year } // truncate let trunc = year.trunc(); // Check if value is in range. if (0.0..=99.0).contains(&year) { return 1900.0 + year } year

I think I wanted to leave this for the optimizations later. For example in this case, every result of make_full_year gets passed to make_day afterwards. So there is definitley some potential to optimize this even across spec method boundaries.

jedel1043

Really like the refactor! It's a shame we cannot fully use integers because of the range of i64, but I think this is solved by Temporal in a much better way.

raskad added 2 commits January 17, 2024 23:39

Refactor date builtin

5b8aaaa

Remove chrono dependency

5d29fe2

raskad added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics waiting-on-review Waiting on reviews from the maintainers labels Jan 18, 2024

raskad added this to the v0.18.0 milestone Jan 18, 2024

raskad requested a review from a team January 18, 2024 01:23

nekevss reviewed Jan 18, 2024

View reviewed changes

nekevss requested a review from a team January 18, 2024 16:30

nekevss approved these changes Jan 19, 2024

View reviewed changes

nekevss requested a review from a team January 19, 2024 04:09

jedel1043 mentioned this pull request Jan 23, 2024

Bump chrono from 0.4.31 to 0.4.32 #3605

Closed

jedel1043 approved these changes Jan 23, 2024

View reviewed changes

Replace custom modulo with rem_euclid

a291e8f

raskad added this pull request to the merge queue Jan 23, 2024

Merged via the queue into main with commit 0ed7da5 Jan 23, 2024
14 checks passed

raskad deleted the date-refactor branch January 23, 2024 21:27

jedel1043 mentioned this pull request Jan 23, 2024

Run cargo update on fuzz crate #3607

Merged

jedel1043 mentioned this pull request Apr 12, 2024

Several Date tests panic on Windows #1570

Closed

jedel1043 removed the waiting-on-review Waiting on reviews from the maintainers label Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Date` refactor #3595

`Date` refactor #3595

raskad commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

nekevss left a comment •

edited

Loading

nekevss Jan 18, 2024

raskad commented Jan 18, 2024

nekevss commented Jan 18, 2024

nekevss left a comment •

edited

Loading

nekevss Jan 19, 2024

jedel1043 Jan 23, 2024

nekevss Jan 23, 2024 •

edited

Loading

jedel1043 Jan 23, 2024

nekevss Jan 23, 2024 •

edited

Loading

raskad Jan 23, 2024

jedel1043 left a comment

Date refactor #3595

Date refactor #3595

Conversation

raskad commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

Test262 conformance changes

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

nekevss left a comment • edited Loading

Choose a reason for hiding this comment

nekevss Jan 18, 2024

Choose a reason for hiding this comment

raskad commented Jan 18, 2024

nekevss commented Jan 18, 2024

nekevss left a comment • edited Loading

Choose a reason for hiding this comment

nekevss Jan 19, 2024

Choose a reason for hiding this comment

jedel1043 Jan 23, 2024

Choose a reason for hiding this comment

nekevss Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

jedel1043 Jan 23, 2024

Choose a reason for hiding this comment

nekevss Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

raskad Jan 23, 2024

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

`Date` refactor #3595

`Date` refactor #3595

codecov bot commented Jan 18, 2024 •

edited

Loading

nekevss left a comment •

edited

Loading

nekevss left a comment •

edited

Loading

nekevss Jan 23, 2024 •

edited

Loading

nekevss Jan 23, 2024 •

edited

Loading