Runtime: API Proposal: Support operators * / etc on type TimeSpan

[saguiitay]

Background

TimeSpan represents a time interval, which makes it a common candidate for "mathematical" processing, such as taking a 1-hour interval, and divide it by 2, resulting a 0.5-hour interval. Currently, the TimeSpan type support only a limited subset of operators (+ and -), and only with a TimeSpan operand.

Proposed API - Approved

Approved in

• https://github.com/dotnet/corefx/issues/7175#issuecomment-262323805
• https://github.com/dotnet/corefx/issues/7175#issuecomment-274889934
  ```C#
  namespace System
  {
  public struct TimeSpan
  {
  public static System.TimeSpan operator *(TimeSpan timeSpan, double factor);
  public static System.TimeSpan operator /(TimeSpan timeSpan, double divisor);

    public static TimeSpan operator *(double factor, TimeSpan timeSpan);
    public static double operator /(TimeSpan t1, TimeSpan t2);
}

}

**Motivation:**
The extra 2 APIs proposal are mainly for supporting the commutative * and / operation (basically adding the same operation with flipped parameters order) and support / with time span.

## Naïve implementation

``` csharp
namespace System
{
    public struct TimeSpan
    {
        public static System.TimeSpan operator *(TimeSpan timeSpan, double factor)
        {
            return TimeSpan.FromTicks(timeSpan.Ticks * factor);
        }

        public static System.TimeSpan operator /(TimeSpan timeSpan, double divisor);
        {
            return TimeSpan.FromTicks(timeSpan.Ticks / divisor);
        }

    }
}

[JonHanna]

I'd prefer this to use double rather than int (though I wouldn't complain about there also being integer overloads). new TimeSpan(1, 0, 0) * 2.5 == new TimeSpan(2, 30, 0)

One could also have:

C# public static double operator /(TimeSpan x, TimeSpan y) { return (double)x.Ticks / y.Ticks; }

[saguiitay]

I wouldn't care either ways, as long as I can perform operations on a TimeSpan...
Notice that + and - operations between a TimeSpan and an int/long might also make sense in certain areas, but I understand that they might create a confusion with users: TimeSpan.FromSeconds(2) + 3 == TimeSpan.FromSeconds(2) + TimeSpan.FromTicks(3)

[JonHanna]

Such addition might make sense in certain areas, but it would make completely different sense in other areas (seconds? days?). I don't think that would be a good idea.

[joshfree]

@terrajobst @KrzysztofCwalina

[KrzysztofCwalina]

@mj1856, any thoughts on this?

[mj1856]

Apologies for the delayed response...

It does make sense logically to support * and / with a double, but + and - should only work with TimeSpan as they currently do. Reason being that a multiplier or divisor doesn't need units, but addition and subtraction does.

6 inches x 3 = 12 inches.   :-)
6 inches + 3 = ???          O__o

The need for * and / on TimeSpan is actually quite common. See here for one example.

Of course, one needs to consider overflow, just like with any math. Fortunately, TimeSpan.MaxValue is about 29,227 years. :wink:

[terrajobst]

It does make sense logically to support * and / with a double, but + and - should only work with TimeSpan as they currently do. Reason being that a multiplier or divisor doesn't need units, but addition and subtraction does.

Agreed. I've also run across usages in the past where I needed multiplication and division. You can work this around today, but it feels quite dirty. I think the proposed APIs make sense.

@tarekgh, any thoughts?

[tarekgh]

I think it make sense to have * and / operations on TimeSpan with double values. what @mj1856 mentioned make sense.

[terrajobst]

So are we concerned about overflows or not? Personally, I think floating points make the most sense.

C# namespace System { public struct TimeSpan { public static System.TimeSpan operator *(TimeSpan t, double d); public static System.TimeSpan operator /(TimeSpan t, double d); } }

[terrajobst]

@tarekgh, could you take a look and make a proposal and provide a recommendation on how to deal with potential loss of precision?

[tarekgh]

The proposal here is we adding only the operators which takes double

c# namespace System { public struct TimeSpan { public static System.TimeSpan operator *(TimeSpan t, double d); public static System.TimeSpan operator /(TimeSpan t, double d); } }

For the loss of precision we already have the logic in the TimeSpan. you can look at

https://github.com/dotnet/coreclr/blob/775003a4c72f0acc37eab84628fcef541533ba4e/src/mscorlib/src/System/TimeSpan.cs#L223

basically we round the calculated ticks double number to the milliseconds and use that. I think this is the best we can do in this situation

[AlexGhiondea]

It looks like the API is ready for review. @terrajobst can you add this to the agenda for the next meeting?

[terrajobst]

We think floating points make the most sense, thus @tarekgh's shape is the approved one:

C# namespace System { public struct TimeSpan { public static System.TimeSpan operator *(TimeSpan timeSpan, double factor); public static System.TimeSpan operator /(TimeSpan timeSpan, double divisor); } }

[JonHanna]

What about dividing a span by a span?

[jnm2]

Yes please. If units of time * scalar = time is allowed, you have to allow time / time = scalar (as well as time / scalar = time).

[AlexGhiondea]

@terrajobst including @JonHanna's suggestion:

namespace System
{
    public struct TimeSpan
    {
        public static System.TimeSpan operator *(TimeSpan timeSpan, double factor);
        public static System.TimeSpan operator /(TimeSpan timeSpan, double divisor);
        public static double operator /(TimeSpan timeSpan, TimeSpan divisor);
    }
}

[JonHanna]

Also, multiplication should be commutative, so something like:

```C#
public static TimeSpan operator *(TimeSpan left, double right);
public static TimeSpan operator *(double left, TimeSpan right) => right * left;
public static TimeSpan operator /(TimeSpan left, double right);
public static double operator /(TimeSpan left, TimeSpan right);

[jnm2]

What's the reasoning behind the names multiplicand and multiplier? I think it should be either unit-specific (timeSpan and factor) or treated equally (left and right as per design guidelines).

[JonHanna]

Of those two, I favour left and right. I've updated my comment above, though I'd only been thinking of the shape for discussion (and must admit I haven't looked at the relevant part of the design guidelines in a while).

[karelz]

We should bring the additional APIs back for API review

[JonHanna]

For the loss of precision we already have the logic in the TimeSpan. you can look at
https://github.com/dotnet/coreclr/blob/775003a4c72f0acc37eab84628fcef541533ba4e/src/mscorlib/src/System/TimeSpan.cs#L223

That covers overflow and NaN inputs. Another possibility is a NaN result. That's fine for TimeSpan.Zero / TimeSpan.Zero but less so for TimeSpan.Zero / 0.0. DivideByZeroException would be one possible result, but I think then for consistency we should also make new TimeSpan(1) / 0.0 throw the same instead of OverflowException. Alternatively it could be an InvalidOperationException, but I think I prefer DivideByZeroException as both a stronger modeling of the error and because catching ArithmeticException will cover both types of error.

[JonHanna]

How about this as a case considering all of the error cases, but omitting error messages and parameter names to ArgumentException?

C# public static TimeSpan operator *(TimeSpan left, double right) { if (double.IsNaN(right)) throw new ArgumentException(); return FromDoubleTicks(left.Ticks * right); } public static TimeSpan operator *(double left, TimeSpan right) => right * left; public static TimeSpan operator /(TimeSpan left, double right) { if (double.IsNaN(right)) throw new ArgumentException(); if (right == 0.0) throw new DivideByZeroException(); // Skip this and just have the catch of infinity below? return FromDoubleTicks(left.Ticks / right); } public static double operator /(TimeSpan left, TimeSpan right) => return left.Ticks / (double)right.Ticks; private static TimeSpan FromDoubleTicks(double ticks) { if (ticks > long.MaxValue | ticks < long.MinValue) throw new OverflowException(); return new TimeSpan(ticks); }

(I'm still not sure if new TimeSpan(1, 2, 3) / 0.0 should be DivideByZeroException or OverflowException).

[tarekgh]

looks we have more APIs need to go through the review. to summarize

Approved APIs are

```C#
namespace System
{
public struct TimeSpan
{
public static System.TimeSpan operator *(TimeSpan timeSpan, double factor);
public static System.TimeSpan operator /(TimeSpan timeSpan, double divisor);
}
}

## APIs need to get reviewed 

```C#

namespace System
{
    public struct TimeSpan
    {
        public static double operator /(TimeSpan timeSpan, TimeSpan divisor);

        public static TimeSpan operator *(double left, TimeSpan right) => right * left;
        public static double operator /(TimeSpan left, TimeSpan right);
    }
}

[tarekgh]

@JonHanna I agree with throwing argument exceptions on the mentioned cases:

• passing NaN
• passing out of range ticks value
• dividing by 0.0 (even if this can just work)

[karelz]

@tarekgh thanks for summary of missing parts, please update the top-most post with your approved / to-be-reviewed summary. Thanks!

[tarekgh]

@karelz I update the top most section and also marked the issue as read for review so it will get scheduled again for the review

[karelz]

I edited the top most post: I removed the duplicate API proposal (we carried the old one), I moved implementation to the bottom (not interesting) and I linked the 1st API approval comment to keep track.

[karelz]

We assume that there is copy-paste error:
```c#
public static double operator /(TimeSpan timeSpan, TimeSpan divisor);

is the same as:
```c#
public static double operator /(TimeSpan left, TimeSpan right);

Approved: (top most post updated)
c# namespace System { public struct TimeSpan { public static TimeSpan operator *(double left, TimeSpan right) => right * left; public static double operator /(TimeSpan left, TimeSpan right); } }

[karelz]

API review: We updated the parameter names to t1/t2 to match existing operators on TimeSpan and matched the double with already approved 2 APIs.

[tarekgh]

thanks @karelz

yes it was copy and paste error and the top most approved list looks good to me.

do we agree on throwing from these APIs with following inputs?

• passing NaN
• passing out of range ticks value
• dividing by 0.0 (even if this can just work)

[karelz]

That was not part of the proposal on the top, so we didn't discuss it. What are the options, what are the pros/cons?

[JonHanna]

I think any failure to throw in those cases would be confusing at best.

If we don't throw for out of range the only reasonable response would be TimeSpan.MinValue or TimeSpan.MaxValue. I think this would hide errors. Throwing OverflowException matches e.g. TimeSpan.FromHours(long.MaxValue), so principle of least surprise suggests it.

If we don't catch NaN we'll end up throwing OverflowException if we convert checked, which wouldn't be too bad, though not the best match for the error IMO. If we convert unchecked we'll return TimeSpan.MinValue which will likely confuse something. I think just letting the OverflowException pass up is fine.

If we don't catch 0.0 we'll likewise end up throwing OverflowException. I think letting it pass up is reasonable.

A case could be made for ArgumentException for some of these cases, and DividedByZeroException for one of them, but I think OverflowException across the board is both simpler and best-matches existing methods.

[mj1856]

Maybe I'm stupid here, but why do we need to throw these exceptions ourselves? If one passes in a zero, and we divide by it, the existing division operator will already throw a DivideByZeroException.

ArgumentException I get, because then you have to return a meaningful message with the name of the argument, but I don't understand why we have to do the others.

[tarekgh]

If one passes in a zero, and we divide by it, the existing division operator will already throw a DivideByZeroException.

This is not true when using double. the division operation will succeed and you will get infinity

[JonHanna]

Maybe I'm stupid here, but why do we need to throw these exceptions ourselves?

I don't think we should, I think we should just let the overflow happen. But deciding later that we should in fact throw something else isn't quite as bad as deciding later that we should have a different signature, but it's close enough to be worth considering up front.

(Also, if not using a checked context then overflow won't happen for some of these, so a policy of passing the exceptions through still needs checked to work, rather than if just coding new TimeSpan((long)(timeSpan.Ticks * factor));)

[mj1856]

Ahh.. good old infinity. I had forgotten. Thanks.

IMHO, then we would should check 0.0, double.IsInfinity and double.IsNaN and any one of them should return an ArgumentException.

[tarekgh]

@mj1856 agree

[JonHanna]

I don't see any problem with infinity as a divisor.

[mj1856]

@JonHanna well, if we think that both of these are true:

TimeSpan.FromHours(5) / Double.PositiveInfinity == TimeSpan.Zero
TimeSpan.FromHours(5) / Double.NegativeInfinity == TimeSpan.Zero

Then I agree. Sure. I just don't know how useful that is, but ok. :)

[mj1856]

I suppose the better question is, what should TimeSpan.FromHours(5) * Double.PositiveInfinity be? TimeSpan.MaxValue? OverflowException?

[JonHanna]

I think so.

It's a real timespan (even has it's own name, eternity) just not one that TimeSpan is capable of representing, the same way it can't represent 30,000 years. It's also consistent with what TimeSpan.FromHours(Double.PositiveInfinity) does.

It also seems strange to consider reallyBigTimeSpan * 2.0 an argument exception rather than an overflow, but if that's an overflow then so should positive infinity be.

In all I think we should just let the OverflowException that will happen by trying to cast any double outside of long's range to long in a checked context pass out of the operators.

The only case I think should perhaps be caught explicitly is NaN. Casting NaN in a checked context will also give an OverflowException, but that doesn't seem quite correct. It's also inconsistent with FromHours(), FromMinutes() etc. which all throw ArgumentException with a message for which the English version is "TimeSpan does not accept floating point Not-a-Number values.". That seems reasonable for the NaN case with multiplication and division too, IMO.

[karelz]

@JonHanna you seem to be working on it, assigning to you ... please shout out if I got it wrong.

[tarekgh]

can we keep this open till we port the changes to corert? Thanks.

[karelz]

It's probably a good idea to track the work end-to-end across both runtimes which are tied together with CoreFX.
However, the fact is that we already have debt in CoreRT which we need to catch up on, and we also tell contributors to pile on the debt by not blocking CoreFX PRs on both CoreCLR and CoreRT changes first.
I think we need to change our process and ask contributors to first change both CoreCLR and CoreRT before we merge new Runtime-supplied APIs in CoreFX. Thoughts?
cc: @stephentoub @jkotas @danmosemsft

@JonHanna do you want to do also the CoreRT part? If not, I will unassign it from you ...

[danmosemsft]

As you know we're in the midst of an effort to bring the two corelibs in alignment and set up a daemon to keep them in alignment. (At least for the shareable parts). So changes like this will port themselves whether before that is complete or after it is complete.

Requiring contributors to corelib to create PR's in 3 repos is something of a deterrent to making a change.

[danmosemsft]

If it's not in the shareable part (I do'nt know the % yet) then we could make this request. Hopefully that's going to be uncommon.

[jkotas]

So changes like this will port themselves

We need to setup the automated mirroring so that it does not have to be done manually. @danmosemsft Do we have the work for it in the plan somewhere - when do we expect it to happen?

[JonHanna]

@karelz Yeah, I'll get the CoreRT bit in later today.

[jkotas]

I think we need to change our process and ask contributors to first change both CoreCLR and CoreRT

I would make it optional. I do not think that it has to be hard rule until we have the automated mirroring in place. The debt is measured in many thousands APIs since we have been pilling it for more than a year. A few more tiny APIs - that would port automatically if there was not the huge existing debt - won't make a material difference.

[karelz]

@jkotas sounds good!

[danmosemsft]

We need to setup the automated mirroring so that it does not have to be done manually. @danmosemsft Do we have the work for it in the plan somewhere - when do we expect it to happen?

I was figuring @alexperovich would do it when he's done the sync but we may need him for other things and it would happen a bit later.

[justinvp]

All of the existing operators that take a single TimeSpan name the parameter t, not timeSpan. There aren't any existing members on TimeSpan with parameters named timeSpan. Should we update the new operators to be consistent with the existing naming?

Note that the named operations (e.g. Add, Subtract, etc.), unlike the operators, all use ts rather than t.