First portion of the Temporal implementation #3277

nekevss · 2023-09-14T04:54:12Z

This PR is to propose the work completed so far on the Temporal branch for initial review and merging. The changes have become quite large (as visible via the diff 😅), but I think just about most of the building blocks are just about finished and/or close to being finished.

It changes the following:

Adds the general structure many of the Temporal built-ins to varying levels Calendar, Duration, Instant, PlainDate, PlainDateTime, PlainYearMonth, and PlainMonthDay saw the most attention. PlainTime, TimeZone, and ZonedDateTime still need a decent amount of work.
Adds parsing and very basic Parse Nodes for ISO8601 grammar.

There's still a decent amount more work to be done, but I think this is close to being a good starting point.

General design decisions that were made while working on this: the parser for iso8601 is currently separate from the Lexer and Parser for JavaScript, due to some grammar incompatibility that came up with the lexer. There is also a BuiltinCalendar trait that's meant to provide anyone who wants to implement any custom calendar. There are a few internal structs that are implemented with the idea of trying to prevent passing around entire JsObjects in favor of the native structs wherever possible. It also turned out that these structs were typically the target of the abstract operations too, so some abstract operations are implemented on the structs (I tried to leave a note in the abstract operations section when possible pointing to where the abstract operation is implemented).

There's probably a lot more I could say, but I think I'm going to leave it at this. 😆 Let me know what you think!

There are a few more things that I'd like to have completed on this before merging (although it's more of a wish list rather than necessary):

Basic Instant and Duration ISO8601 parsing (along with the Parse Nodes).
Clean up the Parse Nodes themselves (They're very stripped down at current).
A completed iso8601 Calendar implementation.

Footnote: I didn't remove Temporal from the test_ignore due to how fragmented the current progress is and the potential for false positives at this stage, but if anyone thinks it should be removed I definitely can.

Edit: Marking calendar complete but it's more of a "complete as can currently be for now".

boa_ast/Cargo.toml

codecov · 2023-09-14T05:25:31Z

Codecov Report

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

Comparison is base (c56a706) 49.56% compared to head (26f6334) 47.09%.
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3277      +/-   ##
==========================================
- Coverage   49.56%   47.09%   -2.47%     
==========================================
  Files         446      474      +28     
  Lines       43716    48737    +5021     
==========================================
+ Hits        21666    22955    +1289     
- Misses      22050    25782    +3732

Files	Coverage Δ
boa_ast/src/lib.rs	`75.00% <ø> (ø)`
boa_ast/src/temporal/mod.rs	`100.00% <100.00%> (ø)`
boa_engine/src/builtins/date/mod.rs	`92.46% <ø> (ø)`
boa_engine/src/builtins/date/utils.rs	`97.26% <100.00%> (ø)`
boa_engine/src/builtins/mod.rs	`92.70% <100.00%> (+0.20%)`	⬆️
boa_engine/src/context/intrinsics.rs	`97.54% <100.00%> (+0.26%)`	⬆️
boa_engine/src/string/common.rs	`48.48% <ø> (ø)`
boa_parser/src/error/mod.rs	`100.00% <ø> (ø)`
...a_parser/src/parser/expression/assignment/yield.rs	`33.92% <ø> (ø)`
boa_parser/src/parser/mod.rs	`42.74% <ø> (ø)`
... and 31 more

... and 41 files with indirect coverage changes

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

jedel1043

Just a preliminary review before checking the rest of the changes, but really nice work implementing all of this!

.gitmodules

boa_engine/src/object/mod.rs

justingrant · 2023-09-14T11:44:29Z

Congratulations! Temporal has a huge surface area, and seeing so much completed already in this PR is really nice to see.

I'm one of the Temporal proposal's champions. We're eager to help implementers be successful. If you have any questions about the spec, or if there's anything we can help with, then feel free to drop an issue in the @tc39/proposal-temporal repo and we'll respond quickly. Thanks again!

Are you collaborating with the folks building ICU4X? (the Rust port of ICU4C the library that powers the calendar and time zone features of V8, JSC, and SpiderMonkey)

nekevss · 2023-09-14T16:37:03Z

seeing so much completed already in this PR is really nice to see.

Thanks! Although, I think there is still more work to be done.

Are you collaborating with the folks building ICU4X?

Not currently, and that's actually probably one of the more debatable aspects/areas to maybe improve on the current progress in this PR. Initially, my intended approach with the BuiltinCalendar trait was to wrap icu_calendar. I initially tried using ICU4X's ISO calendar, but it doesn't really provide a good way to handle the proposal's overflow options that I was seeing besides just throwing an error on overflow, so I figured the best approach was to at least have specification ISO implementation.

jedel1043 · 2023-09-14T18:33:23Z

the parser for iso8601 is currently separate from the Lexer and Parser for JavaScript, due to some grammar incompatibility that came up with the lexer.

I think this is okay. ICU4X will provide a parser in the future, and if they keep doing Temporal compatibility work, we'll eventually be able to replace all types with ICU4X's. So it is good to separate that to avoid integrating it too tightly to the JS parser.

sffc · 2023-09-14T20:13:35Z

It is very much in the design of ICU4X to see the calendar-specific algorithms, including the ones for the ISO calendar, and flags like "overflow" and "constrain", land in icu_calendar.

ICU4X has support for all CLDR calendars, with a focus on conversion to/from ISO-8601. There is a partial implementation of date arithmetic (add/subtract/since/until) but it needs some polishing from someone who understands the Temporal spec.

I'd be very happy to mentor in this area, so that Boa can just pull in the ICU4X code for this.

nekevss · 2023-09-14T20:35:44Z

It is very much in the design of ICU4X to see the calendar-specific algorithms, including the ones for the ISO calendar, and flags like "overflow" and "constrain", land in icu_calendar.

That's great to hear! I actually thought of something that might work earlier when I was writing my reply to switch the ISO calendar implementation to ICU4X. I'm going to try and put it together later while making some of the other changes 😄.

hsivonen · 2023-09-15T12:08:59Z

There is a partial implementation of date arithmetic (add/subtract/since/until) but it needs some polishing from someone who understands the Temporal spec.

The ICU4X issue for providing public methods for these operations

jedel1043

Just a batch of reviews that I wanted to put out to be able to merge them. Mostly doc fixes.

boa_engine/src/builtins/mod.rs

boa_engine/src/context/intrinsics.rs

boa_engine/src/builtins/temporal/mod.rs

nekevss

Just wanted to note some things I've noticed while going through this myself and plan to address 😄.

boa_parser/src/parser/mod.rs

boa_engine/src/builtins/temporal/plain_year_month/mod.rs

github-actions · 2023-09-21T23:52:00Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	95,574	95,574	0
Passed	75,341	75,341	0
Ignored	19,482	19,482	0
Failed	751	751	0
Panics	0	0	0
Conformance	78.83%	78.83%	0.00%

jedel1043

Did a review pass on the parser code, and I have some suggestions.

boa_parser/src/temporal/mod.rs

boa_ast/src/temporal/mod.rs

jasonwilliams

This looks really, good, still a fair few bits to iron out but its a great starting point.

I'm happy for this to go in (bar any comments made)

jasonwilliams · 2023-09-26T20:58:26Z

boa_ast/src/temporal/mod.rs

+/// A full precision `UtcOffset`
+#[derive(Debug, Clone, Copy)]
+#[allow(dead_code)]
+pub struct UtcOffset {


I think you can capitalize UTC to match up with the specification: https://tc39.es/proposal-temporal/#prod-UTCOffsetMinutePrecision

jasonwilliams · 2023-09-26T21:03:52Z

boa_engine/src/builtins/date/mod.rs

@@ -7,7 +7,7 @@
 //! [spec]: https://tc39.es/ecma262/#sec-date-objects
 //! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

-mod utils;
+pub(crate) mod utils;


Why was the visibility of utils changed? Whats now using it?

14.8.1 GetUTCEpochNanoseconds uses the preexisting MakeDay and MakeTime.

jasonwilliams · 2023-09-26T21:11:34Z

boa_engine/src/builtins/temporal/fields.rs

+    }
+}
+
+fn month_code_to_integer(mc: &JsString) -> JsResult<i32> {


why i32? wouldn't a u8 fit this? I don't think theres any negative months and shouldn't be any higher than 13

Ah I guess this is returning a JS integer https://tc39.es/ecma262/#sec-tointegerorinfinity, I wonder if this is ever observable?

In the long run, month-day fields should probably be eventually optimized down to a u8 according to any overflow option, but I think that's more so on IsoDateRecord. TemporalFields is meant to be more of a Rust native representation of the fields object, so the fields mostly align with the conversion value.

jasonwilliams · 2023-09-26T21:22:21Z

boa_engine/src/builtins/temporal/fields.rs

+    }
+
+    /// Resolve the month and monthCode on this `TemporalFields`.
+    pub(crate) fn resolve_month(&mut self) -> JsResult<()> {


Should this be ISO_resolve_month? https://tc39.es/proposal-temporal/#sec-temporal-isoresolvemonth

boa_engine/src/builtins/temporal/duration/record.rs

boa_engine/src/builtins/temporal/duration/mod.rs

jedel1043

Another batch of reviews. I finished reviewing the parser changes and everything looks great!

boa_engine/src/object/mod.rs

jedel1043 · 2023-10-03T08:29:35Z

boa_engine/src/builtins/temporal/options.rs

+    Millisecond,
+    Microsecond,
+    Nanosecond,
+    Auto,


I think it would be preferable to have Auto be a variant of a new generic enum instead of part of the temporal unit. It's just a transitional variant anyways.

I'll take a look at it again. From what I remember, I wasn't initially wanting to add Auto either, but had ran into something that felt like it needed it. But it could probably also be refactored out.

I was looking at the spec again, and Auto is a valid option value that can be provided for Durations round method options, so I'm not entirely sure that it can be totally separated from the enum.

I think it can, with something like:

enum OptionOrAuto<T> { Some(T), Auto } impl<T: FromStr> FromStr for OptionOrAuto<T> { type Err = Error; fn parse(value: &str) -> Result<Self, Self::Err> { if value == "auto" { return Ok(Self::Auto); } Ok(Self::Some(<T>::parse(value)?)) } }

With that definition, we could add something like OptionOrAuto::resolve(t:T) to resolve Auto to a specific value of T if needed.

Auto is technically considered a valid extra value in 3 circumstances in the proposal, Duration.prototype.round, Calendar.prototype.dateUntil, and GetDifferenceSettings. dateUntil does provide a T (Day). But the method that throws a wrench is round where Auto is used to designate the use of the computed defaultLargestUnit value where largestUnitPresent remains true

We would potentially be returning Option<OptionOrAuto<T>> where None, Auto, or T may all be a valid value.

Maybe it looks that way thanks to the repetition of Option. Something like Option<AutoValue<T>>?

We faced similar challenges in defining the TypeScript types for the Temporal API. In case it's helpful, here's the TS types we ended up with. Each of these types defines valid inputs for units of various Temporal methods.

export type LargestUnit<T extends DateTimeUnit> = 'auto' | T | PluralUnit<T>; export type SmallestUnit<T extends DateTimeUnit> = T | PluralUnit<T>; export type TotalUnit<T extends DateTimeUnit> = T | PluralUnit<T>;

BTW, full set of TS types are here: https://github.com/tc39/proposal-temporal/blob/main/polyfill/index.d.ts

@justingrant I've been using that polyfill as a reference! It was super useful for setting up the options initially / finally visualizing all the options 😄 (it was also linked above too). The current options were more or less pulled from that, although I tried to simplify AssignmentOptions and ArithmeticOptions to ArithmeticOverflow (might not be the best name).

boa_engine/src/builtins/temporal/options.rs

boa_engine/src/builtins/temporal/duration/record.rs

Razican

Impressive work! I just had the chance to review part of it and add some comments.

boa_engine/src/bigint.rs

Razican · 2023-10-03T16:32:54Z

boa_ast/Cargo.toml

@@ -13,6 +13,7 @@ rust-version.workspace = true
 [features]
 serde = ["dep:serde", "boa_interner/serde", "bitflags/serde"]
 arbitrary = ["dep:arbitrary", "boa_interner/arbitrary", "num-bigint/arbitrary"]
+experimental = []


In other cases, we have used more specific feature names (such as intl). Should we just name the feature temporal?

We opted for this approach because temporal would be removed anyways when it reaches stage 4. Do other engines use a broad experimental flag for Temporal, or a specific temporal flag?

Makes sense... but going forward, we might want to have features specific to some JavaScript characteristics (maybe a feature per built-in, things like that), so it might not hurt to have a temporal feature, and experimental would just depend on temporal. Could make it easier to support other experimental features in the future.

That makes sense, since it allows users to reduce binary size if they need to.

Should the feature flag update be included in this PR or a different PR?

Can be done separately :)

Razican · 2023-10-03T16:35:36Z

boa_engine/src/builtins/temporal/calendar/iso.rs

+    Calendar, Date,
+};
+
+pub(crate) struct IsoCalendar;


In some cases we use the ISOWhatever naming convention, and in other cases we use IsoWhatever. We should probably decide on one of them.

I think that Rust favors IsoWhatever.

I think the ISOWhatever naming convention is relatively newer and potentially isolated just to the AST nodes. The rust notation (IsoWhatever) is probably better, so I'll switch them.

boa_engine/src/builtins/temporal/date_equations.rs

Razican · 2023-10-04T15:28:09Z

boa_engine/src/builtins/temporal/mod.rs

+    format!("{n:0min_length$}")
+}
+
+// TODO: 13.1 `IteratorToListOfType`


Is this still to-do? Seems pretty good :)

Might need some docs, though

Same for others in this file

boa_engine/src/builtins/temporal/plain_date_time/mod.rs

jedel1043 · 2023-10-06T03:14:11Z

boa_engine/src/builtins/temporal/calendar/mod.rs

+mod iso;
+pub(crate) mod utils;
+
+#[cfg(feature = "experimental")]


Does this need an experimental gate? The module is already gated behind the feature.

Probably not.

The editor I use is vscode. And if I use the builtin run test functionality, it won't run the tests without the configuration flag on the test module itself.

jedel1043 · 2023-10-06T03:42:29Z

boa_engine/src/builtins/temporal/date_equations.rs

+    match month {
+        0 => day_in_year + 1,
+        1 => day_in_year - 30,
+        2 => day_in_year - 59 - in_leap_year,


Suggested change

2 => day_in_year - 59 - in_leap_year,

2 => day_in_year - 58 - in_leap_year,

Also this could also be an array of offsets, then we subtract in_leap_year if month >=2

jedel1043 · 2023-10-11T23:22:00Z

I think this is my last batch of reviews. There may be some patterns that can be improved, but I think we can iterate instead of trying to perfect this PR.

Just resolve my previous comments and we can merge this :)

nekevss · 2023-10-12T01:08:47Z

Linking #1804.

Would it be worth while editing the original issue to make it more of a tracking issue in order to keep track of who's working on what. I plan to continue working on this, but just in case anyone else was interested in working on a part of the API, it could be useful.

Or would it be better to start a new issue altogether for tracking.

jedel1043

I cannot imagine the massive work that was put onto understanding, designing, iterating and developing 13k lines of code for such an important feature. Really impressive work!

jedel1043 · 2023-10-12T01:36:50Z

Or would it be better to start a new issue altogether for tracking.

We can rename that issue and do a checklist with the missing APIs. You're probably the best person to list what's still missing, so I'll leave that task to you.

* Started with the Temporal implementation * Implemented some useful functions * Updaating some spec references * Initial work on TimeZone and Instant * More work completed on Temporal.Duration and Temporal.Instant * General scaffolding and heavy work on Instant and Duration complete * ZonedDateTime and Calendar started with further work on duration abstract ops * Further work on temporal work and clippy fixes * Post rebase fixes/reverts * Add BuiltinCalendar and begin IsoCalendar impl * More work completed on calendar/date/yearmonth/monthday * Calendar and iso impl close to completion - no datelike parsing * Initial work on temporal ISO8601 parsing and grammar * Post rebase fixes and updates * More on parser/Duration and work through clippy lints * Fix bug on peek_n and add temporal cfg * Fix clippy lints on parser tests * Build out calendar with icu_calendar, add some tests, and misc. * Fix spec hyperlinks * Parser clean up and invalid annotations * Add Duration and Temporal Parsing * Remove IsoYearMonthRecord * Post rebase update * Fix and add to ISO Parser docs * Parser/ast cleanup and duration refactor/additions * Review feedback, options update, and duration changes * Review changes, general cleanup, and post rebase fixes * Fix time zone parsing issue/test logic * Clean up parse output nodes * Apply review feedback and various fixes * Review feedback and get_option changes * Review feedback --------- Co-authored-by: Iban Eguia Moraza <[email protected]> Co-authored-by: José Julián Espina <[email protected]>

nekevss requested a review from a team September 14, 2023 05:13

nekevss added the builtins PRs and Issues related to builtins/intrinsics label Sep 14, 2023

jedel1043 reviewed Sep 14, 2023

View reviewed changes

boa_ast/Cargo.toml Outdated Show resolved Hide resolved

jedel1043 reviewed Sep 14, 2023

View reviewed changes

.gitmodules Outdated Show resolved Hide resolved

boa_engine/src/object/mod.rs Outdated Show resolved Hide resolved

jedel1043 requested changes Sep 19, 2023

View reviewed changes

jedel1043 added the enhancement New feature or request label Sep 19, 2023

nekevss commented Sep 19, 2023

View reviewed changes

boa_parser/src/parser/mod.rs Outdated Show resolved Hide resolved

boa_engine/src/builtins/temporal/plain_year_month/mod.rs Outdated Show resolved Hide resolved

nekevss force-pushed the temporal branch 2 times, most recently from 99aafaa to 88f6cee Compare September 21, 2023 23:49

nekevss force-pushed the temporal branch 3 times, most recently from 3f0b666 to 225eb4a Compare September 22, 2023 01:02

jedel1043 requested changes Sep 27, 2023

View reviewed changes

boa_parser/src/temporal/mod.rs Show resolved Hide resolved

boa_ast/src/temporal/mod.rs Outdated Show resolved Hide resolved

jedel1043 mentioned this pull request Sep 27, 2023

Introduce experimental features #3318

Merged

jasonwilliams approved these changes Sep 27, 2023

View reviewed changes

nekevss force-pushed the temporal branch from 225eb4a to 05d4bfc Compare September 30, 2023 00:33

nekevss requested review from jedel1043 and a team September 30, 2023 02:06

jedel1043 requested changes Oct 3, 2023

View reviewed changes

Razican reviewed Oct 3, 2023

View reviewed changes

jedel1043 reviewed Oct 4, 2023

View reviewed changes

boa_engine/src/builtins/temporal/date_equations.rs Outdated Show resolved Hide resolved

Razican reviewed Oct 4, 2023

View reviewed changes

nekevss added 10 commits October 5, 2023 15:09

Add Duration and Temporal Parsing

94cea39

Remove IsoYearMonthRecord

1b39512

Post rebase update

2e692ab

Fix and add to ISO Parser docs

aecf583

Parser/ast cleanup and duration refactor/additions

2762c03

Review feedback, options update, and duration changes

b5a6e4e

Review changes, general cleanup, and post rebase fixes

6eb3b15

Fix time zone parsing issue/test logic

9db8b47

Clean up parse output nodes

b52631b

Apply review feedback and various fixes

dc311e6

nekevss force-pushed the temporal branch 3 times, most recently from 1d6bf18 to ddc20e0 Compare October 5, 2023 21:22

Review feedback and get_option changes

c127bb1

nekevss force-pushed the temporal branch from ddc20e0 to c127bb1 Compare October 10, 2023 23:18

nekevss requested a review from jedel1043 October 11, 2023 22:58

jedel1043 reviewed Oct 11, 2023

View reviewed changes

Review feedback

26f6334

jedel1043 reviewed Oct 12, 2023

View reviewed changes

jedel1043 approved these changes Oct 12, 2023

View reviewed changes

jedel1043 mentioned this pull request Oct 12, 2023

Tracking issue for the Temporal builtin #1804

Open

12 tasks

jedel1043 added this pull request to the merge queue Oct 12, 2023

Merged via the queue into main with commit d281988 Oct 12, 2023
14 checks passed

jedel1043 deleted the temporal branch October 12, 2023 02:14

nekevss mentioned this pull request Oct 12, 2023

Update feature flags to specific feature flag #3376

Merged

	2 => day_in_year - 59 - in_leap_year,
	2 => day_in_year - 58 - in_leap_year,

First portion of the Temporal implementation #3277

First portion of the Temporal implementation #3277

Conversation

nekevss commented Sep 14, 2023 • edited Loading

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

jedel1043 left a comment

Choose a reason for hiding this comment

justingrant commented Sep 14, 2023

nekevss commented Sep 14, 2023

jedel1043 commented Sep 14, 2023

sffc commented Sep 14, 2023

nekevss commented Sep 14, 2023

hsivonen commented Sep 15, 2023

jedel1043 left a comment

Choose a reason for hiding this comment

nekevss left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2023 • edited Loading

Test262 conformance changes

jedel1043 left a comment

Choose a reason for hiding this comment

jasonwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Razican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

jedel1043 commented Oct 11, 2023

nekevss commented Oct 12, 2023

jedel1043 left a comment • edited Loading

Choose a reason for hiding this comment

jedel1043 commented Oct 12, 2023

nekevss commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited

Loading

jedel1043 Oct 4, 2023 •

edited

Loading

jedel1043 Oct 6, 2023 •

edited

Loading

jedel1043 left a comment •

edited

Loading