-
Notifications
You must be signed in to change notification settings - Fork 783
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
Don't emit unnecessary closures #11898
Comments
I agree we should fix this |
But you need the closure to create an |
@kerams. No, closure isn't required, not for static methods nor for instance methods. They're only needed when you're creating anonymous function with captured parameters. Delegates are just classes that inherit from Creating delegate requires several instructions:
Generating closure in this case is unnecessary. Instead of loading If it were impossible to create action without closure compiler would've enter infinite loop of creating closure for closure for closure... |
Well, I've learned something new, thanks. |
I'd like to make progress on this issue, it might need an RFC. There are several separate proposals lurking here and I'd like to tease them apart. The basic situation is System.Action<_,_>(expr) The cases we care about are where the target is a "known" funciton or method: System.Action<_,_>(handler) // non-eta-expanded known target
System.Action<_,_>(fun a b -> handler a b) // eta-expanded known target
System.Action<_,_>(fun a b -> handler (a,b)) // eta-expanded known target with different currying/tupling but same compiled representation
System.Action<_,_>(SomeType.StaticMethod) // same with a static method
System.Action<_,_>(fun a b -> SomeType.StaticMethod a b) // same with a static method
System.Action<_,_>(fun a b -> SomeType.StaticMethod(a,b)) // same with a static method
System.Action<_,_>(SomeType<...>.StaticMethod<...>) // same with a generic static method
System.Action<_,_>(fun a b -> SomeType<...>.StaticMethod<...> a b) // same with a generic static method
System.Action<_,_>(fun a b -> SomeType<...>.StaticMethod<...>(a,b)) // same with a generic static method
System.Action<_,_>(someObject.InstanceMethod) // same with an instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod a b) // same with an instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod(a, b)) // same with an instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod<...> a b) // same with a generic instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod<...>(a, b)) // same with a generic instance method Arguably we also care about partial applications: System.Action<_,_>(handler arg1 arg2) // non-eta-expanded known target via partial application The queestions are
We have to consider these questions for both Debug and Release code. The current situation and proposal is:
This is in theory a potential breaking change because user code doing inspection on delegate value .TargetMethod may detect the difference. However within the context of the language spec it's a reasonable change as the TargetMethod is currently a closure. For this reason it is reasonable to make this change via an RFC and |
I've written a language suggestion for this here: fsharp/fslang-suggestions#1083 |
Compiler generates method closure when that's unneeded.
Repro steps
Compile and run code
Expected behavior
x y
is written to consoleActual behavior
delegateArg0 delegateArg1
is written.Compiler wraps method with static class and calls
Invoke
method from it. Looks like someone has optimised this scenario, but it would be better if compiler wont generate static class at all. Current behavior increases assembly size, causes unnecessary call and most important - mangles names, which causes bugsKnown workarounds
Use anonymous function with explicit argument names
Related information
Windows 10, net5
.NET SDK (reflecting any global.json): Version: 5.0.302 Commit: c005824e35Runtime Environment:
OS Name: Windows
OS Version: 10.0.19043
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\5.0.302\
Host (useful for support):
Version: 5.0.8
Commit: 35964c9215
.NET SDKs installed:
3.1.411 [C:\Program Files\dotnet\sdk]
5.0.301 [C:\Program Files\dotnet\sdk]
5.0.302 [C:\Program Files\dotnet\sdk]
.NET runtimes installed:
Microsoft.AspNetCore.App 3.1.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 3.1.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.1.16 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
The text was updated successfully, but these errors were encountered: