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]

👍 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:

using System;

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

    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<T>()
{
    InvokeAction(
        <>c__1<T>.<>9__1_0 ?? 
        (<>c__1<T>.<>9__1_0 = new Action(<>c__1<T>.<>9.<GenericMethod>b__1_0)));
}

private sealed class <>c__1<T>
{
    public static readonly Test.<>c__1<T> <>9;
    public static Action <>9__1_0;
    internal void <GenericMethod>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. 👍

[davidfowl]

Also since the recent simplifications in ASP.NET Core even more people may be tempted to use static methods in MapXxx calls.

That allocation is once per application (not per request) so its's not a big deal.

[shadow-cs]

Right @davidfowl this is done in startup and then cached internally, I failed to think that far ahead.

[shadow-cs]

Any news? Seems to me more and more people stumble upon this.

[KalleOlaviNiemitalo]

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

The spec has already been modified and published.

ECMA-334 (C# Language Specification) 4th edition (June 2006) disallows caching:

• §13.6 (Method group conversions)

  The compile-time application of the conversion from E to D is the same as the compile-time processing of the delegate creation expression new D(E) (§14.5.10.3).

• §14.5.10.3 (Delegate creation expressions)

  The result is a value of type D, namely a newly created delegate that refers to the method M and target object.

ECMA-334 5th edition (December 2017) allows caching:

• §11.8 (Method group conversions)

  The conversion is permitted (but not required) to use an existing delegate instance that already contains these references.

Also available in dotnet/csharpstandard.

[RikkiGibson]

This is resolved by #58288. Expected to ship in 17.2. I believe the caching behavior only occurs when using a preview language version, and probably with C# 11/.NET 7 and up once they are released.

[jcouv]

FYI, feature was merged for Visual Studio 17.2 preview 2 as preview feature. Will be released as part of .NET 7 and C# 11 by end of year.

[davidfowl]