0.5: Converting the API to return Results (part 1)

[pitdicker]

This is the first small step to convert the API to return Result's.
I have been looking forward to working on this for a long time. Now that most panic cases are fixed, our const work is done, and the 0.5 branch has had some basic clean-ups it seems like a good time to start.

Error enum

The first commit introduces a basic Error enum. We can grow it as needed and bikeshed the variant names.

// TODO: Error sources that are not yet covered are the platform APIs, the parsing of a `TZfile` and
// parsing of a `TZ` environment variable.
#[non_exhaustive]
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum Error {
    /// One or more of the arguments to a function are invalid.
    ///
    /// An example is creating a `NaiveTime` with 25 as the hour value.
    InvalidParameter,

    /// The result, or an intermediate value necessary for calculating a result, would be out of
    /// range.
    ///
    /// An example is a date for the year 500.000, which is out of the range supported by chrono's
    /// types.
    OutOfRange,

    /// A date or datetime does not exist.
    ///
    /// Examples are:
    /// - April 31,
    /// - February 29 in a non-leap year,
    /// - a time that falls in the gap created by moving the clock forward during a DST transition,
    /// - a leap second on a non-minute boundary.
    DoesNotExist,

    /// Some of the date or time components are not consistent with each other.
    ///
    /// An example is parsing 'Sunday 2023-04-21', while that date is a Friday.
    Inconsistent,

    /// Character does not match with the expected format.
    ///
    /// Contains the byte index of the character where the input diverges.
    InvalidCharacter(u32),

    /// Value is not allowed by the format (during parsing).
    ///
    /// Examples are a number that is larger or smaller than the defined range, or the name of a
    /// weekday, month or timezone that doesn't match.
    ///
    /// Contains the byte index pointing at the start of the invalid value.
    InvalidValue(u32),

    /// The format string contains a formatting specifier that is not supported.
    ///
    /// Contains the byte index of the formatting specifier within the format string.
    UnsupportedSpecifier(u32),
}

#1049 has a (slightly outdated) summary our API and the error cases that can arise. I diverged a bit from the latest design in #1049 (comment):

• Error::InvalidCharacter and Error::InvalidValue are new. They are for our parsing code, so not entirely relevant yet to this PR. I'll add a comment to Converting the API to return Results #1049 describing them.
• UnsupportedSpecifier is new. My idea was to use Error::ParsingError for parsing errors in both the format string and input string. But it is better to keep them separated.

First methods: NaiveTime::from_hms* and NaiveDate::and_hms*

These methods are reasonably stand-alone and give a taste of the direction.

I added an ok macro to help with converting the API piece-by-piece. We can't use Option::ok because it is not available in const functions.

Next work

• The formatting module is more involved but can probably be done independent of the rest of the API.
• Duration/TimeDelta can probably be done independent of the rest of the API.
• FixedOffset can probably be done independent of the rest of the API.
• Good next candidates are the constructors of NaiveDate. Removing the DateImpl and Of abstractions in Refactor internals module #1212 will help a lot for this. So I'll wait until that makes it to the 0.4 branch and then to 0.5.
• NaiveDateTime becomes easy after NaiveDate is done.
• The Datelike trait depends on NaiveDate.
• DateTime depends on changes to LocalResult and the TimeZone trait first.
• The large chunk of code in local::tz_info can probably do with a refactor first. Or we should just convert its error types at the boundary.

The rest will just be a discovery of which changes don't result in massive commits.

@Zomtir Still interested in biting off a piece of the work?

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (5d1dc65) 94.16% compared to head (ebf70c9) 94.14%.
Report is 1 commits behind head on 0.5.x.

Files	Patch %	Lines
src/error.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            0.5.x    #1410      +/-   ##
==========================================
- Coverage   94.16%   94.14%   -0.03%     
==========================================
  Files          34       35       +1     
  Lines       16824    16849      +25     
==========================================
+ Hits        15842    15862      +20     
- Misses        982      987       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Zomtir]

Still interested in biting off a piece of the work?

Yes! Would commits like this be helpful? It is based on this pull request and prepares to include ParsingError. Once we agree on an error mapping I would propagate the error into the source files.

9ca9a91

It also highlights some difficulties with too simplistic naming. "INVALID" is very dependent on the context. We could either use "INVALID" for invalid fields, parameters or dates or split them into variants. I'm fine with either, the core contributors will surely have a better feedback how the naming schemes will play out best.

Iteratively changing the error names would also be an option and facilitates getting a foot in the door. The downside would be a bit more code churn.

[pitdicker]

Still interested in biting off a piece of the work?

Yes! Would commits like this be helpful?

Great! Thank you.

My plan for the parsing errors was a bit more involved than mapping the existing ParseError variants. We can make them more useful than they are today by including an index of where the parsing fails. Just finished the comment to describe it in #1049 (comment). I am making good progress in that part and would like to work on that myself.

Are you interested in starting with Duration and FixedOffset, and we'll try to find more places?

I'll reply to the rest of your comment later today.

[pitdicker]

It also highlights some difficulties with too simplistic naming. "INVALID" is very dependent on the context. We could either use "INVALID" for invalid fields, parameters or dates or split them into variants.

I did put quite some thought the error variants 😄.
My idea in #1049 was to split the error variants by causes for the error. For example:

• InvalidParameter is for function arguments that are under no circumstances, not in any combination, acceptable input. Depending on your coding style this can indicate a logic error.
• DoesNotExist can arise if a set of inputs lead to a date/datetime that does not exist. But it can also be the result of addition, substraction or other operations.
• OutOfRange is not intended for inputs (which is what InvalidParameter is for) but if the result is out of range. In other words: if you get this error in chrono, a different library that supports a wider range of dates might still return an answer.

I don't want to make the errors much more specific, like splitting InvalidParameter into InvalidYear, InvalidMonth, InvalidDay, InvalidHour etc. It makes returning the correct error more work, and having many variants makes it less likely for users to properly handle an error.

As another example I did propose a split between InvalidParameter and InvalidValue(u32). InvalidParameter is for function arguments, which are often under a programmers control. InvalidValue depends on the input string during parsing, so it can arise any time. And for parsing we can attach the position in the input that causes the error to InvalidValue.

Iteratively changing the error names would also be an option and facilitates getting a foot in the door. The downside would be a bit more code churn.

I am all for iterating. Our core types and operations should be covered, and parsing and formatting are mostly covered. The OS integration part is something to iterate on.

[djc]

Instead of adding this at the beginning, I would prefer to add variants one-by-one as they are needed by the changes you make.

Also, please order the variants alphabetically.

[pitdicker]

Okay, that can work for me.

[djc]

One more nit, I think it would now make sense to just inline this commit into the next one, so we don't introduce unused code here.

[djc]

Given that we have four instances of try_opt!(ok!()) here, I would suggest making a macro specifically for that instead of/in addition to the other ones you've added here?

[pitdicker]

While working on this I expect we are going to frequently hit the case where one method is already converted to a Result while the existing callers are still returning an Option. Having a single macro to convert it with minimal changes to the surrounding code was my goal here.

[djc]

I'm not sure how this is at odds with my suggestion? Maybe just being dense.

[pitdicker]

Thank you for looking at this! Sorry for being difficult in #1417.

[djc]

I'm okay with these code changes, but... I'm not very convinced the distinction between InvalidArgument and DoesNotExist is meaningful? If "the nanosecond part to represent a leap second is not on a minute boundary", isn't that very similar in practice to trying to Feb 29 in a non-leap year? Do we have other intended use cases for DoesNotExist?

[djc]

One more nit, I think it would now make sense to just inline this commit into the next one, so we don't introduce unused code here.

[djc]

I'm not sure how this is at odds with my suggestion? Maybe just being dense.

[pitdicker]

I'm okay with these code changes, but... I'm not very convinced the distinction between InvalidArgument and DoesNotExist is meaningful? If "the nanosecond part to represent a leap second is not on a minute boundary", isn't that very similar in practice to trying to Feb 29 in a non-leap year? Do we have other intended use cases for DoesNotExist?

My idea for DoesNotExist is to use it for cases that require more than a simple range check. From the docs:

    /// Examples are:
    /// - April 31,
    /// - February 29 in a non-leap year,
    /// - a time that falls in the gap created by moving the clock forward during a DST transition,
    /// - a leap second on a non-minute boundary.

If a method can only fail because of InvalidArgument you can safely unwrap it when you know all arguments are in range. Same with the OutOfRange error (not yet in this PR) if you know you are working with reasonable dates.
However once a method documents it can fail because of DoesNotExist you probably want to handle the error, because trying to prevent it will not be easy and duplicate the logic that is already in chrono.

For me it feels like a very different kind of error that arises in other situations than InvalidArgument and that I would like to handle differently.

[djc]

For me it feels like a very different kind of error that arises in other situations than InvalidArgument and that I would like to handle differently.

How would you practically handle it differently in calling code? If I understand correctly you want to yield InvalidArgument for Feb 32 and DoesNotExist for Feb 29 in a non-leap year. I don't think that makes for an easy to understand API.

[pitdicker]

I don't think that makes for an easy to understand API.

At least as a property of the documentation I think keeping them seperate is going to be a great help. My biggest issue before working on chrono was figuring out all the potential failure causes. Now I have already made that part of our documentation in almost all cases. Encoding the causes (at least at a fundamental level) as part of the Result that they return seems only natural.

Then there is the argument that the cause is somewhat different in nature.

• InvalidArgument is always just a simple range check of the function arguments. Unless you work with unvalidated user input I expect it to usually indicate a programming eror.
• DoesNotExist requires calendar or time zone knowledge. It can arise a couple of methods deep in constructors, but also while doing calculations on datetimes. Trying to prevent it would mean implementing the logic in chrono.

Admittedly for code calling only constructor methods such as NaiveTime::from_hms_nano I don't imagine they are going to want to take a different action on it all that often. Once a user writes a function that constructs a datetime and also does calculations on it... Maybe. I suppose they may want to let their caller know if it failed because of non-sensical input or because of a calendar / time zone issue.

Shall we re-evaluate once the work is a bit further along than the first 7 methods? Combining errors is much easier than splitting them up later.

[djc]

Shall we re-evaluate once the work is a bit further along than the first 7 methods? Combining errors is much easier than splitting them up later.

Fair!