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

The new TimeSpan FromMilliseconds overload breaks System.Linq.Expressions #109833

Open
Tragetaschen opened this issue Nov 14, 2024 · 8 comments
Open

Comments

@Tragetaschen
Copy link
Contributor

Tragetaschen commented Nov 14, 2024

Description

I have seen the breaking change notice with regards to F#, but migrating to .NET 9 has revealed a couple of places in our C# projects, where constructing a TimeSpan is part of a System.Linq.Expressions.Expression. These now don't compile with .NET 9

Reproduction Steps

using System.Linq.Expressions;
Expression<Action> a = () => TimeSpan.FromMilliseconds(1000); // compiles with .NET 8, fails with .NET 9

Expected behavior

Code continues to compile

Actual behavior

Code breaks in .NET 9

Regression?

yes

Known Workarounds

You need to force the double overload

Expression<Action> a = () => TimeSpan.FromMilliseconds(1000.0);

or provide the optional parameter

Expression<Action> a = () => TimeSpan.FromMilliseconds(1000, 0); 

Configuration

No response

Other information

The reason is that the optional parameters are not supported in expressions.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 14, 2024
Copy link
Contributor

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

@Tragetaschen
Copy link
Contributor Author

Introduced with #93890

Copy link
Contributor

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

Copy link
Contributor

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

@julealgon
Copy link

I was under the impression that exposing methods with default parameters in public APIs was frowned upon, and that it was best to just have multiple overloads instead.

Does anybody know if that's not really the case, or if it still applies, why these new APIs for TimeSpan didn't follow that practice?

@Tragetaschen
Copy link
Contributor Author

Tragetaschen commented Nov 14, 2024

The approved API said

The methods should be overloaded to have no default parameters per our normal default parameters usability guideline

But the actual API missed the milliseconds overload and consequently didn't implement it in #98633.

@tarekgh tarekgh added this to the 10.0.0 milestone Nov 14, 2024
@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Nov 14, 2024
Copy link
Contributor

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

@Tragetaschen Tragetaschen changed the title The new TimeSpan From* overloads break System.Linq.Expressions The new TimeSpan FromMilliseconds overload break System.Linq.Expressions Nov 14, 2024
@tarekgh
Copy link
Member

tarekgh commented Nov 14, 2024

Just to mention the other workaround can be providing the optional parameter:

Expression<Action> a = () => TimeSpan.FromMilliseconds(1000, 0); 

Thanks @Tragetaschen for reporting the issue.

@Tragetaschen Tragetaschen changed the title The new TimeSpan FromMilliseconds overload break System.Linq.Expressions The new TimeSpan FromMilliseconds overload breaks System.Linq.Expressions Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants