Document all ISO-8601 date parsing extensions

RESOLVED FIXED in Firefox 58

Status

()

--
trivial
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: anba, Assigned: tephra)

Tracking

(Blocks: 1 bug)

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

date_parseISOString() in jsdate.cpp contains three extensions to the simplified ISO-8601 date format specified in ES5.1, all but one are undocumented. Extensions are somewhat unwanted (cf. bug 791320), so they should be documented in jsdate.cpp at least.

1. Timezone specifier without ":" -> the only documented extension (see bug 682754)
2. time-only forms -> leftover from the switch ES5 to ES5.1; not documented
3. fractional seconds -> exactly three decimal digits per ES5.1, currently implemented `at least one` decimal digit; not documented
(Reporter)

Updated

6 years ago
Severity: normal → trivial
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee: general → nobody
(Reporter)

Updated

2 years ago
Blocks: 1274354
Component: JavaScript Engine → JavaScript: Standard Library
(Assignee)

Updated

a year ago
Assignee: nobody → eric
(Assignee)

Updated

a year ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
mozreview-review
Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review195556

Thanks for taking up this bug! I've left a few comments to note which aspects can still be improved or should also be documented.

::: js/src/jsdate.cpp:789
(Diff revision 1)
> - * profile of the ISO 8601 format. Quoted here:
> - *
> - *   Any combination of the date formats with the time formats is
> - *   allowed, and also either the date or the time can be missing.
> - *
> - *   The specification is silent on the meaning when fields are
> + * of the ISO 8601 Extended Format. As per the spec omitted month and day
> + * values are defaulted to '01', omitted HH:mm:ss values are defaulted to '00'
> + * and an omitted sss field is defaulted to '000'.
> + *
> + * For cross compatibility we allow a few extensions to the Ecmascript ISO
> + * style date format.

I'd just stick with:
> For cross compatibility we allow the following extensions.

to avoid the stringing together "Ecmascript ISO style date format"

::: js/src/jsdate.cpp:798
(Diff revision 1)
> - *   missing entirely then we assume 1970-01-01 so that the time can
> - *   be aded to a date later. If the time is missing then we assume
> - *   00:00 UTC.  If the time is present but the time zone field is
> - *   missing then we use local time.
> - *
> - * For the sake of cross compatibility with other implementations we
> + *   Standalone time part:
> + *     Any of the time formats below can be parsed without a date part.
> + *     E.g "T19:00:00Z" will parse successfully. The date part will default
> + *     to 1970-01-01.
> + *
> + *   'T' from the time part can be replaces with one space:

"can be replaces" -> "may be replaced"

may sound more appropriate in this context and there's a typo in "replaced".

Also: "a space character" instead of "one space".

::: js/src/jsdate.cpp:799
(Diff revision 1)
> - *   be aded to a date later. If the time is missing then we assume
> - *   00:00 UTC.  If the time is present but the time zone field is
> - *   missing then we use local time.
> - *
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> + *     Any of the time formats below can be parsed without a date part.
> + *     E.g "T19:00:00Z" will parse successfully. The date part will default
> + *     to 1970-01-01.
> + *
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space

"a single space" instead of "one space" sounds better to me.

::: js/src/jsdate.cpp:800
(Diff revision 1)
> - *   00:00 UTC.  If the time is present but the time zone field is
> - *   missing then we use local time.
> - *
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> + *     E.g "T19:00:00Z" will parse successfully. The date part will default
> + *     to 1970-01-01.
> + *
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.

The either-or in "months, days, hours, minutes and seconds may be __either one or two__ digits long, [...]" is no longer documented, is it? For example the spec requires exactly two digits for the month, but we also accept a single digit.

::: js/src/jsdate.cpp:802
(Diff revision 1)
> - *
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:

Typo: "seconds" -> "milliseconds"

::: js/src/jsdate.cpp:803
(Diff revision 1)
> - * For the sake of cross compatibility with other implementations we
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *   'T' from the time part can be replaces with one space:
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional

"The specification requires exactly [...]" to stress the requirement aspect.

Nit: I'd spell out "three" instead of using the numeral.

::: js/src/jsdate.cpp:804
(Diff revision 1)
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional

Same here, "one" instead of "1".

::: js/src/jsdate.cpp:804
(Diff revision 1)
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional

Unfortunately clipping doesn't quite match our current implementation (bug 746529), but I don't really know how to describe the current implementation using only a few words, because I really don't want to mention IEEE 754 rounding here. :-)

::: js/src/jsdate.cpp:804
(Diff revision 1)
> - * make a few exceptions to the standard: months, days, hours, minutes
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     "1970-01-01 12:00:00Z" will parse successfully. Note that only one space
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional

The line exceeds 80 characters, please break it up into two lines:

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Other_whitespace

::: js/src/jsdate.cpp:805
(Diff revision 1)
> - * and seconds may be either one or two digits long, and the 'T' from
> - * the time part may be replaced with a space. Given that, a date time
> - * like "1999-1-1 1:1:1" will parse successfully.
> + *     is permitted and this is not permitted in the standalone version above.
> + *
> + *   One or more decimal digits for seconds:
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional
> + *     part. E.g a time part with "T19:00:00.1" will parse as "T19:00.00.100" and

Nit: "E.g." (dot after g)

::: js/src/jsdate.cpp:808
(Diff revision 1)
> + *     The specification allows for exactly 3 decimal digits for the fractional
> + *     second part. We allow 1 or more digits either padding or clipping the fractional
> + *     part. E.g a time part with "T19:00:00.1" will parse as "T19:00.00.100" and
> + *     "T19:00:00.9999" will parse as "T19:00:00.999".
> + *
> + *   Timezone specifier without ':':

Nit: "Time zone" instead of "Timezone".

::: js/src/jsdate.cpp:809
(Diff revision 1)
> + *     second part. We allow 1 or more digits either padding or clipping the fractional
> + *     part. E.g a time part with "T19:00:00.1" will parse as "T19:00.00.100" and
> + *     "T19:00:00.9999" will parse as "T19:00:00.999".
> + *
> + *   Timezone specifier without ':':
> + *     We allow the timezone to be specified without a ':'. E.g "T19:00:00+0700" is

Also "E.g." here.
Attachment #8919245 - Flags: review?(andrebargull) → review-
(Assignee)

Comment 3

a year ago
mozreview-review-reply
Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review195556

> The either-or in "months, days, hours, minutes and seconds may be __either one or two__ digits long, [...]" is no longer documented, is it? For example the spec requires exactly two digits for the month, but we also accept a single digit.

Missed this one. We have documented it in the end of the comment (after the "where:" part) but not as a extension. Will add a section about this.

> Unfortunately clipping doesn't quite match our current implementation (bug 746529), but I don't really know how to describe the current implementation using only a few words, because I really don't want to mention IEEE 754 rounding here. :-)

Oh I missed that bug. Two ideas spring to mind:

1. Just mention that we allow for more than one digit
2. Same as above but reference the bug (hoping to get to that bug soon) and then change to reflect when the bug is fixed

Not sure what the best course of action would be.
Comment hidden (mozreview-request)
(Reporter)

Comment 5

a year ago
(In reply to Eric Skoglund [:tephra] from comment #3)
> > Unfortunately clipping doesn't quite match our current implementation (bug 746529), but I don't really know how to describe the current implementation using only a few words, because I really don't want to mention IEEE 754 rounding here. :-)
> 
> Oh I missed that bug. Two ideas spring to mind:
> 
> 1. Just mention that we allow for more than one digit
> 2. Same as above but reference the bug (hoping to get to that bug soon) and
> then change to reflect when the bug is fixed
> 
> Not sure what the best course of action would be.

I don't have any preference at this point, so you're free to pick what ever is easier for you and requires the least amount of time (because I really don't think it's worthwhile to spend too much time on this one). :-)
(Reporter)

Comment 6

a year ago
mozreview-review
Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review196564

Looks good to me. Thanks for cleaning this up! :-)
Attachment #8919245 - Flags: review?(andrebargull) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
(Assignee)

Comment 8

a year ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> Autoland can't push this until all pending issues in MozReview are marked as
> resolved.

Hey Ryan, sorry it's been a while since I did some patches. I've resolved all issues now.
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Unfortunately, this also needs r+ from someone with Level 3 commit access in order for autoland to push it. Sorry for the hassle :(
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
(Reporter)

Comment 10

a year ago
Ah dang it, forgot about that autoland-only restriction.

Comment 11

a year ago
mozreview-review
Comment on attachment 8919245 [details]
bug 802760: Document all ISO-8601 date parsing exceptions

https://reviewboard.mozilla.org/r/190146/#review197392

rubberstamping for :anba
Attachment #8919245 - Flags: review+
(Reporter)

Comment 12

a year ago
Thanks :gandalf! :-)
(Assignee)

Comment 13

a year ago
Thanks for the help team! :)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 14

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b200a66205a2
Document all ISO-8601 date parsing exceptions r=anba,gandalf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b200a66205a2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.