curiosity-ai / h5

🚀 The next generation C# to JavaScript compiler
https://github.com/curiosity-ai/h5
Apache License 2.0
210 stars 30 forks source link

AddTicks not work correctly #53

Open Ali1Jamal opened 2 years ago

Ali1Jamal commented 2 years ago

add ticks not work correctly in this simple : https://deck.net/d8843f388823586dca2a2b94bbbd6af1 Expected Result 2019-12-01T00:00:00.0000000 2019-11-30T23:59:59.9990000 2019-11-30T23:59:59.9990000 Actual Result 2019-12-01T00:00:00.0000000 2019-12-01T00:00:00.0000000 2019-11-30T23:59:59.9990000 i think to fix this problem its to replace: https://github.com/theolivenbaum/h5/blob/d9c8a733e6b55584c3bd37b1f461ae9378882550/H5/H5/shared/System/DateTime.cs#L454 with is : return new DateTime((ulong)(ticks + value)); or with this: return new DateTime((long)(ticks + value));

theolivenbaum commented 2 years ago

Unfortunately that DateTime file is not used and is only kept in the repo for reference, you can see how it's excluded from the H5.csproj file. The actual DateTime code is under the Date.js file, and the other DateTime.cs that simply uses the Template function to redirect methods to the javascript implementation

  <ItemGroup>
    <Compile Remove="shared\System\DateTime.cs" />
    <Compile Remove="shared\System\Globalization\InternalGlobalizationHelper.cs" />
    <Compile Remove="shared\System\Reflection\MethodInfo.cs" />
    <Compile Remove="shared\System\Resources\RuntimeResourceSet.cs" />
  </ItemGroup>
/// <summary>
/// Returns a new DateTime that adds the specified number of ticks to the value of this instance.
/// </summary>
/// <param name="value">A number of 100-nanosecond ticks. The value parameter can be positive or negative.</param>
/// <returns>An object whose value is the sum of the date and time represented by this instance and the time represented by value.</returns>
[H5.Template("System.DateTime.addTicks({this}, {0})")]
public extern DateTime AddTicks(long value);
addTicks: function (d, v) {
    v = System.Int64.is64Bit(v) ? v : System.Int64(v);

    var dt = new Date(d.getTime()),
        ticks = this.getTicks(d).add(v);

    dt.setMilliseconds(dt.getMilliseconds() + v.div(10000).toNumber())
    dt.ticks = ticks;
    dt.kind = (d.kind !== undefined) ? d.kind : 0;

    return dt;
},
theolivenbaum commented 2 years ago

The issue seems to be on the internal Date representation: https://deck.net/b67912ba0b6a60fd45e18cb7a628c38e

public class Program
{
    public static void Main()
    {
        DateTime time1 = new DateTime(2019, 12, 1);
        DateTime time2 = time1.AddTicks(-1);
        DateTime time3 = new DateTime(time1.Ticks - 1);

        Console.WriteLine(time1.Ticks);
        Console.WriteLine(time1.ToString("O"));

        Console.WriteLine(time2.Ticks);
        Console.WriteLine(time2.ToString("O"));

        Console.WriteLine(time3.Ticks);
        Console.WriteLine(time3.ToString("O"));
    }
}

prints:

637107552000000000 2019-12-01T00:00:00.0000000

637107551999999999 // correct 2019-12-01T00:00:00.0000000 // wrong

637107551999999999 // correct 2019-11-30T23:59:59.9990000 //wrong

theolivenbaum commented 2 years ago

It seems to be a precision issue. If we instead subtract 10000 ticks (i.e. 1ms), the results are matching:

https://deck.net/83725242e22d749f012002d409cf330c

637107552000000000 2019-12-01T00:00:00.0000000

637107551999900000 2019-11-30T23:59:59.9900000

637107551999900000 2019-11-30T23:59:59.9900000

theolivenbaum commented 2 years ago

Compared to C#:

https://dotnetfiddle.net/WG0BO9

Subtracting one tick:

637107552000000000 2019-12-01T00:00:00.0000000

637107551999999999 2019-11-30T23:59:59.9999999

637107551999999999 2019-11-30T23:59:59.9999999

You can see the extra precision there

Ali1Jamal commented 2 years ago

Unfortunately that DateTime file is not used and is only kept in the repo for reference, you can see how it's excluded from the H5.csproj file. The actual DateTime code is under the Date.js file, and the other DateTime.cs that simply uses the Template function to redirect methods to the javascript implementation

  <ItemGroup>
    <Compile Remove="shared\System\DateTime.cs" />
    <Compile Remove="shared\System\Globalization\InternalGlobalizationHelper.cs" />
    <Compile Remove="shared\System\Reflection\MethodInfo.cs" />
    <Compile Remove="shared\System\Resources\RuntimeResourceSet.cs" />
  </ItemGroup>
/// <summary>
/// Returns a new DateTime that adds the specified number of ticks to the value of this instance.
/// </summary>
/// <param name="value">A number of 100-nanosecond ticks. The value parameter can be positive or negative.</param>
/// <returns>An object whose value is the sum of the date and time represented by this instance and the time represented by value.</returns>
[H5.Template("System.DateTime.addTicks({this}, {0})")]
public extern DateTime AddTicks(long value);
addTicks: function (d, v) {
    v = System.Int64.is64Bit(v) ? v : System.Int64(v);

    var dt = new Date(d.getTime()),
        ticks = this.getTicks(d).add(v);

    dt.setMilliseconds(dt.getMilliseconds() + v.div(10000).toNumber())
    dt.ticks = ticks;
    dt.kind = (d.kind !== undefined) ? d.kind : 0;

    return dt;
},

oh i'm didn't know this info ... nice Investigation

Ali1Jamal commented 2 years ago

The issue seems to be on the internal Date representation: https://deck.net/b67912ba0b6a60fd45e18cb7a628c38e

public class Program
{
    public static void Main()
    {
        DateTime time1 = new DateTime(2019, 12, 1);
        DateTime time2 = time1.AddTicks(-1);
        DateTime time3 = new DateTime(time1.Ticks - 1);

        Console.WriteLine(time1.Ticks);
        Console.WriteLine(time1.ToString("O"));

        Console.WriteLine(time2.Ticks);
        Console.WriteLine(time2.ToString("O"));

        Console.WriteLine(time3.Ticks);
        Console.WriteLine(time3.ToString("O"));
    }
}

prints:

637107552000000000 2019-12-01T00:00:00.0000000

637107551999999999 // correct 2019-12-01T00:00:00.0000000 // wrong

637107551999999999 // correct 2019-11-30T23:59:59.9990000 //wrong

yes its seems to be .

ghost commented 2 years ago

It seems to be a precision issue. If we instead subtract 10000 ticks (i.e. 1ms), the results are matching:

https://deck.net/83725242e22d749f012002d409cf330c

637107552000000000 2019-12-01T00:00:00.0000000

637107551999900000 2019-11-30T23:59:59.9900000

637107551999900000 2019-11-30T23:59:59.9900000

There are 2 problems there. First of all the following code is wrong:

v.div(10000).toNumber()

v is Int64, and divide in 10000 does not make sense, integer result for -1 / 10000 is zero. Adding zero does nothing ))

But replacement from

dt.setMilliseconds(dt.getMilliseconds() + v.div(10000).toNumber())

to

dt.setMilliseconds(dt.getMilliseconds() + v.toNumber() / 10000);

// ";" in the end is also required

will not work... due to limitations of Date object itself. The maximal precision is 1 ms (1/1000 sec):

(new Date(1633689504074)).getMilliseconds()
74
(new Date(1633689504075)).getMilliseconds()
75

So to make it really compatible with .NET platform we need to replace Date.js by transpiled version of DateTime.cs, I expect significant amount of work here, but finally it should be made. Alternatively we need to use some date library inside Date.js, but I expect such behavior more problematic in case of supporting Calendar object in H5.

Not minified Date.js takes around 50 KB, so from point of view of size it should not be a problem.

theolivenbaum commented 2 years ago

@hardhub I think the reason they didn't implement DateTime as is in the original Bridge is due to different calendars and the general complexity around supporting dates. I doubt this is going to be simple, as the .NET runtime uses information from the OS which would not be accessible to the javascript world without using their Date data type.

Unless we have someone that can dig deep into this, I would prefer to focus any work here on fixing the obvious bugs with time zones and such.

ghost commented 2 years ago

@hardhub I think the reason they didn't implement DateTime as is in the original Bridge is due to different calendars and the general complexity around supporting dates. I doubt this is going to be simple, as the .NET runtime uses information from the OS which would not be accessible to the javascript world without using their Date data type.

Unless we have someone that can dig deep into this, I would prefer to focus any work here on fixing the obvious bugs with time zones and such.

But AddTicks does not work. It disallow to create ranges. For example you need to create period of current week. The end date should be AddDays(+7).AddTicks(-1). So it is last possible time of desired week. But in case of wrong implementation of AddTicks it is not possible. And if it is cross-compiled code then all is even worse.

Probably we can replace js code of addTicks by code of constructor used here:

DateTime time3 = new DateTime(time1.Ticks - 1);

I will check the source code.

ghost commented 2 years ago

@theolivenbaum I think you can use create$2 version inside AddTicks. Please use the following code (and I will not fork the repo for the moment).

addTicks: function (d, v) {
                v = System.Int64.is64Bit(v) ? v : System.Int64(v);

                var ticks = this.getTicks(d).add(v);
                var kind = d.kind;

                if (ticks.lt(this.TicksPerDay)) {
                    d = new Date(0);
                    d.setMilliseconds(d.getMilliseconds() + this.$getTzOffset(d).div(10000).toNumber());
                    d.setFullYear(1);
                } else {
                    d = new Date(ticks.sub(this.$getMinOffset()).div(10000).toNumber());

                    if (kind !== 1) {
                        d.setTime(d.getTime() + (d.getTimezoneOffset() * 60000));
                    }
                }

                d.kind = (kind !== undefined) ? kind : 0;
                d.ticks = ticks;

                return d;
            },

I have adjusted it a bit for needs of AddTicks. And it shows more correct result.

2019-12-01T00:00:00.0000000
2019-11-30T23:59:59.9990000
2019-11-30T23:59:59.9990000

Of course it is not 999 9999, but at least the behaviour is equal to other DateTime logic and bug is solved.

theolivenbaum commented 2 years ago

Looks good, I'll add over the weekend when I've some time, thanks!