[WIP] #39 Support for chrono

[l-7-l]

#39

• NaiveTime
• NaiveDate
• NaiveDateTime
• DateTime<Utc>
• DateTime<FixedOffset>

[l-7-l]

CC: @finnbear, @caibear 中秋快乐 happy Moon Festival🥮

[l-7-l]

🫣

[dsegovia90]

This would be lovely to have, nice work 👍🏻

[finnbear]

Sorry for the delay. I took a cursory look and found lots of unwrap, unchecked math, and unwrap_or that look suspiciously like they could trigger crashes or ignore malformed input. bitcode isn't supposed to crash, regardless of the input, or ignore malformed input.

I don't work with date libraries very often (I always use Unix time in my code) so I could be wrong about the existence or scope of the problem here.

ConvertFrom is generally best for infallible stuff (bijective mapping between "from" and "to"), whereas a custom decoder may be required to handle data with validity constraints. Remember: bitcode always validates data up-front while decoding, and then often does unwrap_unchecked to decode individual items. One way to validate your implementation is by adding the new types to the fuzzer.

TODO for us

It might be a good idea to introduce TryConvertFrom to make it easier, albeit slower, to add support for data with validity constraints.

Finally, for code style, we would prefer if each date library used its own date types (Second, Nanosecond, etc.), rather than factoring those into common code. Each library should have a self-contained implementation in ext/library_name.rs).

[l-7-l]

@finnbear

Sorry for the delay. I took a cursory look and found lots of unwrap, unchecked math, and unwrap_or that look suspiciously like they could trigger crashes or ignore malformed input. bitcode isn't supposed to crash, regardless of the input, or ignore malformed input.

I don't work with date libraries very often (I always use Unix time in my code) so I could be wrong about the existence or scope of the problem here.

ConvertFrom is generally best for infallible stuff (bijective mapping between "from" and "to"), whereas a custom decoder may be required to handle data with validity constraints. Remember: bitcode always validates data up-front while decoding, and then often does unwrap_unchecked to decode individual items. One way to validate your implementation is by adding the new types to the fuzzer.
TODO for us

It might be a good idea to introduce TryConvertFrom to make it easier, albeit slower, to add support for data with validity constraints.

Finally, for code style, we would prefer if each date library used its own date types (Second, Nanosecond, etc.), rather than factoring those into common code. Each library should have a self-contained implementation in ext/library_name.rs).

Sorry for the delay.

thank you for your patient and excellent review. It was very helpful to me.

Here is what I need to do:

1. Remove all uses of unwrap as much as possible.
2. Use ConvertFrom wherever possible.
3. Use the crate’s own types instead of common code (however, types such as Hour, Minute, Second, and Month are essentially the same across any time library. Thus, once it is confirmed that they will not change, I still recommend using common code—though keeping them separate does facilitate maintenance. Additionally, regarding support for the time library, I only changed types like year, month, day, hour, minute, and second to use common types; the rest of the code remains unchanged. Support for chrono was implemented by referring to the approach used for the time library.)

Is my understanding correct?

Best regards

[finnbear]

Remove all uses of unwrap as much as possible.

To be clear, the unwrap function (and unchecked math) is only permissible if it can never trigger a crash, regardless of the input. Whatever you do, please add the new types to the fuzzer in fuzz/ to validate this. Rather than unwrap_or(default), an error should be returned.

Use ConvertFrom wherever possible.

What I meant was ConvertFrom is probably not appropriate for date types, because they have validity constraints. The alternative is to write a new View/Decoder implementation that checks constraints and decodes in populate, storing the items in a CowSlice to return one by one in decode. We could make this easier by creating a new TryConvertFrom trait and decoder that abstracts away this complexity.

Use the crate’s own types instead of common code

Yes, please use separate types for the crate (whether from the crate or new types in bitcode). While it might seem unnecessary, it is the only way to guarantee compatibility with future time libraries.