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

Support for ref fields #1143

Open
4 of 5 tasks
jaredpar opened this issue May 10, 2022 · 3 comments
Open
4 of 5 tasks

Support for ref fields #1143

jaredpar opened this issue May 10, 2022 · 3 comments

Comments

@jaredpar
Copy link

jaredpar commented May 10, 2022

I propose that F# add support for consuming ref fields.

The .NET 7 runtime is adding support for defining ref fields inside ByRefLike structs and the .NET SDK will expose them via new constructors on Span<T> and ReadOnlySpan<T>. Specifically the types will now have effectively the following members:

[<IsByRefLike; Struct>]
type Span<'T>(field: byref<'T>) =

[<IsByRefLike; Struct>]
type ReadOnlySpan<'T>(field: inref<'T>) =

These APIs will create a Span<T> and ReadOnlySpan<T> respectively that contain the byref<_> argument as a ref field.

let mutable local = 42
let span = Span<int>(&local)
span[0] <- 13
printf "%d" local // prints 13

The issue here is that the F# rules for ref-like safety do not account for this type of API. It does not consider that the return of the Span<T> constructor can contain a byref<_> to local.
As such it will allow code to unsafely return Span<T> that refer to locals from a method:

let example (): Span<int> =
    let mutable local = 42
    let span = Span<int>(&local)
    span // refers to &local and hence is unsafe

The F# ref-like safety rules are touched on here but those don't have the detailed rules around ref safety. I've worked them out, to the best of my ability and with help from @TIHan, by going through the CheckCallLimitArgs function in PostInferenceChecks. The rules relevant to this dicussion are:

  1. A byref<_> return from a function is not returnable if any argument is ...
    1. A byref<_> which is not returnable (typically referring to a local)
    2. A ByRefLike struct which is potentially stack referring
  2. A ByRefLike struct return is not returnable if any argument is
    1. A ByRefLike struct which is potentially stack referring
    2. A byref<_> of a ByRefLike struct which is potentially stack referring

Note: when byref<_> is used above it's meant to encompass both byref<_>, inref<_> and outref<_>.

To support the new APIs the rules should be adjusted to treat byref<_> and ByRefLike structs equally with respect to returns. They are now interchangable in terms of ref safety. Below byref-like will be shorthand for byref<_> or ByRefLike structs. As such the rules should be adjusted to:

  1. A byref-like return from a function is not returnable if any argument is a byref-like which is not returnable.
    1. The receiver of a struct instance method shall not be considered in the above calculations. It is not considered returnable via byref<_>

The F# implementation differentiates between ByRefLike structs and span-like types which potentially refer to the stack. I believe this distinction can be removed once this is implemented as there isn't a meaningful distinction anymore.

Pros and Cons

The advantage of making this adjustment is it keeps F# memory safe in the face of new APIs that are coming online in .NET 7. Given the prominance of Span<T> and ReadOnlySpan<T> in high performance scenarios it's important that F# customers continue to be able to use them safely. These rule changes will achieve that.

Con1: Breaking change

This suggestion represents a breaking change for APIs that return ByRefLike structs and take byref<_> arguments today. Previously the scope of the returned value had no relationship to the byref<_> argument and that allowed for the following pattern:

let createSpan (x: byref<int>): Span<int> = 
    let array: int array = [| |]
    array.AsSpan()

let example (): Span<int> =
    let mutable local = 42
    let span = createSpan &local
    span

Going forward the return of span above would be an error as it would be scoped to the method example, just as a ref local would be today in a similar pattern

C# has the same issue and as part of their due dillegence on ref fields wrote a tool to spot this API pattern. They then ran it on several repositories that have high usage of byref<_> and ByRefLike structs. In total they found two APIs that were impacted and hence they consider this a very small risk and accpeted the breaking change.

Con2: Not full ref field support

The ref field support being added to C# is more extensive than what is being proposed here. Specifically it allows for:

  1. Declaration of ref fields
  2. The ability to mark byref<_> or ByRefLike structs as scoped / non returning. This provides more flexbility to APIs that deal heavily in ref-like types as the compiler understands which ones can and cannot be returned.

This proposal did not include them because it's unclear if it would meet the bar for F# inclusion. They are only beneficial to code bases that deal heavily in APIs with ref-like types and want to manipulate stack based values. If the F# team believes that is beneficial to customers then the proposal can be extended to include those scenarios.

In chatting with @TIHan he believes (2) would be reasonable to layer onto the current implementation of ref safety in the compiler. It would be a small to medium level addition to the code base.

The expectation is (1) would be more costly but likely still in the medium range.

Con3: Lack of F# code to validate change against

When making this change to C# there was a huge amount of Span<T> based code in dotnet/runtime I could model the rules against to see the impact. I am not aware of any F# code bases that make heavy use of Span<T> that I could do similar exercises with. If readers could point a few out to me I'd be appreciative as I'd feel better if I could look at real code to evaluate the rules.

Extra information

The estimated cost of this is medium to small. The change has the appearance of being straight forward in the code but I have a strong reluctance to mark any compiler change as small hence I'd error on calling it medium.

Related suggestions:

Affidavit (please submit!)

Please tick this 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
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • 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.

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.

Thanks to @TIHan for helping me understand the existing rules and code!

@cartermp
Copy link
Member

I'll defer to @dsyme on final judgement here, but I absolutely think we should support consuming them. Declaring them would also be good, but at least as a first pass this seems reasonable. F# being able to access the high-perf .NET stuff is a differentiator from the likes of Python.

I would also support implementing the full feature, but just covering consumption interop feels fine too, as a first pass.

@jaredpar
Copy link
Author

I'm happy to help spec out support for implementing the full feature if it's a plausible addition to the language.

One part I thought about after this proposal is whether F# should reconsider special casing stackalloc. At the moment it's not special cased and is essentially untracked.

Supporting consumption of ref fields though means F# will need to add rules for ByRefLike structs that potentially refer to the stack. The easiest example being:

let mutable local: int = 42
let span = Span<int>(&local)

Once the rules to make this safe are in place it's pretty trivial to extend them to include expressions involving stackalloc. There is additional work to get stackalloc to convert to Span<_>. My intuition is that would be doable.

@dsyme
Copy link
Collaborator

dsyme commented May 16, 2022

Labelled as approved in principle

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

3 participants