Closed Bug 1205298 Opened 9 years ago Closed 9 years ago

Date.parse() should accept a wider range of potential formats

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox43 --- affected
firefox47 --- fixed

People

(Reporter: xidorn, Assigned: mrrrgn, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: site-compat, Whiteboard: [parity-edge][parity-blink][parity-webkit])

Attachments

(2 files, 3 obsolete files)

'YYYY-MM-DD HH:mm:ss' is a common time format in China, and since Chrome support that format, some of the websites start relying on this behavior, which cause compatibility issue for us.
Summary: Date.parse() should accept format 'YYYY-MM-DD HH:mm:ss' → Date.parse() should accept a wider range of potential formats
Whiteboard: [parity-blink] → [parity-edge][parity-blink][parity-webkit]
Attached file testcase
This page lists some cases that are not accepted by Firefox but accepted by Chrome and Edge.

(Note that this page requires Edge 13+, and doesn't work on the current Safari release)
According to a friend working in Alibaba, "90% of compatibility issues of Firefox on our internal system is due to this". [1]

[1] https://twitter.com/ayanamist/status/694854889730486272 (Chinese)
jorendorff, any chance we can get this fixed and make it into the ES spec?
Flags: needinfo?(jorendorff)
FWIW, V8's Date parser is here:

https://github.com/v8/v8/blob/007e14ce4ba444531e9ec27b3e2a760f80cb801c/src/dateparser-inl.h#L15

And Chakra's:

https://github.com/Microsoft/ChakraCore/blob/9229c3387b695b2e2fb247681b26d6e6514bc6d1/lib/Runtime/Library/DateImplementation.cpp#L648

We could just port one of these, or compare them to our parser and see where the differences are. Sounds like a fun project actually.
And JSC:

https://github.com/WebKit/webkit/blob/67985c34ffc405f69995e8a35f9c38618625c403/Source/JavaScriptCore/runtime/JSDateMath.cpp#L241

Both JSC and Chakra have a 1-slot cache for this. I'm not aware of any benchmarks where that would help, but might be interesting to investigate that as well.
Assignee: nobody → winter2718
Attached patch date.diff (obsolete) — Splinter Review
I'm going to sit on this patch over the weekend. Right now we parse dates very conservatively according to the W3C "NOTE-datetime" specification, a restricted subset of the ISO 8601 format. I agree that we should open this up, but only a small amount. Other implementations have implemented heuristics that are nothing short of foot guns in my opinion.

Given that, I'm proposing two tiny changes to our current parsing:

1.) Allow single digit months and days: "2016-1-1" will be treated like "2016-01-01"

2.) Allow a space to act in place of a 'T' to begin the time part of a date: "2016-01-01T01:01:01" will be treated like "2016-01-01 01:01:01".

These small changes should solve many of the issues users are having if I'm reading things correctly.
Attached patch date.diff (obsolete) — Splinter Review
Attachment #8716513 - Attachment is obsolete: true
Comment on attachment 8716517 [details] [diff] [review]
date.diff

Review of attachment 8716517 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/Date/non-iso.js
@@ +4,5 @@
> + */
> +
> +/*
> + * For the sake of cross compatibility with other implementations we
> + * make two exceptions to the standard: months and days may be either

I should say which standard.
Attached patch date.diff (obsolete) — Splinter Review
Attachment #8716517 - Attachment is obsolete: true
Comment on attachment 8716562 [details] [diff] [review]
date.diff

Review of attachment 8716562 [details] [diff] [review]:
-----------------------------------------------------------------

How do we feel about these changes?
Attachment #8716562 - Flags: review?(jdemooij)
Thanks for working on this!

With this patch I still get a number of failures when I open the attached testcase (I ran it in the shell but that shouldn't matter). How hard would it be to fix the remaining issues?
Flags: needinfo?(winter2718)
(In reply to Jan de Mooij [:jandem] from comment #12)
> Thanks for working on this!
> 
> With this patch I still get a number of failures when I open the attached
> testcase (I ran it in the shell but that shouldn't matter). How hard would
> it be to fix the remaining issues?

Discussing this with jorendorff, I'm not sure we should support all of the cases. For instance, allowing two digit years opens up a huge can of worms. I was shooting for a very conservative change that should cover most use cases.

If we want to implement a date parser just like what, say, v8 has it will take a good bit of extra work I think. There's a real gray area around date parsing, and I'm not sure I like what other engines are doing. For instance, try running this in Chrome: https://jsfiddle.net/6288uhh4/3/
Flags: needinfo?(winter2718)
What I mean up above is, in many cases perhaps "Invalid Date" really is the correct result.
Hmm, that said, I can easily expand this patch to cover single digits hours and minutes. I definitely don't feel good about two digit years.
(In reply to Morgan Phillips [:mrrrgn] from comment #15)
> Hmm, that said, I can easily expand this patch to cover single digits hours
> and minutes.

I think that would be nice for consistency with the month and days.

> I definitely don't feel good about two digit years.

Ah, I just tried your test in Safari and it totally doesn't match Chrome :(

  new Date(0-01-01): Sat Jan 01 0000 01:00:00 GMT+0100 (CET)
  ...
  new Date(99-01-01): Thu Jan 01 0099 01:00:00 GMT+0100 (CET)

Since engines disagree on this anyway, I agree we should keep Invalid Date for now.
Attachment #8716562 - Flags: review?(jdemooij) → feedback+
Attached patch date.diffSplinter Review
Thank you for the quick feedback ! :) I've expanded the changes to cover hours, minutes, and seconds as well.
Attachment #8716562 - Attachment is obsolete: true
Attachment #8717112 - Flags: review?(jdemooij)
I'm actually a bit concerned about the function name "ParseISODate", and that was why I didn't try to fix this issue myself. Should we rename this function to reflect the fact that it no longer only parses the ISO date string?
(In reply to Xidorn Quan [:xidorn] (UTC+8) (less responsive until Feb 22) from comment #18)
> I'm actually a bit concerned about the function name "ParseISODate", and
> that was why I didn't try to fix this issue myself. Should we rename this
> function to reflect the fact that it no longer only parses the ISO date
> string?

Perhaps we could change it to something like ParseISOStyleDate? We still follow the standard fairly closely with a few exceptions now introduced. I left a comment explaining the exceptions in the function and in new test cases.
Comment on attachment 8717112 [details] [diff] [review]
date.diff

Review of attachment 8717112 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Renaming to ParseISOStyleDate or something makes sense.

::: js/src/jsdate.cpp
@@ +745,5 @@
>   *   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, mintues

Nit: minutes, typo

@@ +781,3 @@
>   *   hh   = two digits of hour (00 through 23) (am/pm NOT allowed)
>   *   mm   = two digits of minute (00 through 59)
>   *   ss   = two digits of second (00 through 59)

Nit: update the hh/mm/ss lines as well

@@ +834,3 @@
>  
>   done_date:
> +    if (PEEK('T') || PEEK(' ')) {

Nit: single-line then/else bodies don't get {}

@@ +842,2 @@
>      NEED(':');
> +    NEED_NDIGITS_OR_LESS(2, min);

Nit: maybe add a test to ensure "n digits or less" does not include 0 digits. Something like "1997-03-08 11::20". A future refactoring could get that wrong.
Attachment #8717112 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #20)
> Nit: single-line then/else bodies don't get {}

(That's a bit ambiguous, more correct: iff each of the condition + then block + else block fit on a single line, the if/else statement doesn't get braces.)
https://hg.mozilla.org/mozilla-central/rev/7e504b40d6f8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Passing this ni? on to efaust, who can decide whether this is worth standardizing.
Flags: needinfo?(jorendorff) → needinfo?(efaustbmo)
It would be really great to get this standardized; nobody has made the time to do the necessary testing to find intersection semantics yet, but if someone has that time the world would become a better place.
Blocks: 1274354
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: