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.

Sign up for GitHub

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

Jump to bottom

TimeZoneUTCOffsetName and Etc/GMT are ignored in ParseTemporalTimeZoneString #1805

Closed
Constellation opened this issue Sep 6, 2021 · 6 comments · Fixed by #2200
Closed

TimeZoneUTCOffsetName and Etc/GMT are ignored in ParseTemporalTimeZoneString #1805

Constellation opened this issue Sep 6, 2021 · 6 comments · Fixed by #2200
Assignees
@ptomato
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Milestone
Next batch of nor...

Comments

@Constellation
Copy link
Member

ParseTemporalTimeZoneString only cares TimeZoneIANAName.
https://tc39.es/proposal-temporal/#sec-temporal-parsetemporaltimezonestring

However, there are other two productions.

TimeZoneBracketedName :
    TimeZoneIANAName
    Etc/GMT ASCIISign Hour
    TimeZoneUTCOffsetName

So, if we use [Etc/GMT-09] etc. then it is ignored in ParseTemporalTimeZoneString.
@justingrant
Copy link
Collaborator

This seems like a duplicate (or at least closely related to) the discussion here: #1797 (comment). Want to continue discussion over there?

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Sep 8, 2021
@Constellation
[JSC] Implement Temporal.PlainTime
3b99c5b 
https://bugs.webkit.org/show_bug.cgi?id=229892

Reviewed by Darin Adler.

JSTests:

* stress/temporal-calendar.js:
* stress/temporal-duration.js:
* stress/temporal-plaintime.js: Added.
(shouldBe):
(shouldThrow):
(shouldBe.String.Temporal.PlainTime.from):
(let.time.Temporal.PlainTime.from.shouldBe):
(let.text.of.failures.shouldThrow):
(print):
(shouldBe.Temporal.PlainTime.from):
(new.Temporal.PlainTime.valueOf):
(shouldBe.String.time.until.Temporal.PlainTime.from):
* stress/temporal-timezone.js:
(let.text.of.failures.shouldThrow): Deleted.
* test262/config.yaml:

Source/JavaScriptCore:

This patch implements Temporal.PlainTime[1]. This is time representation which is not associated to
calendars and timezones. This is tuple of hour, minute, second, millisecond, microsecond, and nanosecond.

1. We add full-fledged ISO8601 DateTime / Time parser, so that Temporal.PlainTime.from can extract
   time as specified.

2. ISO8601::PlainTime is used for already-validated PlainTime data. When performing arithmetics, we first
   do that in ISO8601::Duration, and then we validate and convert it to PlainTime.

We also found several spec issues, and reported in [2,3,4].

[1]: https://tc39.es/proposal-temporal/#sec-temporal-plaintime-objects
[2]: tc39/proposal-temporal#1803
[3]: tc39/proposal-temporal#1804
[4]: tc39/proposal-temporal#1805

* CMakeLists.txt:
* DerivedSources-input.xcfilelist:
* DerivedSources-output.xcfilelist:
* DerivedSources.make:
* Sources.txt:
* runtime/CommonIdentifiers.h:
* runtime/ISO8601.cpp:
(JSC::ISO8601::parseTimeZoneName):
(JSC::ISO8601::parseDecimalInt32):
(JSC::ISO8601::parseDuration):
(JSC::ISO8601::parseTimeSpec):
(JSC::ISO8601::parseTimeZoneNumericUTCOffset):
(JSC::ISO8601::parseTimeZoneBracketedAnnotation):
(JSC::ISO8601::canBeTimeZone):
(JSC::ISO8601::parseTimeZone):
(JSC::ISO8601::parseTime):
(JSC::ISO8601::daysInMonth):
(JSC::ISO8601::parseDate):
(JSC::ISO8601::parseDateTime):
(JSC::ISO8601::formatTimeZoneOffsetString):
(JSC::ISO8601::temporalTimeToString):
(JSC::ISO8601::isValidDuration):
* runtime/ISO8601.h:
(JSC::ISO8601::Duration::Duration):
(JSC::ISO8601::Duration::operator[]):
(JSC::ISO8601::Duration::operator[] const):
(JSC::ISO8601::Duration::begin const):
(JSC::ISO8601::Duration::end const):
(JSC::ISO8601::Duration::clear):
(JSC::ISO8601::Duration::operator- const):
(JSC::ISO8601::PlainTime::PlainTime):
(JSC::ISO8601::PlainTime::operator==):
(JSC::ISO8601::PlainDate::PlainDate):
(JSC::ISO8601::PlainDate::year const):
(JSC::ISO8601::PlainDate::month const):
(JSC::ISO8601::PlainDate::day const):
* runtime/IntlObject.cpp:
(JSC::utcTimeZoneIDSlow):
* runtime/IntlObject.h:
(JSC::utcTimeZoneID):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildrenImpl):
* runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::plainTimeStructure):
* runtime/TemporalCalendarConstructor.cpp:
(JSC::TemporalCalendarConstructor::finishCreation):
* runtime/TemporalDuration.cpp:
(JSC::TemporalDuration::tryCreateIfValid):
(JSC::TemporalDuration::fromNonDurationValue):
(JSC::TemporalDuration::toDuration):
(JSC::TemporalDuration::toDurationRecord):
(JSC::TemporalDuration::toString const):
(JSC::TemporalDuration::toString):
(JSC::isValidDuration): Deleted.
(JSC::TemporalDuration::fromObject): Deleted.
* runtime/TemporalDuration.h:
* runtime/TemporalNow.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/TemporalObject.cpp:
(JSC::createPlainTimeConstructor):
(JSC::secondsStringPrecision):
(JSC::toTemporalOverflow):
* runtime/TemporalObject.h:
* runtime/TemporalPlainTime.cpp: Added.
(JSC::TemporalPlainTime::create):
(JSC::TemporalPlainTime::createStructure):
(JSC::TemporalPlainTime::TemporalPlainTime):
(JSC::TemporalPlainTime::finishCreation):
(JSC::TemporalPlainTime::visitChildrenImpl):
(JSC::toPlainTime):
(JSC::TemporalPlainTime::tryCreateIfValid):
(JSC::nonNegativeModulo):
(JSC::balanceTime):
(JSC::roundTime):
(JSC::TemporalPlainTime::round const):
(JSC::TemporalPlainTime::toString const):
(JSC::propertyName):
(JSC::toTemporalTimeRecord):
(JSC::toPartialTime):
(JSC::constraintTime):
(JSC::regulateTime):
(JSC::toTemporalCalendarWithISODefault):
(JSC::getTemporalCalendarWithISODefault):
(JSC::TemporalPlainTime::from):
(JSC::TemporalPlainTime::compare):
(JSC::toLimitedTemporalDuration):
(JSC::addTime):
(JSC::TemporalPlainTime::add const):
(JSC::TemporalPlainTime::subtract const):
(JSC::TemporalPlainTime::with const):
(JSC::differenceTime):
(JSC::extractDifferenceOptions):
(JSC::TemporalPlainTime::until const):
(JSC::TemporalPlainTime::since const):
* runtime/TemporalPlainTime.h: Added.
* runtime/TemporalPlainTimeConstructor.cpp: Added.
(JSC::TemporalPlainTimeConstructor::create):
(JSC::TemporalPlainTimeConstructor::createStructure):
(JSC::TemporalPlainTimeConstructor::TemporalPlainTimeConstructor):
(JSC::TemporalPlainTimeConstructor::finishCreation):
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/TemporalPlainTimeConstructor.h: Copied from Source/JavaScriptCore/runtime/TemporalTimeZone.h.
* runtime/TemporalPlainTimePrototype.cpp: Added.
(JSC::TemporalPlainTimePrototype::create):
(JSC::TemporalPlainTimePrototype::createStructure):
(JSC::TemporalPlainTimePrototype::TemporalPlainTimePrototype):
(JSC::TemporalPlainTimePrototype::finishCreation):
(JSC::JSC_DEFINE_HOST_FUNCTION):
(JSC::JSC_DEFINE_CUSTOM_GETTER):
* runtime/TemporalPlainTimePrototype.h: Copied from Source/JavaScriptCore/runtime/TemporalTimeZone.h.
* runtime/TemporalTimeZone.cpp:
(JSC::TemporalTimeZone::from):
(JSC::TemporalTimeZone::idForTimeZoneName): Deleted.
* runtime/TemporalTimeZone.h:
* runtime/TemporalTimeZoneConstructor.cpp:
(JSC::TemporalTimeZoneConstructor::finishCreation):
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/VM.cpp:
* runtime/VM.h:

Source/WTF:

* wtf/text/IntegerToStringConversion.h:
* wtf/text/StringParsingBuffer.h:

Canonical link: https://commits.webkit.org/241422@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@282125 268f45cc-cd09-0410-ab3c-d52691b4dbfc
philn pushed a commit to philn/old-webkit that referenced this issue Sep 14, 2021
[JSC] Implement Temporal.PlainTime
5a6031d 
https://bugs.webkit.org/show_bug.cgi?id=229892

Reviewed by Darin Adler.

JSTests:

* stress/temporal-calendar.js:
* stress/temporal-duration.js:
* stress/temporal-plaintime.js: Added.
(shouldBe):
(shouldThrow):
(shouldBe.String.Temporal.PlainTime.from):
(let.time.Temporal.PlainTime.from.shouldBe):
(let.text.of.failures.shouldThrow):
(print):
(shouldBe.Temporal.PlainTime.from):
(new.Temporal.PlainTime.valueOf):
(shouldBe.String.time.until.Temporal.PlainTime.from):
* stress/temporal-timezone.js:
(let.text.of.failures.shouldThrow): Deleted.
* test262/config.yaml:

Source/JavaScriptCore:

This patch implements Temporal.PlainTime[1]. This is time representation which is not associated to
calendars and timezones. This is tuple of hour, minute, second, millisecond, microsecond, and nanosecond.

1. We add full-fledged ISO8601 DateTime / Time parser, so that Temporal.PlainTime.from can extract
   time as specified.

2. ISO8601::PlainTime is used for already-validated PlainTime data. When performing arithmetics, we first
   do that in ISO8601::Duration, and then we validate and convert it to PlainTime.

We also found several spec issues, and reported in [2,3,4].

[1]: https://tc39.es/proposal-temporal/#sec-temporal-plaintime-objects
[2]: tc39/proposal-temporal#1803
[3]: tc39/proposal-temporal#1804
[4]: tc39/proposal-temporal#1805

* CMakeLists.txt:
* DerivedSources-input.xcfilelist:
* DerivedSources-output.xcfilelist:
* DerivedSources.make:
* Sources.txt:
* runtime/CommonIdentifiers.h:
* runtime/ISO8601.cpp:
(JSC::ISO8601::parseTimeZoneName):
(JSC::ISO8601::parseDecimalInt32):
(JSC::ISO8601::parseDuration):
(JSC::ISO8601::parseTimeSpec):
(JSC::ISO8601::parseTimeZoneNumericUTCOffset):
(JSC::ISO8601::parseTimeZoneBracketedAnnotation):
(JSC::ISO8601::canBeTimeZone):
(JSC::ISO8601::parseTimeZone):
(JSC::ISO8601::parseTime):
(JSC::ISO8601::daysInMonth):
(JSC::ISO8601::parseDate):
(JSC::ISO8601::parseDateTime):
(JSC::ISO8601::formatTimeZoneOffsetString):
(JSC::ISO8601::temporalTimeToString):
(JSC::ISO8601::isValidDuration):
* runtime/ISO8601.h:
(JSC::ISO8601::Duration::Duration):
(JSC::ISO8601::Duration::operator[]):
(JSC::ISO8601::Duration::operator[] const):
(JSC::ISO8601::Duration::begin const):
(JSC::ISO8601::Duration::end const):
(JSC::ISO8601::Duration::clear):
(JSC::ISO8601::Duration::operator- const):
(JSC::ISO8601::PlainTime::PlainTime):
(JSC::ISO8601::PlainTime::operator==):
(JSC::ISO8601::PlainDate::PlainDate):
(JSC::ISO8601::PlainDate::year const):
(JSC::ISO8601::PlainDate::month const):
(JSC::ISO8601::PlainDate::day const):
* runtime/IntlObject.cpp:
(JSC::utcTimeZoneIDSlow):
* runtime/IntlObject.h:
(JSC::utcTimeZoneID):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildrenImpl):
* runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::plainTimeStructure):
* runtime/TemporalCalendarConstructor.cpp:
(JSC::TemporalCalendarConstructor::finishCreation):
* runtime/TemporalDuration.cpp:
(JSC::TemporalDuration::tryCreateIfValid):
(JSC::TemporalDuration::fromNonDurationValue):
(JSC::TemporalDuration::toDuration):
(JSC::TemporalDuration::toDurationRecord):
(JSC::TemporalDuration::toString const):
(JSC::TemporalDuration::toString):
(JSC::isValidDuration): Deleted.
(JSC::TemporalDuration::fromObject): Deleted.
* runtime/TemporalDuration.h:
* runtime/TemporalNow.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/TemporalObject.cpp:
(JSC::createPlainTimeConstructor):
(JSC::secondsStringPrecision):
(JSC::toTemporalOverflow):
* runtime/TemporalObject.h:
* runtime/TemporalPlainTime.cpp: Added.
(JSC::TemporalPlainTime::create):
(JSC::TemporalPlainTime::createStructure):
(JSC::TemporalPlainTime::TemporalPlainTime):
(JSC::TemporalPlainTime::finishCreation):
(JSC::TemporalPlainTime::visitChildrenImpl):
(JSC::toPlainTime):
(JSC::TemporalPlainTime::tryCreateIfValid):
(JSC::nonNegativeModulo):
(JSC::balanceTime):
(JSC::roundTime):
(JSC::TemporalPlainTime::round const):
(JSC::TemporalPlainTime::toString const):
(JSC::propertyName):
(JSC::toTemporalTimeRecord):
(JSC::toPartialTime):
(JSC::constraintTime):
(JSC::regulateTime):
(JSC::toTemporalCalendarWithISODefault):
(JSC::getTemporalCalendarWithISODefault):
(JSC::TemporalPlainTime::from):
(JSC::TemporalPlainTime::compare):
(JSC::toLimitedTemporalDuration):
(JSC::addTime):
(JSC::TemporalPlainTime::add const):
(JSC::TemporalPlainTime::subtract const):
(JSC::TemporalPlainTime::with const):
(JSC::differenceTime):
(JSC::extractDifferenceOptions):
(JSC::TemporalPlainTime::until const):
(JSC::TemporalPlainTime::since const):
* runtime/TemporalPlainTime.h: Added.
* runtime/TemporalPlainTimeConstructor.cpp: Added.
(JSC::TemporalPlainTimeConstructor::create):
(JSC::TemporalPlainTimeConstructor::createStructure):
(JSC::TemporalPlainTimeConstructor::TemporalPlainTimeConstructor):
(JSC::TemporalPlainTimeConstructor::finishCreation):
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/TemporalPlainTimeConstructor.h: Copied from Source/JavaScriptCore/runtime/TemporalTimeZone.h.
* runtime/TemporalPlainTimePrototype.cpp: Added.
(JSC::TemporalPlainTimePrototype::create):
(JSC::TemporalPlainTimePrototype::createStructure):
(JSC::TemporalPlainTimePrototype::TemporalPlainTimePrototype):
(JSC::TemporalPlainTimePrototype::finishCreation):
(JSC::JSC_DEFINE_HOST_FUNCTION):
(JSC::JSC_DEFINE_CUSTOM_GETTER):
* runtime/TemporalPlainTimePrototype.h: Copied from Source/JavaScriptCore/runtime/TemporalTimeZone.h.
* runtime/TemporalTimeZone.cpp:
(JSC::TemporalTimeZone::from):
(JSC::TemporalTimeZone::idForTimeZoneName): Deleted.
* runtime/TemporalTimeZone.h:
* runtime/TemporalTimeZoneConstructor.cpp:
(JSC::TemporalTimeZoneConstructor::finishCreation):
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/VM.cpp:
* runtime/VM.h:

Source/WTF:

* wtf/text/IntegerToStringConversion.h:
* wtf/text/StringParsingBuffer.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@282125 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@ptomato
Copy link
Collaborator

ptomato commented Sep 15, 2021

Dunno if this is a duplicate - it seems like a related but separate normative change that we should use TimeZoneBracketedName in ParseTemporalTimeZoneString.

@ptomato ptomato mentioned this issue Sep 16, 2021
Merged
@ptomato ptomato mentioned this issue Dec 1, 2021
Closed
@ptomato
Copy link
Collaborator

ptomato commented Dec 1, 2021

From the other duplicate #1924, this bug causes the assertion in step 5.d of ToTemporalZonedDateTime to fail, as well as the following test262 tests:

  • test262/built-ins/Temporal/ZonedDateTime/from/zoneddatetime-sub-minute-offset
  • test262/built-ins/Temporal/ZonedDateTime/prototype/equals/sub-minute-offset
  • test262/built-ins/Temporal/ZonedDateTime/prototype/since/sub-minute-offset
  • test262/built-ins/Temporal/ZonedDateTime/prototype/until/sub-minute-offset

@ptomato
Copy link
Collaborator

ptomato commented Dec 1, 2021

#1941 is an in-progress attempt to fix this.

@wirthi
Copy link
Contributor

wirthi commented Dec 17, 2021

I second that bug.

The newly added built-ins/Temporal/Instant/prototype/toZonedDateTime/timezone-string-multiple-offsets.js and similar also fail because of this.

@ptomato ptomato mentioned this issue Mar 18, 2022
Merged
@ptomato ptomato added spec-text Specification text involved normative Would be a normative change to the proposal labels May 10, 2022
ptomato added a commit that referenced this issue May 14, 2022
@ptomato
Normative: Fix production for time zone name in time zone strings
0e7bb9a 
ParseTemporalTimeZoneString erroneously assumed that any bare time zone
identifier or bracketed name would be an IANA name. That is not the case,
it can be a UTC offset name as well.

Closes: #1805
@ptomato ptomato mentioned this issue May 14, 2022
Merged
@ptomato ptomato self-assigned this May 14, 2022
@ptomato
Copy link
Collaborator

ptomato commented May 14, 2022

@Constellation @wirthi @FrankYFTang After all this time, I think #2200 is all that's needed to fix this. Would you mind taking a look and confirming?

ptomato added a commit that referenced this issue May 17, 2022
@ptomato
Normative: Fix production for time zone name in time zone strings
2900ad7 
ParseTemporalTimeZoneString erroneously assumed that any bare time zone
identifier or bracketed name would be an IANA name. That is not the case,
it can be a UTC offset name as well.

Closes: #1805
@ptomato ptomato mentioned this issue May 17, 2022
Closed
ptomato added a commit that referenced this issue Jun 23, 2022
@ptomato
Normative: Fix production for time zone name in time zone strings
cf7b9f1 
ParseTemporalTimeZoneString erroneously assumed that any bare time zone
identifier or bracketed name would be an IANA name. That is not the case,
it can be a UTC offset name as well.

Closes: #1805
ptomato added a commit that referenced this issue Jun 26, 2022
@ptomato
Normative: Fix production for time zone name in time zone strings
caf013a 
ParseTemporalTimeZoneString erroneously assumed that any bare time zone
identifier or bracketed name would be an IANA name. That is not the case,
it can be a UTC offset name as well.

Closes: #1805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ptomato ptomato

Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Milestone
Next batch of normative changes
Development

Successfully merging a pull request may close this issue.

Normative: Fix production for time zone name in time zone strings tc39/proposal-temporal
4 participants
@Constellation @justingrant @ptomato @wirthi