Revert "Expose the useful constant values in TimeSpan"

[tarekgh]

Reverts #103498

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[IDisposable]

Did I break something?

[tarekgh]

@IDisposable I am temporarily reverting the change till we resolve the type of issue. I hurried merging the changes without addressing all comments. or we just implement the original approved proposal.

[tarekgh]

Did I break something?

No, not all reviewers agreed to have all constants as long. We need to have agreement on that before we have that in the product. I'll try to follow up.

[dotnet-policy-service]

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

[IDisposable]

If we want to move most (non Tick*) to int, that's fine. I can make that change as desired. Just let me know and either reopen the PR or I'll start a new one.

[tarekgh]

If we want to move most (non Tick*) to int, that's fine. I can make that change as desired. Just let me know and either reopen the PR or I'll start a new one.

Can we go exact as the approved proposal? #94545 (comment).

Or you think int still can cause problems? if you are ok, we can close this revert PR and apply the changes over your previous changes in a new PR and we'll be fine. what do you think?

[IDisposable]

I don't know of issues, I was left with the impression that avoiding potential of overflow was desired by this comment from @tannergooding... I welcome a final judgement.

Of course I could build it as proposed and see anything breaks in the tests.

[tarekgh]

Of course I could build it as proposed and see anything breaks in the tests.

Let's do that then. I'll close this PR. Are you going to submit a new PR? or I can do it if you are not interested?

[tarekgh]

We'll submit a change in another PR.

[tannergooding]

Or you think int still can cause problems?

The main consideration from @bartonjs was that there are some constants, such as HoursPerDay which developers may want to use with int based scenarios (like int totalHours = days * TimeSpan.HoursPerDay).

Inversely, the concern I raised is that there are scenarios this can cause problems since days * TimeSpan.HoursPerDay could overflow. For example, if days > 89,478,485 then this will overflow to a negative int.

HoursPerDay happens to be the only case where it is "sensible" for it to be int, since both MaxDays (10,675,199) and MaxHours (256,204,778) are within int.MaxValue. That is also why the signatures are FromDays(int days, int hours, long minutes, long milliseconds, long microseconds). So other values, like MinutesPerDay (1440), MinutesPerHour (60), or even the existing MillisecondsPerTick (10,000) are all expected to be used in contexts where long is the target type.

[tarekgh]

@tannergooding let's apply the approved proposal first as the changes merged and we need to fix it before it goes out. Any other suggestion can be tracked to bring it again to the design review.

@IDisposable please let me know if you'll submit a new PR or want me to do it.

[bartonjs]

They're also expected to be used in cases where int is the target type, and being long would be surprising. Sure, TimeSpan can go up to 10.6M days, but most people work on the order of 0..1000 days, and would be surprised that days * TimeSpan.SecondsPerDay is a long.

int totalSeconds = ...;
int totalDays = totalSeconds / TimeSpan.SecondsPerDay;

Nope. Doesn't compile.

[tannergooding]

Nope. Doesn't compile.

Yes, which is good because it is better for code to not compile than to silently fail due to an accidental overflow.

The user will get an error and they resolve the code by doing int totalDays = (int)(totalSeconds / TimeSpan.SecondsPerDay);. It may also make them aware of potential failures in the computation of totalSeconds since there are many scenarios under which it could have overflowed.

That is a stark contrast from the inverse case where int totalSeconds = x * TimeSpan.SecondsPerHour could trivially overflow and do so silently, for which the user likely won't find out until they hit a bug in production.

When considering TimeSpan, which is where these constants are defined, hours and days are int, all other units are long.

[IDisposable]

I've put together the changes as originally approved and it builds on my machine.. I'll push a PR in a second.

[IDisposable]

New PR to drop to int for most constants #103721

[tarekgh]

/ba-g this is straight reverted of other change. The failures are unrelated to the change.

[IDisposable]

After the revert, we've settled on exposing everything except HoursPerDay as a long.

New PR #103993