IDE0200 recommends change that increases allocation

[stephentoub]

Version Used:
Version 17.11.0 Preview 1.0 [34810.211.main]

Steps to Reproduce:

public class C 
{
    private static readonly Invoker s_invoker = new();
    
    public void M1() => Use(s_invoker.Invoke);
    public void M2() => Use(text => s_invoker.Invoke(text));
    
    public void Use(Func<string, int> func) {}
}

public class Invoker
{
    public int Invoke(string text) => 42;
}

Diagnostic Id:
IDE0200

Expected Behavior:
No diagnostic, or at least a different diagnostic code indicating the change will be making the code less efficient.

Actual Behavior:
IDE0200 gets raised on M2 and recommends it be changed to the equivalent of M1. But M1 always allocates a new delegate whereas M2 as written above caches the delegate such that ammortized it only ever allocates once.

[CyrusNajmabadi]

Our view here when we last looked at this was that for code that wants to ensure perf above clarity here, it should mark the lambda explicitly to ensure it has non-alloc semantics. So, in this case, add static to the lambda. Do that, and we won't touch it.

[stephentoub]

"Unnecessary" suggests it serves no purpose / has zero benefit. That is of course not the case.

[sharwell]

I agree with @stephentoub here. IDE0200 certainly attempts to avoid being reported in cases where a new allocation is introduced. For this case, it appears it was missed either because we did not previously optimize M2() or because we missed this edge case in testing.

The specific trigger for M2() being optimized here is the use of static on s_invoker. It may be sufficient to modify the compiler to optimize M1() for this case instead of disabling IDE0200.

[CyrusNajmabadi]

This case was not missed. IT was an intentional part of the original design here. And it's one of the main reasons i championed and implemented teh static-lambda proposal. The general feeling being that it's fine to have clear code that occasionally allocs, and if you're in a perf sensitive domain, you take the steps necessary to annotate your code to make that clear.

[stephentoub]

At a minimum, the "unnecessary" in "Remove unnecessary lambda expression" should be clarified. The lambda or something in its stead are in fact necessary to avoid the allocation. If we can improve the compiler to actually avoid the allocation, all the better.

[CyrusNajmabadi]

It's not necessary in terms of strict semantics. You'll get the same result, just with potentially different performance characteristics (which is how virtually all of our fixups work). In this case, we really did say that if perf here, through single alloc, is critical, then it should be marked as such with static. That way you don't accidentally break this yourself in the future by capturing something that makes this alloc each time.

We've gone down this path before. When we insisted that the features not impact potentially anything observable (including perf) we found we couldn't supply the vast majority of features. Our current position (which has been this way for nearly a decade, and which IDE0200 was written under) is intentional. We go for simplicity/clarity, with an eye towards idiomatic code that is more likely than not what it good enough for a codebase. We recognize this means that certain codebases (especially exceptional ones like Runtime) may either have to disable some of these, or update their code to indicate their intent. It's an intentional decision that that's how things have fallen out. And, given the almost zero pieces of community feedback on these behavior choices, i believe we should continue sticking with this, even if it makes things less palatable for a small handful of repos.

[stephentoub]

You'll get the same result

You won't, actually: it is observable, in that the delegate you get in one case points directly to the target method and in another case points to a compiler-generated one. e.g. this:

C c = new();
c.M1();
c.M2();

public class C
{
    private static readonly Invoker s_invoker = new();

    public void M1() => Use(s_invoker.Invoke);
    public void M2() => Use(text => s_invoker.Invoke(text));

    public void Use(Func<string, int> func) => Console.WriteLine(func.Target);
}

public class Invoker
{
    public int Invoke(string text) => 42;
}

prints:

Invoker
C+<>c

I'm not suggesting we remove the analyzer. I'm simply suggesting at a minimum we clarify what we mean. It's not at all obvious, especially for someone doing a bulk fix-all, that this can silently lead to additional allocation happening.

[CyrusNajmabadi]

I'm simply suggesting at a minimum we clarify what we mean.

I'm ok with that.