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 to F# local functions #5270

Closed
vasily-kirichenko opened this issue Jun 30, 2018 · 17 comments
Closed

Support byref-like parameters to F# local functions #5270

vasily-kirichenko opened this issue Jun 30, 2018 · 17 comments

Comments

@vasily-kirichenko
Copy link
Contributor

vasily-kirichenko commented Jun 30, 2018

I'm trying to convert examples from https://www.codemag.com/Article/1807051/Introducing-.NET-Core-2.1-Flagship-Types-Span-T-and-Memory-T, namely

public static int SpanSum(Span<byte> data)
{
    int sum = 0;
    for (int i = 0; i < data.Length; i++)
    {
        sum += data[i];
    }
    return sum;
}

The code

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

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

image

try to add converting to int

image

@majocha
Copy link
Contributor

majocha commented Jun 30, 2018

image
The above compiles and works.

open System

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

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

[<EntryPoint>]
let main argv =
    let span = "abcdefg"B.AsSpan()
    printfn "%d" (spanSum span)
    0 // return an integer exit code

@vasily-kirichenko
Copy link
Contributor Author

Wow. Why the difference between top level and local functions??

@majocha
Copy link
Contributor

majocha commented Jun 30, 2018

Oh, I see it now.

image

image

@TIHan
Copy link
Contributor

TIHan commented Jul 2, 2018

This is intended design I believe. Locals functions and lambdas cannot have byref-like parameters. We should probably update the error message and its range because that doesn't look too good.

@zpodlovics
Copy link

Could you please share the design considerations / decisions / technical issues behind local functions / local lambdas ? I guess it will help a bit to improve the developers mental model about F#.

@cartermp
Copy link
Contributor

cartermp commented Jul 9, 2018

Note that we document what you can use these for here: https://github.com/fsharp/fslang-design/blob/master/FSharp-4.5/FS-1053-span.md#byreflike-structs

  • Can be used as function parameters, method parameters, local variables, method returns
  • Cannot be static or instance members of a class or normal struct
  • Cannot be captured by any closure construct (async methods or lambda expressions)
  • Cannot be used as a generic parameters

And I think the key bit is here: They declare struct types that are never allocated on the heap.

That would imply that any use case where it would be heap-allocated is disallowed. Is that correct, @TIHan / @dsyme? To @zpodlovics's point, this should probably be called out more clearly in the RFC and with some negative examples showing what you cannot do.

@zpodlovics
Copy link

@cartermp I am perfectly fine with the Span documentation, in case more deep understanding needed the C# and F# testsuite (positive/negative examples) make things more clear. The whole F# team really did an amazing work here! Could you share any roadmap when we could expect Span support in .NET Core SDK ?

My question is the reasons (any developer could accept/learn that these things should coded in a specific way, but my question is why it works on that way? eg.: compiled to X for non local function and compiled to Y for local functions, .entrypoint startup documented in coreclr Z and ECMA W doc) behind the limitations of local functions / local lambdas. The workaround is trivial, it's not worth the time to dig deep in the source code - but sometimes the language creators could share lot more interesting design details.

Eg.: why this example is allowed and why not allowed when spanSum moved to a local function into the main entrypoint?

open System

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

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

[<EntryPoint>]
let main argv =
    let span = "abcdefg"B.AsSpan()
    printfn "%d" (spanSum span)
    0 // return an integer exit code

@TIHan
Copy link
Contributor

TIHan commented Jul 9, 2018

@zpodlovics This is fair. Looking at the spec, it doesn't specify the reasoning. In testing C#, I'm able to use byref-like parameters for local functions.

Perhaps we could make a feature improvement to relax these rules.

@TIHan
Copy link
Contributor

TIHan commented Jul 10, 2018

After talking with @dsyme ,

There isn't a notion of a distinction between a "local function" or lambda at the language spec; though it could in theory. There are even some small bits in later stages of the compiler that look at this distinction.

This means today, a "local function" is just like a lambda. Which, a lambda is first class and has a type of FSharpFunc<T, U>. Any byref-like type used as a type argument is not allowed, which is why it doesn't work on "local functions" today. So, getting it to work is quite tricky.

@cartermp cartermp added this to the 16.0 milestone Jul 10, 2018
@cartermp cartermp changed the title Failed to use Span<'a>.[i] anywhere Support byref-like parameters to F# local functions Jul 10, 2018
@cartermp cartermp modified the milestones: 16.0, Unknown Aug 25, 2018
@zpodlovics
Copy link

zpodlovics commented Sep 10, 2018

Before this feature improvement starts, I would like to suggest to add the indirect calls supports to this list. Probably also worth to create a proposal/design document to this. It would be good to have a somewhat relaxed but single FSharpFunc concept to support every kinds of calls (~method handles in .net):

"We use ldftn + calli in lieu of delegates (which incur an object allocation) in performance-critical pieces of our code where there is a need to call a managed method indirectly. This change allowed method bodies with a calli instruction to be eligible for inlining. Our dependency injection framework generates such methods." [1] [2] [3]

"This proposal provides language constructs that expose low level IL opcodes that cannot currently be accessed efficiently, or at all: ldftn, ldvirtftn, ldtoken and calli. These low level op codes can be important in high performance code and developers need an efficient way to access them." [2] [3]

[1] https://blogs.msdn.microsoft.com/dotnet/2018/08/20/bing-com-runs-on-net-core-2-1/
[2] dotnet/coreclr#13756
[3] https://github.com/dotnet/csharplang/blob/master/proposals/intrinsics.md

@cartermp
Copy link
Contributor

@zpodlovics can you file an issue on fsharp/language-suggestions for that? Thanks!

@cartermp
Copy link
Contributor

cartermp commented Jul 8, 2019

Linking to here, since I forgot about this suggestion: fsharp/fslang-suggestions#765

Basically, we should extend compiler analysis similar to what @dsyme mentioned:

  • If the local function or lambda only uses what is explicitly passed, it should be allowable
  • If a local function or lambda has a byref or byref-like struct passed to it, and the programmer modifies this local function or lambda to access a value that is not declared as a parameter or within the function, an error is given
  • If a local function or lambda uses a value not declared as a parameter or within the function, it is not possible to pass a byref or byref-like struct to it
  • Any first-class usage of a local function that takes a byref or byref-like struct as a parameter gives an error

@dsyme @TIHan thoughts?

@DalekBaldwin
Copy link

Just pointing out a workaround: anonymous function closures can't be written this way, but object expressions can.

[<AbstractClass>]
type Closure () =
    abstract member Execute : byref<string> -> string

let dostuff (x:string) =
    let x' = sprintf "x %s x" x
    { new Closure () with
          override __.Execute (s:byref<string>) =
              sprintf "%s %s" x' s
      }

let mutable x = "foo" in (dostuff "bar").Execute(&x)

This allows you use the result as if it were a first-class function and to close over additional variables in the surrounding context (as long as they are not themselves byref/byref-like), at the cost of writing extra boilerplate type definitions at the top level for each local function type you need to convert. You can't pass the result to client code that expects a first-class function, but it's useful for internal implementation details involving lots of precomputed nested closures. This is a case where fsharp/fslang-suggestions#277 could help.

@NatElkins
Copy link
Contributor

NatElkins commented Jun 11, 2020

Will this be semi-resolved with #9017 ? Because I think now local functions that don't close over anything will be emitted as as static class and a static member, for example as in #9299 (comment) . Please ignore if that's not right, I'm not 100% sure about that.

@TIHan
Copy link
Contributor

TIHan commented Jun 11, 2020

@NatElkins that's only for delegates, not lambdas. Local functions are lambdas in the context of F#. You are partially right though; if they don't capture anything, always emitting local functions as static members if they don't capture anything will be part of the solution. Right now, this isn't true, as lifting local functions to be top-level decls are disabled when optimizations are off.

The gist of supporting byref-like parameters for local functions is asking the question, is the local function being used in a way that isn't invocation? i.e. Such as assignment or as an argument to another function. When it's used in those ways, it will most likely have to emit a class that inherits from FSharpFunc<_, _> and byref-like types are not supported as generic type args on the CLR (core reason why this feature isn't available today). At the moment, these classes might be emitted even when a local function is used purely for invocation, especially when optimizations are off.

The majority of the work might simply be turning on optimizations to lift local functions to be top-level decls even with the compiler option --optimize-. Though, I don't know what the performance impact would be on the compiler when compiling with --optimize-.

@dsyme
Copy link
Contributor

dsyme commented Jun 17, 2020

This should be in fslang-suggestions.

@cartermp
Copy link
Contributor

Closing in favor of fsharp/fslang-suggestions#887

@cartermp cartermp removed this from the Unknown / not bug milestone Aug 27, 2020
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

8 participants