Make most constructors fallible (relates #815) #817

udoprog · 2022-09-11T15:30:02Z

most of this message is copied from this comment

This is just a suggestion for changes to accomplish what's being proposed in #815. Consider it heavily WIP and any naming is preliminary.

I've opted for a single ChronoError type which has a private kind field that determines the message presented to the user (but is not public). I initially started with one error type per kind of error, but eventually noticed that it would make propagating errors overly hard like how constructing a DateTime either raises an error because the date or the time was invalid.

I've opted to make loading the local timezone fallible instead of panicking. I think this is motivated because technically the underlying platform might perform checks which might fail all though what these look like exactly is a bit unclear. These kind of errors are represented through ChronoErrorKind::SystemError(std::io::Error).
This has the side effect of making a number of TimeZone methods fallible - which is unfortunate because a number of timezones can easily be interacted with without erroring (Utc and FixedOffset). To mitigate this I've introduced a separate FixedTimeZone trait that extends the TimeZone trait which has a number of non-fallible methods. Note that this could also in theory have a suite of methods which do not produce LocalResult values, because the time zone used is never ambiguous.

FixedOffset::{east, west} now returns an error if the provided values is outside of a 24-hour offset. This might legitimately be a case that we are OK with panicking for instead.

Making things fallible has the effect of clearly marking the standard trait implementations which might panic, these are:

The From<T> implementations which interacts with the Local timezone, because this doesn't implement FixedTimeZone. Where appropriate these have been moved to be `TryFrom<Error = ChronoError> implementations.
As mentioned above Add and Sub.

The last big change is that LocalResult::None has been removed in favor of handling the absent case as an error in the unix implementation. This has the nice side effect that LocalResult::earlest and LocalResult::latest are neither fallible nor needs to return an Option<T>.

udoprog · 2022-09-12T14:38:33Z

Looks like an Actions a hick-up, can someone re-trigger?

udoprog · 2022-09-12T15:00:48Z

Here's a brief guide to how the new API's are used:

Note that most (if not all) *_opt methods are gone.

Methods on TimeZone have been made fallible. This is necessary since the Local TZ might need to fallibly talk to the system to figure things out.

pub trait TimeZone: Sized + Clone {
    type Offset: Offset;

    fn ymd(&self, year: i32, month: u32, day: u32) -> Result<LocalResult<Date<Self>>, Error>;
    fn yo(&self, year: i32, ordinal: u32) -> Result<LocalResult<Date<Self>>, Error>;
    fn isoywd(&self, year: i32, week: u32, weekday: Weekday) -> Result<LocalResult<Date<Self>>, Error>;

    fn timestamp(&self, secs: i64, nsecs: u32) -> Result<DateTime<Self>, Error>;
    fn timestamp_millis(&self, millis: i64) -> Result<DateTime<Self>, Error>;
    fn timestamp_nanos(&self, nanos: i64) -> Result<DateTime<Self>, Error>;

    fn datetime_from_str(&self, s: &str, fmt: &str) -> ParseResult<DateTime<Self>>;

    fn from_offset(offset: &Self::Offset) -> Self;

    fn offset_from_local_date(&self, local: &NaiveDate) -> Result<LocalResult<Self::Offset>, Error>;
    fn offset_from_local_datetime(&self, local: &NaiveDateTime) -> Result<LocalResult<Self::Offset>, Error>;

    fn from_local_date(&self, local: &NaiveDate) -> Result<LocalResult<Date<Self>>, Error>;
    fn from_local_datetime(&self, local: &NaiveDateTime) -> Result<LocalResult<DateTime<Self>>, Error>;

    fn offset_from_utc_date(&self, utc: &NaiveDate) -> Result<Self::Offset, Error>;
    fn offset_from_utc_datetime(&self, utc: &NaiveDateTime) -> Result<Self::Offset, Error>;

    fn from_utc_date(&self, utc: &NaiveDate) -> Result<Date<Self>, Error>;
    fn from_utc_datetime(&self, utc: &NaiveDateTime) -> Result<DateTime<Self>, Error>;
}

To cover some of the shortfall, the following trait is defined which is implemented for Utc and FixedOffset, the result of operating with it is unambiguous so LocalResult no longer needs to be used:

pub trait FixedTimeZone: TimeZone {
    fn offset_from_utc_date_fixed(&self, utc: &NaiveDate) -> Self::Offset;
    fn offset_from_utc_datetime_fixed(&self, utc: &NaiveDateTime) -> Self::Offset;

    fn from_utc_date_fixed(&self, utc: &NaiveDate) -> Date<Self>;
    fn from_utc_datetime_fixed(&self, utc: &NaiveDateTime) -> DateTime<Self>;
}

With this Date<Tz> and DateTime<Tz> gets a with_fixed_timezone function, since these are used in places where non-fallible operations would be useful like From impls:

impl<Tz: TimeZone> Date<Tz> {
    pub fn with_fixed_timezone<Tz2: FixedTimeZone>(&self, tz: &Tz2) -> Date<Tz2>;
}

impl<Tz: TimeZone> DateTime<Tz> {
    pub fn with_fixed_timezone<Tz2: FixedTimeZone>(&self, tz: &Tz2) -> DateTime<Tz2>;
}

Note that it would be possible to evolve FixedTimeZone to get more infallible methods.

The following changes have been done to DateLike:

pub trait Datelike: Sized {
    fn with_year(&self, year: i32) -> Result<Self, Error>;
    fn with_month(&self, month: u32) -> Result<Self, Error>;
    fn with_month0(&self, month0: u32) -> Result<Self, Error>;
    fn with_day(&self, day: u32) -> Result<Self, Error>;
    fn with_day0(&self, day0: u32) -> Result<Self, Error>;
    fn with_ordinal(&self, ordinal: u32) -> Result<Self, Error>;
    fn with_ordinal0(&self, ordinal0: u32) -> Result<Self, Error>;
}

The following changes have been done to to TimeLike:

pub trait TimeLike: Sized {
    fn with_hour(&self, hour: u32) -> Result<Self, Error>;
    fn with_minute(&self, min: u32) -> Result<Self, Error>;
    fn with_second(&self, sec: u32) -> Result<Self, Error>;
    fn with_nanosecond(&self, nano: u32) -> Result<Self, Error>;
}

The following methods have been changed on DateTime<Tz>:

impl<Tz: TimeZone> DateTime<Tz> {
    pub fn with_timezone<Tz2: TimeZone>(&self, tz: &Tz2) -> Result<DateTime<Tz2>, Error>;
}

The following methods have been changed on Date<Tz>:

impl<Tz: TimeZone> Date<Tz> {
    pub fn and_time(&self, time: NaiveTime) -> Result<DateTime<Tz>, Error>;
    pub fn and_hms(&self, hour: u32, min: u32, sec: u32) -> Result<DateTime<Tz>, Error>;
    pub fn and_hms_milli(&self, hour: u32, min: u32, sec: u32, milli: u32) -> Result<DateTime<Tz>, Error>;
    pub fn and_hms_micro(&self, hour: u32, min: u32, sec: u32, micro: u32) -> Result<DateTime<Tz>, Error>;
    pub fn and_hms_nano(&self, hour: u32, min: u32, sec: u32, nano: u32) -> Result<DateTime<Tz>, Error>;
    pub fn succ(&self) -> Result<Date<Tz>, Error>;
    pub fn pred(&self) -> Result<Date<Tz>, Error>;
    pub fn with_timezone<Tz2: TimeZone>(&self, tz: &Tz2) -> Result<Date<Tz2>, Error>;
}

The following methods have been changed/added on LocalResult<T>:

impl<T> LocalResult<T> {
     pub fn single(self) -> Result<T, Error>;
     pub fn earliest(self) -> T;
     pub fn latest(self) -> T;
     pub fn try_map<U, E, F: FnMut(T) -> Result<U, E>>(self, mut f: F) -> Result<LocalResult<U>, E>;
}

Changes to FixedOffset. This might not be necessary, but the bounds are a bit awkward so I've currently opted for making it fallible.

impl FixedOffset {
    pub fn east(secs: i32) -> Result<FixedOffset, Error>;
    pub fn west(secs: i32) -> Result<FixedOffset, Error>;
}

esheppa · 2022-09-13T12:00:32Z

Methods on TimeZone have been made fallible. This is necessary since the Local might need to fallibly talk to the system to figure things out.

Potentially we could include the error as an associated type (potentially with default) in the trait, such that implementors can specify infallible for timezones like FixedOffset and Utc

djc · 2022-09-13T12:28:12Z

Here are some thoughts/suggestions so far (haven't dug into the code yet):

I feel like we should name the type chrono::Error and leave prefixing it to downstream users.
Instead of adding a _fixed suffix for infallible methods, we could consider a try_ prefix on fallible methods.
For checked_ methods I feel like the convention is to yield Option instead of Result, and we should stick with that.
For cases where the only error case is out of range (assuming there's a decent number of these?), I wonder whether we should yield OutOfRange (which IIRC we already have in 0.5) as a separate error type, instead of yielding a more generic error type with variants that actually aren't applicable.

udoprog · 2022-09-13T12:58:30Z

Instead of adding a fixed suffix for infallible methods, we could consider a try prefix on fallible methods.

Are you suggesting this applies to all methods that have been made fallible or just the ones associated with TimeZone / FixedTimeZone? I was considering inverting the relationship between the two so that TimeZone is the infallible one (Utc / FixedOffset) and some other trait yet to be named would be the fallible one only implemented by Local but that change seemed more involved and can wait.

@esheppa This also relates to your suggestion since you wanted a generic error type. All though I'm not super happy with Infallible since the Result-oriented methods for these aren't stable yet and somewhat unergonomic. But maybe less so then a separate family of methods distinguished by a suffix like with_fixed_timezone.

For cases where the only error case is out of range (assuming there's a decent number of these?), I wonder whether we should yield OutOfRange (which IIRC we already have in 0.5) as a separate error type, instead of yielding a more generic error type with variants that actually aren't applicable.

Many things could be considered out of range, so what specific standard do you have in mind to apply here? I'm tentative towards considering invalid dates to be out of range (even though they are defined to be outside MIN_UTC / MAX_UTC) because I want the error to say the same thing as the panic currently does ("invalid date") rather than something generic ("out of range").

EDIT: Also want to emphasize that I have no opinions on naming, so I'll adopt whatever naming changes are being suggested.

udoprog · 2022-09-13T13:05:55Z

Also since this is a big and involved PR, if you feel like you can quickly just make the necessary changes to merge this don't feel obliged to block on me. So feel free to just make the changes and merge.

djc · 2022-09-13T13:12:31Z

Sorry, I have way too much on my plate in terms of maintenance (as well as moving to a new place with my family) so I'm all too happy having you take care of the code changes here.

But, maybe to make this easier to digest, it might be worth splitting it up in chunks, roughly along the lines that you presented in your comment? I think many of these changes are pretty independent so it might be nice to iron out some of the non-controversial stuff first and work from there.

README.md

udoprog · 2022-09-13T13:35:58Z

But, maybe to make this easier to digest, it might be worth splitting it up in chunks, roughly along the lines that you presented in your comment? I think many of these changes are pretty independent so it might be nice to iron out some of the non-controversial stuff first and work from there.

Don't think this is easily achievable. Most of the PR essentially flowed from "what if I do the changes to TimeZone and see what needs to be fixed". Doing it piecemeal would also leave the codebase in peculiar states and nontrivially increase the work involved to deal with intermediate compatibility (e.g. use of fallible methods in infallible ones would need to .unwrap() which would then have to be removed in subsequent PRs).

An alternative approach could be that you don't necessarily have to reach the perfect end result in this PR, but can work on changing it to what you want for release in subsequent ones. Which is easy enough to apply to changes like naming and moving things around.

Anyway, I'll do any concrete changes you're requesting to the best of my ability.

udoprog · 2022-09-14T11:28:08Z

Ok, unless I'm missing something I've made the concrete changes which are no longer being discussed. The remaining considerations are:

@esheppa Proposes a change to TimeZone to add an Error associated type instead of introducing FixedTimeZone. This is something I'd personally like to punt to a future PR that in more detail thinks over the design of the fallible / non-fallible methods because there's a lot of potential to get stuck here. I've made FixedTimeZone non-public for now since it's only used in private APIs. This should also allow for punting the naming concern @djc had over them.

@djc Proposed to consider using OutOfRange. I'd personally want to punt this for the future. It echoes the consideration I had initially on introducing detailed error types, but from a broader public API perspective and I feel it's a more complex discussion which I'd be happy to commit to putting work into before a 0.5 release. I.e.: apply a coherent pattern towards error handling.

Some markdown reflow is still present, but hopefully it's only in the odd API documentation and shouldn't be too much of a bother.

djc

I reviewed the date module. It looks like several unrelated changes have been made which I'd like to have reverted in order to keep this PR as a focused change, making it easier to review only the changes necessary to making everything fallible. As such, any other changes to the code style/structure should be avoided.

djc · 2022-09-21T11:02:40Z

src/date.rs

@@ -70,6 +69,11 @@ pub const MIN_DATE: Date<Utc> = Date::<Utc>::MIN_UTC;
 pub const MAX_DATE: Date<Utc> = Date::<Utc>::MAX_UTC;

 impl<Tz: TimeZone> Date<Tz> {
+    /// The minimum possible `Date`.


This looks like an unrelated change.

djc · 2022-09-21T11:03:16Z

src/date.rs

-    pub fn and_hms(&self, hour: u32, min: u32, sec: u32) -> DateTime<Tz> {
-        self.and_hms_opt(hour, min, sec).expect("invalid time")
+    pub fn and_time(&self, time: NaiveTime) -> Result<DateTime<Tz>, Error> {
+        let dt = self.naive_local().and_time(time);


Please keep the localdt variable name here to avoid unrelated changes.

djc · 2022-09-21T11:05:11Z

src/date.rs

-    pub fn and_hms_milli(&self, hour: u32, min: u32, sec: u32, milli: u32) -> DateTime<Tz> {
-        self.and_hms_milli_opt(hour, min, sec, milli).expect("invalid time")
+    pub fn and_hms(&self, hour: u32, min: u32, sec: u32) -> Result<DateTime<Tz>, Error> {
+        let time = NaiveTime::from_hms(hour, min, sec)?;


Would prefer to avoid the intermediate time binding here, similar to the previous structure of and_hms_opt().

djc · 2022-09-21T11:05:33Z

src/date.rs

-    pub fn and_hms_micro(&self, hour: u32, min: u32, sec: u32, micro: u32) -> DateTime<Tz> {
-        self.and_hms_micro_opt(hour, min, sec, micro).expect("invalid time")
+    ) -> Result<DateTime<Tz>, Error> {
+        let time = NaiveTime::from_hms_milli(hour, min, sec, milli)?;


djc · 2022-09-21T11:08:31Z

src/date.rs

        let date = self.date.checked_add_signed(rhs)?;
-        Some(Date { date, offset: self.offset })
+        Some(Self { date, offset: self.offset })


Although I am in favor of the Date<Tz> -> Self change, I'd prefer to move that to a separate PR to limit the scope in this PR.

udoprog · 2022-09-21T11:41:09Z

I reviewed the date module. It looks like several unrelated changes have been made which I'd like to have reverted in order to keep this PR as a focused change, making it easier to review only the changes necessary to making everything fallible. As such, any other changes to the code style/structure should be avoided.

So this might be why it's better for a maintainer do these kind of large changes. I'm sorry, but I don't have the time to put in work to undo stylistic changes that slipped through due to the size of this change, so unless there's a good reason (e.g. there's an obvious non-improvement) I can't do that for you. I understand if that doesn't work for you so feel free to close the PR and maybe this can be done at a later time. I'll hold off resolving the conflict for now until we've sorted this out.

djc · 2022-09-21T11:55:33Z

I'm sorry that you feel this is an unreasonable request. I understand that it may take some time and it is not fun work to do (but then the same is true for making the changes in this PR in the first place, to large extent). Personally I feel that it is generally reasonable to reduce time spent by the maintainer (that is, by making changes optimally easy to review) at the cost of requiring contributors to put in more time, given that maintainers are required to spend much more time, and their time is (within this particular context) "more valuable". (I am aware that you also maintain a whole bunch of projects -- although maybe you aren't as strict with contributors, so there's a different balance?)

I don't maintain chrono because I find the job particularly fun -- I do it because it needs to be done, and at the time I was the only person willing to put in the time and energy.

I hope you're willing to reconsider so that I can spend my time elsewhere (for example, by making similar changes in 0.4.x to deprecate non-_opt() constructors).

WhyNotHugo · 2022-09-21T12:07:37Z

Easiest way to get rid of the unwanted stylistic changes is is by restoring the index and adding only the wanted changes:

git fetch upstream pull/817/head  # Fetch the PR
git checkout -b 817-v2 pull/817/head  # Create a new branch based on it
git reset upstream/main  # Keep changes, but drop commits
git add -up  # Add individual chunks
# Hint: use `s` to split large chunks
git commit

This would take a while, but it's a matter of picking y/n for each change. Do remember to keep authorship information in the new commit.

udoprog · 2022-09-21T12:52:48Z

I'm sorry that you feel this is an unreasonable request.

The request to undo .and_then removal is largely what would cost a lot of time to implement for no benefit other than this review. If issues similar to that require reverting it would basically mean redoing everything (except maybe test cases). But I don't think it's an unreasonable request, I should've been more prudent.

djc · 2022-09-21T13:07:05Z

Easiest way to get rid of the unwanted stylistic changes is is by restoring the index and adding only the wanted changes:
git fetch upstream pull/817/head  # Fetch the PR
git checkout -b 817-v2 pull/817/head  # Create a new branch based on it
git reset upstream/main  # Keep changes, but drop commits
git add -up  # Add individual chunks
# Hint: use `s` to split large chunks
git commit
This would take a while, but it's a matter of picking y/n for each change. Do remember to keep authorship information in the new commit.

Unfortunately many of the changes do not neatly align to diff hunk boundaries, so it's not as easy as this. That said, if you want to contribute, that would be great!

djc · 2022-09-21T13:08:34Z

I'm sorry that you feel this is an unreasonable request.

The request to undo .and_then removal is largely what would cost a lot of time to implement for no benefit other than this review. If issues similar to that require reverting it would basically mean redoing everything (except maybe test cases). But I don't think it's an unreasonable request, I should've been more prudent.

FWIW, I would prefer and_then() over introducing single-use bindings even if this was new code.

udoprog · 2022-09-21T23:28:15Z

All right. I'll close for now and re-open if I ever have the time to redo the PR so it conforms to expectation. If anyone else wants to pick it up, feel free.

- Creates a global Error enum - Breaks backwards compatiblility mainly because of promoting fallable functions (chronotope#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 chronotope#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 (chronotope#829) or mainlined commits (chronotope#271) Co-authored-by: John-John Tedro <udoprog@tedro.se>

Make most constructors fallible (relates chronotope#815)

ee60951

udoprog marked this pull request as draft September 11, 2022 15:30

udoprog mentioned this pull request Sep 11, 2022

Consider refactoring chrono to avoid systematic misuse of panics #815

Closed

udoprog added 3 commits September 11, 2022 17:33

Fix lints

4ed9a6e

Fix feature issues

c76a799

Fix wasm build

42236f0

udoprog force-pushed the fallible-constructors branch from f3625b5 to 42236f0 Compare September 11, 2022 15:52

udoprog added 3 commits September 11, 2022 17:55

Fix 1.38.0 compat issue

43247cf

Fix more wasm compat

08595c3

Adjust documentation

1af5687

udoprog marked this pull request as ready for review September 11, 2022 16:42

udoprog added 2 commits September 12, 2022 17:03

Fix Date::succ / Date::pred

6e3e5cc

Merge remote-tracking branch 'upstream/main'

7635423

esheppa added this to the 0.5 milestone Sep 13, 2022

djc reviewed Sep 13, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

udoprog added 4 commits September 14, 2022 12:47

Make checked_* operations return Option again

c0f7116

Rename ChronoError to Error

fd691d0

Revert reflow in README and lib.rs

949fb99

Make FixedTimeZone private

3ea74cc

djc reviewed Sep 21, 2022

View reviewed changes

udoprog closed this Sep 21, 2022

Zomtir mentioned this pull request Mar 22, 2023

Fallable constructors for less panics (#815) #996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make most constructors fallible (relates #815) #817

Make most constructors fallible (relates #815) #817

udoprog commented Sep 11, 2022 •

edited

Loading

udoprog commented Sep 12, 2022 •

edited

Loading

udoprog commented Sep 12, 2022 •

edited

Loading

esheppa commented Sep 13, 2022

djc commented Sep 13, 2022

udoprog commented Sep 13, 2022 •

edited

Loading

udoprog commented Sep 13, 2022

djc commented Sep 13, 2022

udoprog commented Sep 13, 2022 •

edited

Loading

udoprog commented Sep 14, 2022 •

edited

Loading

djc left a comment

djc Sep 21, 2022

djc Sep 21, 2022

djc Sep 21, 2022

djc Sep 21, 2022

djc Sep 21, 2022

udoprog commented Sep 21, 2022

djc commented Sep 21, 2022

WhyNotHugo commented Sep 21, 2022

udoprog commented Sep 21, 2022

djc commented Sep 21, 2022

djc commented Sep 21, 2022

udoprog commented Sep 21, 2022

Make most constructors fallible (relates #815) #817

Make most constructors fallible (relates #815) #817

Conversation

udoprog commented Sep 11, 2022 • edited Loading

udoprog commented Sep 12, 2022 • edited Loading

udoprog commented Sep 12, 2022 • edited Loading

esheppa commented Sep 13, 2022

djc commented Sep 13, 2022

udoprog commented Sep 13, 2022 • edited Loading

udoprog commented Sep 13, 2022

djc commented Sep 13, 2022

udoprog commented Sep 13, 2022 • edited Loading

udoprog commented Sep 14, 2022 • edited Loading

djc left a comment

Choose a reason for hiding this comment

djc Sep 21, 2022

Choose a reason for hiding this comment

djc Sep 21, 2022

Choose a reason for hiding this comment

djc Sep 21, 2022

Choose a reason for hiding this comment

djc Sep 21, 2022

Choose a reason for hiding this comment

djc Sep 21, 2022

Choose a reason for hiding this comment

udoprog commented Sep 21, 2022

djc commented Sep 21, 2022

WhyNotHugo commented Sep 21, 2022

udoprog commented Sep 21, 2022

djc commented Sep 21, 2022

djc commented Sep 21, 2022

udoprog commented Sep 21, 2022

udoprog commented Sep 11, 2022 •

edited

Loading

udoprog commented Sep 12, 2022 •

edited

Loading

udoprog commented Sep 12, 2022 •

edited

Loading

udoprog commented Sep 13, 2022 •

edited

Loading

udoprog commented Sep 13, 2022 •

edited

Loading

udoprog commented Sep 14, 2022 •

edited

Loading