Panic in `parse_from_rfc2822` with overflowing date

[5225225]

#[test]
fn panic() {
    chrono::DateTime::parse_from_rfc2822("31 DEC 262143 23:59 -2359");
}

[djc]

Sorry for the slow response -- would you be able to submit a PR for this, including this as a test?

[botahamec]

I took a bit of a look at this. The panic seems to be caused by this function in offset/mod.rs

/// Converts the local `NaiveDateTime` to the timezone-aware `DateTime` if possible.
#[allow(clippy::wrong_self_convention)]
fn from_local_datetime(&self, local: &NaiveDateTime) -> LocalResult<DateTime<Self>> {
    self.offset_from_local_datetime(local)
        .map(|offset| DateTime::from_utc(*local - offset.fix(), offset))
}

I don't think it makes sense to return a LocalResult::None here. The function could be refactored to return an option or a result. I could also try changing Parsed::to_datetime to return OUT_OF_RANGE if the date is too large:

/// Returns a parsed timezone-aware date and time out of given fields.
///
/// This method is able to determine the combined date and time
/// from date and time fields or a single [`timestamp`](#structfield.timestamp) field,
/// plus a time zone offset.
/// Either way those fields have to be consistent to each other.
pub fn to_datetime(&self) -> ParseResult<DateTime<FixedOffset>> {
    let offset = self.offset.ok_or(NOT_ENOUGH)?;
    let datetime = self.to_naive_datetime_with_offset(offset)?;
    let offset = FixedOffset::east_opt(offset).ok_or(OUT_OF_RANGE)?;
    match offset.from_local_datetime(&datetime) {
        LocalResult::None => Err(IMPOSSIBLE),
        LocalResult::Single(t) => Ok(t),
        LocalResult::Ambiguous(..) => Err(NOT_ENOUGH),
    }
}

I'm wondering what would be preferable.

[djc]

We can't change the public API for now. I guess we could (a) clearly document panicking in the method documentation and (b) deprecate the method in favor of an alternative that returns a Result.

[5225225]

Well, it already returns a ParseResult, so we could do the same as what we do if it's 31 DEC 200000000 00:00 -1000 and return Err(ParseError(OutOfRange)). Maybe not strictly true, but it's more useful than panicking.

[djc]

Oh yeah, that sounds good to me!

[botahamec]

I got the test to pass by adding

datetime
    .checked_sub_signed(time::Duration::seconds(i64::from(offset.local_minus_utc())))
    .ok_or(OUT_OF_RANGE)?;

but that feels hacky. Maybe a better solution would be to add a FixedOffset::checked_from_local_datetime that isn't part of the TimeZone trait? If the answer is that I'm overthinking it, that's cool too. It just feels weird since it's very similar to what's already happening inside the from_local_datetime.

[djc]

Stuff is getting reorganized in #677 -- probably pays off to keep it simple?

[botahamec]

Alright then. I'll make a PR.