Skip to content

Allow naive datetime into chrono DateTime<Local> conversion #5178

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow naive datetime into chrono DateTime<Local> conversion #5178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/5178.changed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow converting naive datetime into chrono `DateTime<Local>`.
109 changes: 74 additions & 35 deletions src/conversions/chrono.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,26 +336,28 @@ impl<Tz: TimeZone + for<'py> FromPyObject<'py>> FromPyObject<'_> for DateTime<Tz
"expected a datetime with non-None tzinfo",
));
};
let naive_dt = NaiveDateTime::new(py_date_to_naive_date(dt)?, py_time_to_naive_time(dt)?);
match naive_dt.and_local_timezone(tz) {
LocalResult::Single(value) => Ok(value),
LocalResult::Ambiguous(earliest, latest) => {
#[cfg(not(Py_LIMITED_API))]
let fold = dt.get_fold();

#[cfg(Py_LIMITED_API)]
let fold = dt.getattr(intern!(dt.py(), "fold"))?.extract::<usize>()? > 0;

if fold {
Ok(latest)
} else {
Ok(earliest)
}

py_datetime_to_datetime_with_timezone(dt, tz)
}
}

#[cfg(feature = "chrono-local")]
impl<'py> FromPyObject<'py> for DateTime<Local> {
fn extract_bound(dt: &Bound<'_, PyAny>) -> PyResult<DateTime<Local>> {
let dt = dt.downcast::<PyDateTime>()?;

if let Some(tzinfo) = dt.get_tzinfo() {
let local_tz = Local.into_pyobject(dt.py())?;
if !(tzinfo.eq(local_tz)?) {
let name = local_tz.getattr("key")?.downcast_into::<PyString>()?;
return Err(PyValueError::new_err(format!(
"expected a datetime without timezone or with local timezone {}",
name.to_cow()?
)));
}
LocalResult::None => Err(PyValueError::new_err(format!(
"The datetime {dt:?} contains an incompatible timezone"
))),
}
};

py_datetime_to_datetime_with_timezone(dt, Local)
}
}

Expand Down Expand Up @@ -474,22 +476,6 @@ impl<'py> IntoPyObject<'py> for &Local {
}
}

#[cfg(feature = "chrono-local")]
impl FromPyObject<'_> for Local {
fn extract_bound(ob: &Bound<'_, PyAny>) -> PyResult<Local> {
Comment on lines -477 to -479
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this is breaking, but I guess it's unlikely to be a problem? The DateTime conversion is still going to work and I would be quite surprised if users ever tried to extract Local directly.

Regardless we should add a 5178.removed.md entry in the changelog which says this implementation is no longer provided by the chrono-local feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removing this is unfortunate. An other option is to conditionally branch on TypeId. Which in const is nicely optimized away.

Do you prefer the following over removing impl FromPyObject<'_> for Local?

impl<Tz: TimeZone + 'static + for<'py> FromPyObject<'py>> FromPyObject<'_> for DateTime<Tz> {
    fn extract_bound(dt: &Bound<'_, PyAny>) -> PyResult<DateTime<Tz>> {
        let dt = dt.downcast::<PyDateTime>()?;
        let tzinfo = dt.get_tzinfo();

        let tz = if let Some(tzinfo) = tzinfo {
            tzinfo.extract()?
        } else {

            // TODO make this check const when PartialEq is const stable
            #[cfg(feature = "chrono-local")]
            if TypeId::of::<Tz>() == TypeId::of::<Local>() {
                // Safety: Tz is Local, so we can safely transmute Local to Tz.
                let tz = unsafe { std::mem::transmute(Local) };
                return py_datetime_to_datetime_with_timezone(dt, tz);
            }

            return Err(PyTypeError::new_err(
                "expected a datetime with non-None tzinfo",
            ));
        };

        py_datetime_to_datetime_with_timezone(dt, tz)
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, this does not work in practice.

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
   --> src\conversions\chrono.rs:341:35
    |
341 |                 let tz = unsafe { std::mem::transmute(Local) };
    |                                   ^^^^^^^^^^^^^^^^^^^
    |
    = note: source type: `Local` (0 bits)
    = note: target type: `Tz` (this type does not have a fixed size)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option might be to add a hidden specialization to the FromPyObject trait similar to #5244

Though if we go that way, probably we should land this in 0.27 when I think we'll land the rest of the FromPyObject changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, this does not work in practice.

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
   --> src\conversions\chrono.rs:341:35
    |
341 |                 let tz = unsafe { std::mem::transmute(Local) };
    |                                   ^^^^^^^^^^^^^^^^^^^
    |
    = note: source type: `Local` (0 bits)
    = note: target type: `Tz` (this type does not have a fixed size)

transmute_copy does work here. I wander if there is a safe api to do the TypeId::of check and transmute safely that optimises nicely 🤔.

let local_tz = Local.into_pyobject(ob.py())?;
if ob.eq(local_tz)? {
Ok(Local)
} else {
let name = local_tz.getattr("key")?.downcast_into::<PyString>()?;
Err(PyValueError::new_err(format!(
"expected local timezone {}",
name.to_cow()?
)))
}
}
}

struct DateArgs {
year: i32,
month: u8,
Expand Down Expand Up @@ -590,6 +576,32 @@ fn py_time_to_naive_time(py_time: &Bound<'_, PyAny>) -> PyResult<NaiveTime> {
.ok_or_else(|| PyValueError::new_err("invalid or out-of-range time"))
}

fn py_datetime_to_datetime_with_timezone<Tz: TimeZone>(
dt: &Bound<'_, PyDateTime>,
tz: Tz,
) -> PyResult<DateTime<Tz>> {
let naive_dt = NaiveDateTime::new(py_date_to_naive_date(dt)?, py_time_to_naive_time(dt)?);
match naive_dt.and_local_timezone(tz) {
LocalResult::Single(value) => Ok(value),
LocalResult::Ambiguous(earliest, latest) => {
#[cfg(not(Py_LIMITED_API))]
let fold = dt.get_fold();

#[cfg(Py_LIMITED_API)]
let fold = dt.getattr(intern!(dt.py(), "fold"))?.extract::<usize>()? > 0;

if fold {
Ok(latest)
} else {
Ok(earliest)
}
}
LocalResult::None => Err(PyValueError::new_err(format!(
"The datetime {dt:?} contains an incompatible timezone"
))),
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -982,6 +994,33 @@ mod tests {
})
}

#[test]
#[cfg(feature = "chrono-local")]
fn test_pyo3_naive_datetime_frompyobject_local() {
Python::with_gil(|py| {
let year = 2014;
let month = 5;
let day = 6;
let hour = 7;
let minute = 8;
let second = 9;
let micro = 999_999;
let py_datetime = new_py_datetime_ob(
py,
"datetime",
(year, month, day, hour, minute, second, micro),
);
let py_datetime: DateTime<Local> = py_datetime.extract().unwrap();
let expected_datetime = NaiveDate::from_ymd_opt(year, month, day)
.unwrap()
.and_hms_micro_opt(hour, minute, second, micro)
.unwrap()
.and_local_timezone(Local)
.unwrap();
assert_eq!(py_datetime, expected_datetime);
})
}

#[test]
fn test_pyo3_datetime_frompyobject_fixed_offset() {
Python::with_gil(|py| {
Expand Down
Loading