Unused type parameters in local functions could be elided

[Sergio0694]

Overview

Local functions can be extremely useful to make code more modular or to perform certain optimizations that rely on helper methods, while keeping auxiliary methods constrained to the current scope to keep the code leaner and easier to maintain. They have the unfortunate habit of always inheriting all type parameters from the parent method, with no way for developers to control this. While this makes them easier to write (you can just not care and the code will work), this can result in inefficiencies when one or more of the inherited type parameters is not used, especially if one or more of them happens to be substituted by a value type argument, which will cause the same method to be JIT-ted multiple times for no reason, if that type parameters is never referenced.

Here's an example I stumbled upon recently that illustrates the issue:

// NOTE: simplified code just to illustrate the issue
public void Send<TMessage, TToken>(TMessage message, TToken token)
{
    // Do some setup...

    static void Broadcast(TMessage message)
    {
         // Do some stuff...
    }

    Broadcast(message);
}

In this case the TToken parameter is never used in the Broadcast method, which the compiler can validate, yet the generated method will still inherit both type parameters. This means that the same method will be JIT-ted again for every different TToken type argument that happens to be a value type, despite the fact that this method could just be compiled once and then always reused (if TMessage is always a reference type, as is the case for us in the MVVM Toolkit).

Proposal

For each type parameter in a local function, Roslyn could check whether that type parameter is actually used at all (ie. whether removing it causes the build to fail or any kind of difference in semantics for the local function), and if so, it should elide it.

cc. @333fred who mentioned (in #lowlevel on the C# Discord) that this could technically be allowed by the spec as there are no guarantees on the exact signature for generated methods from local functions.

[svick]

Would something similar also apply to type parameters from the containing type?

[Sergio0694]

I mean that would also be nice, but for this proposal I was just focusing on the ones inherited by the containing method. I'd imagine removing them from the containing type would be trickier as they could no longer just be declared within it as they currently are, so they would need to be generated in some other type that'd be either nongeneric, or with the specific subset of used type parameters for that method, and it all sounds like quite some more work.

Of course, I'm happy to be proven wrong in case I'm mistaking here 😄

[Sergio0694]

Was wondering - would a fix for this also affect closure/delegate types, or should I open a separate issue for that? I noticed the same (presumably incorrect) behavior in some code from the MVVM Toolkit as well.

Here's a minimal repro:

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

class WeakReferenceMessenger
{
    private readonly ConditionalWeakTable<object, object> recipientsMap = new();

    public void Register2<TRecipient, TMessage, TToken>(TRecipient recipient, TToken token, Action<TRecipient, TMessage> handler)
        where TRecipient : class
        where TMessage : class
        where TToken : IEquatable<TToken>
    {
        _ = recipientsMap.GetValue(recipient, static _ => new Dictionary<TToken, object>());
    }
}

This results in this (decompiled) code:

internal class WeakReferenceMessenger
{
    private sealed class <>c__1<TRecipient, TMessage, TToken>
        where TRecipient : class
        where TMessage : class
        where TToken : IEquatable<TToken>
    {
        public static readonly <>c__1<TRecipient, TMessage, TToken> <>9 = new <>c__1<TRecipient, TMessage, TToken>();

        public static ConditionalWeakTable<object, object>.CreateValueCallback <>9__1_0;

        internal object <Register2>b__1_0(object _)
        {
            return new Dictionary<TToken, object>();
        }
    }

    private readonly ConditionalWeakTable<object, object> recipientsMap = new ConditionalWeakTable<object, object>();

    public void Register2<TRecipient, TMessage, TToken>(TRecipient recipient, TToken token, Action<TRecipient, TMessage> handler)
        where TRecipient : class
        where TMessage : class
        where TToken : IEquatable<TToken>
    {
        recipientsMap.GetValue(
            recipient,
            <>c__1<TRecipient, TMessage, TToken>.<>9__1_0 ??
            (<>c__1<TRecipient, TMessage, TToken>.<>9__1_0 = new ConditionalWeakTable<object, object>.CreateValueCallback(
                <>c__1<TRecipient, TMessage, TToken>.<>9.<Register2>b__1_0)));
    }
}

In this case the generated delegate is also capturing the TRecipient and TMessage types, unnecessarily. This would also cause multiple delegates being created instead of reused, which could be done as long as TToken was the same. Of course, developers can also work around this by manually moving that code out of generics, but it seems like having "silent pit of failures" like this where you don't even get a warning is not ideal. Thoughts? 😄

[Sergio0694]

Noticed this issue also currently forces developers not to use local functions for throw helpers to avoid unnecessary generic instantiations. Consider this example where a generic method has a small throw helper used to validate inputs:

static T[] CreateArray<T>(int size)
{
    if (size < 0)
    {
        ThrowArgumentOutOfRangeExceptionForNegativeCapacity();
    }

    return new T[size];

    static void ThrowArgumentOutOfRangeExceptionForNegativeCapacity()
    {
        throw new ArgumentOutOfRangeException("size", "The array size must be a positive number.");
    }
}

This results in the following (built from main, https://github.com/dotnet/roslyn/commit/e79be97e37654f8389f9d0d49bb27d39dd413577):

[CompilerGenerated]
internal static T[] <<Main>$>g__CreateArray|0_0<T>(int size)
{
    if (size < 0)
    {
        <<Main>$>g__ThrowArgumentOutOfRangeExceptionForNegativeCapacity|0_1<T>();
    }
    return new T[size];
}

[CompilerGenerated]
internal static void <<Main>$>g__ThrowArgumentOutOfRangeExceptionForNegativeCapacity|0_1<T>()
{
    throw new ArgumentOutOfRangeException("size", "The array size must be a positive number.");
}

You can see the throw helper also retains the T parameter, which is just completely unnecessary and causes extra code size.