[Merged by Bors] - Refactor the `Date` builtin by jedel1043 · Pull Request #2449 · boa-dev/boa

jedel1043 · 2022-11-20T22:46:20Z

Just a general cleanup of the Date builtin to use slightly better patterns and to fix our warnings about deprecated functions.

About the regressed tests. It seems to be a chrono bug, so I opened up an issue (chronotope/chrono#884) for it and they've already opened a PR fixing it (chronotope/chrono#885).

However, while checking out the remaining failing tests, I realized there's a more fundamental limitation with the library. Currently, chrono specifies:

Date types are limited in about +/- 262,000 years from the common epoch.

While the ECMAScript spec says:

The smaller range supported by a time value as specified in this section is approximately -273,790 to 273,790 years relative to 1970.

The range allowed by the spec is barely outside of the range supported by chrono! This is why the remaining Date tests fail.
Seeing that, I would like to ping @djc and @esheppa (the maintainers of chrono) to ask if it would be feasible to add a feature, akin to the large-dates feature from the time crate, that expands the supported range of chrono.

EDIT: Filed chronotope/chrono#886

github-actions · 2022-11-20T22:53:23Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	93,831	93,831	0
Passed	69,620	69,626	+6
Ignored	18,472	18,472	0
Failed	5,739	5,733	-6
Panics	0	0	0
Conformance	74.20%	74.20%	+0.01%

Fixed tests (10):

test/harness/assertRelativeDateMs.js [strict mode] (previously Failed)
test/harness/assertRelativeDateMs.js (previously Failed)
test/built-ins/Date/prototype/valueOf/S9.4_A3_T1.js [strict mode] (previously Failed)
test/built-ins/Date/prototype/valueOf/S9.4_A3_T1.js (previously Failed)
test/built-ins/Date/UTC/non-integer-values.js [strict mode] (previously Failed)
test/built-ins/Date/UTC/non-integer-values.js (previously Failed)
test/built-ins/Date/UTC/overflow-make-day.js [strict mode] (previously Failed)
test/built-ins/Date/UTC/overflow-make-day.js (previously Failed)
test/built-ins/Date/UTC/overflow-make-time.js [strict mode] (previously Failed)
test/built-ins/Date/UTC/overflow-make-time.js (previously Failed)

Broken tests (4):

test/annexB/built-ins/Date/prototype/setYear/year-number-absolute.js [strict mode] (previously Passed)
test/annexB/built-ins/Date/prototype/setYear/year-number-absolute.js (previously Passed)
test/annexB/built-ins/Date/prototype/getYear/return-value.js [strict mode] (previously Passed)
test/annexB/built-ins/Date/prototype/getYear/return-value.js (previously Passed)

codecov · 2022-11-20T22:58:03Z

Codecov Report

Merging #2449 (8a4dc5c) into main (5125da0) will increase coverage by 0.13%.
The diff coverage is 70.12%.

@@            Coverage Diff             @@
##             main    #2449      +/-   ##
==========================================
+ Coverage   52.39%   52.52%   +0.13%     
==========================================
  Files         330      329       -1     
  Lines       35041    34793     -248     
==========================================
- Hits        18359    18276      -83     
+ Misses      16682    16517     -165

Impacted Files	Coverage Δ
boa_engine/src/builtins/date/mod.rs	`90.66% <ø> (-0.94%)`	⬇️
boa_engine/src/builtins/weak/weak_ref.rs	`73.80% <ø> (ø)`
boa_engine/src/context/mod.rs	`65.50% <ø> (ø)`
boa_engine/src/object/builtins/jsdate.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/mod.rs	`47.97% <ø> (ø)`
boa_engine/src/object/mod.rs	`46.17% <71.42%> (-0.25%)`	⬇️
boa_engine/src/value/mod.rs	`65.32% <83.33%> (+0.24%)`	⬆️
boa_engine/src/builtins/date/utils.rs	`98.79% <98.79%> (ø)`
boa_engine/src/builtins/set/mod.rs	`73.65% <100.00%> (+3.23%)`	⬆️
boa_engine/src/builtins/set/ordered_set.rs	`64.28% <100.00%> (+2.74%)`	⬆️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

djc · 2022-11-20T23:13:18Z

Please file an issue against the chrono project instead of requesting a feature in a PR to your own project.

(Adding such a feature could make sense, I suppose.)

jedel1043 · 2022-11-20T23:17:49Z

Please file an issue against the chrono project instead of requesting a feature in a PR to your own project.

(Adding such a feature could make sense, I suppose.)

Thanks for the quick response! Just wanted to confirm with you before opening an issue, mainly to avoid polluting the issue tracker with unfeasible feature requests :)

Razican

Looks pretty good! Thanks :) I added some comments in places where I saw the code could be enhanced. It's an interesting usage of the const generics, btw! maybe this needs a minimum supported rust version bump?

boa_engine/src/builtins/set/ordered_set.rs

boa_engine/src/builtins/date/mod.rs

boa_engine/src/vm/mod.rs

jedel1043 · 2022-11-22T06:38:58Z

... maybe this needs a minimum supported rust version bump?

We set rust-version to 1.65 on the workspace Cargo.toml, so we're already targeting the latest rustc.

HalidOdat

Looks perfect to me !

Razican · 2022-11-22T20:03:45Z

bors r+

@djc

Just a general cleanup of the `Date` builtin to use slightly better patterns and to fix our warnings about deprecated functions. About the regressed tests. It seems to be a `chrono` bug, so I opened up an issue (chronotope/chrono#884) for it and they've already opened a PR fixing it (chronotope/chrono#885). However, while checking out the remaining failing tests, I realized there's a more fundamental limitation with the library. Currently, [`chrono`](https://github.com/chronotope/chrono) specifies: > Date types are limited in about +/- 262,000 years from the common epoch. While the [ECMAScript spec](https://tc39.es/ecma262/#sec-time-values-and-time-range) says: > The smaller range supported by a time value as specified in this section is approximately -273,790 to 273,790 years relative to 1970. The range allowed by the spec is barely outside of the range supported by `chrono`! This is why the remaining `Date` tests fail. Seeing that, I would like to ping @djc and @esheppa (the maintainers of `chrono`) to ask if it would be feasible to add a feature, akin to the `large-dates` feature from the `time` crate, that expands the supported range of `chrono`. EDIT: Filed chronotope/chrono#886

bors · 2022-11-22T20:13:36Z

Pull request successfully merged into main.

Build succeeded:

This Pull Request restructures the lint deny/warn/allow lists in `boa_engine`. It adds a lot of documentation to pup functions. There are still a few clippy lints that are not fixed, mainly regarding casting of number types. Fixing those lints effectiveley would in some cases probably require bigger refactors. This should probably wait for #2449 to be merged, because that PR already fixes that lints regarding the `Date` built-in.

jedel1043 added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels Nov 20, 2022

jedel1043 added this to the v0.17.0 milestone Nov 20, 2022

jedel1043 requested review from HalidOdat, RageKnify, Razican, jasonwilliams and raskad November 20, 2022 22:46

jedel1043 mentioned this pull request Nov 20, 2022

Deny warnings and add miri to CI #2448

Closed

Razican approved these changes Nov 21, 2022

View reviewed changes

raskad mentioned this pull request Nov 22, 2022

[Merged by Bors] - Restructure lint lists in boa_engine #2455

Closed

jedel1043 added 5 commits November 22, 2022 00:29

Refactor the Date builtin

4dfa7d3

Fix clippy

c345d27

Fix tests

1204103

Fix doc

fc791e1

Apply review

658d685

jedel1043 force-pushed the date-cleanup branch from 6db29c1 to 658d685 Compare November 22, 2022 06:29

Move inline tags

2a4e2e0

Use const generics for getters

8a4dc5c

HalidOdat approved these changes Nov 22, 2022

View reviewed changes

bors bot changed the title ~~Refactor the Date builtin~~ [Merged by Bors] - Refactor the Date builtin Nov 22, 2022

bors bot closed this Nov 22, 2022

bors bot deleted the date-cleanup branch November 22, 2022 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Merged by Bors] - Refactor the `Date` builtin#2449

[Merged by Bors] - Refactor the `Date` builtin#2449
jedel1043 wants to merge 7 commits intomainfrom
date-cleanup

jedel1043 commented Nov 20, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Nov 20, 2022 •

edited

Loading

Uh oh!

codecov bot commented Nov 20, 2022 •

edited

Loading

Uh oh!

djc commented Nov 20, 2022

Uh oh!

jedel1043 commented Nov 20, 2022 •

edited

Loading

Uh oh!

Razican left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jedel1043 commented Nov 22, 2022

Uh oh!

HalidOdat left a comment

Uh oh!

Razican commented Nov 22, 2022

Uh oh!

bors bot commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jedel1043 commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test262 conformance changes

Uh oh!

codecov bot commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djc commented Nov 20, 2022

Uh oh!

jedel1043 commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Razican left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jedel1043 commented Nov 22, 2022

Uh oh!

HalidOdat left a comment

Choose a reason for hiding this comment

Uh oh!

Razican commented Nov 22, 2022

Uh oh!

bors bot commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jedel1043 commented Nov 20, 2022 •

edited

Loading

github-actions bot commented Nov 20, 2022 •

edited

Loading

codecov bot commented Nov 20, 2022 •

edited

Loading

jedel1043 commented Nov 20, 2022 •

edited

Loading