time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.13k stars 281 forks source link

Solve "large-dates" ambiguity #683

Open dennisorlando opened 6 months ago

dennisorlando commented 6 months ago

Hi, bson enables the "large-dates" feature, which means that when you add it to your crate, everything that parsed some time::PrimitiveDateTimes using the plain version of time will potentially break, due to some documented ambiguities. Is it possible to provide a way to solve those ambiguity, so that we don't have to change our program in order to handle different date-time formats? Maybe a way to provide the number of characters dedicated to the [year] value?

This is the datetime which causes problems when "large-dates" is enabled: 20240602205731Z

jhpratt commented 5 months ago

This is related to #650. After some thought, I should be able to add a modifier whose value would control whether the extended range can be used. This is what I want eventually anyway; the only difference would be that the default has to be backwards-compatible (essentially opt-out instead of opt-in).

morganava commented 2 months ago

I recently ran into this exact problem with the simple_asn1 crate when attempting to use bson in the same binary.

Upstream issue: https://github.com/acw/simple_asn1/issues/34

This problem is ultimately the root cause of this issue in the arti-client crate (when bson crate is also present): https://gitlab.torproject.org/tpo/core/arti/-/issues/1632

jhpratt commented 2 months ago

@morganava Feel free to create a pull request if you are able. Otherwise you'll be waiting on me to eventually get around to this (and there is no time frame).

morganava commented 2 months ago

@jhpratt would the acceptable solution be to make an explicit [short-year] (or some other better name) format specifier which forces years into the range -9999 to 9999 even if time/large-dates is enabled?

jhpratt commented 2 months ago

The solution would be what I described in a previous comment. Basically it would be a modifier rather than its own component.

dennisorlando commented 2 months ago

What do you mean by modifier? This -> https://docs.rs/modifier ?

jhpratt commented 2 months ago

@dennisorlando See the current reference, which contains terminology and all current supported syntax.

https://time-rs.github.io/book/api/format-description.html

dennisorlando commented 2 months ago

Minimal reproducable example, which panics when "large-dates" is enabled:

use time::{macros::format_description, PrimitiveDateTime};

fn main() {
    let format = format_description!("[year][month][day][hour][minute][second]");
    let datetime = PrimitiveDateTime::parse("20240602205731", format).unwrap();
    println!("{}", datetime);
}
dennisorlando commented 2 months ago

Anyways: I'm currently working on this. Is this new modifier suitable? I.e. an explicit 4 digit modifier?

#[non_exhaustive]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum YearRepr {
    /// The full value of the year.
    Full,
    /// Standard 4 digit format, to be used when `large-dates` feature is enabled
    Four,
    /// Only the last two digits of the year.
    LastTwo,
}
dennisorlando commented 1 month ago

I'm continuing the discussion here since the nature of the problem appears to have changed due to 19120acaf47945e98473a4c48e504cd98939a51d.

The M.R.C. has become:

use time::{macros::format_description, PrimitiveDateTime};

fn main() {
    let format = format_description!("[year][month][day][hour][minute][second]");
    let datetime = PrimitiveDateTime::parse("-20240602205731", format).unwrap();
    println!("{}", datetime);
}
dennisorlando commented 1 month ago

Anyways: I saw that you mentioned in #650 the idea of removing the large dates feature in favour of supporting it by default via a new modifier. Wouldn't removing such feature be the best design choice? By introducing repr:century, a new ambiguity is being introduced due to large-dates. Adding a new modifier which fixes this ambiguity feels weird..

What about this course of action:

The fact that it's a "maximum" means that it will also support years with less digits, which is useful when such year is separated from the other components by whitespaces, slashes or other characters. For example, this crashes because the year component has only three digits: "204/12/12" ("[year]/[month]/[day]") Even tho it's not ambiguous.

(I'm writing from my phone, sorry for the terrible text that I just wrote)

dennisorlando commented 1 week ago

Anyways: I saw that you mentioned in #650 the idea of removing the large dates feature in favour of supporting it by default via a new modifier. Wouldn't removing such feature be the best design choice? By introducing repr:century, a new ambiguity is being introduced due to large-dates. Adding a new modifier which fixes this ambiguity feels weird..

What about this course of action:

* remove large-dates (via previous deprecation to allow projects to adapt to the newer version)

* add a "maxdigits" modifier which defaults to 4 (when no sign is present) and 6 (when a sign is present; this should allow compatibility with existing extended year ranges) and lastly 2 when representing a century (I mean, who writes centuries If not with only 2 digits?)

The fact that it's a "maximum" means that it will also support years with less digits, which is useful when such year is separated from the other components by whitespaces, slashes or other characters. For example, this crashes because the year component has only three digits: "204/12/12" ("[year]/[month]/[day]") Even tho it's not ambiguous.

(I'm writing from my phone, sorry for the terrible text that I just wrote)

@jhpratt (should I have already pinged?)

jhpratt commented 1 week ago

Sorry for the delay! Feel free to ping if I don't respond within ~1 week. Even if there's a reason I hadn't responded (as was the case here due to being busy), I can at least state as such.

By introducing repr:century, a new ambiguity is being introduced due to large-dates. Adding a new modifier which fixes this ambiguity feels weird.

It's actually the same ambiguity, just presented in a different way. The proposed range modifier is a way to have the smallest API for opting in to the ambiguity in the future while being able to opt out for the time being.

The fact that it's a "maximum" means that it will also support years with less digits, which is useful when such year is separated from the other components by whitespaces, slashes or other characters. For example, this crashes because the year component has only three digits: "204/12/12" ("[year]/[month]/[day]") Even tho it's not ambiguous.

This is already supported by using padding:none.