Fix ArgumentsSource on external types not working if the argument type is not primitive

Merged
JI
1y ago
+114-16

Followup to #2748. As of BDN 0.15.2, you can't use [ArgumentsSource] with an external type providing the arguments if the argument type is not primitive. This happens because the generated code has compiler errors.

The issue

If you have code like this:

[Benchmark]
[ArgumentsSource(typeof(DiagnosticsCases), nameof(DiagnosticsCases.MethodACases))]
public MethodAResults BenchmarkMethodA(MethodACase @case)
    => CodeBeingBenchmarked.MethodA(@case.Param1, @case.Param2, @case.Param3);

Then BDN currently generates code like this:

public Runnable_0()
{
    globalSetupAction = () => { };
    globalCleanupAction = () => { };
    iterationSetupAction = () => { };
    iterationCleanupAction = () => { };
    overheadDelegate = __Overhead;
    workloadDelegate = WithChildrenAdded;
    __argField0 = (ExampleNamespace.MethodACase)BenchmarkDotNet.Parameters.ParameterExtractor.GetParameter(MethodACases(), 0);;
}

The problem is that MethodACases is not in the type that the generated class (Runnable_0) inherits from, it's in an external type (DiagnosticsCases). So there's a compiler error here ("error CS0103: The name 'MethodACases' does not exist in the current context")

This only happens if the argument type is non-primitive (like MethodACase). If it's a primitive type (like int) then the argument value is inlined as a literal, instead of using ParameterExtractor.

The fix

In this example, we need to put the fully qualified type name before the method call. The last line of the generated code should instead read:

__argField0 = (ExampleNamespace.MethodACase)BenchmarkDotNet.Parameters.ParameterExtractor.GetParameter(ExampleNamespace.DiagnosticsCases.MethodACases(), 0);;

However, we can't always add the fully qualified type name, because ArgumentsSource can also be used with non-static methods/properties.

So, this PR checks if ArgumentsSource is being used with a static type. If so, the fully qualified type name is added at this location; otherwise, the generated code is unchanged compared to before this PR.

I am worried about external instance methods

Using ArgumentsSource for non-primitive arguments with different kinds of members:

Does it work before this PRDoes it work after this PR
Static member from the same class✔✔
Static member from an external class❌✔
Instance member from the same class✔✔
Instance member from an external class❌❌

As you can see from this helpful table, this PR fixes one broken case but there is still one broken case remaining. How best to fix this? Should the generated code create an instance of the member's type, and call the member from the new instance?

To be honest, I think there's no benefit to supporting instance members at all. @timcassell also dislikes that we support instance members and we discussed it a bit here: https://github.com/dotnet/BenchmarkDotNet/issues/2744#issuecomment-2937665583.

Maybe we should take this opportunity to just drop support for instance members in ArgumentsSource and ParamsSource?

Other things I am worried about

  • The way this PR checks if the member is static is kind of brittle and makes me nervous. Is there a better way of doing it? (If we just drop support for instance members, this problem goes away!!)
  • Is the code I wrote the correct way to fix the described issue? I would appreciate some feedback from someone more familiar with the codegen parts of BDN. It's very complex code and I couldn't fully wrap my head around it.