Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.DateTime.AddDays() adds additional tick #98305

Closed
rs-blade opened this issue Feb 11, 2024 · 9 comments
Closed

System.DateTime.AddDays() adds additional tick #98305

rs-blade opened this issue Feb 11, 2024 · 9 comments

Comments

@rs-blade
Copy link

rs-blade commented Feb 11, 2024

Description

Running the following code:

new DateTime(2024, 2, 11, 11, 6, 7).AddDays(-1.2).Ticks

results in 638431426870000000 on .NET 6 (as expected)
but 638431426870000001 on .NET 7 and 8!

Configuration

.NET 7 and .NET 8

Regression?

Worked .NET <= 6

@rs-blade rs-blade changed the title AddDays adds additional tick System.DateTime.AddDays adds additional tick Feb 11, 2024
@rs-blade rs-blade changed the title System.DateTime.AddDays adds additional tick System.DateTime.AddDays() adds additional tick Feb 11, 2024
@steveharter steveharter transferred this issue from dotnet/core Feb 12, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 12, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 12, 2024
@ghost
Copy link

ghost commented Feb 12, 2024

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Running the following code:

new DateTime(2024, 2, 11, 11, 6, 7).AddDays(-1.2).Ticks

results in 638431426870000000 on .NET 6 (as expected)
but 638431426870000001 on .NET 7 and 8!

Configuration

.NET 7 and .NET 8

Regression?

Worked .NET <= 6

Author: rs-blade
Assignees: -
Labels:

untriaged, area-System.DateTime, needs-area-label

Milestone: -

@steveharter steveharter added untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Feb 12, 2024
@danmoseley
Copy link
Member

Likely just floating point precision. We have become more compliant since .NET Framework and that sometimes gives results that look less correct, but round trip correctly.

@tarekgh
Copy link
Member

tarekgh commented Feb 12, 2024

@tarekgh tarekgh closed this as completed Feb 12, 2024
@tannergooding
Copy link
Member

To help explain the why for this particular example, double is a binary-based floating-point format and thus is represented as multiples of powers of 2.

Thus, 1.2 is not exactly representable, the literal is parsed as given and then rounded to the nearest representable result which is 1.1999999999999999555910790149937383830547332763671875.

So you aren't subtracting 1.2 days (103680 seconds or 1 day, 4 hours, 48 minutes), but rather around 103679.999999999996163069226895458996295928955078125 seconds. This leads to minor differences in the result seen.

If you want precision here, you should avoid double and instead use whole integers where possible to avoid this type of issue (subtract the exact number of seconds or subtract the number of days, then the number of hours, then the number of minutes; etc).

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Feb 12, 2024
@tarekgh tarekgh removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 12, 2024
@rs-blade
Copy link
Author

rs-blade commented Feb 12, 2024

Hm. Okay...
But 103679.999999999996163069226895458996295928955078125 seconds is 1036799999999.99996163069226895458996295928955078125 ticks. If I round this to long I should get 10368000000. So it should subtract 10368000000 ticks but it subtracts 1036799999999 ticks!? Why?. It seems that instead of rounding to long just some kind of flooring is used.
In fact this means an implementation would be able to produce results that meet the expectation of the developer as long as the number of days to add/subtract is somewhere in the range of 0..9999.

Another proof is that I already have an alternative implementation of AddDays(). I wrote that for another reason (to limit the result instead of throwing an exception in case of an overflow). That implementation had the full resolution since it existed and never had that "1 tick issue" for small numbers of days.

My guess is that the current implementation is also highly optimized which leads to that fault...sorry: breaking change.

@tannergooding
Copy link
Member

Truncation is indeed used by default, because that's how double->long conversions behave ((long)(3.9) is 3, not 4).

But even if rounding were done, it would still lead to bugs and unexpected behaviors for users in other cases. Consider that 0.1 is actually 0.1000000000000000055511151231257827021181583404541015625 while 0.3 is actually 0.299999999999999988897769753748434595763683319091796875.

Depending on the exact value used and the exact tick counts involved, you may end up just over or just under what you intended. As much as rounding would fix this particular case, it would inversely break another users case (and we've had past issues where such a scenario cropped up and we had to explain it there as well).

The best practice here is really to just not use non-integral values.

That implementation had the full resolution since it existed and never had that "1 tick issue" for small numbers of days.

If you're taking in double, then you have results that some user would quantify as a bug and you've just not personally hit them yet. This is because double fundamentally cannot represent the full range of ticks without issue. There are some approaches that can help minify this at the cost of perf, but its never truly all gone.

@rs-blade
Copy link
Author

So what are methods to add time spans with integers?
AddDays/AddHours/... do not have overloads for int or long. And I did not find other methods.
(Using DateTime.Add(TimeSpan) isn't very helpful because TimeSpan.FromDays/FromHours/... also do not have overloads for integer types...).

@tarekgh
Copy link
Member

tarekgh commented Feb 14, 2024

We have #93890 which addressing the TimeSpan issues.

@tannergooding
Copy link
Member

tannergooding commented Feb 14, 2024

Notably in addition to the new APIs which will cover explicitly passing in integers. Integers up to 2^53 are exactly representable as double and so for any typical input (like add 1-365 days or 103680 seconds and so on) will work exactly as expected. It really only becomes a problem when you're trying to use a double which cannot be exactly represented, like 1.2

@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants