-
Notifications
You must be signed in to change notification settings - Fork 169
Description
I discussed this a bit in the Matrix room first to get a temperature check. Apologies in advance for the large pile of text
Background
temporal_rs
I've been implementing Temporal in V8, using the temporal_rs
library, used by boa. Most of the implementation concerns here pertain to both V8 and Boa.
temporal_rs is a library whose API can be tweaked if necessary1, but overall I think its API is a good example of an attempt to split surface-level "JS interaction" code from the underlying spec logic/algorithms2. I think that this type of implementation hygeine is worth supporting as a specification, and if temporal_rs' model is incompatible with the spec, it's potentially a sign for a spec change.
WebIDL
(This section is not really a motivation, but is an attempt to explain the type of validation/parsing model I am aiming for)
I like the model WebIDL presents, providing a very clear distinction between "JS interaction" code and the underlying spec logic. Specifications using WebIDL seldom have to explicitly interact with JS beyond throwing errors; the JS layer is basically handled by the structure provided by WebIDL itself.
For example, Instant.since()
would typically have the "input types" all clearly specified in WebIDL, which would imply a bunch of pre-parsing and type validation and the actual specification would do things like validating whether the settings are compatible (e.g. Instant.since()
doesn't want you to use "day" units) and then performing the actual algorithm.
An example `Instant.since()` in WebIDL
typedef (Instant or string) InstantLike;
enum Unit {
"auto",
"years",
....
"nanoseconds",
};
enum RoundingMode {
"ceil",
...
"halfEven",
}
struct DifferenceSettings {
Unit? largestUnit;
Unit? smallestUnit;
long? roundingIncrement;
RoundingMode? roundingMode;
}
interface Instant {
Duration since(InstantLike other, DifferenceSetting settings);
}
In the runtime, all of this validation would be deterministically performed at the boundary of since
(throwing any errors necessary), and only then would the spec code run.
I do not wish to suggest this specification move over to WebIDL: I'm using WebIDL to illustrate a good way of thinking about the JS-to-spec layer that is consistently used in Web specifications.
A different perspective on all of this is separating out "type validation" from "value validation", though that usage of terminology may be too ambiguous to usefully convey an idea.
Options loading and observability
By and large, all of this is mostly relevant to the part of the spec that deals with options loading, like GetDifferenceSettings
and GetTemporalUnitValuedOption
.
The order in which operations are performed does matter; it is observable to external code if e.g. smallestUnit
is read before largestUnit
, or if smallestUnit
and largestUnit
are read before validating their values in case, say, smallestUnit
has an invalid value for the context (you could notice using a proxy or custom getter that largestUnit
was still read before the routine raised an exception).
The current specification mixes up options loading and validation. For example, in GetDifferenceSettings
, the steps are:
Step 1: not a step
Step 2: get field (unit)
Step 3: validate
Step 4-5: get field
Step 6: independent of other steps
Step 7: get field (unit)
Step 8: validate
Step 9-10: resolve defaults/auto
Step 11-13: validate
Furthermore, each "get field (unit)" performs unit group validation and default resolution on top of parsing. This is a fair amount of validation overall.
Status quo in different implementations
Firefox matches the current spec exactly. Boa uses temporal_rs and batches up "get field" and option variant parsing (into typed values), but defers further validation to later; as such it is currently spec non compliant.
I am working on the V8 implementation and it does a bit of both, but I'm rapidly hitting a hard tradeoff. If I eagerly validate as the spec wants me to, I end up doing a bunch of operations twice (since temporal_rs
will do them again), AND I risk performing non-idempotent operations multiple times (e.g. Instant.prototype.since
"inverts" the rounding mode, which you don't want to do twice). If I lazily validate, it is far easier and more efficient, but I am not spec compliant.
The "non idempotent operations get performed multiple times" is by far the largest risk in my view. Performance hits are suboptimal, as is the maintenance burden of more complex validation code being carried in two places, but the issue with non idempotent options can easily cause subtle bugs.
Proposal
I haven't fully investigated the situations where Temporal performs interspersed loading and validation, but my understanding is that it's basically around all of the options bags (there aren't too many of those) and some select enumerated options like "unit". They show up a lot, but it's all shared code.
I'd like to propose that we move in the direction of consistently performing load-and-parse operations (GetOption
) first, to get the entire options bag, and any further validation can be performed after the full options bag has been loaded.
This would mean e.g. GetTemporalUnitValuedOption
would become a simple three-line GetOption
call like GetTemporalOverflowOption
. Potentially with default-handling like GetTemporalOffsetOption
. There can be a separate ValidateTemporalUnitValuedOption
that does unit group validation/etc.
This would also mean that GetDifferenceSettings
would call GetFooOption
for each object field first, and only then perform parsing and default resolution steps.
I don't necessarily want to turn this into some type of blocker where the Temporal team must agree to fully change everything in this spec, I'm mostly looking for general assent that this is a good direction to move in, and individual PRs could be approved on their merits. I don't think this is a large endeavor, but I don't want to commit the Temporal team to this endeavor, especially in case I'm wrong about the size.
I suspect what will end up happening is that I will discover these cases during the process of implementation, and while doing so open PRs to the Temporal repo, but I am open to other ideas.
There's also probably some leeway on how things are sliced in individual cases; e.g. Unit could still be validated into unit groups by "splitting" it into a timeunit and dayunit set of types. I don't think that's necessary, people may disagree.
Footnotes
-
Not that I claim to speak for the Boa/temporal_rs team here! They may have a certain plan for how it is designed and not wish to change it in certain directions. ↩
-
I think there are, really, three layers to this, not just two. You have very surface level "JS interaction" code (going from JS types into algorithm types), then you have "orientation" where you're validating and just generally figuring out what you've been asked, and then you have the underlying algorithm.
servo-media
andwebxr
draw the line between the second two.temporal_rs
draws the line between the first two. ↩
Activity
Manishearth commentedon May 27, 2025
I was asked to provide more background on the risks for implementors here if the spec remains unchanged.
There are a couple options moving forward, for boa and v8.
Do not change temporal_rs, be spec noncompliant
This is by far the least work involved for everyone. Of course, it is spec-noncompliant, has (minor) interop risks, and is a bad sign for the spec.
This is the status quo for boa (though I cannot claim to speak with their plans around this). While I'm working on the V8 implementation I'm somewhat doing this, but if the spec stays the same I will likely switch to a different strategy eventually. This will likely delay implementation, though.
Do not change temporal_rs, comply with the spec
This would essentially mean that engines and temporal_rs would inefficiently perform validation and defaults resolution multiple times. In V8; this would mean performing all of the validation steps in V8 C++, and then performing them again in temporal_rs Rust code.
Some checks could be skipped in V8 C++ if it's clear that there is no further user-observable steps after them. Determining that is not always easy and it's easy to make mistakes.
Some steps will have to be skipped: for example GetDifferenceSettings "inverts" the rounding mode based on the operation being performed. It's important that this isn't done twice.
For GetDifferenceSettings, the end result would be code that:
It would be nice if steps 8-14 could be deferred to the Rust code: they happen after all user-visible gets in GetDifferenceSettings. However, they do not happen after user-visible error cases in callers of GetDifferenceSettings. It's fine in DifferenceTemporalInstant: there's no additional steps. But DifferenceTemporalPlainDate does many further steps which can error.
Overall, this is inefficient, and extra maintainence burden on the engines.
Change temporal_rs to deal with "resolved" options, comply with the spec
temporal_rs could be tweaked to publicly accept "resolved" options, where all of these validation and transformation steps have already been applied. A separate API could be provided to go from specified to resolved options.
This is doable, but it further splits the spec across the library and the engine.
Furthermore, temporal_rs would then have to determine what to do when fed inconsistent options. It is good practice for libraries to validate their inputs, and either it has to re-perform validation, or it has to forgo it. A different way to do it would be to split up the types into differently validated types; so you might have a TimeUnit, DateTimeUnit, and DateUnit enum, but this would need to also account for types like TimeAndDay, DateTimeAndAuto, etc. This quickly explodes the number of types in play.
Finally, this diminishes temporal_rs' utility as a general purpose Rust library that has an API similar to Temporal (a well designed datetime library), since all of its endpoints will be lower level ones pulled from the bowels of the spec.
I think if forced to choose I would prefer to have the previous inefficient option over this behavior; I'd rather not make temporal_rs less useful to others.
ptomato commentedon Jun 25, 2025
We should discuss this in the following Temporal meeting. I'll summarize my opinion that I already communicated in the Matrix room.
I am strongly against this change. It would involve a lot of churn in the proposal, potentially introducing new bugs, while the benefit to JS programmers would be nonexistent or negligible (because all it would do is shuffle the order in which getters would be called.)
We have continued to make a few normative changes — against my better judgement, and each time using up patience from TC39 and from stakeholders funding the work — but those changes have all been at most a handful of lines, and actually solving a problem that would affect JS programmers and/or their users. I do not think "an implementation changed their underlying tech stack and now the code is a little ugly" is anywhere near sufficient motivation for a change of this scope. The time for that was 2 years ago.
I think it's fine for temporal_rs to either revalidate the arguments (how many extra CPU cycles does it cost to check something like
unit != YEAR
anyway) or lean on use of extra types like TimeOrDayUnit (which costs nothing at runtime).Manishearth commentedon Jun 25, 2025
I accept that it might be too big a change, but I want to respond specifically to this point, because I think it is phrased too strongly or leaves me confused as to what the ECMA process is.
We are currently in the stage of the process where it gets implemented and it gets iterated on due to feedback from implementors. As an implementor I am left confused as to what you mean to say here.
If this feedback is something the team finds too big to act on, that is fine, but if you wanted this feedback earlier I don't see how that could happen. I don't think it's fair for spec authors of a Stage 3 proposal to tell implementors that their feedback is "too late", it's a bait and switch when it comes to the ECMA process, or at least what I understand of it.
I'd also hope that this feedback nudges future ECMA API work to consider WebIDL-like load-and-parse-first models.
Also it's not just about ugly code, it's about correctness; not all of these steps are idempotent.
I'll note: I think the spec spends a lot of extra effort carefully doing options getting in alphabetic order while there is no benefit to JS programmers either. Sure, this change does not benefit JS programmers, but the spec is already spending a lot of effort into choices that do not benefit JS programmers. (Those are some of the choices I'd like to undo if possible, too, but it doesn't sound like that's happening)
I'll also argue that something that hampers implementation also indirectly hurts JS programmers: this issue is perhaps the one with the potential to most drastically change the amount of work involved in V8 spec compliance, which means that V8 will either ship noncompliant1, or ship later, both of which hurt JS programmers.
Footnotes
This is an option I am seriously considering leaving on the table: it is not my decision, but as the immediate implementor it may not be work I have time to do, unless Boa is also interested in full spec compliance when it comes to observable options getters and is able to help. ↩
nekevss commentedon Jun 25, 2025
Just going to leave a couple comments since this has partially been around
temporal_rs
.I could be misunderstanding the exact scope of Manish's proposal above, but I don't think the proposal, at least with
GetDifferenceSettings
, is to change the order of the options getters. Technically, everything can be left in the same order. It is primarily to separate the concerns of calling getters and algorithm specific validation of those getters to two separate abstract ops.The reason why this came up is that in order to address the
GetDifferenceSettings
, the getters and validation were separated with the ordering of the operations preserved, which is not technically spec conformant but it is, as of right now, test262 conformant.Which leads to a second point.
I will acknowledge that it would technically be a normative change, but it is a change that is not currently tested in test262. Boa is currently 100% conformant on Temporal difference operations because there are no tests about throwing in the middle of order of operations.
Beyond the above, this may be worth further consideration because the above approach mirrors the general approach to
PrepareCalendarFields
andCalendarResolveFields
. Primarily, these abstract ops act in two different steps to parse and validate an object to a calendar fields record object, which is then later resolved via algorithm specific validation.ptomato commentedon Jun 26, 2025
We discussed this in the Temporal meeting today (2025-06-26), but didn't have enough of a quorum to make a decision. Something that would be helpful, but not required, to move the discussion forward, would be a spec PR that shows the scope of what's being proposed for change.
(I'll respond to some of the above questions with my opinion-based answers in a separate post)
nicolo-ribaudo commentedon Jun 30, 2025
Drive-by comment, since I just learned about this discussion.
Temporal here is following a strong precedent set by basically every other ECMAScript API: the order of operations is to, for each option, read/cast-and-validate, and not to first read/cast all the options and then validate all of them. Some examples are:
@@toPrimitive
on it, gets@@match
fromsearchString
to validate it and then casts it, casts and validatesendPosition
)If we think that it is a good practice to first read all the values and then validate all of them, it would be great to see this presented in TC39 as a "normative convention" discussion, where rather than talking specifically about temporal we decide how we want the language to evolve, with two possible outcames:
Some examples of these conventions for proposal designers are at https://github.com/tc39/how-we-work/blob/56550835b23bca5689835cf37daa873fe75ba9e2/normative-conventions.md. Some examples of discussions about it are https://github.com/search?q=repo%3Atc39%2Fagendas%20stop%20coercing%20things&type=code.
ptomato commentedon Jul 1, 2025
Removing my Temporal meeting note-taker hat, here is my opinion.
About this particular change
Similar suggestions come up from time to time in various TC39 spaces. For example: tc39/ecma402#132, tc39/proposal-intl-numberformat-v3#122, tc39/ecma402#747. Historically, there has not been a lot of appetite for implementing it. I think we considered it at one point for Temporal as part of #1388 and declined to do it. (possibly here?)
Maybe the lack of appetite will change now that there has been a credible proposal on the table (in May's TC39 meeting) for a JS flavour of WebIDL. But even if we were redesigning Temporal from scratch now, I'd be reluctant to deviate from the precedent set by ECMA-402's options bags unless there was a clear signal from TC39 that we want the new behaviour going forward, as @nicolo-ribaudo suggested.
About the TC39 process
@Manishearth I specifically wanted to react to this:
I do in fact think this is a misunderstanding of an aspect of the TC39 process. As stage 3 goes on, the scope of requests that we are supposed to consider from implementations gets smaller. (This is not just my opinion; it's also a strong signal we have gotten from the TC39 committee meetings, particularly in relation to Temporal, where there seems to be a widespread feeling that Temporal has historically been too willing to make changes during stage 3, and patience is wearing thin.) Stage 3 is supposed to be like a soft freeze that gets harder the more time is spent in the stage. That is why I say the time for this feedback would have been earlier. We cannot keep redesigning broadly-scoped things right up until the very end of stage 3, and nobody has ever promised that we would.
I get that it's frustrating that I'm negative about what seems from Boa or V8's point of view like a superior technical solution. And I understand that you personally, Manish or Kevin, were not in a position to give this feedback 2 years ago. But to put it bluntly, other implementations did give feedback on similar issues when we were in a position to do something about it. Now we are much less in a position to do something about it.
About other changes with low benefit to JS programmers
I take the point about why we spent effort putting options property reads in alphabetical order. I personally don't think that effort was particularly well-spent either, but this particular thing was raised by other delegates as a requirement for going to stage 3 (#1388, #1430), and stage 2 was an appropriate time to do it.
About what's best for users
I do not see this as a choice between V8 shipping earlier or later. As far as I'm concerned, this is a choice between only V8 shipping later, or delaying Temporal for everyone. (Possibly indefinitely, while we battle skepticism about Temporal's readiness for stage 4.)
Forward?
I agree with the conclusion we reached in the last Temporal meeting, I think the best way forward is for us to talk concretely about a PR with the changes you want to see. Let's figure out exactly how broad the scope is that we are talking about for this change. But hopefully this explains the lens through which I am looking at the situation.
Manishearth commentedon Jul 1, 2025
That's not my proposal. This issue is where I was gauging appetite for this type of change, with targeted proposals to come if I felt it was worth the effort:
What I was looking for was some agreement that this direction is a better direction to move in than the status quo1, and then we can make individual decisions on whether status quo inertia should win out in a particular case. That's a valid state to be in! It's totally valid for a group to decide "yes, we think that that is a better choice, but also we don't wish to make that change".
Perhaps this explains your strong response and my confusion at this response. "The time for this was 2 years ago" makes a lot of sense for a broad, cross-cutting proposal that fundamentally changes a lot of text. It makes a little less sense for targeted proposals like "change the ordering of operations in GetDifferenceSettings and GetTemporalUnit", which is likely to be the result of this (maybe some more APIs. Like I said, it can be looked at case-by-case.
A normative change that is unobservable to all but the most meticulously instrumented JS code seems like exactly the type of change that is fine for stage 3, even late in stage 3. I can understand implementors pushing back against it if it is already implemented, but I would like to hear from the implementors on that.
Sounds good. I'm going on vacation shortly, but @sffc did say he was interested in helping write proposals for this. The two endpoints listed in the issue (GetDifferenceSettings and GetTemporalUnit) are the main ones I think could benefit here.
Footnotes
In part because I'm sure there are reasons behind the status quo incompatible with this direction, and it's worth weighing tradeoffs! ↩
sffc commentedon Jul 10, 2025
I took a closer look at this today.
The main issue, as I understand it, involves where we draw the boundary between engine code and library code. Ideally, we want to push as much of the implementation into the library as possible, and keep the engine layer as thin as possible.
The assertion is that the read-validate-read-validate paradigm makes it harder to shift logic into the library, leaving a lot of logic in the engine. The proposed solution is to use a scalpel to switch the spec to read-read-validate-validate in areas where the current behavior is difficult to implement correctly.
Unfortunately, I'm not convinced that the proposed solution is possible without the cross-cutting change that the champions don't want.
To illustrate, consider
Temporal.Duration.prototype.round
. The spec says:Ideally, the engine would like to write this pseudocode:
In words, it handles the narrow task of interfacing between the JS options object (which calls getters on the JS Object) and the library's options object (which takes a plain data value, perhaps something like an enumeration between a string, number, and
undefined
).In order for this pseudocode to be possible as written, all of these functions would need to be fully re-structured all the way down to the implementation of
GetOption
.It doesn't help if I narrowly rewrite
GetDifferenceSettings
andGetTemporalUnitValuedOption
: those AOs are basically just fancy wrappers overGetOption
. Every prototype function in Temporal that callsGetOption
more than once would be impacted by this change.As a consequence, I do not need to take a position on "is it too late in Stage 3 to make this change", for which I have a conflict of interest as both a Temporal champion who wants to see this reach Stage 4 without any more disruptions and a V8 implementer who wants to implement this in the most maintainable way possible.
With that said, I think libraries implementing Temporal have other options available to them, at the cost of a less simple library interface (especially when FFI is involved).
Instead of
Library::Duration::round
taking the same polymorphic options object, the library can take an options object narrowly tailored to just that function, with fields that do the eager validation.For example, in Rust-like library pseudocode:
Then, an engine written in Rust would write code such as
This should work because we are performing the read-validate-read-validate in the order required by the spec.
This type of library interface is easy to use from Boa (Rust-to-Rust), but more verbose to use from V8 (C++-to-Rust), but still much less than writing the whole spec in C++, and most of the complexity lives in the library's Diplomat bindings, not in V8 code. It seems to me like a reasonable solution.
Note: this is the "different way to do it" from @Manishearth's second post. In that post, @Manishearth raised concerns that this would make temporal_rs less useful as a Rust library. That might be true, but I don't actually think the difference is very big; the userland code either way is basically the same. There are just more types to sift through, but the number of types is bounded.
@nekevss, wdyt?
sffc commentedon Jul 11, 2025
Based on today's discussion, I realized that there is a middle ground I was missing. If we refactor
GetTemporalValuedUnitOption
to always validate against the same set of allowed values, and then check for a subset later, then libraries like Temporal_rs can use a singleTemporalUnit
type as the options bag argument, making their APIs much simpler.I wrote up this middle ground in #3130, which consists of a 60-line editorial change and a 3-line normative change.
I can make a similar PR forEDIT: I went ahead and fixed it in the same PR.GetDifferenceSettings
.3 remaining items