Editor review: Kevin Gibbons #1424
Comments
|
Thanks. I think this can all be treated as a checklist of edits to make, it doesn't seem like anything needs much discussion here. I've added a few comments below:
It's invalid for a Temporal.Instant to represent a time outside of that range. Maybe SystemInstant should specify that it will never report a time outside of that range, or there should be an intervening step that throws if the system clock reports a time outside of that range? For previous discussion on why we have this limit, see #24.
That is correct.
I think this was left over from when monthCode did not have the M and the 0-padding, the current text is definitely incorrect. |
Specifying that it will never report a time outside the range means the spec isn't timeless, which seems unfortunate. Throwing has the unfortunate downside of implying the operation can throw in practice, which it can't. It might be simplest to pick a default for use when outside the range and then have a NOTE explaining it's not actually expected to come up (or tell implementations to do that). I don't expect any option here to actually affect implementations, so it's just whatever is editorially cleanest.
Oh, got it. |
All of these seem slightly icky but I think it doesn't matter since this is not practically going to come up. Picking a default value does mean that if someone sets their system clock to this out-of-range date, then things will break, but that's not the only problem they'll have; legacy Date methods will also start returning NaN at that point. |
|
Yeah, I mean, if your clock is off by 300,000 years, you're going to have a bad time. One option for a default is to clamp the value to the extreme value in the range, which would minimize the issue - at least it'd still be monotonic, it's just that time would stop observably passing at that point. |
|
I'm looking through these to see if there are any normative changes that we'd have to make in order to fulfill the Stage 3 conditions (as opposed to editoral changes which I do intend to work on soon but are less of a priority and don't hold anything up)
|
|
I think also
Unless you really did intend a RangeError here. By the way, feel free to edit the OP comment to strike through issues as they're addressed. |
In NormalizeOptionsObject, which comes from ECMA-402 originally, when an
options parameter is undefined, an object with null prototype is created.
Make everywhere else that an options parameter is synthesized, consistent
with that, so create objects with null prototype rather than {}.
Ensure that separate objects are created and not re-used once they have
been passed in to user code.
(We continue to return plain objects whose prototype is Object.prototype
from methods such as getISOFields())
See: #1424
This was a spec text bug, left over from when the "M" and the zero-padding were added. See: #1424
This was inconsistent across the various types; make it consistently assert, not throw, that the given calendar is an object. See: #1424
Times outside of a range of about half a million years cannot be represented by Temporal.Instant. Clamp the system time to that range so that the operation is guaranteed not to throw, which is editorially the cleanest solution. If your clock is 300,000 years off then Temporal is not the biggest problem you will have, so this is not expected to come up in practice. See: #1424
Times outside of a range of about half a million years cannot be represented by Temporal.Instant. Clamp the system time to that range so that the operation is guaranteed not to throw, which is editorially the cleanest solution. If your clock is 300,000 years off then Temporal is not the biggest problem you will have, so this is not expected to come up in practice. See: #1424
…alendarDateUntil. Previous optimizations had the effect of removing checks for internal slots on returned values by bypassing CalendarDateAdd and CalendarDateUntil. This fixes those bugs. See: #1424
In NormalizeOptionsObject, which comes from ECMA-402 originally, when an
options parameter is undefined, an object with null prototype is created.
Make everywhere else that an options parameter is synthesized, consistent
with that, so create objects with null prototype rather than {}.
Ensure that separate objects are created and not re-used once they have
been passed in to user code.
(We continue to return plain objects whose prototype is Object.prototype
from methods such as getISOFields())
See: #1424
This was a spec text bug, left over from when the "M" and the zero-padding were added. See: #1424
This was inconsistent across the various types; make it consistently assert, not throw, that the given calendar is an object. See: #1424
Times outside of a range of about half a million years cannot be represented by Temporal.Instant. Clamp the system time to that range so that the operation is guaranteed not to throw, which is editorially the cleanest solution. If your clock is 300,000 years off then Temporal is not the biggest problem you will have, so this is not expected to come up in practice. See: #1424
In NormalizeOptionsObject, which comes from ECMA-402 originally, when an
options parameter is undefined, an object with null prototype is created.
Make everywhere else that an options parameter is synthesized, consistent
with that, so create objects with null prototype rather than {}.
Ensure that separate objects are created and not re-used once they have
been passed in to user code.
(We continue to return plain objects whose prototype is Object.prototype
from methods such as getISOFields())
See: #1424
This was a spec text bug, left over from when the "M" and the zero-padding were added. See: #1424
This was inconsistent across the various types; make it consistently assert, not throw, that the given calendar is an object. See: #1424
Times outside of a range of about half a million years cannot be represented by Temporal.Instant. Clamp the system time to that range so that the operation is guaranteed not to throw, which is editorially the cleanest solution. If your clock is 300,000 years off then Temporal is not the biggest problem you will have, so this is not expected to come up in practice. See: #1424
I double-checked the callers of
The last one may have been an oversight, but the others are probably fine. |
Does this also apply to comparisons like "If id is not "iso8601", return false."? |
|
Taking a look at the remaining unchecked items! Re. OrdinaryObjectCreate(%Object.prototype%), see #1494 (comment) - I think for options objects created internally and accessed internally, OrdinaryObjectCreate(null) is fine, but for objects returned to user code, you actually want fully-functional Object.prototype objects. I agree MergeLargestUnitOption is probably an oversight, that should be OrdinaryObjectCreate(null), though I'm not sure it's worth submitting a normative change for? Up to the editors, I guess. Re. PrepareTemporalFields preparing an Object and not a Record, and the possibility of a note saying the Object is never directly accessible to ECMAScript code; there are paths where the Object is accessible to user code. For example, Re. CalendarDateAdd says its last argument is optional, but it's always passed. → I think this is still valid — I assume the comment with the ❌ is from you @Ms2ger? Maybe you missed a negation? I checked a few items off the list that had been done elsewhere in the meantime. I think all the other remaining ones that I didn't mention above are still valid. |
No, but perhaps I should have written "the last argument is passed in only one of the first five callers I checked." So there's one where's it's passed, and four where it's omitted. That seems optional to me. |
You're right and I completely blanked on this yesterday - I was looking at the options argument, not the dateAdd argument. This one should be checked off the list, then. |
|
I filed new issues for the remaining comments. |
I haven't yet been able to complete my review, but since the meeting is looming I figured I should post what I have so far.
Type confusions
modulois defined only over mathematical values, but it talks about*-0*, which isn't a mathematical value. (The linter ought to warn for*-0*when not followed by<sub>𝔽</sub>, sorry about that.)1. Set _ns_ to ℤ(_ns_)..abs(offsetNanoseconds), butabsis defined only for real numbers, and hereoffsetNanosecondsis a Number.This type confusion extends to its callers: some callers of GetOffsetNanosecondsFor assume its result is a Number, some assume it is a real integer.
11. Let referenceISOYear be the first leap year after the Unix epoch (1972).This should probably specify a type. I'd suggest maybeLet referenceISOYear be 1972 (the first leap year after the Unix epoch).Also, if its arguments really are always integers - which appears to be the case - it can't throw, so it should be invoked as such.
1. If day is +∞ or −∞, then, but its callsites can only pass it integers, so this is never hit.Also, when BalanceISODate calls BalanceISOYearMonth, it is infallible.
Infallible operations
"Infallible" meaning these can't throw, and so should be called with
!, not?.Various operations are invoked as if they are fallible in various places, but are in fact infallible as far as I can tell, including:
GetBuiltinCalendar("iso8601")is infallible.? OrdinaryObjectCreate(%Object.prototype%). This operation is infallible, so it should be!.!, not?.Getcalls are infallible, sinceToTemporalDateFieldswill return an ordinary object with own data fields for the fields being looked up.Going through an actual object here is pretty weird; as far as I can tell the object is not observable. If it's editorially inconvenient to refactor it to be a Record, it would at least benefit from adding
NOTE: the object returned by ToTemporalDateFields is never directly accessible to ECMAScript code..Getcalls in ResolveISOMonth are infallible, for the same reason the calls in ISODateFromFields are.Argument issues
A number of abstract operations refer to an optional argument without first checking if it is present, including at least
(The "assume missing arguments are undefined" thing applies only to built-in functions, not abstract operations.)
GetOptionsObjectvsOrdinaryObjectCreate(%Object.prototype%)#2098In
there is a call to CalendarDateAdd whose arguments are in the wrong order (they switch options and constructor).
Misc
ℝas a subscript, which isn't necessary (or defined); in the specification, numbers without subscripts are real numbers. (I will also add that to the linter.)nanoseconds, which is a string which is to 18 digits long, which exceeds the precision of Number. But ToIntegerOrInfinity passes its argument through the Number type, and hence loses precision. Is that intentional?On that note, I'm not sure that ToIntegerOrInfinity is the right thing to be calling in this method. The grammar constrains the four things on which ToIntegerOrInfinity is invoked in this method to to be strings of digits, so despite what ToIntegerOrInfinity implies, there's no coercion, rounding, or possible infinities involved. Perhaps it would be better to call
ℝ(ToNumber(_foo_))and assert that the result is a nonnegative integer? You could also introduce an abstract operation which does this, optionally.6. If Type(calendar) is not Object, throw a RangeError exception.. Should that be a TypeError?If the value of _property_; it should sayIf _property_.Also, they're not very useful; it seems like you could just call PrepareTemporalFields directly.
ToTemporalDateFields is just a wrapper around PrepareTemporalFields with an empty list the required fields. But here the resulting object is immediately checked for "year". Why not just call PrepareTemporalFields with "year" as a required field? Ditto "day".
In ResolveISOMonth:
M08Lwould be a valid month code. Is the 3 here specifically because it's an a ISO month, and ISO does not have repeated months?monthCodewas supposed to be like "M02", so how can this ever be true?Return _y_ formatted as a four-digit decimal number.. It should probably be more explicit that this is a String, as inReturn the String representation of _y_, formatted as a four-digit decimal number.There's a number of places missing a
_. Just search for_; almost all occurrences are typos.There's also a number of places which appear to have
ES.before the name of some abstract operation, possibly because of a copy-paste error.The text was updated successfully, but these errors were encountered: