CA1822 is not triggered when instance method without instance references is passed as argument to another method

[bjornhellander]

Analyzer

Diagnostic ID: CA1822:

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 8.0.0

Describe the bug

See title.

Steps To Reproduce

using System;

class TestClass
{
    public void TestMethod()
    {
        Caller(Callee);
    }

    public static void Caller(Action a)
    {
    }

    private void Callee()
    {
    }
}

Expected behavior

CA1822 should be triggered for Callee, since that method can be marked static.

Actual behavior

It is not.

Additional context

[CollinAlpert]

That's because Callee() is used as a delgate. The analyzer ignores methods which are used as delegates. More info here.

[bjornhellander]

Thanks, @CollinAlpert! I read the issue you linked, but don't really understand. I checked the IL now and the Action is getting cached when using a static method, but not for instance methods, so it would seems that static methods is at least easier on the GC . What is the reason for not triggering in this case?

[CollinAlpert]

From what I understand, marking a method as static is good when this method is called directly, however not so good when it is called via a delegate. I agree with you, I find it surprising as well, however that is the rationale behind the analyzer's behavior.

If you want to create a benchmark demonstrating how static methods are more performant than instance methods, you could open a new issue with the results and subsequently get the analyzer implementation to change.

[bjornhellander]

I tried with this (very simplified) benchmark code:

using System;
using System.Diagnostics;

namespace TestStuff2
{
    internal class Program
    {
        private const int Iterations = 1_000_000_000;
        private readonly Action savedStaticMethod;
        private readonly Action savedInstanceMethod;

        public static void Main()
        {
            var program = new Program();
            program.Run();
        }

        public Program()
        {
            savedStaticMethod = StaticMethod;
            savedInstanceMethod = InstanceMethod;
        }

        private void Run()
        {
            Measure("Static", CallStaticMethod);
            Measure("Instance", CallInstanceMethod);
            Measure("Saved Static", CallSavedStaticMethod);
            Measure("Saved Instance", CallSavedInstanceMethod);
        }

        private void Measure(string title, Action action)
        {
            var sw = new Stopwatch();
            sw.Start();

            for (var i = 0; i < Iterations; i++)
            {
                action();
            }

            sw.Stop();
            Console.WriteLine($"{title}: {sw.Elapsed}");
        }

        private void CallStaticMethod()
        {
            Call(StaticMethod);
        }

        private static void StaticMethod()
        {
        }

        private void CallInstanceMethod()
        {
            Call(InstanceMethod);
        }

        private void InstanceMethod()
        {
        }

        private void CallSavedStaticMethod()
        {
            Call(savedStaticMethod);
        }

        private void CallSavedInstanceMethod()
        {
            Call(savedInstanceMethod);
        }

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

and got this result (Release configuration, without debugger):

Static: 00:00:03.6247816
Instance: 00:00:11.9858278
Saved Static: 00:00:04.8198289
Saved Instance: 00:00:02.5756172

[bjornhellander]

I have a hard time understanding the last two (in relation to each other, i.e. why would it be worse to store a static method in a field and call that, compared to a stored instance method), but the one standing out is calling a method with an instance method as argument. The first one caches a delegate and has to check each time if the delegate has been created yet, but the second one creates the delegate every time, If this diagnostic always ignores methods which in any way are used to create a delegate, then I can understand the behavior. Based on these results, if seems it would be beneficial to not ignore methods that are only passed as arguments to other methods.

[CollinAlpert]

Using a Stopwatch can be unreliable for benchmarking. I have run a couple of benchmarks using BenchmarkDotNet. Here is the benchmark:

public class Benchmarks
{
    private static Random random = default!;
    private Func<int> instanceMethod = default!;
    private Func<int> staticMethod = default!;

    [GlobalSetup]
    public void Setup()
    {
        random = Random.Shared;
        instanceMethod = InstanceMethod;
        staticMethod = StaticMethod;
    }

    [Benchmark(Baseline = true)]
    public int CallInstanceMethod()
    {
        return instanceMethod();
    }

    [Benchmark]
    public int CallStaticMethod()
    {
        return staticMethod();
    }

    private int InstanceMethod()
    {
        return random.Next();
    }

    private static int StaticMethod()
    {
        return random.Next();
    }
}

And here are the results:

It seems that calling a static method from a delegate is indeed slower.

[bzd3y]

Just to chime in on this, in case it is of any value: This seems like a good example of why CA1822 is a "bad" rule or is maybe just too aggressive. It seems to change the semantics of static or imply or assume different semantics. The rule seems to be "make this method static just because you can", but that isn't what the semantics (or mechanics/implementation?) of static are.

static seems like it is supposed to be for things that are static, as in you would want to call them statically, e.g. Type.SomeMethod().

Anyway, as for this being a good example of a problem with the rule, the reason is that the use of a method can change over time. It might access instance members at one time and then not access them after some change or vice versa. Similarly, it could be passed in as a delegate somewhere or it might not be. That is true for any static method and might not be under my control.

So this rule to me seems to just encourage premature optimization that might even end up being de-optimization. It also encourages locking APIs down in ways where something that might normally be a non-breaking change would be a breaking change just by maybe flip-flopping between static and not based on how it access instance members, which could be different between derived types based on the type and so on.

Long story short, this seems like something the compiler should be optimizing, sort of like how developers don't have complete control over inlining, which also means that they don't have to worry about it and can't overdo it and so on.