Closed addisoncrump closed 1 month ago
To be clear, I think part of the problem here is that the printed duration now reads quite strangely. 10518456960 minutes AND 972891790359s? This is quite unintuitive, since I would never expect to need to compute the actual number of minutes based on the number of seconds as well. In other words, I think it is confusing that the seconds value ever is greater than or equal to 60 when minutes are presented, and similarly for other units.
Catching up here, here is a concrete program that fails that I believe should succeed:
use jiff::{Span, Unit};
fn main() -> anyhow::Result<()> {
let span1: Span = "Pt843517081,1H".parse()?;
let iso = span1.to_string();
eprintln!("span1 serialized: {iso:?}");
let span2: Span = iso.parse()?;
assert_eq!(span1.total(Unit::Hour)?, span2.total(Unit::Hour)?);
Ok(())
}
Output:
$ cargo -q r
span1 serialized: "PT175307616h10518456960m1774446656760s"
Error: failed to parse ISO 8601 duration string into `Span`: failed to set value 1774446656760 as second unit on span: parameter 'seconds' with value 1774446656760 is not in the required range of -631107417600..=631107417600
So this looks to me like there is a real bug here. Nice find! I'll investigate more later.
But yes, there are some shenanigans happening here:
1 minute 120 seconds
are unbalanced and weird, but totally valid. Serializing them should generally proceed as-is whenever possible.Span
values, but is a necessary trade-off for ISO 8601 support. However, the actual elapsed time represented by the span should never change.Span
has minimum and maximum values that are less than the minimum and maximum Span
value. So you can use the maximum amount of hours, but still add minutes on top of that to represent the span. And that is likely the source of the unbalanced spans you're seeing.But I can't investigate more closely right now. I'll do so later.
The following testcases trigger the observed behaviour:
Looking a bit further, it seems that the routine for parsing fractional time is a bit strange in the presence of large values. Specifically (for the case of minutes), this code region:
This code is explained by the following text:
It is unclear to me why the units would be backfilled to the smallest unit. Shouldn't this be implemented in the other direction, preferring larger units for larger values?
Regardless, the result is that the
nanos
value is set to something large. For the first testcase, the resulting computed timestamp is printed asPT10518456960m972891790359s
. This value is correct, but not parseable because the seconds unit is out of range:Note that in memory, this value is actually:
So it would appear that this issue is in part induced by the printer implementation.
This was caught by #57 because the printer emitted invalid timestamps.