time-rs / time

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

Possible error parsing negative UTC offset with zero hours #522

Closed tmpfs closed 1 year ago

tmpfs commented 1 year ago

Hi,

I may be misunderstanding how negative offsets work when the hours are zero but I just found an unexpected positive offset with this code:

use time::{UtcOffset, format_description};

fn main() {
    let offset_format = format_description::parse(
        "[offset_hour sign:mandatory][offset_minute]",
    ).unwrap();
    let value = "-0057";
    let result = UtcOffset::parse(value, &offset_format).unwrap();
    // Yields "+00:57:00" but expecting "-00:57:00"
    println!("{}", result);
}

I was expecting a negative UtcOffset 57 minutes to the West, is this a bug?

jhpratt commented 1 year ago

This is definitely a bug. I would've sworn I fixed this before, but apparently not. I'll get a patch pushed up soon for this, but it won't be released until November 19 at the earliest. This is because there is an MSRV bump on main that cannot be released before then.

jhpratt commented 1 year ago

Hmm…this is actually a bit trickier than I anticipated. Basically, the values are assigned directly into the Parsed struct, which is then converted into final type. As a result of the way things are parsed, the sign is always attached to the offset hour. One problem: Parsed::set_hour, which is currently used internally, accepts the hour. This naturally means that negative zero is semantically lost.

What I'm attempting is to have an additional flags that's set when negative zero hours is encountered, thus indicating that the value is negative. It's a bit of a hack, but short of forcing ones' complement, that's the best I can do. The flag will be wholly internal, such that it's not possible for a user to directly access it.

jhpratt commented 1 year ago

Fixed on main.

tmpfs commented 1 year ago

Hmm…this is actually a bit trickier than I anticipated. Basically, the values are assigned directly into the Parsed struct, which is then converted into final type. As a result of the way things are parsed, the sign is always attached to the offset hour. One problem: Parsed::set_hour, which is currently used internally, accepts the hour. This naturally means that negative zero is semantically lost.

What I'm attempting is to have an additional flags that's set when negative zero hours is encountered, thus indicating that the value is negative. It's a bit of a hack, but short of forcing ones' complement, that's the best I can do. The flag will be wholly internal, such that it's not possible for a user to directly access it.

Thanks for the clear explanation and quick fix. Can we expect a patch release to be published on or after the 19th? I would prefer to avoid depending on main in the meantime if a published version is not too far away.

jhpratt commented 1 year ago

Yes, plan on a release on or within a couple days after the 19th.

tmpfs commented 1 year ago

Apologies for the ping @jhpratt , just wondering when this fix might be published? Thanks 🙏

jhpratt commented 1 year ago

Soon. Main is not currently in a state suitable for release. Given the holiday in the US, I've been quite busy this week. I haven't even been on my laptop in a couple days.

tmpfs commented 1 year ago

Ok thanks @jhpratt, enjoy the holiday, will check back in a week or two 🙌

tmpfs commented 1 year ago

Happy new year @jhpratt , I noticed you bumped the MSRV, could I please get a release that includes this fix?

jhpratt commented 1 year ago

I need to document other changes that have been made on main before creating a release. That's the blocker.