Open jl0pd opened 3 years ago
I agree we should fix this
it would be better if compiler wont generate static class at all
But you need the closure to create an Action
instance, don't you?
@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 System.MulticastDelegate
. Each delegate have public constructor with object
and IntPtr
args. For example constructor of Action<T1, T2>
have signature public extern Action(object @object, IntPtr method)
. If method is static - @object
is null, otherwise @object
should be instance of some class. IntPtr
contains pointer to method.
Creating delegate requires several instructions:
ldnull // load null because method is static
ldftn void className::methodName(argType1, argType2)
newobj instance void class System.Action`2<argType1, argType2>::.ctor(object, native int)
Generating closure in this case is unnecessary. Instead of loading Program::problematicMethod(string, int)
directly, compiler generates new class and calls Program/problematicMethod@9::Invoke(string, int)
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:
Direct delegate?
Closure argument names drawn from direct target?
Are these changes visible in quotations?
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 /langversion
I've written a language suggestion for this here: https://github.com/fsharp/fslang-suggestions/issues/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: c005824e35 Runtime 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]