System.Reflection.Emit: ILGenerator.Emit(OpCodes.Ldtoken, <MethodInfo>) produces incorrect metadata for methods with C# 7.2 in parameters in generic types

[stakx]

@TAGC recently noted in castleproject/Core#339 (comment) that DynamicProxy-based mocking libraries (e.g. Moq, NSubstitute) appear to have problems mocking the following generic interface:

public interface IGenericStructByRefConsumer<T>
{
    T Consume(in Struct message);
}

Mocking such a type will typically result in a MissingMethodException. It only seems to occur with generic types/methods, and in the presence of a C# 7.2 in parameter modifier.

I've been able to track this down to what appears to be a bug in System.Reflection.Emit. A brief description follows below; if more detail or code is required, I can probably provide it... please ask.

---

Given this C# 7.2+ interface:

public interface WithIn<T>
{
    void Method(in int arg);
}

(Expand to see the equivalent type definition in IL.)

.class public abstract interface auto ansi WithIn`1<T>
{
  .method public hidebysig newslot abstract virtual instance void Method([in] int32& modreq([mscorlib]System.Runtime.InteropServices.InAttribute) arg) cil managed
  {
    .param [1]
    .custom instance void [mscorlib]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
  }
}

DynamicProxy would need to generate the following IL instruction sequence using a System.Reflection.Emit.ILGenerator (because it caches MethodInfo for proxied methods):

ldtoken    method instance void class WithIn`1<int32>::Method(int32& modreq([mscorlib]System.Runtime.InteropServices.InAttribute))
ldtoken    class WithIn`1<int32>
call       class [mscorlib]System.Reflection.MethodBase 

This is how one would supposedly do it with ILGenerator:

var methodType = typeof(WithIn<int>);
var method = methodType.GetMethod("Method");
var getMethodFromHandle = typeof(MethodBase).GetMethod("GetMethodFromHandle", new[] { typeof(RuntimeMethodHandle), typeof(RuntimeTypeHandle) });

ilGenerator.Emit(OpCodes.Ldtoken, method);  // <-- !!!
ilGenerator.Emit(OpCodes.Ldtoken, methodType);
ilGenerator.EmitCall(OpCodes.Call, getMethodFromHandle, null);

I've verified (by saving the dynamic assembly to disk using the .NET Framework 4.7, then dumping it with ILDASM) that the following IL is generated instead:

ldtoken    method instance void class WithIn`1<int32>::Method(int32&)
ldtoken    class WithIn`1<int32>
call       class [mscorlib]System.Reflection.MethodBase [mscorlib]System.Reflection.MethodBase::GetMethodFromHandle(valuetype [mscorlib]System.RuntimeMethodHandle,

Note the absence of the modreq in the first instruction's method reference. This will result in a MissingMethodException (because method signatures don't match) when the generated code is run:

System.MissingMethodException: Method not found: 'Void IWithIn`1.Method(Int32 ByRef)'.

The same MissingMethodException occurs with .NET Core 2.0, so I suspect it inherited the exact same problem from the .NET Framework. If you'd rather have repro code for .NET Core, you'll find it below.

(Expand for an error reproduction on .NET Core.)

using System;
using System.Diagnostics;
using System.Reflection;
using System.Reflection.Emit;

public interface WithIn<T>
{
    void Method(in int arg);
}

class Program
{
    static void Main()
    {
        var methodType = typeof(WithIn<int>);
        var method = methodType.GetMethod("Method");
        var getMethodFromHandle = typeof(MethodBase).GetMethod("GetMethodFromHandle", new[] { typeof(RuntimeMethodHandle), typeof(RuntimeTypeHandle) });

        var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("DynamicAssembly"), AssemblyBuilderAccess.Run);
        var moduleBuilder = assemblyBuilder.DefineDynamicModule("DynamicModule");
        var typeBuilder = moduleBuilder.DefineType("DynamicType", TypeAttributes.Public | TypeAttributes.Abstract | TypeAttributes.Class);

        var methodBuilder = typeBuilder.DefineMethod("Get", MethodAttributes.Public | MethodAttributes.Static, typeof(MethodBase), new Type[0]);
        var ilBuilder = methodBuilder.GetILGenerator();
        ilBuilder.Emit(OpCodes.Ldtoken, method);
        ilBuilder.Emit(OpCodes.Ldtoken, methodType);
        ilBuilder.EmitCall(OpCodes.Call, getMethodFromHandle, null);
        ilBuilder.Emit(OpCodes.Ret);

        var type = typeBuilder.CreateType();

        var il = type.GetMethod("Get").GetMethodBody().GetILAsByteArray();
        Debug.Assert(il[0] == OpCodes.Ldtoken.Value, "First op-code is not `ldtoken`.");

        var ilMethodMetadataToken = BitConverter.ToInt32(il, 1);
        Debug.Assert(type.Module.ResolveMethod(ilMethodMetadataToken) == method, "First `ldtoken` does not refer to the correct method.");
    }
}

Note again that this problem only occurs for methods in generic types/methods, and in the presence of an in parameter modifier.

/cc @jonorossi (for DynamicProxy), @zvirja (for NSubstitute), @thomaslevesque (for FakeItEasy)

[TAGC]

Shame this issue missed the 2.1.0 bus. It will become increasingly difficult to use any sort of mocking framework as in parameters start becoming common in people's codebases.

[stakx]

@atsushikan - I'd like to give this a try. Would you accept a PR?

[ghost]

Sure, go for it.

[stakx]

Just a quick note about this issue's current state: it is still open because the PR which was supposed to fix this (dotnet/coreclr#17881) caused a regression (https://github.com/dotnet/coreclr/issues/18034) and because of that, was reverted (dotnet/coreclr#18036). No attempt has been made since to provide another patch for this problem.

[TIHan]

I think I am having this problem for FSI (F# Interactive). I discovered this while working on this PR: dotnet/fsharp#6213 ; I built a test on ReadOnlySpan using FSI as a script runner, but the test is failing due to a missing method, get_Item which requires the modreq.

FSI uses System.Reflection.Emit to emit IL dynamically. This means F# interactive will fail on a call to a method that has modreq/modopt in any part of its signature.

Is this likely to be fixed by .NET Core 3.0?

[TIHan]

@steveharter , will this make the bar for .NET Core 3.0? @terrajobst told me to ask you :)

[steveharter]

Proposing moving to 3.0 unless someone picks this up.

[danmoseley]

@steveharter I assume you mean moving out of 3.0. I'll move it (since it's not necessary to ship 3.0) but for now there is still a little room to fix non 3.0 bugs.

[hamish-milne]

Had a look into this and found what was causing the regression in dotnet/coreclr#18034

https://github.com/dotnet/coreclr/pull/17881/files#diff-e3bb64cfa9da76e2fab3eaabc103af22L450-L456

GetParameters is not supported for MethodBuilder/ConstructorBuilder for whatever reason, hence the previous use of GetParameterTypes. But it's OK because in that case, we can get the SignatureHelper directly:

https://github.com/dotnet/coreclr/blob/073ad7ef1b6a7112eefc965aed362c7b5923682a/src/System.Private.CoreLib/src/System/Reflection/Emit/MethodBuilder.cs#L342

(Am I allowed to say all this code is kind of a mess? :P)

@stakx If this wouldn't work, or you want to do it, let me know. But otherwise I'll resurrect your PR with that one change. At some point. When I get around to it.

[stakx]

@hamish-milne, I touched this code once but I'm certainly not territorial about it 😄. If you think you can fix this, I'll be happy to defer to you. Please do reuse whatever seems useful from my old PR. It would be great to finally see this fixed!

P.S.: I didn't realise I was still assigned to this issue after all that time 😲, I haven't looked at this code closely for many months! Unassigned now.

[steveharter]

Changing milestone to 6.0 for risk (5.0 is almost in ask-mode and no more previews); see the PR for more information. cc @GrabYourPitchforks

[dsyme]

@steveharter Just to check, is it likely that #40587 fixes this problem dotnet/fsharp#9401 (comment)

That is, a Reflection emit call to ReadOnlySpan get_Item code?

[wzchua]

Ok there was a mistake in my IL. It works =)

var a = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("aa"), AssemblyBuilderAccess.Run);
var m = a.DefineDynamicModule("MyDynamicAsm");
var t = m.DefineType("MyDynamicType", TypeAttributes.Public);

 var mb = t.DefineMethod("foo", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, typeof(char), new Type[] { typeof(string) });
var ilg = mb.GetILGenerator();
var minfo1 = typeof(MemoryExtensions).GetMethod("AsSpan", new Type[] { typeof(string) });
var minfo2 = typeof(ReadOnlySpan<char>).GetMethod("get_Item", BindingFlags.Public | BindingFlags.Instance);
LocalBuilder myLB1 = ilg.DeclareLocal(typeof(ReadOnlySpan<char>));
            
ilg.Emit(OpCodes.Ldarg_0);
ilg.Emit(OpCodes.Call, minfo1);
ilg.Emit(OpCodes.Stloc_0);
ilg.Emit(OpCodes.Ldloca_S, 0);
ilg.Emit(OpCodes.Ldc_I4_0);
ilg.Emit(OpCodes.Call, minfo2);
ilg.Emit(OpCodes.Ldind_U2);
ilg.Emit(OpCodes.Ret);
t.CreateType();

var m2 = t.GetMethod("foo");
byte[] il = m2.GetMethodBody().GetILAsByteArray();
int ilMethodMetadataToken = BitConverter.ToInt32(il, 2);
MethodBase resolvedMethod = t.Module.ResolveMethod(ilMethodMetadataToken);
Assert.Equal(minfo1, resolvedMethod);
int ilMethodMetadataToken2 = BitConverter.ToInt32(il, 14);
MethodBase resolvedMethod2 = t.Module.ResolveMethod(ilMethodMetadataToken2);
Assert.Equal(minfo2, resolvedMethod2);
var c = (char)m2.Invoke(null, new object[] { "abc" });
Assert.Equal('a', c);