AddSeconds and FromSeconds rounding issues

[mattjohnsonpint]

Description

• DateTimeOffset.AddSeconds(double) and DateTime.AddSeconds(double) both round the input value to the nearest millisecond, despite both structures being able to support precision to 7 decimal places. This is mentioned in the remarks section of the docs, but is surprising non-obvious behavior, IMHO.
• TimeSpan.FromSeconds(double) also rounds the input to the nearest millisecond, and again is mentioned in the docs and is surprising, IMHO. However, it only does so on .NET Framework and .NET Core 2.1 and lower. From .Net Core 3.0 and higher (through to 6.0 latest), it does not round.
• The only way to add a double of seconds without losing precision is to convert to ticks and add ticks instead.

Reproduction Steps

[Fact]
public void TestDateTimeOffsetAddSeconds()
{
    var seconds = 0.9999999;
    var ticks = DateTimeOffset.MinValue.AddSeconds(seconds).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeOffsetAddTimeSpanFromSeconds()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromSeconds(seconds);
    var ticks = DateTimeOffset.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeOffsetAddTimeSpanFromTicks()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromTicks((long)(seconds * TimeSpan.TicksPerSecond));
    var ticks = DateTimeOffset.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeOffsetAddTicks()
{
    var seconds = 0.9999999;
    var ticks = DateTimeOffset.MinValue.AddTicks((long)(seconds * TimeSpan.TicksPerSecond)).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddSeconds()
{
    var seconds = 0.9999999;
    var ticks = DateTime.MinValue.AddSeconds(seconds).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddTimeSpanFromSeconds()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromSeconds(seconds);
    var ticks = DateTime.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddTimeSpanFromTicks()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromTicks((long)(seconds * TimeSpan.TicksPerSecond));
    var ticks = DateTime.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddTicks()
{
    var seconds = 0.9999999;
    var ticks = DateTime.MinValue.AddTicks((long)(seconds * TimeSpan.TicksPerSecond)).Ticks;
    Assert.Equal(9999999, ticks);
}

Expected behavior

Preferably, all tests should pass. Rounding should not occur in any scenario. However, at minimum, the behavior should be consistent across all supported implementations.

Actual behavior

• The AddTimeSpanFromSeconds tests fail on .NET Framework (all versions) and .NET Core 2.1 and earlier, but pass on .NET Core 3.0 and higher (through 6.0 latest).
• The AddSeconds tests fail on all versions.
• The other two tests pass on all versions.

Regression?

?

Known Workarounds

Compute ticks and add ticks, or create a timespan from those ticks.

Configuration

Tested on Windows x64 on .NET Core 2.1, 3.0, 3.1, .NET 5.0, 6.0 and .NET Framework 4.6.1, 4.6.2, 4.7.1, 4.7.2, and 4.8.
Also tested on macOS arm64 (Apple Silicon) on .NET 5.0 and 6.0, and .NET Core 3.1 with macOS x64 via Rosetta.
(I don't believe this is platform specific.)

Other information

No response

[mattjohnsonpint]

@tarekgh

[ericsampson]

It might be somewhat worse than the title/issue describe Matt...

It's not just FromSeconds, I think it might also be present in FromDays/Hours/Milliseconds/Minutes.
but I haven't looked at the source or written a test to verify ;)

Also, like you said, the behavior of all the DateTimeOffset.AddXXX() non-tick methods is surprising, if we could go back in time I wish it would be exactly equivalent to calling DateTimeOffset.Add(TimeSpan.FromXXX())

[Clockwork-Muse]

if we could go back in time I wish it would be

If we could go back in time, I want something closer to JSR310/NodaTime.

[huoyaoyuan]

I'm pretty sure there's a dupe, but can't find it now.
Obviously, it's impacted by the FP precision change introduced in 3.0.

[ghost]

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

Issue Details

---

Description

• DateTimeOffset.AddSeconds(double) and DateTime.AddSeconds(double) both round the input value to the nearest millisecond, despite both structures being able to support precision to 7 decimal places. This is mentioned in the remarks section of the docs, but is surprising non-obvious behavior, IMHO.
• TimeSpan.FromSeconds(double) also rounds the input to the nearest millisecond, and again is mentioned in the docs and is surprising, IMHO. However, it only does so on .NET Framework and .NET Core 2.1 and lower. From .Net Core 3.0 and higher (through to 6.0 latest), it does not round.
• The only way to add a double of seconds without losing precision is to convert to ticks and add ticks instead.

Reproduction Steps

[Fact]
public void TestDateTimeOffsetAddSeconds()
{
    var seconds = 0.9999999;
    var ticks = DateTimeOffset.MinValue.AddSeconds(seconds).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeOffsetAddTimeSpanFromSeconds()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromSeconds(seconds);
    var ticks = DateTimeOffset.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeOffsetAddTimeSpanFromTicks()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromTicks((long)(seconds * TimeSpan.TicksPerSecond));
    var ticks = DateTimeOffset.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeOffsetAddTicks()
{
    var seconds = 0.9999999;
    var ticks = DateTimeOffset.MinValue.AddTicks((long)(seconds * TimeSpan.TicksPerSecond)).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddSeconds()
{
    var seconds = 0.9999999;
    var ticks = DateTime.MinValue.AddSeconds(seconds).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddTimeSpanFromSeconds()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromSeconds(seconds);
    var ticks = DateTime.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddTimeSpanFromTicks()
{
    var seconds = 0.9999999;
    var timeSpan = TimeSpan.FromTicks((long)(seconds * TimeSpan.TicksPerSecond));
    var ticks = DateTime.MinValue.Add(timeSpan).Ticks;
    Assert.Equal(9999999, ticks);
}

[Fact]
public void TestDateTimeAddTicks()
{
    var seconds = 0.9999999;
    var ticks = DateTime.MinValue.AddTicks((long)(seconds * TimeSpan.TicksPerSecond)).Ticks;
    Assert.Equal(9999999, ticks);
}

Expected behavior

Preferably, all tests should pass. Rounding should not occur in any scenario. However, at minimum, the behavior should be consistent across all supported implementations.

Actual behavior

• The AddTimeSpanFromSeconds tests fail on .NET Framework (all versions) and .NET Core 2.1 and earlier, but pass on .NET Core 3.0 and higher (through 6.0 latest).
• The AddSeconds tests fail on all versions.
• The other two tests pass on all versions.

Regression?

?

Known Workarounds

Compute ticks and add ticks, or create a timespan from those ticks.

Configuration

Tested on Windows x64 on .NET Core 2.1, 3.0, 3.1, .NET 5.0, 6.0 and .NET Framework 4.6.1, 4.6.2, 4.7.1, 4.7.2, and 4.8.
Also tested on macOS arm64 (Apple Silicon) on .NET 5.0 and 6.0, and .NET Core 3.1 with macOS x64 via Rosetta.
(I don't believe this is platform specific.)

Other information

No response

Author:	mattjohnsonpint
Assignees:	-
Labels:	area-System.Runtime
Milestone:	-

[mattjohnsonpint]

I think there's a policy issue here around breaking behavior changes that needs clarification. It would seem that the original implementation rounded to milliseconds (though I can't fathom why), but that behavior was accidentally broken for TimeSpan.FromSeconds(double) in .NET Core 3.0. But it's been many releases now, so changing back to rounding would also be a breaking behavior - especially since the newer non-rounding way aligns more to expectations. Is there precedent for what to do in such situations?

[mattjohnsonpint]

FWIW - this is not critical for us. I just thought it was important to report it.

[tarekgh]

Please look at #23771 (comment). We had an app compatibility issue when we tried to fix that. That is why we had the proposal #23799 to give better control.

@mattjohnsonpint we needed feedback on it #23799 (comment) :-)

I suggest closing this issue and focus more on the new proposal.

[ericsampson]

that doesn't make sense to me Tarek.

At the very least, the documentation needs to be updated so that it's correct for .NET Core 3.0+

[tarekgh]

that doesn't make sense to me Tarek.

What exactly doesn't make sense?

At the very least, the documentation needs to be updated so that it's correct for .NET Core 3.0+

Do you suggest documenting the rounding behavior? I am fine updating the documentation as needed. We can log a doc issue or even if you prefer submitting PR, you are welcome to do so.

[GSPP]

I was surprised to learn this. Went to review my code and found some places where this would definitely be undesirable, bordering on being a bug. I advocate for making this change.

[ericsampson]

@mattjohnsonpint would it be crazy pants to propose removing all rounding from these, as part of a breaking change? At least there would be consistency, rather than the current mix of behavior (and also difference in behavior from the docs, which has been in .NET Core for too long to revert)

DateTimeOffset.AddXXX
TimeSpan.FromXXX

Did you by chance check if it was just the behavior of FromSeconds that changed in .NET Core 3.0, or was it all the FromDays/Hours/Milliseconds/Minutes methods?

[tarekgh]

I believe the change done during 3.1 was 2503500. So, it is scoped to TimeSpan.FromXXX methods.

@mattjohnsonpint, the only complaint in this issue is AddSeconds methods in DateTimeOffset and DateTime? do you have any issues with other methods?

[mattjohnsonpint]

I tested some more. For clarity, I will restate my concerns:

• The behavior of TimeSpan.FromDays, TimeSpan.FromHours, TimeSpan.FromMinutes, and TimeSpan.FromSeconds changed starting with .NET Core 3.0.

  • They all used to round the input to the nearest millisecond, and now do not.
  • The API docs for each still remark on the old behavior and do not mention the change in behavior.
  • This change is good IMHO, but the docs need updating.

• The behavior of DateTime.AddDays, DateTime.AddHours, DateTime.AddMinutes, DateTime.AddSeconds, DateTimeOffset.AddDays, DateTimeOffset.AddHours, DateTimeOffset.AddMinutes, and DateTimeOffset.AddSeconds was not changed to coincide with the TimeSpan behavior change.

  • They all still round the input to the nearest millisecond, which is now inconsistent with the TimeSpan API.
  • That is surprising and unobvious, IMHO. For example, while refactoring or reviewing a PR, a developer might decide to simplify dateTime + TimeSpan.FromSeconds(d) as dateTime.AddSeconds(d), without realizing that they are introducing a bug if d could have finer precision than milliseconds. (I got caught on this, and I thought I knew these APIs better than most.)
  • They also do not align with the behavior of TimeOnly.AddHours and TimeOnly.AddMinutes, which does not round.

My recommendation / ask is as follows:

• Leave the TimeSpan methods as they currently are.
  • Update the API docs to describe the change of behavior starting with .NET Core 3.0.
• Change the behavior of the DateTime and DateTimeOffset methods to match that of TimeSpan and TimeOnly. The input parameter should not be rounded.
  • Update the API docs to describe the change of behavior starting with whichever versions this will be released in.
  • This would also align with the new APIs proposed in Add Microseconds and Nanoseconds to TimeStamp, DateTime, DateTimeOffset, and TimeOnly #23799.

I recognize that the change of behavior could be considered breaking, but we have already precedent for it with the TimeSpan methods, and the inconsistency is a footgun.

[ericsampson]

I 100% agree with the reasoning and plan that @mattjohnsonpint laid out. Thank you Matt <3