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 byref-like parameters (e.g., Span) in local functions that don't capture and are only invoked #887

Open
5 tasks done
cartermp opened this issue Jun 17, 2020 · 10 comments

Comments

@cartermp
Copy link
Member

Local function that don't capture and are only invoked should allow the use of byref-like types such as Span. It would enable code like this:

let f (arr: byte[]) =
    let spanSum (data: Span<byte>) =
        let mutable sum = 0
        for i = 0 to data.Length - 1 do
            sum <- sum + int (data.[i])
        sum
    spanSum (arr.AsSpan())

Today, this emits three errors:

error FS0412: A type instantiation involves a byref type. This is not permitted by the rules of Common IL.
error FS0412: A type instantiation involves a byref type. This is not permitted by the rules of Common IL.
error FS0425: The type of a first-class function cannot contain byrefs

Although the last error is very clear about what's going on, there's really no good reason (other than compiler analysis being hard) to disallow the above code.

The existing way of approaching this problem in F# is to break out the local function into a separate one, optionally marked as private so it doesn't participate in scope pollution:

let private spanSum (data: Span<byte>) =
    let mutable sum = 0

    for i = 0 to data.Length - 1 do
        sum <- sum + int (data.[i])
        
    sum

let f (arr: byte[]) = spanSum(arr.AsSpan())

However, an error should still be emitted under the following circumstances:

  • The function captures a value
  • The function is treated as a first-class value (assigned, passed as argument, returned)
  • The function is partially applied (there is already analysis for this)

Pros and Cons

The advantages of making this adjustment to F# are:

  • Local functions for handling details like how data gets processed is an extremely common pattern in F# code, so allowing their use when they don't capture aligns well with what people do
  • Less gotchas with byref-likes

The disadvantages of making this adjustment to F# are:

  • If the local function captures, people will still be faced with a compiler error and may not know why - a better error message would help this, though
  • Use as a first-class function will error out, which could confuse people

Extra information

Estimated cost (XS, S, M, L, XL, XXL): M

Related suggestions: #872

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
@dsyme
Copy link
Collaborator

dsyme commented Jun 22, 2020

I'm personally still not a huge fan of this feature, though I probably will end up approving it.

I like the clarity that, when using byrefs, there is a need to make your code first-order and flat, and eliminate nearly all nesting and closure capture, except where using objects to do closure capture.

That said, I can kind of see the logic that Span will intrude into F# programming more and more. How serious is the problem?

@abelbraaksma
Copy link
Member

@dsyme, several methods in the BCL take a function that take Span, like String.Create. Currently, this works if you inline the function as a lambda, but not if you pass it an existing function item. That isn't exactly the same issue (it requires conversion to a delegate which isn't possible), but I feel that more and more such callbacks will be created to hide the unsafe code, and if this issue were solved, you could use a local function.

@cartermp
Copy link
Member Author

If #765 were allowed, then this would naturally fall out of it from a compiler analysis standpoint. It was my main motivation for filing that suggestion.

@abelbraaksma
Copy link
Member

I've just been playing with string a bit, but isn't this type of analysis essentially the same as the "generic type could escape its scope" error?

That one blocks (certain) use in generic methods or types, but if the function doesn't capture anything, I think that error can go, which makes this suggestion applicable to a wider range of issues.

@cartermp
Copy link
Member Author

cartermp commented Jul 21, 2020

Just to comment on this with a real-world case - I took a little dive into what it would look like to "spannify" some of the internals of the compiler. We do a large amount of copying of various buffers, especially strings: the perfect candidate for span. Additionally, all the places where we iterate over things without copying would speed up since spans elide bounds checks at runtime. I'm sure more sophisticated mechanisms could be used, but some of these code paths are exactly what the abstraction was made for. Hooray!

Unfortunately, "spannifying" has four major problems:

  1. Many of these buffers are ultimately packaged up into other types, like tuples or options, which means that more severe rewriting would be required. We would need to use a byref-like value option in place of options. We'd need to eliminate tupling of that data where it makes sense into byref-like structs. Due to Allow IsByRefLike and IsReadOnly anonymous records #712 these could not be anonymous records, which is a shame since that would be nearly as convenient as the tupling.

  2. Many of these routines, especially when string copying is concerned, do processing with inner functions. Some of these inner functions have inner functions themselves. All of that would need to be flattened. This suggestion would significantly reduce code churn for those cases.

  3. Most of these routines that work on strings call things like .Contains or .StartsWith or .EndsWith. There are .NET overloads that take ReadOnlySpan<char> for example, but the other parameter needs to be "spanned up" before you can actually call that API since we don't offer an implicit conversion. Allow implicit conversions for F# intrinsic buffers to span for members and constructors #896 would make that better by offering a type-directed rule much like we do for nullable-typed parameters.

  4. F# lists are used everywhere and constructed in various ways. I can't see a good way to "spannify" that without completely rewriting some routines.

I don't have a good solution for (1) other than replacing many options internally with a ByRefLikeOption or whatever that's only accessible internal to the compiler. But if (2) and (3) could both be resolved, then we could probably do a lot of "spannification" without too much code churn. I think (4) can be resolved in some cases without too much churn, but in other cases the use of lists is pretty fundamental to the current design. Span couldn't easily be dropped in there.

Separately, if we were fine with introducing a dependency on System.Memory to FSharp.Core (it's already present in .NET Core ... 🙂) then we could also "spannify" some internals of FSharp.Collections and get significant perf boosts with very very little actual code change, independently of these problems.

@mciccottidmg
Copy link

Another case:

type JsonConverter<'T>
  : unit -> JsonConverter
  abstract member HandleNull:  bool
  abstract member CanConvert: typeToConvert: Type -> bool
  abstract member Read: reader: byref<Utf8JsonReader> * typeToConvert: Type * options: JsonSerializerOptions -> 'T
  abstract member ReadAsPropertyName: reader: byref<Utf8JsonReader> * typeToConvert: Type * options: JsonSerializerOptions -> 'T
  abstract member Write: writer: Utf8JsonWriter * value: 'T * options: JsonSerializerOptions -> unit
  abstract member WriteAsPropertyName: writer: Utf8JsonWriter * value: 'T * options: JsonSerializerOptions -> unit
  abstract property HandleNull:  bool

System.Text.Json.Serialization.JsonConverter has members with byref parameters and there isn't any way to do partial application with functions depending on byref.

@cartermp
Copy link
Member Author

@mciccottidmg That is actually by design and not related to this suggestion. Partial application implies boxing, which contradicts the rules for byrefs.

@mciccottidmg
Copy link

@cartermp is that still true when the byref is the last parameter of the function? Also, is there any documentation on boxing in relation to partial application?

@NinoFloris
Copy link

NinoFloris commented Aug 3, 2022

#887 (comment)

How serious is the problem?

Pretty serious, simple things like the function below can't be defined as local functions.

// Could also take span, same problem as it returns a span regardless.
let trimSpaces (str: string) =
    str.AsSpan().Trim(' ')

They have to be pulled out of context into the module as a private top level function, in this way F# is worse off than C#.

When it comes to the entire set of suggestions for improving byref use in F# (#688 being a prime example) enabling local functions involving byref looks to be - by far - the biggest value for the least amount of work and complications.

@Thorium
Copy link

Thorium commented Jun 21, 2024

If we look the code in this C# example, what would the corresponding be in F#?

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1846#how-to-fix-violations

var x = int.TryParse(iniFileLine.AsSpan(2, 5), out int y);

This won't compile (FS0193 error):

let y, x = Int32.TryParse (iniFileLine.AsSpan(2, 5))

You'd think implicit cast is not causing new string-instances, but this won't compile either (FS0412 error):

let y, x = Int32.TryParse (string(iniFileLine.AsSpan(2, 5)))

This does compile, but does the .ToString() take away all the benefits of AsSpan in the first place?

let y, x = Int32.TryParse ((iniFileLine.AsSpan(2, 5)).ToString())

Edit: This question was pointless. There is actually Int32.TryParse having ReadOnlySpan<char>, if you target only .NET Standard 2.1 (and not 2.0). However, there are other similar methods not providing ReadOnlySpan<char>, then I guess it's better to just stick with Substring.

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

6 participants