Problem with generating Durations from non-numeric input components

[gibson042]

NaN is replaced with zero by ToIntegerWithoutRounding step 2, but this leads to some surprising and IMO bad consequences:

new Temporal.Duration(Symbol(2022)) // TypeError 👍
new Temporal.Duration(0.5) // RangeError 👍
new Temporal.Duration(NaN) // => PT0S 😱
new Temporal.Duration("foo") // => PT0S 😱
new Temporal.Duration(/regex/) // => PT0S 😱
new Temporal.Duration(Temporal) // => PT0S 😱

Temporal.PlainDateTime.from("2022-03-20T00:00").add({days: Symbol(0)}) // => TypeError 👍
Temporal.PlainDateTime.from("2022-03-20T00:00").add({days: 0.5}) // => RangeError 👍
Temporal.PlainDateTime.from("2022-03-20T00:00").add({days: NaN}) // => 2022-03-20T00:00 😱
//…

[ptomato]

I find this consistent with how coercion to a number usually works in JS:

[1, 2, 3].slice(Symbol()) // TypeError
[1, 2, 3].slice(NaN) // => [1, 2, 3]
[1, 2, 3].slice("foo") // => [1, 2, 3]
[1, 2, 3].slice(/regex/) // => [1, 2, 3]
[1, 2, 3].slice(Math) // => [1, 2, 3]

Not that I think the status quo is the best choice that could've been made, but it doesn't seem good to me to deviate from it now.

[gibson042]

I think "usually" is overly broad there; many operations behave differently:

[].length = NaN // RangeError
String.fromCodePoint("foo") // RangeError

Particularly some that are closely related to Temporal:

new Date(/regex/) // => Invalid Date
Date.UTC(Math) // => NaN

And I think this behavior is particularly bad in duration-like objects, where bad data associated with a field is silently ignored unless it is only slightly bad—what is the point of rejecting days: 0.5 if days: NaN and days: "foo" are coerced to zero?

[justingrant]

I agree with @gibson042. The fact that some other APIs are bad doesn't mean that new APIs should also be bad.

[ptomato]

I ran across "Fix .at()" with a comment from @bakkot, among others:

It's (in my opinion) unfortunate that JS is so aggressive about doing type coercion rather than rejecting unreasonable arguments, but it's a very, very strong precedent at this point, and it would probably be more confusing to have only some functions work that way.

[bakkot]

To be clear, that opinion applies mainly to new functions on existing APIs. I think there's a little more room to provide a better experience on entirely new APIs. I've commented as much on this very similar issue on iterator helpers.

[ptomato]

I took an action in the Temporal champions meeting to informally poll delegates in the Matrix channel to see if this would be contentious at plenary. Before I do that, let's clarify what changes we are taking about here. As I understand it, the suggestion would be to change ToIntegerThrowOnInfinity to look something like this:

1. If Type(argument) is Object, set argument to ? ToPrimitive(argument, number).
2. If argument is undefined, return 0.
3. Let number be ? ToNumber(argument).
4. If number is NaN, -∞, or +∞, throw a RangeError exception.
5. If number is -0_𝔽, return 0.
6. Let integer be floor(abs(ℝ(number))).
7. If number < -0_𝔽, set integer to -integer.
8. Return integer.

And change ToIntegerWithoutRounding to look something like this:

1. If Type(argument) is Object, set argument to ? ToPrimitive(argument, number).
2. If argument is undefined, return 0.
3. Let number be ? ToNumber(argument).
4. If number is NaN, -∞, or +∞, throw a RangeError exception.
5. If number is -0_𝔽, return 0.
6. If IsIntegralNumber(number) is false, throw a RangeError exception.
7. Return ℝ(number).

This would mean the following changes in how values would be coerced to mathematical values in Temporal:

Value (example)	Before	After
integer Number (3)	3	unchanged
finite non-integer Number (2.5)	2, or RangeError for Duration	unchanged
infinite Number	RangeError	unchanged
integer numeric String (" 3 ")	3	unchanged
non-integer numeric String (" 2.5 ")	2, or RangeError for Duration	unchanged
empty or whitespace String ("")	0	unchanged
true	1	unchanged
false	0	unchanged
Symbol	TypeError	unchanged
BigInt	TypeError	unchanged
null	0	unchanged
undefined	0	unchanged
legacy Date (new Date(3))	3	unchanged
Temporal object	TypeError (via valueOf())	unchanged
NaN	0	RangeError
non-numeric String ("foo")	0	RangeError
regexp (/foo/)	0	RangeError (via toString())
Temporal	0	RangeError (via toString())
Object in general	as above with the return value from [Symbol.toPrimitive]('number'), valueOf() or toString(), in that order	same

Booleans, legacy Dates, and null seem like things that we might consider throwing on as well, here.

Note, this would also depart from the WebIDL ConvertToInt algorithm which does the same thing with NaN, unless it is annotated with [EnforceRange].

On the other hand, there is already a precedent because all public entry points to Temporal that expect a number, already go through ToIntegerThrowOnInfinity, ToPositiveInteger, or ToIntegerWithoutRounding, and not through ToIntegerOrInfinity, so we already throw on infinities where other entry points don't.

[ptomato]

Note that this also might apply to entry points like Temporal.Instant.fromEpochSeconds.

[gibson042]

@ptomato I agree with your assessment, and support the changes described therein.

I would also include some preceding refactorings for clarity:

• ToIntegerThrowOnInfinity → TruncateToInteger
  1. If argument is undefined, return 0.
  2. Let number be ? ToNumber(argument).
  3. If number is NaN, +∞_𝔽, or -∞_𝔽, throw a RangeError exception.
  4. Return truncate(ℝ(number)).
  5. NOTE: truncate will be added to ECMA-262 by Normative: Use DefaultTimeZone to get the local time zone for Date ecma262#2781 .
• ToIntegerWithoutRounding → InsistInteger
  1. If argument is undefined, return 0.
  2. Let number be ? ToNumber(argument).
  3. If IsIntegralNumber(number) is false, throw a RangeError exception.
  4. Return ℝ(number).
• ToPositiveInteger → TruncateToPositiveInteger
  1. Let integer be ? TruncateToInteger(argument).
  2. If integer ≤ 0, throw a RangeError exception.
  3. Return integer.

[anba]

I think I'd prefer if the operations still all start with "To" similar to most other type conversion operations. (CanonicalNumericIndexString is currently the only outlier.)

[gibson042]

I want these names to not start with "To" precisely because the behavior is dissimilar from operations with that prefix—unlike e.g. ToNumber and ToIntegerOrInfinity and ToLength and ToIndex, these throw exceptions for input that has a compatible type but is out-of-range in some way.

[anba]

ToIndex throws for out-of-range values: ToIndex(2**53) throws a RangeError in step 2.c.

[ptomato]

OK, I'll do the informal poll once I have cleared out a bit more of my backlog of notifications from while I was away.

I don't have an opinion on the names of the operations. (Except that I find "InsistInteger" slightly ungrammatical)

[domenic]

In case it helps, in modern Web IDL we suggest people use basically these types: [EnforceRange] long long, [EnforceRange] unsigned long long, double, and unrestricted double. From trying to read this thread it seems like you're not following any of those recommendations exactly, but maybe the fact that modern Web IDL suggests [EnforceRange] for integer types is relevant here.

[ptomato]

Picking this up again; the discussion hasn't moved forward in the meantime, but maybe it will help if we have a proof of concept to discuss.

I'll start with an editorial PR renaming the operations without changing any of the behaviour, then create a proof of concept for the behaviour change.

As for renaming, I started with TruncateToInteger, but after trying it out I have to agree with @anba that it should start with "To"; TruncateToInteger sounds like it's meant to operate only on a mathematical value. How about this bikeshed?

• ToIntegerThrowOnInfinity → ToTruncatedInteger
• ToPositiveInteger → ToTruncatedPositiveInteger
• ToIntegerWithoutRounding → ToEnforcedInteger

[ptomato]

In case it helps, in modern Web IDL we suggest people use basically these types: [EnforceRange] long long, [EnforceRange] unsigned long long, double, and unrestricted double.

I've looked into the Web IDL conversion algorithms and for our purposes it looks like the closest equivalent would be [EnforceRange] long long for non-Duration APIs and double for Duration APIs. The differences would be:

1. Infinity, NaN, non-numeric strings and things that convert to them via ToPrimitive, would throw RangeError, not TypeError
2. undefined would be interpreted as 0, not throw a TypeError
3. Additionally, for Duration APIs, we would have the throw-instead-of-truncate behaviour.

Re. 1), I'm not sure of the reason that Web IDL chooses TypeError for out-of-range values rather than RangeError, but it seems more usual within ECMA-262 to use RangeError here.

Changing 2) is something that could make sense. IIRC we have undefined converting to 0 primarily because of things like the minutes and lower defaulting to 0 in new Temporal.PlainDateTime(2022, 10, 25, 16). However, maybe it makes sense to treat missing args / trailing undefineds differently from non-trailing undefineds? e.g., new Temporal.PlainTime(undefined, 30) would throw instead of giving 00:30. (This would really only have any effect in constructors. Property bags would still behave the same.)

Re. 3), the throw-instead-of-truncate behaviour for Duration is well-motivated in my opinion and has already been discussed extensively elsewhere.

[ptomato]

The editorial PR: #2430

[ptomato]

In the Temporal champions meeting of 2022-10-27, we reached an agreement on this.

Regarding Web IDL throwing on converting undefined to an integer, Richard noted that the overload resolution algorithm in Web IDL treats undefined/absent values for optional arguments differently from required arguments. e.g.,

The consensus is that we will proceed with the above table, with one change to align better with Web IDL: converting undefined to an integer will throw RangeError rather than resulting in 0. Optional arguments with default values will "override" undefined with the default value. So, concretely,

• new Temporal.PlainDate(undefined, 10, 27) - previously October 27, 1 BCE; now RangeError (because the year argument is not optional)
• new Temporal.PlainDate(2022, 10) - RangeError (unchanged) (previously because 0 is out of range for the day argument; now because the day argument is not optional)
• new Temporal.PlainTime() - 00:00 (unchanged) (because all the arguments to the PlainTime constructor are optional)
• new Temporal.PlainTime(undefined, 30) - 00:30 (unchanged) (because the hour argument is optional)