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

Native interop for C#13 params enhancements #1377

Open
5 of 6 tasks
psfinaki opened this issue Aug 8, 2024 · 8 comments
Open
5 of 6 tasks

Native interop for C#13 params enhancements #1377

psfinaki opened this issue Aug 8, 2024 · 8 comments

Comments

@psfinaki
Copy link

psfinaki commented Aug 8, 2024

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:

  • This is not a question (e.g. like one you might ask on StackOverflow) and I have searched StackOverflow for discussions of this issue
  • This is a language change and not purely a tooling change (e.g. compiler bug, editor support, warning/error messages, new warning, non-breaking optimisation) belonging to the compiler and tooling repository
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

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

@vzarytovskii
Copy link

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

@brianrourkeboll
Copy link

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

The other big part of this is OverloadResolutionPriorityAttribute.

@vzarytovskii
Copy link

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
Copy link

T-Gro commented Aug 15, 2024

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
Copy link

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
Copy link

T-Gro commented Aug 15, 2024

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
Copy link

T-Gro commented Aug 15, 2024

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.

@vzarytovskii
Copy link

vzarytovskii commented Aug 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants