Editor review: Kevin Gibbons

Question

Editor review: Kevin Gibbons

bakkot opened this issue 5 years ago · 12 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

NonNegativeModulo: it is not clear what the types are here. modulo is 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.)

ValidateInstant takes a BigInt but then does comparisons against reals. It should do ℝ(ns) before comparing.

In SystemUTCEpochNanoseconds, "the approximate current UTC date and time, in nanoseconds since the epoch" is a real value, but then it is used as a BigInt. It should either be prefixed with "the BigInt value for" or followed by 1. Set _ns_ to ℤ(_ns_)..

GetOffsetNanosecondsFor asserts IsInteger, which is an operation which no longer exists as of tc39/ecma262#2007; instead, we just assert "is an integer" or "is an integral Number". Also, it calls abs(offsetNanoseconds), but abs is defined only for real numbers, and here offsetNanoseconds is 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.

Temporal.PlainDate converts its arguments ToIntegerOrInfinity, but then passes them to CreateTemporalDate, which asserts its arguments are integers. They could be infinity.

ISOMonthDayFromFields has 11. Let referenceISOYear be the first leap year after the Unix epoch (1972). This should probably specify a type. I'd suggest maybe Let referenceISOYear be 1972 (the first leap year after the Unix epoch).

BalanceISOYearMonth asserts that year is an integer Number value, but then a) checks if it's infinity and b) operates on it as if it is a real number.

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.

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

AddISODate claims its arguments are integral Numbers, but it operates on them as if they are real integers.

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:

FormatTimeZoneOffsetString
SystemUTCEpochNanoseconds
SystemInstant (:x: not obviously infallible through OrdinaryCreateFromConstructor)
GetISO8601Calendar (:x: not obviously infallible through OrdinaryCreateFromConstructor)
SystemTimeZone (:x: not obviously infallible through OrdinaryCreateFromConstructor)
BalanceISODate
DifferenceISODate
FormatCalendarAnnotation

In SystemTimeZone, the call to CreateTemporalTimeZone(identifier) is infallible.
- ❌ not obviously infallible through OrdinaryCreateFromConstructor

In SystemInstant, the call to CreateTemporalInstant is infallible.
- ❌ not obviously infallible through OrdinaryCreateFromConstructor

In GetISO8601Calendar, the call to GetBuiltinCalendar("iso8601") is infallible.
- ❌ not obviously infallible through OrdinaryCreateFromConstructor

A few places do ? OrdinaryObjectCreate(%Object.prototype%). This operation is infallible, so it should be !.

All calls to CreateArrayFromList are infallible, so they should be called as !, not ?.

In ISODateFromFields, the Get calls are infallible, since ToTemporalDateFields will 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..

I believe the Get calls in ResolveISOMonth are infallible, for the same reason the calls in ISODateFromFields are.

DifferenceISODate calls AddISODate with "constrain", which makes it infallible, so those calls should be marked as such. That means DifferenceISODate is itself infallible, as noted above.

Argument issues

SystemDateTime claims its first argument is optional, but it is always provided.

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

Sometimes the phrase "is not given" is used to check for the presence of an optional argument. The convention is "is not present".

#2098

ToTemporalDate should probably assert that options is an Object if it is present; it's not obvious from context, but it's asserted later: it passes options to ToTemporalOverflow which passes it to GetOption which asserts it is an Object.

In

there is a call to CalendarDateAdd whose arguments are in the wrong order (they switch options and constructor).

Misc

DefaultTimeZone says "If an ECMAScript implementation does not include the ECMA-402 API the following specification of the DefaultTimeZone abstract operation is used.", but no specification follows. From context, I think it should probably be

1. Return *"UTC"*.

Various places use ℝ 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.)

SystemInstant calls CreateTemporalInstant with the current time in nanoseconds, which then asserts ValidInstant of that value, which then asserts that value is less than 8.64 * 10^21, i.e., that the current time is not later than year 275760. I'm not convinced it makes sense to assert this, even though we expect it to be true in practice, because it's not a property which follows from the specification: there's nothing in the specification which says that it is not valid to use it in the year 275761.

InterpretISODateTimeOffset says "is an integer mathematical value"; it should just say "is an integer".

#1566 ParseTimeZoneOffsetString calls ToIntegerOrInfinity on 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.

Another nit about ParseTimeZoneOffsetString: "the substring of fraction consisting of the code units with indices 0 (inclusive) through 9 (exclusive)" can just be "the substring of fraction from 0 to 9". Ditto the uses of "substring" in ParseISODateTime, ParseTemporalDurationString, and ParseTemporalTimeZoneString.

RejectISODate rejects things which are not ISO dates, which is not what I expected from the name.

CreateTemporalDate says 6. If Type(calendar) is not Object, throw a RangeError exception.. Should that be a TypeError?

DurationSign, RejectDurationSign, and ValidateTemporalDuration should have angle brackets around the lists they're iterating over.

PrepareTemporalFields says If the value of _property_; it should say If _property_.

ToTemporalDateFields, ToTemporalDateTimeFields, ToTemporalYearMonthFields, and ToTemporalMonthDayFields are identical.

Also, they're not very useful; it seems like you could just call PrepareTemporalFields directly.

#2099

3. Set fields to ? ToTemporalDateFields(fields, « "day", "month", "monthCode", "year" »).
4. Let year be ? Get(fields, "year").
5. If year is undefined, throw a TypeError exception.

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:

6. If _monthLength_ is not 3, throw a RangeError exception.

The docs imply that M08L would be a valid month code. Is the 3 here specifically because it's an a ISO month, and ISO does not have repeated months?

7. Let numberPart be the substring of _monthCode_ from 2.

I suspect this should be from 1; strings are 0-indexed.

8. Set numberPart to ! ToIntegerOrInfinity(numberPart).
9. If numberPart is NaN, throw a RangeError exception.

ToIntegerOrInfinity cannot return NaN, so I'm not sure what's going on with this check.

11. If ! SameValueNonNumeric(monthCode, ! ToString(numberPart)) is false, then

I thought monthCode was supposed to be like "M02", so how can this ever be true?

ConstrainToRange isn't necessary; the spec defines "the result of clamping x between lower and upper".

PadISOYear says Return _y_ formatted as a four-digit decimal number.. It should probably be more explicit that this is a String, as in Return the String representation of _y_, formatted as a four-digit decimal number.
- there's another thirty-ish occurrences
- Ref tc39/ecma262#2693

CalendarEquals uses "is" to compare strings; it should use "is the same sequence of code units as".

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.

CalendarDateAdd says its last argument is optional, but it's always passed.
- ❌ the last argument is passed in one of the first five callers I checked.

ConstrainISODate has only one callsite, and is very short; it might make sense to inline it.

Answer 1 · 2021-03-08T19:22:50.000Z

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:

SystemInstant calls CreateTemporalInstant with the current time in nanoseconds, which then asserts ValidInstant of that value, which then asserts that value is less than 8.64 * 10^21, i.e., that the current time is not later than year 275760. I'm not convinced it makes sense to assert this, even though we expect it to be true in practice, because it's not a property which follows from the specification: there's nothing in the specification which says that it is not valid to use it in the year 275761.

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.

ParseTimeZoneOffsetString calls ToIntegerOrInfinity on 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.

nanoseconds is set to a substring of the first 9 characters in step 10b, so I don't think it can be 18 digits long when passed to ToIntegerOrInfinity. Nonetheless, what you suggest in the second paragraph seems fine.

The docs imply that M08L would be a valid month code. Is the 3 here specifically because it's an a ISO month, and ISO does not have repeated months?

That is correct.

I thought monthCode was supposed to be like "M02", so how can this ever be true?

I think this was left over from when monthCode did not have the M and the 0-padding, the current text is definitely incorrect.

Answer 2 · 2021-03-08T20:20:09.000Z

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?

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.

nanoseconds is set to a substring of the first 9 characters in step 10b

Oh, got it.

Answer 3 · 2021-03-08T22:40:43.000Z

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.

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.

Answer 4 · 2021-03-08T23:28:59.000Z

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.

Answer 5 · 2021-04-16T18:50:04.000Z

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)
Here's what I think falls in that category:

Replacing options objects created with OrdinaryObjectCreate(%Object.prototype%) with NormalizeOptionsObject / OrdinaryObjectCreate(null)
Missing algorithm steps for DefaultTimeZone
SameValueNonNumeric(monthCode, ! ToString(numberPart)) in ResolveISOMonth
Default value for out-of-range system clock

Answer 6 · 2021-04-16T19:08:32.000Z

I think also

CreateTemporalDate says 6. If Type(calendar) is not Object, throw a RangeError exception.. Should that be a TypeError?

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.

Answer 7 · 2021-06-24T11:29:50.000Z

NormalizeOptionsObject is used in some places when an options argument is needed, which creates a new object inheriting from null instead of Object.prototype. A bunch of other places instead do OrdinaryObjectCreate(%Object.prototype%). It seems like these should be consistent, and (as a non-editorial opinion) NormalizeOptionsObject seems better: I wouldn't expect omitting an options argument to cause lookups on Object.prototype.

I double-checked the callers of OrdinaryObjectCreate(%Object.prototype%):

*#getISOFields
DefaultMergeFields, PrepareTemporalFields, PreparePartialTemporalFields, Temporal.Calendar.prototype.mergeFields
MergeLargestUnitOption

The last one may have been an oversight, but the others are probably fine.

Answer 8 · 2021-06-24T13:39:11.000Z

CalendarEquals uses "is" to compare strings; it should use "is the same sequence of code units as".

Does this also apply to comparisons like "If id is not "iso8601", return false."?

Answer 9 · 2021-06-28T21:54:01.000Z

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, plainDate.toPlainYearMonth() → PrepareTemporalFields followed by YearMonthFromFields → calendar.yearMonthFromFields(fields). However, in other cases, the Object is internal only. It would be possible to refactor only the internal-only Objects to be Records, or to refactor all of them as suggested and create Objects only as necessary, but I'm not sure it's worth it.

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.

Answer 10 · 2021-06-29T11:49:49.000Z

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 x is from you @Ms2ger? Maybe you missed a negation?

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.

Answer 11 · 2021-06-29T14:21:00.000Z

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 x is from you @Ms2ger? Maybe you missed a negation?

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.

Answer 12 · 2022-03-16T15:52:36.000Z

I filed new issues for the remaining comments.