Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: don't coerce non-string primitives to strings, esp. time zones, offsets, and calendars #2574

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented May 13, 2023

This normative PR removes string coercion that is currently used in Temporal methods where inputs are expected to be strings. This change is an implementer-recommended fix to the problem that numbers can sometimes be coerced into syntactically valid Temporal string inputs, especially dates and time zone offsets.

Although coercing numbers to strings sometimes works, this behavior is brittle and unpredictable. For example, -1000 is a valid time zone offset when coerced to a string, but neither 1000 nor -900 can be coerced into valid offsets.

Calendars are also affected because while a number cannot be a calendar identifier, an ISO string can be passed in place of a calendar ID. This means that a number like 20200101 (if coerced to a string) is a valid ISO string representing the date 2020-01-01 that has a default calendar of iso8601.

Here's a few examples of confusing and inconsistent code that this PR prevents by throwing TypeErrors in all the cases below:

zdt = Temporal.ZonedDateTime.from('2023-05-17T00:00-07:00[America/Los_Angeles]')
zdt.with({ offset: -10 })
// => no error

zdt.with({ offset: -7 })
// RangeError: invalid time zone offset: -7

zdt.with({ offset: -10.5 })
// => RangeError: invalid time zone offset: -10.5

Temporal.TimeZone.from(-10).id
// => "-10:00"

Temporal.TimeZone.from(-10.5).id
// => RangeError: invalid time zone offset: -10.5

Temporal.Calendar.from(20200101)
// => iso8601

Temporal.PlainDate.from(20200101)
// => 2020-01-01

Temporal.PlainDate.from(-20200101)
// => RangeError: invalid ISO 8601 string: -20200101

This PR throws a TypeError in response to non-String primitive inputs in:

  • Parameters that accept an ISO string (or a subset of an ISO string) in methods like from, with, or add
  • The relativeTo option accepted in Temporal.Duration methods like compare or add, because relativeTo can also be an ISO string.
  • offset, era, and monthCode properties in property bags that are accepted in place of an ISO string in the methods and relativeTo option noted above.

If the input in the places above is an object and not a primitive, this PR changes ToString to ToPrimitive(_value_, ~string~) and if the resulting primitive is not a String, then it will throw a TypeError.

The Temporal.TimeZone and Temporal.Calendar constructors now throw a TypeError for all non-String inputs, including objects.

Other than relativeTo, options behavior is unchanged in this PR so that we can maintain consistency with 402's precedent to coerce options to the expected type before interpreting them.

For cases where an exception was already thrown in current spec text (for example, no non-String primitive value can be cast to a valid monthCode) this PR changes RangeError to TypeError when a non-String value is provided.

Test changes are at tc39/test262#3847.

@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Merging #2574 (944ad1a) into main (c429eed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 944ad1a differs from pull request most recent head 54d5ce7. Consider uploading reports for the commit 54d5ce7 to get more accurate results

@@           Coverage Diff           @@
##             main    #2574   +/-   ##
=======================================
  Coverage   95.96%   95.96%           
=======================================
  Files          20       20           
  Lines       11528    11536    +8     
  Branches     2193     2191    -2     
=======================================
+ Hits        11063    11071    +8     
  Misses        401      401           
  Partials       64       64           
Impacted Files Coverage Δ
polyfill/lib/calendar.mjs 86.84% <100.00%> (-0.04%) ⬇️
polyfill/lib/ecmascript.mjs 98.26% <100.00%> (+<0.01%) ⬆️
polyfill/lib/timezone.mjs 100.00% <100.00%> (ø)

@anba
Copy link
Contributor

anba commented May 17, 2023

In the 2023-05-11 meeting, we discussed how calling ToString in ToTemporal(Calendar|TimeZone)SlotValue is unnecessary, because no non-String, non-object type can be coerced to a built-in calendar or time zone identifier. Numbers, "undefined", "null", "true", or "false" will not be built-in IDs.

This isn't quite correct, because the string representation of some Number and BigInt values can be parsed successfully:

js> Temporal.Calendar.from(10).id
"iso8601"
js> Temporal.TimeZone.from(-10).id
"-10:00"

But I still support this change, FWIW.

@ptomato
Copy link
Collaborator

ptomato commented May 17, 2023

@anba Can you say more about your reason for supporting this change? Does it solve a problem or enable any particular optimization in Firefox?

(Unless we have a justification of that sort, I'd say that we shouldn't be making this kind of change at stage 3 — it seems to me that it's an adjustment of something that works fine but could equally well work differently.)

@justingrant
Copy link
Collaborator Author

js> Temporal.Calendar.from(10).id
"iso8601"

Ugh. Why does this work? Seems.... weird.

@ptomato
Copy link
Collaborator

ptomato commented May 17, 2023

I agree, that doesn't seem correct at all, and I think it's not compliant with the spec.

@justingrant
Copy link
Collaborator Author

js> Temporal.TimeZone.from(-10).id
"-10:00"

This one seems somewhat less bad than the calendar case, but still seems unexpected, esp. because Temporal.TimeZone.from(-10.5) throws.

@justingrant
Copy link
Collaborator Author

justingrant commented May 17, 2023

Also, I have no idea what to make of this polyfill output.

zdt = Temporal.ZonedDateTime.from('2023-05-17T00:00-07:00[America/Los_Angeles]')
zdt.with({ offset: -10 })
// => 2023-05-17T00:00:00-07:00[America/Los_Angeles]
// ^ note that the offset isn't actually changed to -10:00. This seems to be a no-op.

zdt.with({ offset: -7 })
// RangeError: invalid time zone offset: -7

??? If these results match the spec, then they further convince me that allowing non-string inputs here is a bad idea.

Somewhat related discussion about unexpected coercion: #2112

@justingrant justingrant added the normative Would be a normative change to the proposal label May 18, 2023
@justingrant justingrant changed the title Avoid unnecessary ToString in ToTemporal(Calendar|TimeZone)SlotValue Normative: reject calendar and time zone IDs that are not strings or objects May 18, 2023
@justingrant justingrant changed the title Normative: reject calendar and time zone IDs that are not strings or objects Normative: reject calendar IDs, time zone IDs, and offset values that are not strings nor or objects May 18, 2023
@justingrant justingrant marked this pull request as draft May 18, 2023 00:24
@justingrant justingrant changed the title Normative: reject calendar IDs, time zone IDs, and offset values that are not strings nor or objects Normative: reject calendar IDs, time zone IDs (and maybe offset values?) that are not strings nor or objects May 18, 2023
@justingrant
Copy link
Collaborator Author

I updated the OP to add cases of offset and the code samples in the comments above. I'm still curious why @anba's Temporal.Calendar.from(10).id example doesn't throw. It does throw in the Temporal polyfill, FWIW.

@justingrant justingrant changed the title Normative: reject calendar IDs, time zone IDs (and maybe offset values?) that are not strings nor or objects Normative: reject calendar IDs, time zone IDs (and maybe offset values?) that are not strings nor objects May 18, 2023
@anba
Copy link
Contributor

anba commented May 18, 2023

Temporal.Calendar.from(10).id is parsed successfully via:
ParseTemporalCalendarString("10")
 → ParseISODateTime("10")
  → ParseText(StringToCodePoints("10"), goal=TemporalTimeString)

Where TemporalTimeString takes the following productions:
AnnotatedTime
 → TimeSpecWithOptionalOffsetNotAmbiguous
  → TimeSpec
   → TimeHour
    → Hour
     → 1 DecimalDigit

That case was a test which I still need to upstream to test262:

// Copyright (C) 2023 André Bargull. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.

/*---
esid: sec-temporal-totemporalcalendarslotvalue
description: >
  Parse calendar from numeric values.
info: |
  ToTemporalCalendarSlotValue ( temporalCalendarLike [ , default ] )
  ...
  3. Let identifier be ? ToString(temporalCalendarLike).
  4. Set identifier to ? ParseTemporalCalendarString(identifier).
  ...
features: [Temporal]
---*/

const validDateTimeLike = [
  // TemporalTimeString : AnnotatedTime : TimeSpecWithOptionalOffsetNotAmbiguous
  10,
  11,
  23,

  // TemporalTimeString : AnnotatedTime : TimeSpecWithOptionalOffsetNotAmbiguous
  10_00,
  10_32,
  10_59,
  11_00,
  11_31,
  11_59,
  12_00,
  12_32,
  12_59,
  23_59,
  13_00,
  13_01,
  13_59,

  // TemporalMonthDayString : AnnotatedMonthDay : DateSpecMonthDay
  10_01,
  10_31,
  11_01,
  11_30,
  12_01,
  12_31,

  // TemporalYearMonthString : AnnotatedYearMonth : DateSpecYearMonth
  1000_01,
  1000_12,
  1100_01,
  1100_12,
  2200_01,
  2200_12,
  2300_01,
  2300_12,
  2400_01,
  2400_12,
  9999_01,
  9999_12,

  // TemporalTimeString : AnnotatedTime : TimeSpecWithOptionalOffsetNotAmbiguous
  10_00_00,
  10_00_13,
  10_00_59,
  10_00_60,
  11_00_00,
  11_00_13,
  11_00_59,
  11_00_60,
  22_00_00,
  22_00_13,
  22_00_59,
  22_00_60,
  23_00_00,
  23_00_13,
  23_00_59,
  23_00_60,

  // TemporalYearMonthString : AnnotatedYearMonth : DateSpecYearMonth
  -100000_01,
  -100000_12,
  -999912_01,
  -999912_12,
  -999999_01,
  -999999_12,

  // TemporalDateTimeString : AnnotatedDateTime : DateTime : Date
  1000_01_01,
  1000_01_31,
  1000_02_28,
  1000_03_01,
  1000_03_31,
  1000_04_01,
  1000_03_30,
  1000_12_01,
  1000_12_31,

  // Leap year.
  1004_02_29,

  // Maximum four digit year.
  9999_01_01,
  9999_01_31,
  9999_02_28,
  9999_03_01,
  9999_03_31,
  9999_04_01,
  9999_04_30,
  9999_12_01,
  9999_12_31,

  // Six digit years.
  -100000_01_01,
  -100000_12_31,
  -999999_01_01,
  -999999_12_31,
];

for (let valid of validDateTimeLike) {
  assert.sameValue(Temporal.Calendar.from(valid).id, "iso8601");

  // Also test with BigInt.
  assert.sameValue(Temporal.Calendar.from(BigInt(valid)).id, "iso8601");

  // And finally test with String.
  assert.sameValue(Temporal.Calendar.from(String(valid)).id, "iso8601");
}

const invalidDateTimeLike = [
  0,
  1,
  9,
  24,
  99,

  100,
  101,
  999,

  10_60,
  10_99,
  11_60,
  11_99,
  12_60,
  12_99,
  13_60,
  13_99,

  24_00,
  24_01,
  24_99,
  99_99,

  1_00_00,
  1_00_01,
  1_01_01,
  1_59_59,
  1_99_99,

  9_00_00,
  9_00_01,
  9_01_01,
  9_59_59,
  9_99_99,

  10_00_61,
  10_00_99,
  11_00_61,
  11_00_99,
  22_00_61,
  22_00_99,
  23_00_61,
  23_00_99,
  24_00_00,
  24_00_13,
  24_00_59,
  24_00_60,
  24_00_61,
  24_00_99,

  -100_01_01,
  -999_01_01,
  +100_01_01,
  +999_01_01,

  -100000_00,
  -100000_13,
  -999912_00,
  -999912_13,
  -999999_00,
  -999999_13,

  +100000_00,
  +100000_01,
  +100000_12,
  +100000_13,
  +999999_00,
  +999999_01,
  +999999_12,
  +999999_13,

  1000_00_00,
  1000_00_01,
  1000_01_00,
  1000_01_32,
  1000_02_00,
  1000_02_29,
  1000_03_00,
  1000_03_32,
  1000_04_00,
  1000_04_31,
  1000_12_00,
  1000_12_32,
  1000_13_00,
  1000_13_01,

  9999_00_00,
  9999_00_01,
  9999_01_00,
  9999_01_32,
  9999_02_00,
  9999_02_29,
  9999_03_00,
  9999_03_32,
  9999_04_00,
  9999_04_31,
  9999_12_00,
  9999_12_32,
  9999_13_00,
  9999_13_01,

  -100000_01_00,
  -100000_12_32,
  -999999_01_00,
  -999999_12_32,

  +100000_01_01,
  +100000_12_31,
  +999999_01_01,
  +999999_12_31,
];

for (let invalid of invalidDateTimeLike) {
  assert.throws(RangeError, () => Temporal.Calendar.from(invalid));

  // Also test with BigInt.
  assert.throws(RangeError, () => Temporal.Calendar.from(BigInt(invalid)));

  // And finally test with String.
  assert.throws(RangeError, () => Temporal.Calendar.from(String(invalid)));
}

And a similar test for time zone strings:

// Copyright (C) 2023 André Bargull. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.

/*---
esid: sec-temporal-totemporaltimezoneslotvalue
description: >
  Parse time zone offset from numeric values.
info: |
  ToTemporalTimeZoneSlotValue ( temporalTimeZoneLike )
  ...
  2. ToTemporalTimeZoneSlotValue ( temporalTimeZoneLike )
  3. Let parseResult be ? ParseTemporalTimeZoneString(identifier).
  ...
features: [Temporal]
---*/

const validOffsets = [
  [-10, "-10:00"],
  [-23, "-23:00"],

  [-1000, "-10:00"],
  [-1059, "-10:59"],
  [-2300, "-23:00"],
  [-2359, "-23:59"],

  [-100000, "-10:00"],
  [-105959, "-10:59:59"],
  [-230000, "-23:00"],
  [-235959, "-23:59:59"],

  [-10n, "-10:00"],
  [-23n, "-23:00"],

  [-1000n, "-10:00"],
  [-1059n, "-10:59"],
  [-2300n, "-23:00"],
  [-2359n, "-23:59"],

  [-100000n, "-10:00"],
  [-105959n, "-10:59:59"],
  [-230000n, "-23:00"],
  [-235959n, "-23:59:59"],
];

for (let [valid, canonical] of validOffsets) {
  assert.sameValue(Temporal.TimeZone.from(valid).id, canonical);

  // Also test with BigInt.
  assert.sameValue(Temporal.TimeZone.from(BigInt(valid)).id, canonical);

  // And finally test with String.
  assert.sameValue(Temporal.TimeZone.from(String(valid)).id, canonical);
}

const invalidOffsets = [
  // Positive numbers can't be parsed as a time zone offset.
  +10,
  +23,

  +1000,
  +1059,
  +2300,
  +2359,

  +100000,
  +105959,
  +230000,
  +235959,

  // ToString representation can be parsed as a date-time string, but doesn't
  // contain any time zone information.
  -123456_01_01,

  // ToString representation can be parsed as a month-day string, but doesn't
  // contain any time zone information.
  11_11,

  // ToString representation can be parsed as a year-month string, but doesn't
  // contain any time zone information.
  1111_11,

  // ToString representation can be parsed as a time string, but doesn't
  // contain any time zone information.
  11,
  11_00,
  11_59,
];

for (let invalid of invalidOffsets) {
  assert.throws(RangeError, () => Temporal.TimeZone.from(invalid));

  // Also test with BigInt.
  assert.throws(RangeError, () => Temporal.TimeZone.from(BigInt(invalid)));

  // And finally test with String.
  assert.throws(RangeError, () => Temporal.TimeZone.from(String(invalid)));
}

@anba
Copy link
Contributor

anba commented May 18, 2023

@anba Can you say more about your reason for supporting this change? Does it solve a problem or enable any particular optimization in Firefox?

No, it doesn't enable any optimisations. Earlier this year when I was trying to improve error messages for invalid inputs to ToTemporal(Calendar|TimeZone), I was first rejecting any non-string input, but then I realised that certain Number/BigInt values are actually allowed. That led to creating the test cases shown in my previous comment.

I support this change, because it may be confusing for users when only certain numeric values are allowed. And it's not always easy to determine which numeric values can actually be parsed. For example most users should understand why Temporal.PlainDate.from(20230501) works, whereas Temporal.Calendar.from(...) is more tricky to understand due to the various parse goals which are tried in ParseISODateTime.

@justingrant
Copy link
Collaborator Author

justingrant commented May 25, 2023

Meeting 2023-05-25: Consensus of the champions was that if implementers think this is important to fix, then we'll fix it. Where "fix" means to throw a TypeError for non-string inputs when parsing ISO strings or ISO string components.

The champions' consensus was that if we do fix it, then we should throw for all Temporal types instead of only throwing when parsing the most confusing strings: calendars, time zones, and UTC offsets.

The rationale was that all Temporal types have numeric values that seem like they should work but which yield invalid ISO strings. Examples:

  • Temporal.PlainDate.from(-5000101) throws instead of Jan 1 in 499 BCE.
  • Temporal.PlainMonthDay.from(525) (or Temporal.PlainMonthDay.from(0525) is even worse because 0525 in non-strict mode is an octal!)

The champions are aware that there may be precedent to treat 8-digit numbers as dates, but those precedents are sufficiently brittle (e.g. years before 999 or after 9999) and unpredictably ambiguous (is the number interpreted as an ISO string, minutes like Date.p.getTimezoneOffset(), or as a number of milliseconds like Date ?) that we think the caller should explicitly cast to a string themselves.

@anba, you previously expressed support for this change when it was limited to only calendars, time zones, and UTC offsets. Do you also support throwing for parsing of non-string primitive values for all Temporal types?

@justingrant justingrant changed the title Normative: reject calendar IDs, time zone IDs (and maybe offset values?) that are not strings nor objects Normative: don't coerce non-strings to strings before parsing ISO strings (esp. calendars, time zones and offsets) May 25, 2023
@justingrant justingrant force-pushed the remove-tz-cal-tostring branch 4 times, most recently from a4b9604 to c64ad0e Compare May 25, 2023 21:46
@justingrant
Copy link
Collaborator Author

PR is now updated with the consensus from today's meeting. I'll add polyfill and tests changes if we get 👍 feedback from @anba about the spec changes.

spec/abstractops.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I didn't have time to check if this covers every place where the conversion needs to be performed, but I'm pretty sure Richard would've noticed if that wasn't the case!

polyfill/lib/calendar.mjs Show resolved Hide resolved
spec/instant.html Outdated Show resolved Hide resolved
spec/abstractops.html Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Show resolved Hide resolved
spec/instant.html Outdated Show resolved Hide resolved
spec/plaindate.html Show resolved Hide resolved
@ptomato
Copy link
Collaborator

ptomato commented Jul 13, 2023

This PR reached consensus at the TC39 plenary on 2023-07-12. Give me a shout when the editorial comments are resolved, and I'll merge tc39/test262#3847 then push an update to the test262 submodule to this branch.

@ptomato ptomato mentioned this pull request Jul 13, 2023
91 tasks
justingrant added a commit to justingrant/test262 that referenced this pull request Jul 16, 2023
Edits Temporal tests to account for changes in
tc39/proposal-temporal#2574.

This PR stops coercing non-string primitive inputs to strings
in Temporal methods, to avoid cases where numbers
are coerced to syntactically valid but often unexpected
string results.
@tc39 tc39 deleted a comment from gibson042 Jul 16, 2023
@justingrant justingrant force-pushed the remove-tz-cal-tostring branch 2 times, most recently from 278a5fb to 98873f7 Compare July 16, 2023 04:36
@justingrant justingrant marked this pull request as ready for review July 16, 2023 04:40
justingrant added a commit to justingrant/test262 that referenced this pull request Jul 17, 2023
Edits Temporal tests to account for changes in
tc39/proposal-temporal#2574.

This PR stops coercing non-string primitive inputs to strings
in Temporal methods, to avoid cases where numbers
are coerced to syntactically valid but often unexpected
string results.
justingrant added a commit to justingrant/test262 that referenced this pull request Jul 17, 2023
Edits Temporal tests to account for changes in
tc39/proposal-temporal#2574.

This PR stops coercing non-string primitive inputs to strings
in Temporal methods, to avoid cases where numbers
are coerced to syntactically valid but often unexpected
string results.
This commit stops converting non-string inputs to strings when
parsing ISO strings or property bag fields.

When numbers are coerced to strings, the result is sometimes a valid
ISO string subset but it often doesn't behave as expected. The result is
often brittle, yielding "data driven exceptions" that we've tried to
avoid.

Numbers can also be ambiguous, e.g. is a Number passed for
`offset` mean hours like an ISO string, minutes like
Date.p.getTimezoneOffset, or msecs like Date.p.getTime().

This commit also removes coercing objects to strings in the
same contexts (like TimeZone and Calendar constructors) because
it's unclear that there are use cases for this coercion.
@justingrant
Copy link
Collaborator Author

justingrant commented Jul 17, 2023

This PR reached consensus at the TC39 plenary on 2023-07-12. Give me a shout when the editorial comments are resolved, and I'll merge tc39/test262#3847 then push an update to the test262 submodule to this branch.

All review comments are resolved. Note that this PR overlaps with #2607, and I don't think tc39/test262#3847 (this PR's tests) has been reviewed yet. So maybe it makes sense to merge #2607 first and then I can rebase this PR (and its tests) on top?

ptomato pushed a commit to tc39/test262 that referenced this pull request Jul 18, 2023
Edits Temporal tests to account for changes in
tc39/proposal-temporal#2574.

This PR stops coercing non-string primitive inputs to strings
in Temporal methods, to avoid cases where numbers
are coerced to syntactically valid but often unexpected
string results.
@ptomato ptomato merged commit 9253db8 into tc39:main Jul 18, 2023
5 checks passed
justingrant added a commit that referenced this pull request Jul 20, 2023
Adjust docs for Temporal.Calendar for a few reasons: 
* Account for #2574 which stops coercing non-string inputs
* Fix inaccurate claims in the constructor docs
* Reorganize sample code for `from` for clarity
Ms2ger pushed a commit that referenced this pull request Jul 24, 2023
Adjust docs for Temporal.Calendar for a few reasons: 
* Account for #2574 which stops coercing non-string inputs
* Fix inaccurate claims in the constructor docs
* Reorganize sample code for `from` for clarity
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 13, 2023
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting-agenda normative Would be a normative change to the proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants