[Proposal] Simplified ldftn/ldvirtftn
sakno opened this issue · 14 comments
Hi Lucas!
I would like to propose simplified way to use ldftn and ldvirtftn instructions:
public static void Ldftn<TMethod>(TMethod method) where TMethod : Delegate;
public static void Ldvirtftn<TMethod>(TMethod method) where TMethod : Delegate;With these methods I can specify method directly without construction of MethodRef. In case of ldvirtftn it easily to analyze whether the specified method is virtual/abstract or not.
Alternative approach is to use factory method declared in MethodRef class:
public static MethodRef Create<TMethod>(TMethod method) where TMethod : Delegate;Hi!
I'd prefer the factory method approach in the MethodRef class, since it would be more consistent with the existing API.
With the signature you suggested, you'd need to always specify type parameters explicitly, like this:
MethodRef.Create<Func<int, int>>(TargetMethod);I suppose I could provide overloads for all Action<...> and Func<...> delegates, but it wouldn't really improve the situation:
MethodRef.Create<int, int>(TargetMethod);Also, the IL code quickly becomes a mess if you try to use lambdas:
MethodRef.Create<int, int>(i => TargetMethod(i));But then I guess it's the user's fault: you'll get pointers to methods of the compiler-generated class. I suppose it would be a good idea to generate an error in that case.
It would also be interesting to take a look at the new function pointers feature of C# 9, it may be helpful there:
MethodRef.Create<int, int>(&TargetMethod);The proposal says a conversion to void* is possible for function pointers, but apparently the compiler doesn't allow that. I haven't really played with this feature yet, and I guess it's not complete anyway. We'll need to wait and see when it's released. Also, it doesn't look like it works for instance methods (at least in the first version that will ship).
Given that in all cases you need to specify the full function signature, do you still think this feature would be useful?
I think yes, at least for me. The main motivation from my side - this is the only way to obtain method pointer for local methods.
Personally I'm OK with defining delegate type. If it's too verbose you can always use alias for it.
Ah, yes I see, it makes sense for local methods.
But calling local methods may be an issue. Take a look at this example: even static local methods are generated as instance methods of a compiler-generated class, as it optimizes shuffle thunks for delegate calls. This changes when you don't create a delegate from the static method.
Basically, your code would need to provide a seemingly unused parameter to the local method, it would be confusing and brittle since it would make your code depend on a compiler implementation detail. And the compiler would generate less optimal code because of the delegate on the local method.
As an aside, I agree that the first approach with the full delegate type as generic parameter is the best one, since it lets you handle methods with ref parameters for instance.
BTW function pointers don't have this problem with local methods (static local methods are still generated as static). Maybe we should just wait for C# 9?
Also, you should be directly able to use IL.Push to emit a ldftn by inserting an explicit cast. I didn't check but I think it should just work.
Unfortunately, waiting for C# 9 is not an option. I'm using InlineIL for my library .NEXT which is based on .NET Standard 2.1. I'm not planning to upgrade to .NET 5 immediately because it's not LTS version.
Moreover, I don't see any evidences that function pointers will be included in C# 9. Proposal is not assigned to any milestone. Mads in Welcome to C# 9 article doesn't mention this feature.
You don't need to migrate to .NET 5 in order to use C# 9. The language version is independent of the runtime, except for a few features like default interface implementations.
Function pointers will be in C# 9 according to the language feature status doc.
Anyway, I can implement the feature you're asking for. It could be useful to reference normal methods (and it will hopefully be future-proof), but you just need to be aware that it will lead Roslyn to generate non-optimal code for local methods, and you'll be relying on a compiler implementation detail.
In other words, it won't be great for the purpose you actually want to use it for. 😕 Function pointers would be much better, so if you still want to go this way, you should consider updating your code once C# 9 is released.
I'm just not entirely sure about the factory method name. I'll either add an overload to Method, or add a FromDelegate method or something. In any case, the signature should be the one you originally suggested:
public static MethodRef FromDelegate<TDelegate>(TDelegate @delegate) where TDelegate : DelegateIf referencing local methods and lambdas is too dangerous then InlineIL can analyze whether the target method is marked with CompilerGeneratedAttribute and produce error/warning. I can live without local methods but IMO such way to reference regular methods is less verbose.
I suppose I can add a warning for these cases, mostly to prevent unintentional misuse.
You can suppress InlineIL warnings through the weaver config.
I got it mostly implemented in the methodref-delegate branch. I just need to make sure I didn't forget anything, and maybe do some cleanup. I should release it soon.
I changed one thing though: referencing a compiler-generated method will generate an error instead of a warning. After seeing what the C# compiler generates for the various cases, I realized there's really no good way to use such a method in a sane way.
I've released v1.5.0 which adds MethodRef.FromDelegate. Let me know if you find any issues.
Oh, I forgot: there's one more thing you need to be careful about: if you try to get a reference to a virtual method override, you'll get a reference to the base method instead (it's the one that Roslyn references).
This may especially be an issue for GetHashCode and other methods of System.Object, particularly for structs.
Thanks, Lucas!