Roslyn: Use a cached delegate for method group conversion

[gafter]

The method group conversion today always creates a fresh delegate instance.

Instead, for a method group conversion from a static method we should cache it and only create it once, like we do for non-capturing lambda expressions.

[The C# language spec is being modified to permit this]

[stephentoub]

WOO HOO!!!

[danieljohnson2]

Isn't this a breaking change? I don't think I've ever written code that cares about delegate identity (as opposed to equality), but if it anybody has, this could break it.

Also, it used to be that you got crazy compiler magic if you used the delegate { ... } or foo => bar syntaxes, and heaven knows what code gets generated then (caching the delegate is the least of it) But the magic happens only if you ask for it.

Now, the rule is to be that the crazy compiler magic happens unless you say new. Admittedly, the 'magic' here is really just injecting a small, bounded memory leak, but I think it's still surprising. I was surprised when I found out that => does this sometimes, and I knew about Expression<T> already, which is far crazier.

I guess this is mostly harmless, so long as you do not also break the equality semantics of the delegates produced, and so long as you can turn this off with the new EventHandler(...) syntax.

[mburbea]

Yes please. Can we do something about delegates in generic methods? Currently they are still a much slower caching (if they get cached at all) strategy then for delegates created in non-generic methods

[Lukazoid]

:+1: for this, also as per the comment by @mburbea is there any interest in caching for delegates in generic methods? Is it worth creating a separate issue for it.

[stephentoub]

is there any interest in caching for delegates in generic methods? Is it worth creating a separate issue for it.

Roslyn already does this. For example, with this code:

``` C#
using System;

public class Test
{
public static void Main() { GenericMethod(); }

private static void GenericMethod<T>()
{
    InvokeAction(() => Console.WriteLine(typeof(T)));
}

private static void InvokeAction(Action action)
{
    action();
}

}

the code generated for `GenericMethod<T>` looks like this:

private static void GenericMethod()
{
InvokeAction(
<>c__1.<>9__1_0 ??
(<>c__1.<>9__1_0 = new Action(<>c__1.<>9.b__1_0)));
}

private sealed class <>c__1
{
public static readonly Test.<>c__1 <>9;
public static Action <>9__1_0;
internal void b__1_0();
...
}
```

[Lukazoid]

Oh sweet, I did verify the IL but forgot I wasn't using Roslyn yet at work!

[sharwell]

So what you're saying is the compiler should handle the insanity that was DotNetAnalyzers/StyleCopAnalyzers#1689?

I'm totally in agreement there. :+1:

[mattwar]

Yes, do this. 👍

[jcouv]

We were just discussing this with @CyrusNajmabadi and @cston. We were surprised by the current behavior, as calling with a static method seems like it should not allocate on every invocation.
There is a workaround (pass () => StaticMethod() instead of StaticMethod) but it is inconvenient.

Examples

[HFadeel]

Rpslyn can cache () => StaticMethod() but not StaticMethod
I think should be fixed.

[marksmeltzer]

This was brought up in the discussion of #23557....

Any reason this is still pending? I see above that the lang spec requires an update.

[yaakov-h]

[The C# language spec is being modified to permit this]

Is there a corresponding issue on csharplang, or has the spec already been modified?

[ghost]

if what @danieljohnson2 said above is taken into account and there is no breaking change when it comes to equality comparison, why is the spec modification required for a performance enhancement of roslyn, like already happening for non-capturing lambda expressions?

[danieljohnson2]

@kasper3, I don't think they are 'taking it into account'- I think they are going ahead with a breaking change. The spec used to promise a new delegate instance each time you did a method group conversion, and this won't be the case anymore. The new behavior will be less predictable.

That said, while this does demand a spec change, I doubt it will be a real problem. It would take some very strange code to actually break due to this. I expect real code will be slightly faster or slower and nothing more.

[marksmeltzer]

Has there been any progress on this issue? I see some merged and closed statuses above.

[sungam3r]

@stephentoub @gafter It has been a long time since posting that issue. Has a positive or negative decision been made on the problem described?