chronotope / chrono

Date and time library for Rust
Other
3.25k stars 515 forks source link

Operations on NaiveDate are not commutative. #1345

Closed dcechano closed 6 months ago

dcechano commented 9 months ago

When taking an arbitrary date, 2022-05-15 for example, and performing operations on that date, the order of operations does appear to matter.

For example, taking the date above and subtracting 20 years, then adding 111 months, then 1000 days is not the same as taking the same date, adding 1000 days, subtracting 20 years and adding 111 months. I have pasted some code below that shows this:

        let expected = NaiveDate::parse_from_str("2014-05-11","%Y-%m-%d").unwrap();

        let mut date = NaiveDate::parse_from_str("2022-05-15", "%Y-%m-%d").unwrap();
        date = date.sub(Months::new(20 * 12)).add(Months::new(111)).add(Duration::days(1000));
        assert_eq!(expected, date);

        let mut date = NaiveDate::parse_from_str("2022-05-15", "%Y-%m-%d").unwrap();
        date = date.add(Duration::days(1000)).sub(Months::new(20 * 12)).add(Months::new(111));
        assert_eq!(expected, date);

The 2nd assertion fails with the date variable showing 2014-05-08, which is 3 days off from the expected 2014-05-11.

djc commented 9 months ago

This is expected, and documented:

An addition of months to NaiveDate clamped to valid days in resulting month.

Note the use of clamped.

dcechano commented 9 months ago

Thank you for your reply. However, I'm not sure I understand. I saw this but it didn't appear to be relevant to my issue. Why would clamping of days result in operations being non-commutative?

djc commented 9 months ago

Because NaiveDate::from_ymd(2023, 1, 31).add(Months::new(1)).add(Months::new(1)) is not the same as NaiveDate::from_ymd(2023,1, 31).add(Months::new(2)).

dcechano commented 9 months ago

Why not? I suppose you are saying one results in clamping while the other doesn't? I'm sorry, I am still not sure I follow. In your example nothing is being commuted.

djc commented 9 months ago

You can try the statements I posted in my last comment and see why not, right? I understand in my example there is no commutation but I think it shows that any expression containing add(Months) can never be commutative.

dcechano commented 9 months ago

I appreciate you trying to help me. I won't be able to run your example until later this evening. For now, I can be satisfied with the problem being tied to how addition of months is implemented.

pitdicker commented 9 months ago

It is not really a question of how things are implemented in chrono. Working with dates is just tricky with all sorts of failure cases because months and years don't have a fixed number of days. (Let's forget the stuff when you add the time-of-day.)

We have Add and Sub implementations for convenience, but the domain just does not allow the operations to always be commutative. And as @djc said adding and subtracting Months does clamping instead of returning an error, which I suppose makes sense in some cases. Not my preference though.

Is there a specific problem you are trying to solve that we can help with?

dcechano commented 9 months ago

Yes, I am building a parser that can take an arbitrary string such as "2022-05-15 + 1 Month -10 years + 10 hours" and give the resultant date back. The problem I ran into is that the date returned is in some cases dependant on the order of operations. I was tempted to ask what the "correct" order of operations was but I felt that it may be too arbitrary.

pitdicker commented 9 months ago

Ah, cool. You may want to read a bit about the plans in https://github.com/chronotope/chrono/issues/1282. It is sadly not ready yet, but shows the 'least bad' order of operations. Try to group things like year and month so they are added in one operation. The fewer different components you add, the more you avoid intermediate values that may not exist.

dcechano commented 9 months ago

Try to group things like year and month so they are added in one operation. The fewer different components you add, the more you avoid intermediate values that may not exist.

Thank you! Grouping operations is very helpful advice. Having looked at #1282 it sounds like a great feature and may help with our issues over at coreutils.