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

Date object has significant UTC errors #114

Closed
jsobell opened this issue Mar 26, 2018 · 16 comments
Closed

Date object has significant UTC errors #114

jsobell opened this issue Mar 26, 2018 · 16 comments

Comments

@jsobell
Copy link
Contributor

jsobell commented Mar 26, 2018

The code at the moment within Date has some serious fundamental issues.
The use of UTC requires correctly handling DST times, and at the moment they don't work correctly.
The most noticeable issue is that in many places the code says TimeZoneInfo.Local.GetUtcOffset(DateTime.Now). This is only correct in operations specifically referring to .now or the current time, as UTC offset is different for different times of year.

For example, in Victoria, Australia, the offset on Jan 1st is one hour different from that on July 1st, so
TimeZoneInfo.Local.GetUtcOffset(new DateTime(2018,1,1)) (11 hours) is different to TimeZoneInfo.Local.GetUtcOffset(new DateTime(2018,7,1)) (10 hours) so the use of .Now will give incorrect readings for six months of the year in UTC related operations.

The same applies to functions such as setHour, where the time must be correctly used in local structure for the calculation, so the offset is critical to get correct before setting individual parts of the date.

I'd suggest abandoning ticks as as the underlying value, and use the native DateTime object instead. This simplifies things hugely, as it already honours DST, and supports UTC and local natively. There's an example I adapted from Jurassic here: https://github.com/questmetrics/NiL.JS/blob/develop/NiL.JS/BaseLibrary/Date.new.cs

@nilproject
Copy link
Owner

I will try to understand and fix this bug.
You can use Date.new.cs, but I can not include part of another library in my.

@jsobell
Copy link
Contributor Author

jsobell commented Mar 28, 2018

I rewrote the whole thing, and while it hasn't been extensively tested it seems to work nicely: https://github.com/questmetrics/NiL.JS

nilproject added a commit that referenced this issue Apr 16, 2018
@nilproject
Copy link
Owner

I made some fixes for Date. Please, check it for you system.

@jsobell
Copy link
Contributor Author

jsobell commented Apr 16, 2018

No, it still has lots of errors related to UTC and DST. When you ask for things like the hour, day, month, or even year of a UTC based date you can't just call the local one.
e.g.

public JSValue getUTCFullYear()
        {
            return getFullYear();
        }

There are lots of issues like this through the code, which is why I rewrote it based on the framework DateTime, which correctly calculates every daylight saving correctly, leap-years etc.

Here's an example of the issues:

[(new Date('2018-01-01 23:00')).getHours(), (new Date('2018-01-01 23:00')).getUTCHours()]
23,23
[(new Date('2018-07-01 23:00')).getHours(), (new Date('2018-07-01 23:00')).getUTCHours()]
23,23

The values should be:

[(new Date('2018-01-01 23:00')).getHours(), (new Date('2018-01-01 23:00')).getUTCHours()]
23,12
[(new Date('2018-07-01 23:00')).getHours(), (new Date('2018-07-01 23:00')).getUTCHours()]
23,13

because daylight saving changes during the year, and every UTC hour calculation has to take the target date into account.
This applies to all date functions:

[(new Date('2018-07-01 00:00')).getFullYear(), (new Date('2018-01-01 00:00')).getUTCFullYear()]
2018,2017

(these only apply in Australia, as we're +10/+11)
Your existing code gives 2018,2018 for this (well it actually gives 1999,1999 because the parser fails to read the date, but use 2018-07-01T00:00 and it works)

Check out my replacement here: https://github.com/questmetrics/NiL.JS/blob/develop/NiL.JS/BaseLibrary/Date.cs
I believe this addresses all the issues, and it's the one we use for our live system.

nilproject added a commit that referenced this issue Apr 17, 2018
@nilproject
Copy link
Owner

nilproject commented Apr 17, 2018

@jsobell, I tested this with your example. Work good.

nilproject added a commit that referenced this issue Apr 19, 2018
@jsobell
Copy link
Contributor Author

jsobell commented Apr 22, 2018

Nope:

new Date()
Sun Apr 23 2018 01:05:29 GMT+1000 (AUS Eastern Standard Time)

Should say

new Date()
Sun Apr 22 2018 15:05:29 GMT+1000 (Local Standard Time)

Seriously, instead of messing around with this, why not use the DateTime object built into .NET like I did? The whole point of that object is that it tracks all of this stuff automatically, and it makes marshalling back and for to .NET so much simpler.
The code I wrote addresses all of these issues, accepts a DateTime object as a constructor when required, and even returns a DateTime object natively when required.
I don't want to fork your code and offer it as a 'working' version because I want you to retain all the credit for producing this, but without pre-release builds for testing and hotfixes on NuGet it's a pain. I think you have to start accepting contributions, otherwise it won't gain traction.

@nilproject
Copy link
Owner

I maked hotfix for it. And add test for this case. The storage of time was slightly reworked and this bug is a consequence of it.
Well, I suppose it's really worth rewriting Date as a wrapper for DateTime

@nilproject
Copy link
Owner

nilproject commented Apr 22, 2018

I checked your implementation with unit tests and found that it's incorrect. I got a lot test for fail. See sections s9_4 and s15_9

@jsobell
Copy link
Contributor Author

jsobell commented Apr 22, 2018

Well, it's getting very complicated now.
It appears Chrome has a bug, and it reports the daylight saving for Australia incorrectly.
For example, our DST in 2000 changed on 26 March 2000 02:00 from +11 to +10.
Try this in your browser:

[new Date(953996400000), new Date(954000000000), new Date(954003600000)]

Chrome gives

Sun Mar 26 2000 02:00:00 GMT+1100 (Local Daylight Time) {}
Sun Mar 26 2000 03:00:00 GMT+1100 (Local Daylight Time) {}
Sun Mar 26 2000 04:00:00 GMT+1100 (Local Daylight Time) {}

which is wrong!!!!
IE gives

[date] Sun Mar 26 2000 02:00:00 GMT+1100 (AUS Eastern Summer Time)
[date] Sun Mar 26 2000 02:00:00 GMT+1000 (AUS Eastern Standard Time)
[date] Sun Mar 26 2000 03:00:00 GMT+1000 (AUS Eastern Standard Time)

which is correct!
FireFox is also correct with

0: "3/26/2000, 2:00:00 AM"
​1: "3/26/2000, 2:00:00 AM"
​2: "3/26/2000, 3:00:00 AM"

This made testing a little tricky, as I was testing against the Clearscript (Chromium) engine.

Re the s9_4 and s15_9 tests, I'd consider modifying the DateTime based solution for the bizarre edge-cases in those tests, but the project on GitHub doesn't currently compile due to missing IndexersSupportMode, so I can't run the tests at the moment.

Having said that, your code is getting closer... but it gives the wrong day at the moment:

Sun Apr 22 2018 09:00:00 GMT+1000 (AUS Eastern Standard Time) != Sat Apr 22 2018 09:00:00 GMT+1000 (AUS Eastern Standard Time)

Today is Sunday. This is because getDay() isn't checking for UTC (so toString() is also incorrect).

The other main functions seem to give the correct value now, with the only discrepancy being that in 2000 the Sydney Olympics meant they moved daylight saving to be a month earlier, and Microsoft honour this, whereas javascript doesn't. I think we can safely ignore this and return the correct value the same as .NET does :)

@jsobell
Copy link
Contributor Author

jsobell commented Apr 22, 2018

You need this:

        [DoNotEnumerate]
        public JSValue getDay()
        {
            return getDayImpl(true);
        }

        [DoNotEnumerate]
        public JSValue getUTCDay()
        {
            return getDayImpl(false);
        }

        private int getDayImpl(bool withTzo)
        {
            var t = System.Math.Abs(_time + (withTzo ? _timeZoneOffset : 0));
            return (int)((t / _dayMilliseconds + 1) % 7);
        }

and this change in toString():

       daysOfWeek[getDayImpl(true)] + " "

Also, getUTCString() can't just call ToString(), as it also has to honour UTC.

nilproject added a commit that referenced this issue Apr 22, 2018
nilproject added a commit that referenced this issue Apr 22, 2018
@jsobell
Copy link
Contributor Author

jsobell commented Apr 22, 2018

ToUTCString is still wrong, and should result in a different format to toString:
Chrome:
Sun, 21 Jan 2018 21:00:00 GMT
NiL:
Sun Jan 21 2018 21:00:00 GMT+1100 (AUS Eastern Summer Time)

Fix is:


        private string stringifyDate(bool withTzo)
        {
            if (_error)
                return "Invalid date";

            var res =
                    withTzo
                        ? daysOfWeek[(getDayImpl(withTzo) + 6) % 7] + " "
                                                                    + months[getMonthImpl(withTzo)]
                                                                    + " " 
                                                                    + getDateImpl(withTzo).ToString("00") 
                                                                    + " "
                                                                    + getYearImpl(withTzo)
                        : daysOfWeek[(getDayImpl(withTzo) + 6) % 7] + ", "
                                                                    + getDateImpl(withTzo).ToString("00")
                                                                    + " "
                                                                    + months[getMonthImpl(withTzo)]
                                                                    + " "
                                                                    + getYearImpl(withTzo);
            return res;
        }

        private string stringifyTime(bool withTzo)
        {
            if (_error)
                return "Invalid date";

            var offset = new TimeSpan(_timeZoneOffset * _timeAccuracy);
            var timeName = CurrentTimeZone.IsDaylightSavingTime(new DateTimeOffset(_time * _timeAccuracy, offset)) ? TimeZoneInfo.Local.DaylightName : TimeZoneInfo.Local.StandardName;
            var res = getHoursImpl(withTzo).ToString("00:")
                      + getMinutesImpl(withTzo).ToString("00:")
                      + getSecondsImpl().ToString("00")
                      + " GMT"
                      + (withTzo
                          ? (offset.Ticks > 0 ? "+" : "") + (offset.Hours * 100 + offset.Minutes).ToString("0000") +
                            " (" + timeName + ")"
                          : "");
            return res;
        }

@jsobell
Copy link
Contributor Author

jsobell commented Apr 22, 2018

Also, toISOString is wrong, needs:

       private JSValue toIsoString()
        {
     :
            var y = getYearImpl(false);

@jsobell
Copy link
Contributor Author

jsobell commented Apr 22, 2018

With these changes the only failing tests appear to be the weird edge-cases like

date = new Date(1970, 0, 100000001, 0, 0 + timeZoneMinutes - 60, 0, -1);

which I think we can live without.
My test code for the date objects runs against ClearScript, and apart from their buggy DST issues, all of them seem fine.
FYI, here is the script I was running to test compatibility with Chromium engine version of Dates:

    internal class Program
    {
        public static void Main(string[] args)
        {
            ScriptEngine nilQM = new V8ScriptEngine();
            Context nilNiL = new Context();

            DateTime d = DateTime.Parse("2 Oct 2017 01:00");
            DateTime end = DateTime.Parse("30 Mar 2018 01:00");

            while (d < end)
            {
                var init = $"var d=new Date({d.Year},{d.Month - 1},{d.Day},{d.Hour},{d.Minute},{d.Second})";
                nilQM.Evaluate(init);
                nilNiL.Eval(init);

                Compare(d, "d.valueOf()", nilQM, nilNiL, "new Date({x}).toString()");
                Compare(d, "d.toString()", nilQM, nilNiL);
                Compare(d, "d.toUTCString()", nilQM, nilNiL);
                Compare(d, "d.getDay()", nilQM, nilNiL);
                Compare(d, "d.getHours()", nilQM, nilNiL);
                Compare(d, "d.getUTCHours()", nilQM, nilNiL);
                Compare(d, "d.getTimezoneOffset()", nilQM, nilNiL);
                Compare(d, "d.getUTCDate()", nilQM, nilNiL);
                Compare(d, "d.toISOString()", nilQM, nilNiL);
                Compare(d, "d.getUTCHours()", nilQM, nilNiL);
                Compare(d, "d.getUTCFullYear()", nilQM, nilNiL);
                d = d.AddHours(1);
            }
            Console.WriteLine("Done!");
        }

        private static void Compare(DateTime d, string script, ScriptEngine nilQM, Context nilNiL, string reverse=null)
        {
            var qmresult = nilQM.Evaluate(script).ToString();
            var nilresult = nilNiL.Eval(script).As<string>();

            if (nilresult != qmresult)
            {
                string feedback = "";
                if (!String.IsNullOrWhiteSpace(reverse))
                {
                    feedback = 
                        $"  (QM={nilQM.Evaluate(reverse.Replace("{x}", qmresult))} , NiL={nilQM.Evaluate(reverse.Replace("{x}", nilresult))})";
                }

                Console.WriteLine($"{d} : {script} : Chrome {qmresult} != NiL {nilresult}  {feedback}");
            }
        }
    }

jsobell added a commit to questmetrics/NiL.JS.QM that referenced this issue Apr 23, 2018
@jsobell jsobell mentioned this issue Apr 23, 2018
nilproject added a commit that referenced this issue Apr 23, 2018
@nilproject
Copy link
Owner

nilproject commented Apr 23, 2018

Output format of ToUTCString is not described in spec of JS. There is two popular standards of output not dependent on the language: RFC-822 and RFC-1123. I will fix it according to the RFC-1123 standard.

@nilproject
Copy link
Owner

Also I fixed timezone parsing, remove timezone offset from results of toUTCString and toGMTString and change result format of toGMTString.

@nilproject
Copy link
Owner

Duplicate of #133

@nilproject nilproject marked this as a duplicate of #133 May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants