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

Sub-millisecond component inaccuracy in OffsetDateTime conversion from js_sys::Date #635

Open
trrk opened this issue Nov 4, 2023 · 1 comment
Labels
A-third-party Area: implementations of traits from other crates C-bug Category: bug in current code

Comments

@trrk
Copy link

trrk commented Nov 4, 2023

When generating an OffsetDateTime from a js_sys::Date, there are cases where the sub-millisecond components are not set to zero. This behavior is unexpected and can lead to inaccuracies in the resulting OffsetDateTime.

Steps to Reproduce

  1. Use the following code to reproduce the issue:
let millis = 1699083911579i64; // 2023-11-04T07:45:11.579Z
let js_date = js_sys::Date::new(&JsValue::from_f64(millis as f64));
let datetime = OffsetDateTime::from(js_date);

println!("{}", datetime);
// => 2023-11-04 7:45:11.579000064 +00:00:00

Reason for Considering It a Bug

The precision of js_sys::Date is limited to milliseconds. Therefore, when converting it to an OffsetDateTime, we expect the sub-millisecond components to be set to zero for consistency and accuracy.

Cause

time/time/src/date_time.rs

Lines 1226 to 1233 in 72f03e0

impl From<js_sys::Date> for DateTime<offset_kind::Fixed> {
fn from(js_date: js_sys::Date) -> Self {
// get_time() returns milliseconds
let timestamp_nanos = (js_date.get_time() * Nanosecond::per(Millisecond) as f64) as i128;
Self::from_unix_timestamp_nanos(timestamp_nanos)
.expect("invalid timestamp: Timestamp cannot fit in range")
}
}

The issue stems from the conversion process of milliseconds to nanoseconds. It appears that the code converts milliseconds to nanoseconds using f64, and subsequently converts it to i128, which can introduce inaccuracies and prevent sub-millisecond components from being zeroed out.

Proposed Solution

To address this issue, consider converting milliseconds to i128 first and then perform the conversion to nanoseconds. This approach should maintain accuracy and ensure that sub-millisecond components are set to zero.

Additional Information

I encountered this issue while using OffsetDateTime::now_utc() in a WASM environment.

@jhpratt
Copy link
Member

jhpratt commented Nov 4, 2023

Without even looking into this, I am confident that the multiplication magnifies the inherent error of representing decimal values in floating point numbers. I'm happy to accept your proposed solution as a PR, as I'm sure it'll fix the issue.

@jhpratt jhpratt added C-bug Category: bug in current code A-third-party Area: implementations of traits from other crates labels Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-third-party Area: implementations of traits from other crates C-bug Category: bug in current code
Projects
None yet
Development

No branches or pull requests

2 participants