Open jaredpar opened 2 years ago
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.
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.
Labelled as approved in principle
I propose that F# add support for consuming
ref
fields.The .NET 7 runtime is adding support for defining
ref
fields insideByRefLike
structs and the .NET SDK will expose them via new constructors onSpan<T>
andReadOnlySpan<T>
. Specifically the types will now have effectively the following members:These APIs will create a
Span<T>
andReadOnlySpan<T>
respectively that contain thebyref<_>
argument as aref
field.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 abyref<_>
tolocal
. As such it will allow code to unsafely returnSpan<T>
that refer to locals from a method: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:byref<_>
return from a function is not returnable if any argument is ...byref<_>
which is not returnable (typically referring to a local)ByRefLike
struct which is potentially stack referringByRefLike
struct return is not returnable if any argument isByRefLike
struct which is potentially stack referringbyref<_>
of aByRefLike
struct which is potentially stack referringNote: when
byref<_>
is used above it's meant to encompass bothbyref<_>
,inref<_>
andoutref<_>
.To support the new APIs the rules should be adjusted to treat
byref<_>
andByRefLike
structs equally with respect to returns. They are now interchangable in terms of ref safety. Belowbyref-like
will be shorthand forbyref<_>
orByRefLike
structs. As such the rules should be adjusted to:byref-like
return from a function is not returnable if any argument is abyref-like
which is not returnable.struct
instance method shall not be considered in the above calculations. It is not considered returnable viabyref<_>
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>
andReadOnlySpan<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 takebyref<_>
arguments today. Previously the scope of the returned value had no relationship to thebyref<_>
argument and that allowed for the following pattern:Going forward the return of
span
above would be an error as it would be scoped to the methodexample
, just as aref
local would be today in a similar patternC# 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 ofbyref<_>
andByRefLike
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:ref
fieldsbyref<_>
orByRefLike
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 ofSpan<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:
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.
Thanks to @tihan for helping me understand the existing rules and code!