Normative: Reject invalid output from custom Calendar/TimeZone methods #2456
Conversation
gibson042
added
behavior
Codecov Report
@@ Coverage Diff @@
## main #2456 +/- ##
==========================================
- Coverage 94.94% 94.64% -0.30%
==========================================
Files 20 20
Lines 10821 10971 +150
Branches 1957 1987 +30
==========================================
+ Hits 10274 10384 +110
- Misses 487 528 +41
+ Partials 60 59 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -157,8 +157,9 @@ <h1> | |||
| </dl> | |||
| <emu-alg> | |||
| 1. Let _result_ be ? Invoke(_calendar_, *"year"*, « _dateLike_ »). | |||
| 1. If _result_ is *undefined*, throw a *RangeError* exception. | |||
| 1. Return ? ToIntegerWithTruncation(_result_). | |||
| 1. If Type(_result_) is not Number, throw a *TypeError* exception. | |||
There was a problem hiding this comment.
I think the currently preferred phrasing in ECMA-262 is "If result is not a Number, ..." (here and elsewhere)
There was a problem hiding this comment.
It is, but this preserves internal consistency in the proposal.
There was a problem hiding this comment.
I have a slight preference for having new sections follow the new conventions over internal consistency (less work for me later), but I don't feel strongly about it; it's fine to leave it like this.
| 1. If IsIntegralNumber(_result_) is *false*, throw a *RangeError* exception. | ||
| 1. Return ℝ(_result_). |
There was a problem hiding this comment.
I guess this is a matter of preference, but once we've established it's a Number, you could use "Return ? ToIntegerIfIntegral(result)" here. (Although maybe it's better not to do that since there isn't a convenient ToIntegerIfPositiveAndIntegral operation to call in the positive-integer ones)
There was a problem hiding this comment.
Right, this implements structural similarity across the operations (but I have no objections to the alternative).
| @@ -193,8 +197,8 @@ <h1> | |||
| </dl> | |||
| <emu-alg> | |||
| 1. Let _result_ be ? Invoke(_calendar_, *"monthCode"*, « _dateLike_ »). | |||
| 1. If _result_ is *undefined*, throw a *RangeError* exception. | |||
| 1. Return ? ToString(_result_). | |||
| 1. If Type(_result_) is not String, throw a *TypeError* exception. | |||
There was a problem hiding this comment.
Similarly, "If result is not a String, ..." (here and elsewhere)
| @@ -373,7 +402,8 @@ <h1> | |||
| </dl> | |||
| <emu-alg> | |||
| 1. Let _result_ be ? Invoke(_calendar_, *"inLeapYear"*, « _dateLike_ »). | |||
| 1. Return ToBoolean(_result_). | |||
| 1. If Type(_result_) is not Boolean, throw a *TypeError* exception. | |||
There was a problem hiding this comment.
Similarly, "If result is not a Boolean, ..."
|
Draft because it needs to be presented to TC39. |
ptomato
force-pushed
the
gh-2443-reject-invalid-custom-method-output
branch
from
8866c51
to
a8bc580
Compare
|
This achieved consensus in the 2023-01-31 TC39 plenary meeting. |
Fixes #2443
It looked to me like the TimeZone operations already behaved appropriately1, but please verify in review (along with confirmation that I have preserved the distinction between fields that must be positive vs. those that can be zero or negative).
test262 pull request: tc39/test262#3761
Footnotes
GetOffsetNanosecondsFor requires
getOffsetNanosecondsForto return an integral Number with absolute value no greater than nsPerDay, and GetPossibleInstantsFor requiresgetPossibleInstantsForto return an iterable of Temporal Instant instances. ↩