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

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

Closed
wants to merge 7 commits into from
Closed

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Nov 20, 2022

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

@jedel1043 jedel1043 added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels Nov 20, 2022
@jedel1043 jedel1043 added this to the v0.17.0 milestone Nov 20, 2022
@github-actions
Copy link

github-actions bot commented Nov 20, 2022

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
Copy link

codecov bot commented Nov 20, 2022

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
Copy link

djc commented Nov 20, 2022

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
Copy link
Member Author

jedel1043 commented Nov 20, 2022

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 :)

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
boa_engine/src/builtins/date/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/date/mod.rs Show resolved Hide resolved
boa_engine/src/builtins/date/mod.rs Show resolved Hide resolved
boa_engine/src/builtins/date/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/mod.rs Outdated Show resolved Hide resolved
@jedel1043
Copy link
Member Author

... 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.

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks perfect to me !

@Razican
Copy link
Member

Razican commented Nov 22, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 22, 2022
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
Copy link

bors bot commented Nov 22, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Refactor the Date builtin [Merged by Bors] - Refactor the Date builtin Nov 22, 2022
@bors bors bot closed this Nov 22, 2022
@bors bors bot deleted the date-cleanup branch November 22, 2022 20:13
bors bot pushed a commit that referenced this pull request Nov 23, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants