Fallable constructors for less panics (#815)

[Zomtir]

This is an updated version of fallable constructors. Unfortunately it's quite impossible to split this up into smaller chunks, as mentioned in the original pull request. There are still quite some issues, but would be glad to have some feedback.

• Creates a global Error enum, the naming convention, scope and documentation style needs feedback
• Breaks backwards compatiblility mainly because of promoting fallable functions (Improvements to parsing into Utc, converting from other timezones to Utc #263)
• Some tests still fall
• Not all doctests are fixed
• to_naive_datetime_with_offset function is broken and needs fixing
• serde related stuff is not checked properly

This is a rebase of #817 onto the 0.5 main branch. Main differences:

• Unify three different error structs
• Removed ErrorKind
• Adapted a lot of unit tests
• Removed some commits from presumably unrelated branches (now() functionality for FixedOffest and a Utc::fixed_now() for DateTime w/ TimeZone (set to 0) #829) or mainlined commits (Add std::convert::From conversion between different DateTime offsets #271)

[jtmoon79]

Overall, I like this PR because it carries Error information forward to the user. The user can decide what to do with it. The user might print the error to the console so the end-user will have a hint about what went wrong.

[jtmoon79]

IIUC, it looks like the recently added API fn and_hms_opt are being removed, and the "old" API like fn and_hms (previously marked #[deprecated(...)]) is being resurrected.

I'm afraid this will be too much churn for users. They will have switched from fn and_hms to fn and_hms_opt. With this PR, they must now switch back to fn and_hms.

My recommendation is to:

1. leave fn and_hms_opt in-place, so to not annoy current users of that API.
2. Then rewrite fn and_hms with the changes proposed here (returning Result<DateTime<...>, Error>).
3. The fn and_hms_opt should wrap a call to fn and_hms.

Of course, this should apply to all similar interfaces.

[Zomtir]

Thanks for your input! I can not give specific reasoning for the case of fn and_hms and fn and_hms_opt, but in general I try to follow this pattern:

Return Result<T,E> for fallible functions and return T for "safe" functions, with "safe" being explicitly mentioned/required by the documentation or const context.

Prefixing all fallible functions with try_ was also suggested in #815, but this seems excessive when applied consequently. Once the error-propagation is done, the backwards-compability may be more in focus. This targets v0.5 and will introduce breaking changes.

The mess with fn and_hms might also be a byproduct of the rebase, which I am still working on and might solve itself later in the progress. The functions related to with_ymd_and_hms also have a pending cleanup.

---

Right now I'm stuck on Timezone::offset_from_utc_date() and Timezone::offset_from_utc_datetime(). Both are described as "cannot fail", although they should. Neither are deprecated.

TimeZone::from_utc_date() is deprecated and may panic/fail. TimeZone::from_utc_datetime() is suggested instead and may panic/fail.

It is hard to judge what behavior is intended and sometimes I have to guess or wait for feedback.

[jtmoon79]

This targets v0.5 and will introduce breaking changes.

I guess it's up to @djc and @esheppa how many breaking changes they want in 0.5 release. My concern is overwhelming users with a seeming "back and forth" approach to and_hms and and_hms_opt. (use and_hms! don't use and_hms, switch to and_hms_opt! use and_hms because and_hms_opt was removed!)

The mess with fn and_hms might also be a byproduct of the rebase

Sorry, I don't know what you mean.

Right now I'm stuck on Timezone::offset_from_utc_date() and Timezone::offset_from_utc_datetime(). Both are described as "cannot fail", although they should. Neither are deprecated.

Good point. Seems like trying to be consistent about "cannot fail" and "should fail" might be a bit of work.

[Zomtir]

My concern is overwhelming users with a seeming "back and forth" approach to and_hms and and_hms_opt.

I picked up your proposal and added the wrapper for backwards compatibility. This could be applied in other cases as well. However many function had a fingerprint change without receiving a new function name (returning Result<T,E> instead of just T). This might lead to additional confusion ...

    /// Wraps `and_hms` for backwards compability.
    #[deprecated(since = "0.5.0", note = "Use and_hms instead")]
    pub fn and_hms_opt(&self, hour: u32, min: u32, sec: u32) -> Result<DateTime<Tz>, Error> {
        self.and_hms(hour, min, sec)
    }

The mess with fn and_hms might also be a byproduct of the rebase

Sorry, I don't know what you mean.

The main trunk code base has diverged quite a lot since the original patch in #817. For many conflicts I chose the branch version and therefor dropped changes from the trunk. A few functions have been deprecated in the main trunk while they were still heavily used in the branch (e.g. Utc.ymd() ). I am not sure if and_hms_opt has already been present in the old branch.

[jtmoon79]

My concern is overwhelming users with a seeming "back and forth" approach to and_hms and and_hms_opt.

I picked up your proposal and added the wrapper for backwards compatibility. This could be applied in other cases as well. However many function had a fingerprint change without receiving a new function name (returning Result<T,E> instead of just T). This might lead to additional confusion ...

/// Wraps `and_hms` for backwards compability.
#[deprecated(since = "0.5.0", note = "Use and_hms instead")]
pub fn and_hms_opt(&self, hour: u32, min: u32, sec: u32) -> Result<DateTime<Tz>, Error> {
    self.and_hms(hour, min, sec)
}

Notice that fn and_hms_opt (and similar functions) return Option<T>.

https://github.com/chronotope/chrono/blob/v0.4.23/src/date.rs#L109-L111

Whereas in your proposed change, which you provided a sample of, it is returning Result<T>. That would still require users to change every call to fn and_hms_opt (and similar functions). So not very "backwards compatible". The fn and_hms_opt (and similar functions) should continue to return Option<T>.

[Zomtir]

Whoopsie, changed in 33c0647.

[Zomtir]

I would need help with the last two build errors:

• The implications of thumbv6m-none-eabi with #![no_std] are not clear to me.
• serde_json is a #[test] requirement, but I failed to map this condition properly onto From<serde_json::Error> in error.js. I elevated serde_json to a full dependency as a short-term solution because I don't know better.

---

Apart from that, it looks somewhat reviewable. Input is especially welcome on the following topics:

The chrono::Error enum names should probably be more consistent and have more documentation. I kept the old names for the most part, but when they all are in the same enum, they should be more self-explanatory.

Utc.with_ymd_and_hms returns LocalResult although it doesn't have to in principle. Single(_) is always expected. If I remember correctly, just one fn implementation of TimeZone was fallible and blocked this to always return Single(_). This is different to Local, where Ambiguous(_) is a possibility.

LocalResult.unwrap() is done by returning single().unwrap(). This does not sound right as it hides the fact that it only checks for the single value. LocalResult.unwrapSingle() would be a more fitting name, but LocalResult.single()? does the same job safer and shorter. I'm in favour of removing LocalResult.unwrap().

Utc.with_ymd_and_hms and Local.with_ymd_and_hms should probably use LocalResult.earliest() by default and this should be put explicitly in the documentation. Both are helper functions in the first place and users will do the same in most cases anyway. If you require a detailed breakdown, you should use NaiveDate.from_ymd()?.and_hms()?.and_local_timezone().

Lastly in many instances those functions would have been helpful. I would gladly add them:

• Utc.with_ymd_and_hmsm for UTC milliseconds
• Utc.with_ymd_and_hmsn for UTC nanoseconds
• Local.with_ymd_and_hmsm for Local milliseconds
• Local.with_ymd_and_hmsn for Local nanoseconds

[esheppa]

This will be causing the MSRV failure (one of the two CI failures) due to:

Note: The dep: syntax is only available starting with Rust 1.60. Previous versions can only use the implicit feature name.

There are suggestions that we will probably go higher than the current 1.48 (see: #995), but it may not go as high as 1.60 (put another way, we probably can't justify moving to 1.60 just to get this dep:{crate} syntax)

[pitdicker]

@Zomtir Impressive work!

I am interested in the direction of this PR. Maybe I can help move it forwards a bit...

Error enum in this PR

You currently define an Error enum with many variants:

pub enum Error {
    InvalidDate,
    InvalidTime,
    InvalidDateTime,
    InvalidTimeZone,
    AmbiguousDate,
    MissingDate,
    SystemTimeBeforeEpoch,
    ParsingOutOfRange,
    ParsingImpossible,
    ParsingNotEnough,
    ParsingInvalid,
    ParsingTooShort,
    ParsingTooLong,
    ParsingBadFormat,
    DateTime(&'static str),
    FindLocalTimeType(&'static str),
    LocalTimeType(&'static str),
    InvalidSlice(&'static str),
    InvalidTzFile(&'static str),
    InvalidTzString(&'static str),
    Io(std::io::ErrorKind),
    OutOfRange(&'static str),
    ParseInt(core::num::ParseIntError),
    ProjectDateTime(&'static str),
    SystemTime,
    TimeZone(&'static str),
    TransitionRule(&'static str),
    UnsupportedTzFile(&'static str),
    UnsupportedTzString(&'static str),
    Utf8(core::str::Utf8Error),
    TryFromIntError,
    FromUtf8Error,
    UnexpectedEOF,
    InvalidData,
    DurationExceedsTimestamp,
    DurationExceedsLimit,
    TimestampExceedsLimit,
    SerializationError,
}

I don't think anyone is served by having this many variants.

• For example the second variant, InvalidTime. It is the result of methods on NaiveTime that fail when one of the parameters is invalid: hour > 24, minute > 59, second > 60, millisecond > 2000, etc.
  The same for InvalidDate. One of the methods that can create it is NaiveDate::from_weekday_of_month when n == 0.
  And InvalidTimeZone as returned by FixedOffset::east (or west) when |secs| > 86_400.
  I would expect all methods such as these that fail because of an invalid input parameter to return something like InvalidInput.

• AmbiguousDate should not become part of the Error type, it should remain in LocalResult so library users can choose how to resolve the ambiguity (or do something else).
  Similarly MissingDate should remain part of LocalResult, so users can choose how to deal with a datetime that does not exist in the specific timezone.

• SystemTimeBeforeEpoch on Windows should have no reason for existing.

• Error::DateTime, ErrorProjectDateTime, Error::SystemTime, Error::Utf8 are never used.

• Should all the specific reasons why parsing of timezone data on Unix might fail really be encoded in the error type of chrono?

• OutOfRange can be useful. In this PR I can only see it used internally in the Unix timezone implementation, not exposed in methods to users of chrono. I would expect OutOfRange when some calculation causes a datetime to get out of the representable range of NaiveDateTime.

A more limited error enum

pub enum ChronoError {
    InvalidParameter, // example: hour == 25
    Overflow, // example: year = 500_000
    DateDoesNotExist, // example: 31 February
    DurationExceedsTimestamp, // Can `duration_round` not be made smarter to never fail?
    LocalTzLoadingError, // Catchall for all errors caused by the OS in `offset::local`
    InconsistentDate, // example: parsing Sunday 2023-04-21 (should be Friday)
    ParsingError(ParsingError), // collect all other reasons for failing to parse something in a nested enum.
    /* maybe more ... */
}

Adding useful information for dealing with an error

It would really depend on the method that causes an error, but maybe some enum variants should carry helpful information.
For example Overflow. Maybe I want to deal with an overflow by using the minimum or maximum value instead. I.e., implementing some saturating_add function for whatever reason. How do I know whether it is out of range on the minimum or maximum side, and what the last valid value is?
Or in case of InconsistentDate. Supposedly I would want to know the date and day that conflict, and make a choice.

Parsing errors

In my opinion parsing is such a specific operation, it should keep its own error type. On the other hand are there some overlaps:

• A format string may be invalid or may not contain enough specifiers to construct the expected type. Both would probably match InvalidParameter.
• The parsed date may not exist, DateDoesNotExist.
• The parsed date may be inconsistent, InconsistentDate,

I propose to add it as a nested error type in a ParsingError variant.

Local timezone errors

Not sure how deep to go here.

Errors mapped from external traits

I am not sure if these errors should even be mapped to another type:

    TryFromIntError,
    FromUtf8Error,
    SerializationError, // serde

Splitting up this massive PR

Is there really no way to split up this PR, and make it reviewable?
I would suggest starting with just adding the error enum with helper functions, and in later PRs building on that by doing, one at a time, the Datelike trait, Timelike, NaiveDateTime, etc.

[pitdicker]

@Zomtir You and @udoprog in #817 put a lot of work into this.
But I don't think this PR can be turned into something merge-able. Besides the massive merge conflict and the WIP-style commits it is just not realistic to change almost the entire API surface in one go. It is probably best to start again from the ground up one module at a time.

I propose to close this PR for now.

Speaking personally I would like to spend a couple more months to fix backwards-compatible issues in the 0.4.x branch before working on changing the API in 0.5 to return Results. Because once we do it becomes very difficult to port fixes from the 0.4.x. branch to main.

[Zomtir]

@pitdicker Yes I agree for the most part.

This pull request should definitely stay closed. I could squash, rebase and fix the serde issues, but it is massive pull request built on at least two assumptions:

• One central clean enum chrono::Error is the way to go. Alternative would be an error struct that wraps more stuff, aside from different namings such as chrono::ChronoError.
• Methods that can fail under any circumstance, will return a Result. One could argue that if the error is highly unlikely it's okay to unwrap.

It would be nice to have some initial groundwork/spark by some central contributor and go from there.

I have less spare time at the moment so I can make no promises. This PR was mostly a vacation project over a week. I would like to submit one PR to fix a few macros in the unit tests. In at least one case there were very nasty to work with and this could be made pleasant. A second PR could be just a mostly empty error.rs file with no active usage, just to get things started.

[pitdicker]

I would like to submit one PR to fix a few macros in the unit tests. In at least one case there were very nasty to work with and this could be made pleasant.

👍

It would be nice to have some initial groundwork/spark by some central contributor and go from there.

Shall I cc you when there is a start?