search

squeak-smalltalk / squeak-object-memory

Issues and assets related to the Squeak object memory.
https://bugs.squeak.org
MIT License
11 stars 1 forks source link

DateAndTime>>offset: implementation is incorrect #23

Closed dtlewis290 closed 1 year ago

dtlewis290 commented 2 years ago

DateAndTime>>offset: is part of the 'ansi protocol' of class DateAndTime. Its implementation is not compliant with the ANSI draft spec. The Squeak implementation has been wrong since its original introduction in Squeak 3.7.

The ANSI spec describes #= for DataAndTime as the "equivalence" test, with the refinement "Answer true if the comparand conforms to and if it represents the same UTC time as the receiver. Answer false otherwise. The local times of the receiver and operand are ignored."

For the #offset: method, ANSI says "Answer a equivalent to the receiver but with its local time being offset from UTC by offset ." Thus the result of #offset: must be a DataAndTime that represents the same UTC time as the receiver.

Squeak does this instead:

| dt1 dt2 | dt1 := '2022-05-14T16:45:05.535794-04:00' asDateAndTime. dt2 :=dt1 offset: 5 hours. dt2 - dt1. "==> -0:09:00:00" dt2 asSeconds - dt1 asSeconds. "==> -32400"

Note, Squeak has another (non-ANSI) method that does implement the correct behavior:

| dt1 dt2 | dt1 := '2022-05-14T16:45:05.535794-04:00' asDateAndTime. dt2 :=dt1 utcOffset: 5 hours. dt2 - dt1. "==> 0:00:00:00" dt2 asSeconds - dt1 asSeconds. "==> 0"

This error was present in the original implementation in Squeak 3.7. The error was less obvious at that time because the DataAndTime implemention did not directly show the UTC time value (now in utcMicroseconds instance variable). Squeak 3.7 produces the same results as above.

dtlewis290 commented 2 years ago

Proposed update is in the inbox in Chronology-Core-dtl.80 and Chronology-Tests-dtl.31. Change is to let #offset: adopt the prior behavior of #utcOffset: and let #utcOffset: be a compatibility synonym for #offset:. Add #asLocalAt: to the squeak protocol and let it implement the prior behavior of #offset: Change existing code and tests to use #asLocalAt: but otherwise retain existing behavior.

Expect compatibility issues for external packages that depend on the prior behavior, possibly including handling of timestamps in database applications.

dtlewis290 commented 1 year ago

These changes were being held back in the inbox until the Squeak 6.0 release, so it is time to merge them now. I checked an image with Magma loaded (checking MaDateAndTimeIndex>>indexHashForIndexObject: and any senders of DateAndTime>>offset: or utcOffset: ) and I believe that there will be no problems, nevertheless a second set of eyes on it will be appreciated.

aSqueaker commented 1 year ago

Yes, this is a good fix for Chronology, and won't affect Magma at all. Thank you for the consideration of potential impacts. It seems like #offset: and #utcOffset: would be well to be swapped but, if that were to happen, any apps using them might arrive, quietly, at different calculations than before.

It might be worth considering a conditional warning for one release (6.0) like:

offset: hoursInteger WarnedAboutOffsetChange ifFalse: [ Warning signal: '#offset: calculates differently in Squeak 6.0'. WarnedAboutOffsetChange := true ] .... existing code ...

With a similar one to warn users of utcOffset:, and setters for apps that upgrade. After 6.0, the warnings would be removed.

Not perfect, but suggested due to how such a change in calculation could go unnoticed in a migration-to-6.0 test. "I just upgraded the warp core to Squeak 6.0, it seems fine Jim!" Later, the crew can't figure out why they're 57 light years off course when they came out of hyperspace... :)

dtlewis290 commented 1 year ago

Thanks for the review, I merged the existing code for now, we can consider a WarnAboutOffsetChange warning also.