fsharp / fslang-suggestions

The place to make suggestions, discuss and vote on F# language and core library features
345 stars 21 forks source link

Native interop for C#13 params enhancements #1377

Open psfinaki opened 1 month ago

psfinaki commented 1 month ago

I propose we better interop with the new C# params capabilities

C# 13 is enhancing params. It used to only work with arrays:

public void WriteNames(params string[] names)
    => Console.WriteLine(String.Join(", ", names));

Now it can work with whatever collection:

public void WriteNames(params ReadOnlySpan<string> names)
   => Console.WriteLine(String.Join(", ", names));

public void WriteNames(params IEnumerable<string> names)
   => Console.WriteLine(String.Join(", ", names));

C# works in the way that the most efficient overload is used on the caller side. Hence

WriteNames("Don", "Don");

will be resolved to the ReadOnlySpan overload, providing the best perf possible in this case.

I propose we allow F# do the same here - automatically pick the best overload.

The existing way of approaching this problem in F# is

Currently there is no automagic overload resolution here hence we'd need to construct the collections manually:

WriteNames("Don", "Don") // calls the array overload
WriteNames(["Don"; "Don"]) // calls the IEnumerable overload
WriteNames(ReadOnlySpan([|"Don"; "Don"|])) // calls the ReadOnlySpan overload

Not smooth.

Pros and Cons

The advantages of making this adjustment to F# are

Potential perf perks out of the box (with no code adjustments) when moving to a new lang version. Embracing the power of Span.

The disadvantages of making this adjustment to F# are

Probably not trivial IL complication.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S (for someone who understands how params interop works now) or M (for others)

Related suggestions: (put links to related suggestions here)

1267

1120

1013

Affidavit (please submit!)

Please tick these items by placing a cross in the box:

Please tick all that apply:

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

vzarytovskii commented 1 month ago

Definitely not S, this should go hand in hand or together with type directed / user defined collections.

brianrourkeboll commented 1 month ago

this should go hand in hand or together with type directed / user defined collections.

The other big part of this is OverloadResolutionPriorityAttribute.

vzarytovskii commented 1 month ago

this should go hand in hand or together with type directed / user defined collections.

The other big part of this is OverloadResolutionPriorityAttribute.

I think it's complimentary, yes. My problem with the priority attribute is that we can safely introduce support for it for tiebreakers, but I am still a bit hesitant about just generally supporting it - not 100% sure it's the greatest idea. For tie breakers, I think we'll make it for 9, for broader support, I think it should be a separate suggestion.

T-Gro commented 1 month ago

The motivation is not the same as the proposal here.

The perf benefits would come from passing stackalloc'd values for the WriteNames("Don","Don") case. The benefits would not come from method resolution itself.

Following the text in this suggestion, doing:

 WriteNames(ReadOnlySpan([|"Don"; "Don"|])) // calls the ReadOnlySpan overload 

Would not be any better, since it still allocates an array.

Based on what I have seen in the BCL so far (other usages of this C# feature can vary across libraries of course, but this is the dominant usage so far), the net benefit of this feature meeting the motivation could be achieved by: "If a parameter is marked as params and ReadOnlySpan overload exists, pass the values via stackalloc instead".

That way, this avoids the need to detect all forms user defined collections, as well as following betterness rules for overload picking.

vzarytovskii commented 1 month ago

Would not be any better, since it still allocates an array.

That's why I think it should go together with type directed collections (or universal collections syntax).

T-Gro commented 1 month ago

It is a tradeoff between supporting everything which C# can and keeping up to date with it, vs. being pragmatic and admitting that passing stackalloc'd values to ROS<T> is the real deal and the rest can be constructed by hand.

T-Gro commented 1 month ago

More practically speaking, in the priority choice between:

I see more value in the latter.

vzarytovskii commented 4 weeks ago

More practically speaking, in the priority choice between:

  • C#-matching overload resolution and supporting all collection-forms, but in the style of WriteNames(ReadOnlySpan([|"Don"; "Don"|])) as this issue suggests
  • Actually doing a stackalloc for detected params ReadOnlySpan overloads

I see more value in the latter.

If we are aiming at only treating ROS<_> overloads as special ones, and give them priority, then latter should do it, but it will have to be a very specific suggestion then. I don't think we do it currently (i.e. favour ROS overloads on anything), and it feels a bit ad-hoc-y.

I suppose there are multiple things here:

  1. Do we want to make ROS overloads to take priority (in general and in params specifically).
  2. Do we want to support any type-directed collections in the params. 2.5. If we do, how does it intersect with a compiler intrinsic collection syntax.