[Breaking change]: New TimeSpan.From overloads which take integers

rstm-sf commented 1 week ago

Description

https://github.com/dotnet/runtime/pull/98633

Version

.NET 9 RC 1

Previous behavior

Before .NET 9 there was a version with one float parameter

New behavior

Now the following F# code throws an overload error: TimeSpan.FromMinutes(20)

error FS0041: A unique overload for method 'FromMinutes' could not be determined based on type information prior to this program point. A type annotation may be needed.Known type of argument: intCandidates: - TimeSpan.FromMinutes(minutes: int64) : TimeSpan - TimeSpan.FromMinutes(minutes: int64, ?seconds: int64, ?milliseconds: int64, ?microseconds: int64) : TimeSpan - TimeSpan.FromMinutes(value: float) : TimeSpan

Type of breaking change

[ ] Binary incompatible: Existing binaries might encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
[X] Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code might require source changes to compile successfully.
[ ] Behavioral change: Existing binaries might behave differently at run time.

Reason for change

https://github.com/dotnet/runtime/issues/93890

Recommended action

Specify the compiler what type of argument is being used so that the appropriate overload is selected

Feature area

Core .NET libraries

Affected APIs

No response

Associated WorkItem - 314593

rstm-sf commented 1 week ago

Hello, @bartonjs

When a new API is designed, does it take into account that .NET <> C#?

https://github.com/dotnet/runtime/issues/93890#issuecomment-1777761585

terrajobst commented 5 days ago

@rstm-sf

When a new API is designed, does it take into account that .NET <> C#?

We generally do consider other languages when designing APIs but we don't always remember all the specifics under which a change can become a source breaking change. As soon as overloads are concerned you can almost always find examples where existing code no longer compiles due to ambiguities, which isn't just the case for F#. In order to prevent this class of problems you're describing here we'd have to have a rule that says "don't create overloads between numeric types when previously no such overload existed". I don't think we want to sign up for that because it basically forces us to either not add features or invent new method groups. Neither of which seems appealing to me.

For this particular API: I don't recall why we added the ones taking a single long. Initially I thought it was as a tie breaker to prevent FromHours(12) to be ambiguous between the existing one and the new one with defaults. Looks like we just did that to follow our standard rules of having simple overload without the defaults, not realizing that we already had one from before.

@tannergooding @bartonjs: ignoring the RC bug bar for a moment, would it be possible to just remove these methods?

 namespace System;

 public partial struct TimeSpan
 {
     // Exists prior to .NET 9
     public static TimeSpan FromDays(double days);
     public static TimeSpan FromHours(double hours);
     public static TimeSpan FromMinutes(double minutes);
     public static TimeSpan FromSeconds(double seconds);
     public static TimeSpan FromMicroseconds(double microseconds);

     // New in .NET 9
-    public static TimeSpan FromDays(int days);
     public static TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long  microseconds = 0);
-    public static TimeSpan FromHours(int hours);
     public static TimeSpan FromHours(int hours, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0);
-    public static TimeSpan FromMinutes(long minutes);
     public static TimeSpan FromMinutes(long minutes, long seconds = 0, long milliseconds = 0, long microseconds = 0);
-    public static TimeSpan FromSeconds(long seconds);
     public static TimeSpan FromSeconds(long seconds, long milliseconds = 0, long microseconds = 0);
-    public static TimeSpan FromMicroseconds(long microseconds);
     public static TimeSpan FromMilliseconds(long milliseconds, long microseconds = 0);
 }

tannergooding commented 5 days ago

Those were for perf and convenience.

I believe we still have languages (C++/CLI) that have ambiguities simply from the new overloads with new optional parameters, so it wouldn't "fix" things even if the ones you proposed were removed

I had thought that F# was getting some new features to help with cases like this (but am oof this week and don't have the link handy)

bartonjs commented 5 days ago

I'm not a cconv expert, so I don't know if FromDays(int) and FromDays(int,int,long,long,long,long) have a different impact on the register map. (The second one feels like it has more parameters than the normal callconv, so it would force extra register preservation or even to go into "too many parameters, have to pass things on the stack" territory).

From a C# perspective: I just checked, and if we removed FromDays(int) then FromDays(3) will prefer the int-first-parameter ("the long one") over interpreting the literal 3 as a double.

So if there is a register-map concern with "the long one", it's a question of if it's worse to fire up the FPU (is that even a thing anymore?) to process 3.0d or do an unnecessarily verbose call to stay in integer land.

ignoring the RC bug bar for a moment

FWIW, if I recall correctly, we just (like 5 minutes before your comment) switched from the RC bug bar to the RTM bug bar (any changes now would be changes between RC2 and RTM)... so "un-ignoring" that is slightly more costly than you were considering :smile:

terrajobst commented 5 days ago

FWIW, if I recall correctly, we just (like 5 minutes before your comment) switched from the RC bug bar to the RTM bug bar (any changes now would be changes between RC2 and RTM)... so "un-ignoring" that is slightly more costly than you were considering 😄

I was mostly concerned with "is this even feasible", mostly for my own education. If it's not, I'm not even attempting to see which approvals we'd need :-)

Sounds like "yes we could remove them but we probably don't want to due to perf and even if we did, it won't fix all possible source breaking concerns". Plus, the bar is pretty high now. Therefore I'm going to close as won't fix.

bartonjs commented 5 days ago

Therefore I'm going to close as won't fix.

Since this got opened in dotnet/docs, I think the filer just wants us to doc it as a source breaking change for F#, which seems valid...

terrajobst commented 5 days ago

@gewarren what tags do we need for documenting a known source breaking change?

gewarren commented 4 days ago

@gewarren what tags do we need for documenting a known source breaking change?

This looks good. I'll write up a doc next week.

dotnet / docs