LibJS: new Date(…).getHours() depends on locale

BenWiederhake commented 4 years ago

The JS selftests include, among other things, in Date.js:

    let date = new Date(2019, -13, -33,  -30, -70, -80, -2345);
    expect(date.getHours()).toBe(16);

However, getHours() is 17 on my platform, presumably because my country is in UTC+01 during October.

This can be easily shown:

$ echo "new Date(2019, -13, -33,  -30, -70, -80, -2345)" | ./Meta/Lagom/js
Date Thu Oct 26 2017 17:48:37 GMT+0000 (UTC)
$ echo "new Date(2019, -13, -33,  -30, -70, -80, -2345)" | LC_ALL=C ./Meta/Lagom/js
Date Thu Oct 26 2017 17:48:37 GMT+0000 (UTC)

Also, it doesn't seem to care that during summer, my locale is UTC+02:

$ echo "new Date(2020, 8, 24, 15, 23, 45)" | ./Meta/Lagom/js
Date Thu Sep 24 2020 16:23:45 GMT+0000 (UTC)

In summary:

The output seems weird, maybe there's an issue with LibJS?
Test fails on non-UTC-0 locales, that's definitely not good

CC @nico because you probably know a lot more about LibJS's time handling than I do :)

linusg commented 4 years ago

Also, it doesn't seem to care that during summer, my locale is UTC+02

See here:

https://github.com/SerenityOS/serenity/blob/682b2fdb75d4e20c316e84dbe9416b2e39d5050a/Libraries/LibJS/Runtime/Date.h#L67-L68

Test fails on non-UTC-0 locales, that's definitely not good

Ha so I'm not the only one - definitely not good indeed :)

nico commented 4 years ago

Serenity doesn't have timezones yet. I guess you're running this outside serenity?

Both the Date tuple ctor and getHours treat time in the local timezone. So the check should work, and we're just missing a timezone conversion in one direction.

Oh, what's maybe going wrong is that serenity's localtime doesn't use tm_isdst yet?

Maybe you can check what a different J's engine on your system does to see which step goes wrong.

BenWiederhake commented 4 years ago

Serenity doesn't have timezones yet. I guess you're running this outside serenity?

Indeed, I was running it in Lagom, I was trying to reproduce Travis when it runs a particular part of test-js that fails on my machine.

In Firefox:

new Date(2019, -13, -33,  -30, -70, -80, -2345)
Date Thu Oct 26 2017 16:48:37 GMT+0200 (Mitteleuropäische Sommerzeit)

(Where Mitteleuropäische Sommerzeit stands for CEST.)

In NodeJS:

> new Date(2019, -13, -33,  -30, -70, -80, -2345)
2017-10-26T14:48:37.655Z

Again, I have no idea what the "correct" behavior should be, I'm only complaining that ./Build/Meta/Lagom/test-js fails when run on a non-GMT machine.

nico commented 4 years ago

That's not very interesting output -- toString() can do implementation-specific things (free to choose timezone it shows the time in etc) :) You can call getHours() on the object and that should return the hours in local time. You can also call getTime() to get the timestamp (seconds since jan 1 1970, in utc) and compare that. new Date(2019, -13, -33, -30, -70, -80, -2345).getTime() should have the same behavior in firefox, node, and serenity's js shell (else that's a bug in the local date -> timestamp conversion). And then new Date($that_timestamp).getHours() should return the same hours in local time in all 3 engines. That'd tell us if the date->timestamp or the timestamp->date conversion is borked.

Firefox does use local time in toString() output, so its getHours() there returns 16, like the test wants (and like you'd expect -30 % 24 to behave).

nico commented 4 years ago

You can see if this

$ git diff
diff --git a/Libraries/LibCore/DateTime.cpp b/Libraries/LibCore/DateTime.cpp
index 64c48a907..58f914a62 100644
--- a/Libraries/LibCore/DateTime.cpp
+++ b/Libraries/LibCore/DateTime.cpp
@@ -88,6 +88,7 @@ void DateTime::set_time(unsigned year, unsigned month, unsigned day, unsigned ho
     tm.tm_mday = (int)day;
     tm.tm_mon = (int)month - 1;
     tm.tm_year = (int)year - 1900;
+    tm.tm_isdst = -1;
     // mktime() doesn't read tm.tm_wday and tm.tm_yday, no need to fill them in.

     m_timestamp = mktime(&tm);

helps.

BenWiederhake commented 4 years ago

Firefox (SpiderMonkey, I guess?)

>> var date = new Date(2020, 8, 24, 15, 23, 45);
undefined
>> console.log(date.getFullYear(), date.getMonth(), date.getDate(), date.getHours(), date.getMinutes(), date.getSeconds());
2020 8 24 15 23 45
undefined
>>

NodeJS

$ node 
Welcome to Node.js v12.16.1.
Type ".help" for more information.
> var date = new Date(2020, 8, 24, 15, 23, 45);
undefined
> console.log(date.getFullYear(), date.getMonth(), date.getDate(), date.getHours(), date.getMinutes(), date.getSeconds());
2020 8 24 15 23 45
undefined
>

LibJS without the patch

$ ./Meta/Lagom/js
> var date = new Date(2020, 8, 24, 15, 23, 45);
undefined
> console.log(date.getFullYear(), date.getMonth(), date.getDate(), date.getHours(), date.getMinutes(), date.getSeconds());
2020 8 24 16 23 45
undefined
>

LibJS with the patch

$ ./Meta/Lagom/js
> var date = new Date(2020, 8, 24, 15, 23, 45);
undefined
> console.log(date.getFullYear(), date.getMonth(), date.getDate(), date.getHours(), date.getMinutes(), date.getSeconds());
2020 8 24 15 23 45
undefined
>

Looks like

… the patch is a good idea. I don't know what it really means and which code has what assumptions about that field, so I won't write a PR myself :P.

If it's always "local time", then maybe it's a good idea to not hardcode GMT+0000 (UTC) – is there a way to say "local timezome"?

Unrelated issues

(js in Lagom can't handle very long lines as input; can't handle multi-line pastes; can't handle key combinations like Ctrl-Left or Ctrl-Right; pressing "Ctrl-D" after getting some multi-line suggestions leaves the terminal "dirty". Should I report these, or is js considered to be "just" a way to interact with LibJS?)

linusg commented 4 years ago

If it's always "local time", then maybe it's a good idea to not hardcode GMT+0000 (UTC) – is there a way to say "local timezome"?

We're very limited here.

the "GMT" part is mandated by https://tc39.es/ecma262/#sec-timestring
the offset is mandated by https://tc39.es/ecma262/#sec-timezoneestring, as well as an implementation defined timezone name in parantheses, which optionally can be omitted

Both of these are used in toDateString and toTimeString (and toString which is just toDateString).

Hardcoding UTC seemed like a decent solution when it was first implemented, just to match the format from the spec. Once we know the local timezone difference from UTC, we can fix that :) (maybe we do by now?)

Should I report these

Yes, as LibLine bugs I suppose.

is js considered to be "just" a way to interact with LibJS?

Well yes but actually no - not "just", at least not in a negative way :)

SerenityOS / serenity