TimeSpan.Parse gives inconsistent and incorrect results with 8 fractional digits

[pengowray]

I've made my own TimeSpanParser and have been testing it against System.TimeSpan.Parse() and have come up with some issues with TimeSpan.Parse(string)

In brief, all these tests pass successfully:

Assert.AreEqual(TimeSpan.Parse("0:00:00.0123456"),
                TimeSpan.Parse("0:00:00.00123456"));
Assert.AreEqual(TimeSpan.Parse("0:00:00.0123456").Ticks,  123456);
Assert.AreEqual(TimeSpan.Parse("0:00:00.00123456").Ticks, 123456);

For more examples, here's a walk-through of this issue:
https://github.com/quole/TimeSpanParser/blob/master/TimeParser.Tests/NotWrittenHereUnderflowWeirdnessTests.cs

TimeSpan.Parse() allows seconds with up to 7 decimal places, which is perfect to let you specify a duration down to a tick (100ns). At 9 decimal places it sensibly throws OverflowExceptions. But in between 7 and 9 it's the twilight zone, where you'll find bizarre inconsistent and incorrect results.

I'm guessing the check for "underflows" is off by one decimal place, and is meant to throw an OverflowException for all 8 decimal place numbers, but instead only does it consistently around 9.

Suggestions:

• Not sure how you can fix this without breaking existing code.
• At the least, TimeSpan.Parse() ought to ignore trailing zeros after the decimal place (and definitely shouldn't cause the result to be multiplied by 10)

I couldn't find any related reports. Is this really a new issue?

Tested on net core 2.0 & 2.2

[danmoseley]

@Quole thank you for the report. Are you interested in debugging it?

[pengowray]

Sure, I'll give it a go

[Clockwork-Muse]

Is this related to the rounding (and overflow) that can happen with DateTime.Parse()?

[pengowray]

I don't think it's related though it might be similar. ParseFraction isn't found in TimeSpanParse.cs, which has its own code to "Normalize the fraction component".

edit: Actually they both use static method TimeSpanParse.Pow10() so maybe more similarity than I first noticed.

[pengowray]

Found the bug. See above for details.

So, I should be able to answer if it's related to #27673, the bug you (@Clockwork-Muse) mentioned, but the answer is complicated, and I think is interesting so I'm going to answer it fully.

The resolution to that bug seems to be have been to stop DateTime.TryParse from causing an exception from ticks going beyond MaxTicks or MinTicks. This bug is not related to that issue specifically.

However, your comment on it isn't about the exception handling but about the fact it doesn't truncate the 8th fractional digit, which is what causes the overflow in the first place. I'm not sure if that was addressed in the fix for #27673 ? Sorry, I can't easily test the latest version of the code, but it looks like DateTime.Parse("9999-12-31T23:59:59.99999999Z"); would still throw an OverflowException, and the final 9 would not be truncated, and only the TryParse version was somewhat fixed. (Again, I'm not sure, I don't have the fixed version set up)

Relating it back to this bug, I'm going with idea that TimeSpan.Parse() has already has decided to Overflow rather than truncate the extra digits—as can be seen by its current attempt to throw an exception from them. It attempts to, but it just does it incorrectly. It does correctly overflow at 9 digits and inconsistently at 8 (e.g. numbers ending in 98 and 99 behave differently). That part is fixed by the patch above.

Whether or not TimeSpan.Parse() ought to truncate the extra fractional digits rather than Overflow from them, well that's basically a separate issue, perhaps a policy issues, and at least an additional step. One which is indeed related to your comment on #27673, but isn't related to the actual patch which resolved it. (AFAICT)

One thing worth noting is that while the idea of an Overflow from too many fractional digits doesn't make much sense for a DateTime (as you rightly pointed out), it sometimes does for a TimeSpan, for example if you attempt to store one nanosecond as a TimeSpan, maybe it should actually Overflow rather than round to 0 ticks. It's a bit of a judgement call though. At the moment TimeSpan.Parse is much more zealous than that and will overflow from anything longer than 7 fractional digits, including 8 zeroes, but worth considering if it's going to be changed.

I do agree that both TimeSpan.Parse() and DateTime.Parse() generally ought to simply truncate the additional digits (which is what I did in my own TimeSpanParser), so perhaps that calls for a change.

So in summary: Both this issue and #27673 could also have been resolved by truncating digits, which means on a superficial as well as on a deeper more fundamental level they are related. But as long as the decision is to have those extra digits overflow, they're unrelated issues in that they're in separate .cs files, create unrelated classes of error, and are resolved in different ways. But if suddenly we allowed the truncation of excess digits (which would be largely sensible), then the respective areas of source code these errors were found in would become somewhat redundant.

[joshfree]

@tarekgh could you PTAL

[tarekgh]

This issue is already fixed by the PR dotnet/coreclr#21968